Message-ID:
 <AS2PR09MB598321A95CD8EF0264744498A4572@AS2PR09MB5983.eurprd09.prod.outlook.com>
Date: Wed, 21 Feb 2024 19:06:07 +0000
From: Zachary Goldstein <Zachary.Goldstein@...current-rt.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Shawn Guo <shawnguo@...nel.org>, Madalin Bucur <madalin.bucur@....com>, Li
 Yang <leoyang.li@....com>, Rob Herring <robh+dt@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley
	<conor+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>, Heiner Kallweit
	<hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: Re: EXTERNAL: Re: [PATCH] arm64: ls1046ardb: Replace XGMII with
 10GBASE-R phy mode

On Tue, Feb 20, 2024 at 04:50:37PM +0200, Vladimir Oltean wrote:
> Hi Zachary,
> 
> On Wed, Feb 14, 2024 at 05:21:54PM -0500, Zachary Goldstein via B4 Relay wrote:
> > From: Zachary Goldstein <zachary.goldstein@...current-rt.com>
> > 
> > The AQR107 family does not support XGMII, but USXGMII and
> > 10GBASE-R instead. Since ce64c1f77a9d ("net: phy: aquantia: add USXGMII
> > support and warn if XGMII mode is set") the kernel warns about XGMII
> > being used. The LS1046A SoC does not support USXGMII, so use 10GBASE-R
> > instead.
> > 
> > Signed-off-by: Zachary Goldstein <zachary.goldstein@...current-rt.com>
> > ---
> >  arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > index 07f6cc6e354a..d2066f733dc5 100644
> > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts
> > @@ -149,7 +149,7 @@ ethernet@...00 {
> >  
> >  	ethernet@...00 { /* 10GEC1 */
> >  		phy-handle = <&aqr106_phy>;
> > -		phy-connection-type = "xgmii";
> > +		phy-connection-type = "10gbase-r";
> >  	};
> >  
> >  	ethernet@...00 { /* 10GEC2 */
> > 
> > ---
> > 2.40.1
> > base-commit: 841c35169323cd833294798e58b9bf63fa4fa1de
> > 
> > 
> 
> You do not need this patch in upstream, and I strongly advise against
> merging it as-is. I've just tested pure net-next on LS1046A-RDB, and I
> can bring up fm1-mac9 without any warning.
> 
> You'll notice that commit 5d93cfcf7360 ("net: dpaa: Convert to phylink")
> did this in fman_memac.c:
> 
> 	/* The internal connection to the serdes is XGMII, but this isn't
> 	 * really correct for the phy mode (which is the external connection).
> 	 * However, this is how all older device trees say that they want
> 	 * 10GBASE-R (aka XFI), so just convert it for them.
> 	 */
> 	if (mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII)
> 		mac_dev->phy_if = PHY_INTERFACE_MODE_10GBASER;
> 
> So, what gets passed to the Aquantia PHY is PHY_INTERFACE_MODE_10GBASER,
> even if in the device tree, phy-connection-type = "xgmii".
> 
> Now, _if_ your patch were to be applied on top of upstream, it would
> actually break fm1-mac9 like this (WARN_ON added by me for a call stack
> of the phylink_validate() failure path):
> 
> WARNING: CPU: 2 PID: 1 at drivers/net/phy/phylink.c:763 phylink_create+0x8f8/0x90c
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Tainted: G        W          6.8.0-rc4-01058-g9e1deba407fb #1812
> Hardware name: LS1046A RDB Board (DT)
> Call trace:
>  phylink_create+0x8f8/0x90c
>  dpaa_netdev_init+0x1a8/0x2c8
>  dpaa_eth_probe+0xd70/0xf64
>  platform_probe+0xa8/0xd0
>  really_probe+0x130/0x2e4
>  __driver_probe_device+0xa0/0x128
>  driver_probe_device+0x3c/0x200
>  __driver_attach+0xe8/0x1b4
>  bus_for_each_dev+0xec/0x144
>  driver_attach+0x24/0x30
>  bus_add_driver+0x154/0x244
>  driver_register+0x68/0x100
>  __platform_driver_register+0x24/0x30
>  dpaa_load+0x34/0x64
>  do_one_initcall+0xf8/0x34c
> ---[ end trace 0000000000000000 ]---
> fsl_dpaa_mac 1af0000.ethernet (unnamed net_device) (uninitialized): failed to validate link configuration for in-band status
> fsl_dpaa_mac 1af0000.ethernet: error -EINVAL: Could not create phylink
> fsl_dpa: probe of dpaa-ethernet.4 failed with error -22
> 
> It fails because of this in phylink_validate():
> 
> 	if (!test_bit(state->interface, interfaces))
> 		return -EINVAL;
> 
> And state->interface (PHY_INTERFACE_MODE_10GBASER) is not in
> mac_dev->phylink_config.supported_interfaces, because the fman_memac
> code is not prepared to handle this combination.
> 
> The device tree node for fm1-mac9 looks like this, generated by an
> awkward merge between the following:
> 
> 	ethernet@...00 {
> 		phy-connection-type = "xgmii";	// fsl-ls1046a-rdb.dts
> 		local-mac-address = [...];	// U-Boot
> 		cell-index = <0x8>;		// qoriq-fman3-0-10g-0.dtsi
> 		pcsphy-handle = <0x31>;		// qoriq-fman3-0-10g-0.dtsi
> 		compatible = "fsl,fman-memac";	// qoriq-fman3-0-10g-0.dtsi
> 		reg = <0xf0000 0x1000>;		// qoriq-fman3-0-10g-0.dtsi
> 		phy-handle = <&aqr106_phy>;	// fsl-ls1046a-rdb.dts
> 		fsl,fman-ports = <0x2f 0x30>;	// qoriq-fman3-0-10g-0.dtsi
> 	};
> 
> Notice that unlike fm1-mac10 (node "ethernet@...00"), there is no
> pcs-handle-names property (fm1-mac10 has it defined in fsl-ls1046-post.dtsi,
> whereas fm1-mac9 doesn't. Don't ask me why, I don't know....)
> 
> So, knowing that of_property_match_string(mac_node, "pcs-handle-names", "xfi")
> will return an error code for fm1-mac9, now please walk through memac_initialization()
> and see what happens in the 2 cases:
> 
> - mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII (current device tree). The
>   code creates a default PCS and assigns it to memac->xfi_pcs like this:
> 	if (err == -EINVAL || err == -ENODATA)
> 		pcs = memac_pcs_create(mac_node, 0);
> 	(...)
> 	if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII)
> 		memac->xfi_pcs = pcs;
> 
> - mac_dev->phy_if == PHY_INTERFACE_MODE_10GBASER (your modification).
>   The code will still create the default PCS, but assign it to
>   memac->sgmii_pcs instead!
> 
> 	if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII) // not XGMII, but 10GBASER!
> 		memac->xfi_pcs = pcs;
> 	else
> 		memac->sgmii_pcs = pcs;
> 
> And this is why, with a NULL memac->xfi_pcs, PHY_INTERFACE_MODE_10GBASER
> will not be in phylink's supported_interfaces.
> 

Admittedly, I had made the wrong assumption from looking at the dpaa
driver code and fm1-mac10, that "xgmii" and "10gbase-r" were
interchangeable. 

> To make your device tree patch work as intended with the current
> mainline code, what you want is to also modify the driver like this:
> 
> >From d6bda34b132d17d1c236d27436b9335fac22c062 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@....com>
> Date: Tue, 20 Feb 2024 16:12:27 +0200
> Subject: [PATCH] net: dpaa: fman_memac: accept phy-interface-type =
>  "10gbase-r" in the device tree
> 
> We support the phy-interface-mode = "xgmii" conversion to "10gbase-r"
> through code, but not actually through the device tree proper. This is
> because boards such as LS1046A-RDB do not define pcs-handle-names in the
> ethernet@...00 device tree node, and the code only has fallback xfi_pcs
> determination logic for "xgmii".
> 
> By reversing the order between the fallback xfi_pcs assignment and the
> "xgmii" overwrite with "10gbase-r", we are able to support both values
> in the device tree, with identical behavior.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  .../net/ethernet/freescale/fman/fman_memac.c   | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
> index e30bf75b1d48..0996759907a8 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> @@ -1076,6 +1076,14 @@ int memac_initialization(struct mac_device *mac_dev,
>  	unsigned long		 capabilities;
>  	unsigned long		*supported;
>  
> +	/* The internal connection to the serdes is XGMII, but this isn't
> +	 * really correct for the phy mode (which is the external connection).
> +	 * However, this is how all older device trees say that they want
> +	 * 10GBASE-R (aka XFI), so just convert it for them.
> +	 */
> +	if (mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII)
> +		mac_dev->phy_if = PHY_INTERFACE_MODE_10GBASER;
> +
>  	mac_dev->phylink_ops		= &memac_mac_ops;
>  	mac_dev->set_promisc		= memac_set_promiscuous;
>  	mac_dev->change_addr		= memac_modify_mac_address;
> @@ -1142,7 +1150,7 @@ int memac_initialization(struct mac_device *mac_dev,
>  	 * (and therefore that xfi_pcs cannot be set). If we are defaulting to
>  	 * XGMII, assume this is for XFI. Otherwise, assume it is for SGMII.
>  	 */
> -	if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII)
> +	if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_10GBASER)
>  		memac->xfi_pcs = pcs;
>  	else
>  		memac->sgmii_pcs = pcs;
> @@ -1156,14 +1164,6 @@ int memac_initialization(struct mac_device *mac_dev,
>  		goto _return_fm_mac_free;
>  	}
>  
> -	/* The internal connection to the serdes is XGMII, but this isn't
> -	 * really correct for the phy mode (which is the external connection).
> -	 * However, this is how all older device trees say that they want
> -	 * 10GBASE-R (aka XFI), so just convert it for them.
> -	 */
> -	if (mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII)
> -		mac_dev->phy_if = PHY_INTERFACE_MODE_10GBASER;
> -
>  	/* TODO: The following interface modes are supported by (some) hardware
>  	 * but not by this driver:
>  	 * - 1000BASE-KX
> 
> But!
> 
> Device tree is stable ABI, and changes made to the device tree file are
> meant to be backwards and forwards compatible with the code (it can be
> provided separately and not necessarily in lockstep with the kernel
> version. For example, I understand Arm SystemReady IR wants U-Boot to
> provide its own device tree to Linux through EFI). So, in general,
> device tree changes which only work with a corresponding kernel change
> are frowned upon (unless maybe if the kernel change is a bug fix that is
> backported to all relevant stable kernel branches).
> 
> So at this stage we should take a step back and re-analyze the cost/benefit.
> You said there is a stack trace in the Aquantia PHY driver, which there
> is not (in current mainline kernels). I _think_ you are seeing the stack
> trace with LSDK, which is currently distributed on top of linux-6.1.y
> and has not yet integrated the fman_memac conversion to phylink - thus,
> it does not contain commit 5d93cfcf7360 ("net: dpaa: Convert to phylink").
> At least, I do see this stack trace there. I think it can also be seen
> with mainline kernels before the phylink conversion, but I did not test
> those.
> 
> The main take-away is: ALWAYS test the patch you are submitting to the
> target kernel it is going to be applied to. Especially in the area of
> device tree bindings for DPAA1, things are rarely as simple as they
> appear :) If you don't, you will have an unintended negative effect
> upon current mainline kernels (which must still work), and not the
> intended effect upon LSDK (more below).

This is my mistake, as at the time I had no way of testing this patch on
6.8. After looking at commit 5d93cfcf7360 ("net: dpaa: Convert to
phylink"), I had made the (wrong) assumption that changing to
"10gbase-r" was pretty harmless. 

> 
> There are a few options to go forward from here.
> 
> As there is nothing broken in the mainline kernel where you are
> submitting this patch, the simplest one, as bland as it may sound, is
> simply to wait for a new LSDK release on top of linux-6.6.y. Even in
> lf-6.1.y, AFAICS, nothing is broken except for the stack trace. You can
> keep the patch in your local kernel tree copy to suppress that.
> 
> The other option would be to submit the fman_memac change as a bug fix
> for stable, wait for a while for it to have time to propagate, then
> modify the device tree as well. But, it is much higher effort, and
> there is no procedure in place, AFAIK, for LSDK to integrate your patch
> (other than through upstream + a few months of waiting). The upcoming
> LSDK release will be on top of linux-6.6.y, it will make your effort
> irrelevant if it's only for suppressing the stack trace, and you are
> racing against it. This path is only worth it if you have the dedication
> to dig a bit deeper and tidy things up in the DPAA1 kernel support
> (which would be appreciated though).
> 
> _If_ you are using an older linux-stable branch but not LSDK, yes, the
> feedback loop between your patch and its effect will close much sooner,
> but you will have to convince the linux-stable people that it's worth
> accepting your driver rework patches for a functional reason and not
> just aesthetic (see Documentation/process/stable-kernel-rules.rst for
> reference).

Considering "xgmii" is a deprecated phy type for the AQR107 family, I
still think there is some value to this change beyond aesthetics if
support for "10gbase-r" is ever added.

Powered by blists - more mailing lists