Re: [PATCH] arm64: ls1046ardb: Replace XGMII with 10GBASE-R phy mode

[Vladimir Oltean]

Hi Zachary,

On Wed, Feb 14, 2024 at 05:21:54PM -0500, Zachary Goldstein via B4 Relay wrote:
> From: Zachary Goldstein <zachary.goldstein@xxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxxxx>
> ---
>  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@ea000 {
>  
>  	ethernet@f0000 { /* 10GEC1 */
>  		phy-handle = <&aqr106_phy>;
> -		phy-connection-type = "xgmii";
> +		phy-connection-type = "10gbase-r";
>  	};
>  
>  	ethernet@f2000 { /* 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@f0000 {
		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@f2000"), 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.

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: