| Message ID | 20240214-ls1046-dts-use-10gbase-r-v1-1-8c2d68547393@concurrent-rt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: ls1046ardb: Replace XGMII with 10GBASE-R phy mode | expand |
Hi Zachary, On Wed, Feb 14, 2024 at 05:21:54PM -0500, Zachary Goldstein via B4 Relay wrote: > From: Zachary Goldstein <zachary.goldstein@concurrent-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@concurrent-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@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: From d6bda34b132d17d1c236d27436b9335fac22c062 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <vladimir.oltean@nxp.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@f0000 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@nxp.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). 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).
On 2/20/24 09:50, 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@concurrent-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@concurrent-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@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....) I think this is just because this ethernet is always XFI and never (Q)SGMII. > 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: > > From d6bda34b132d17d1c236d27436b9335fac22c062 Mon Sep 17 00:00:00 2001 > From: Vladimir Oltean <vladimir.oltean@nxp.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@f0000 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@nxp.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 Can you please submit this patch? I noticed this but never had the chance to go back and debug it. --Sean > 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). > > 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). > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=http%3a%2f%2flists.infradead.org%2fmailman%2flistinfo%2flinux%2darm%2dkernel&umid=8bfcbf91-eb84-4daf-8653-378929ed0326&auth=d807158c60b7d2502abde8a2fc01f40662980862-d89d931594581c69e3bf19f86f11d6de905c5a8b [Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
On Tue, Feb 20, 2024 at 02:12:52PM -0500, Sean Anderson wrote: > On 2/20/24 09:50, Vladimir Oltean wrote: > > 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....) > > I think this is just because this ethernet is always XFI and never (Q)SGMII. Is that so? With SerDes protocol 0x3333, won't the PCS that's connected to fm1-mac9 use SGMII/1000BASE-X (thus not 10GBASE-R)? And as for QSGMII, what's the relevance of that? You can't have one device tree good for all SerDes protocols and RCW pinmuxing options. Either there are 4 MACs aggregated onto a single lane, or there is one MAC per lane, but I've never encountered any use case for alternating between these 2 configurations at runtime, or with same device tree for that matter, have you? QSGMII seems to have been the original motivation for listing all possible PCSes of a MAC in pcs-handle-names, but again, why? > Can you please submit this patch? I noticed this but never had the chance to go > back and debug it. https://lore.kernel.org/netdev/20240220223442.1275946-1-vladimir.oltean@nxp.com/
On 2/20/24 17:37, Vladimir Oltean wrote: > On Tue, Feb 20, 2024 at 02:12:52PM -0500, Sean Anderson wrote: >> On 2/20/24 09:50, Vladimir Oltean wrote: >> > 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....) >> >> I think this is just because this ethernet is always XFI and never (Q)SGMII. > > Is that so? With SerDes protocol 0x3333, won't the PCS that's connected > to fm1-mac9 use SGMII/1000BASE-X (thus not 10GBASE-R)? Yeah, so I guess it just can't be QSGMII. > And as for QSGMII, what's the relevance of that? You can't have one > device tree good for all SerDes protocols and RCW pinmuxing options. The goal is for the "generic" includes to be generic. So the board-specific stuff lives in the board-specific device tree source. And the only thing board-specific is the phy interface, since the drivers can figure out the rest. > Either there are 4 MACs aggregated onto a single lane, or there is one > MAC per lane, but I've never encountered any use case for alternating > between these 2 configurations at runtime, or with same device tree for > that matter, have you? QSGMII seems to have been the original motivation > for listing all possible PCSes of a MAC in pcs-handle-names, but again, why? With SGMII and XFI, the PCS sits on the MAC's MDIO bus. So for SGMII and XFI if we don't have any labels we can just assume the PCS handle is for the right PCS. But for QSGMII the PCSs sit on another MAC's MDIO bus. So we need to tell the MAC where to find the PCS. This means we need to supply multiple PCSs to the MAC, and thus we need names to differentiate them. Since MAC9 doesn't have QSGMII, I suppose I never bothered to add names. --Sean [Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
On Tue, Feb 20, 2024 at 05:52:36PM -0500, Sean Anderson wrote: > With SGMII and XFI, the PCS sits on the MAC's MDIO bus. So for SGMII and > XFI if we don't have any labels we can just assume the PCS handle is for > the right PCS. But for QSGMII the PCSs sit on another MAC's MDIO bus. So > we need to tell the MAC where to find the PCS. This means we need to > supply multiple PCSs to the MAC So how did the other Layerscape devices with the same SerDes, PCS and mEMAC manage to get by and support QSGMII without listing all possible PCSes in pcs-handle-names? :-/ DPAA2 has the exact same situation with the QSGMII PCS situated on the internal bus of another DPMAC. It is unnecessary and buggy complexity, and it will only have to become worse when I add support for C73 backplane autoneg in lynx-pcs and the fman_memac driver, because I will need yet another PCS handle, this time not even one that represents a phy-mode in particular, but a PCS handle for C73 (with C73, the autoneg process determines the dynamic phy-mode).
On 2/20/24 18:06, Vladimir Oltean wrote: > On Tue, Feb 20, 2024 at 05:52:36PM -0500, Sean Anderson wrote: >> With SGMII and XFI, the PCS sits on the MAC's MDIO bus. So for SGMII and >> XFI if we don't have any labels we can just assume the PCS handle is for >> the right PCS. But for QSGMII the PCSs sit on another MAC's MDIO bus. So >> we need to tell the MAC where to find the PCS. This means we need to >> supply multiple PCSs to the MAC > > So how did the other Layerscape devices with the same SerDes, PCS and > mEMAC manage to get by and support QSGMII without listing all possible > PCSes in pcs-handle-names? :-/ DPAA2 has the exact same situation with > the QSGMII PCS situated on the internal bus of another DPMAC. I'm not familiar with them. With DPAA we used to just try to configure the QSGMII PCSs on every MAC's MDIO bus. This worked out since if you enabled all the MACs, the right one would eventually configure the PCSs. But it also meant you couldn't determine the link status (since you didn't know where your PCS was). > It is unnecessary and buggy complexity, and it will only have to become > worse when I add support for C73 backplane autoneg in lynx-pcs and the > fman_memac driver, because I will need yet another PCS handle, this time > not even one that represents a phy-mode in particular, but a PCS handle > for C73 (with C73, the autoneg process determines the dynamic phy-mode). There are multiple physical PCSs there must also be multiple PCS devices. Otherwise your software and hardware will get out of sync. If you don't want the complexity, then don't design hardware with multiple PCSs connected to the same MAC. --Sean [Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
On Tue, Feb 20, 2024 at 06:17:02PM -0500, Sean Anderson wrote: > On 2/20/24 18:06, Vladimir Oltean wrote: > > On Tue, Feb 20, 2024 at 05:52:36PM -0500, Sean Anderson wrote: > >> With SGMII and XFI, the PCS sits on the MAC's MDIO bus. So for SGMII and > >> XFI if we don't have any labels we can just assume the PCS handle is for > >> the right PCS. But for QSGMII the PCSs sit on another MAC's MDIO bus. So > >> we need to tell the MAC where to find the PCS. This means we need to > >> supply multiple PCSs to the MAC > > > > So how did the other Layerscape devices with the same SerDes, PCS and > > mEMAC manage to get by and support QSGMII without listing all possible > > PCSes in pcs-handle-names? :-/ DPAA2 has the exact same situation with > > the QSGMII PCS situated on the internal bus of another DPMAC. > > I'm not familiar with them. Take the LS1088A-RDB. It has 8 QSGMII ports (dpmac3-dpmac6 over one lane, dpmac7-dpmac10 over another lane). In fsl-ls1088a-rdb.dts, there are multiple references like this: - dpmac3, dpmac4, dpmac5, dpmac6 all have references to a PCS situated on dpmac3's MDIO bus. - dpmac7, dpmac8, dpmac9, dpmac10 all have references to a PCS situated on dpmac7's MDIO bus. No limitation there that I know of. It's not as if the pcs-handle has to be towards the internal MDIO bus of _that_ dpmac. > With DPAA we used to just try to configure the QSGMII PCSs on every > MAC's MDIO bus. This worked out since if you enabled all the MACs, the > right one would eventually configure the PCSs. But it also meant you > couldn't determine the link status (since you didn't know where your PCS > was). Yeah, the lack of link status reporting from the MAC's PCS is a true design limitation with the old fman_memac approach. But it's not either-or. It's not as if things can only be done like that or like you. See above. Each MAC can talk, through a single pcs-handle, to just its PCS, on whatever other bus it may be on. What is escaping me is the justification for _multiple_ pcs-handles. You accept that you need to have some components of the device tree defined by the board (phy-mode/phy-connection-type), but based on some philosophical notion, you maintain that all PCSes should be listed in the device tree, even if one board could not possibly make use of them all (and specifically not make use of those PCSes for which it cannot be hidden from software that they are distinct, see below). The argument doesn't make sense to me, sorry. > > It is unnecessary and buggy complexity, and it will only have to become > > worse when I add support for C73 backplane autoneg in lynx-pcs and the > > fman_memac driver, because I will need yet another PCS handle, this time > > not even one that represents a phy-mode in particular, but a PCS handle > > for C73 (with C73, the autoneg process determines the dynamic phy-mode). > > There are multiple physical PCSs there must also be multiple PCS > devices. Otherwise your software and hardware will get out of sync. "Must"? All the single-port-per-lane PCSes and all the multi-port-per-lane PCSes can be made to appear to software at the same internal MDIO address. So switching between the SGMII PCS and the 10GBASE-R PCS is never a problem, even if you don't know they're distinct. The only thing that's not possible with this simplification, and would thus justify pcs-handle-names, is switching between a single-port-per-lane PCS (like SGMII) and a multi-port-per-lane PCS (like QSGMII). But why would you want that? > If you don't want the complexity, then don't design hardware with multiple > PCSs connected to the same MAC. In case it wasn't clear, I was talking about unjustified software complexity where there were existing models to follow at the time of DPAA1's phylink conversion (i.e. DPAA2) which solved the same problems in a much simpler way. If by making these choices, you've solved design limitations that are unsolved with DPAA2, please let me know, I really want to know.
On Wed, Feb 21, 2024 at 01:45:26AM +0200, Vladimir Oltean wrote: > On Tue, Feb 20, 2024 at 06:17:02PM -0500, Sean Anderson wrote: > > On 2/20/24 18:06, Vladimir Oltean wrote: > > > So how did the other Layerscape devices with the same SerDes, PCS and > > > mEMAC manage to get by and support QSGMII without listing all possible > > > PCSes in pcs-handle-names? :-/ DPAA2 has the exact same situation with > > > the QSGMII PCS situated on the internal bus of another DPMAC. > > > > I'm not familiar with them. > > Take the LS1088A-RDB. Ah, wait a minute, why am I explaining the LS1088A-RDB to you? You've submitted patches on its SerDes/PCS integration, you even said you tested the QSGMII ports: https://lore.kernel.org/linux-arm-kernel/20230321201313.2507539-14-sean.anderson@seco.com/
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@concurrent-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@concurrent-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@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. > 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@nxp.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@f0000 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@nxp.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.
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 */