| Message ID | 20240220223442.1275946-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 734f06db599f66d6a159c78abfdbadfea3b7d43b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dpaa: fman_memac: accept phy-interface-type = "10gbase-r" in the device tree | expand |
On 2/20/24 17:34, Vladimir Oltean wrote: > Since commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"), we support > the "10gbase-r" phy-mode through a driver-based conversion of "xgmii", > but we still don't actually support it when the device tree specifies > "10gbase-r" proper. > > This is because boards such as LS1046A-RDB do not define pcs-handle-names > (for whatever reason) in the ethernet@f0000 device tree node, and the > code enters through this code path: > > err = of_property_match_string(mac_node, "pcs-handle-names", "xfi"); > // code takes neither branch and falls through > if (err >= 0) { > (...) > } else if (err != -EINVAL && err != -ENODATA) { > goto _return_fm_mac_free; > } > > (...) > > /* For compatibility, if pcs-handle-names is missing, we assume this > * phy is the first one in pcsphy-handle > */ > err = of_property_match_string(mac_node, "pcs-handle-names", "sgmii"); > if (err == -EINVAL || err == -ENODATA) > pcs = memac_pcs_create(mac_node, 0); // code takes this branch > else if (err < 0) > goto _return_fm_mac_free; > else > pcs = memac_pcs_create(mac_node, err); > > // A default PCS is created and saved in "pcs" > > // This determination fails and mistakenly saves the default PCS > // memac->sgmii_pcs instead of memac->xfi_pcs, because at this > // stage, mac_dev->phy_if == PHY_INTERFACE_MODE_10GBASER. > if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII) > memac->xfi_pcs = pcs; > else > memac->sgmii_pcs = pcs; > > In other words, in the absence of pcs-handle-names, the default > xfi_pcs assignment logic only works when in the device tree we have > PHY_INTERFACE_MODE_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. > > Currently, it is impossible to make the s/xgmii/10gbase-r/ device tree > conversion, because it would break forward compatibility (new device > tree with old kernel). The only way to modify existing device trees to > phy-interface-mode = "10gbase-r" is to fix stable kernels to accept this > value and handle it properly. > > One reason why the conversion is desirable is because with pre-phylink > kernels, the Aquantia PHY driver used to warn about the improper use > of PHY_INTERFACE_MODE_XGMII [1]. It is best to have a single (latest) > device tree that works with all supported stable kernel versions. > > Note that the blamed commit does not constitute a regression per se. > Older stable kernels like 6.1 still do not work with "10gbase-r", but > for a different reason. That is a battle for another time. > > [1] https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2fnetdev%2f20240214%2dls1046%2ddts%2duse%2d10gbase%2dr%2dv1%2d1%2d8c2d68547393%40concurrent%2drt.com%2f&umid=9e7ee942-44ce-4bdd-a3c5-0de04f0ee53f&auth=d807158c60b7d2502abde8a2fc01f40662980862-d5a8b704cd04b796a2aa2a861dad36f29073bce5 > > Fixes: 5d93cfcf7360 ("net: dpaa: Convert to phylink") > 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 Reviewed-by: Sean Anderson <sean.anderson@seco.com> [Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: Wednesday, February 21, 2024 12:35 AM > To: netdev@vger.kernel.org > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Madalin Bucur <madalin.bucur@nxp.com>; Sean > Anderson <sean.anderson@seco.com>; Russell King <linux@armlinux.org.uk>; > Zachary Goldstein <zachary.goldstein@concurrent-rt.com>; linux- > kernel@vger.kernel.org > Subject: [PATCH net] net: dpaa: fman_memac: accept phy-interface-type = > "10gbase-r" in the device tree > > Since commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"), we support > the "10gbase-r" phy-mode through a driver-based conversion of "xgmii", > but we still don't actually support it when the device tree specifies > "10gbase-r" proper. > > This is because boards such as LS1046A-RDB do not define pcs-handle-names > (for whatever reason) in the ethernet@f0000 device tree node, and the > code enters through this code path: > > err = of_property_match_string(mac_node, "pcs-handle-names", "xfi"); > // code takes neither branch and falls through > if (err >= 0) { > (...) > } else if (err != -EINVAL && err != -ENODATA) { > goto _return_fm_mac_free; > } > > (...) > > /* For compatibility, if pcs-handle-names is missing, we assume this > * phy is the first one in pcsphy-handle > */ > err = of_property_match_string(mac_node, "pcs-handle-names", > "sgmii"); > if (err == -EINVAL || err == -ENODATA) > pcs = memac_pcs_create(mac_node, 0); // code takes this > branch > else if (err < 0) > goto _return_fm_mac_free; > else > pcs = memac_pcs_create(mac_node, err); > > // A default PCS is created and saved in "pcs" > > // This determination fails and mistakenly saves the default PCS > // memac->sgmii_pcs instead of memac->xfi_pcs, because at this > // stage, mac_dev->phy_if == PHY_INTERFACE_MODE_10GBASER. > if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII) > memac->xfi_pcs = pcs; > else > memac->sgmii_pcs = pcs; > > In other words, in the absence of pcs-handle-names, the default > xfi_pcs assignment logic only works when in the device tree we have > PHY_INTERFACE_MODE_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. > > Currently, it is impossible to make the s/xgmii/10gbase-r/ device tree > conversion, because it would break forward compatibility (new device > tree with old kernel). The only way to modify existing device trees to > phy-interface-mode = "10gbase-r" is to fix stable kernels to accept this > value and handle it properly. > > One reason why the conversion is desirable is because with pre-phylink > kernels, the Aquantia PHY driver used to warn about the improper use > of PHY_INTERFACE_MODE_XGMII [1]. It is best to have a single (latest) > device tree that works with all supported stable kernel versions. > > Note that the blamed commit does not constitute a regression per se. > Older stable kernels like 6.1 still do not work with "10gbase-r", but > for a different reason. That is a battle for another time. > > [1] https://lore.kernel.org/netdev/20240214-ls1046-dts-use-10gbase-r-v1-1- > 8c2d68547393@concurrent-rt.com/ > > Fixes: 5d93cfcf7360 ("net: dpaa: Convert to phylink") > 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 > -- > 2.34.1 Thank you for the detailed description of the situation. Acked-by: Madalin Bucur <madalin.bucur@oss.nxp.com>
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Wed, 21 Feb 2024 00:34:42 +0200 you wrote: > Since commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"), we support > the "10gbase-r" phy-mode through a driver-based conversion of "xgmii", > but we still don't actually support it when the device tree specifies > "10gbase-r" proper. > > This is because boards such as LS1046A-RDB do not define pcs-handle-names > (for whatever reason) in the ethernet@f0000 device tree node, and the > code enters through this code path: > > [...] Here is the summary with links: - [net] net: dpaa: fman_memac: accept phy-interface-type = "10gbase-r" in the device tree https://git.kernel.org/netdev/net/c/734f06db599f You are awesome, thank you!
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
Since commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"), we support the "10gbase-r" phy-mode through a driver-based conversion of "xgmii", but we still don't actually support it when the device tree specifies "10gbase-r" proper. This is because boards such as LS1046A-RDB do not define pcs-handle-names (for whatever reason) in the ethernet@f0000 device tree node, and the code enters through this code path: err = of_property_match_string(mac_node, "pcs-handle-names", "xfi"); // code takes neither branch and falls through if (err >= 0) { (...) } else if (err != -EINVAL && err != -ENODATA) { goto _return_fm_mac_free; } (...) /* For compatibility, if pcs-handle-names is missing, we assume this * phy is the first one in pcsphy-handle */ err = of_property_match_string(mac_node, "pcs-handle-names", "sgmii"); if (err == -EINVAL || err == -ENODATA) pcs = memac_pcs_create(mac_node, 0); // code takes this branch else if (err < 0) goto _return_fm_mac_free; else pcs = memac_pcs_create(mac_node, err); // A default PCS is created and saved in "pcs" // This determination fails and mistakenly saves the default PCS // memac->sgmii_pcs instead of memac->xfi_pcs, because at this // stage, mac_dev->phy_if == PHY_INTERFACE_MODE_10GBASER. if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII) memac->xfi_pcs = pcs; else memac->sgmii_pcs = pcs; In other words, in the absence of pcs-handle-names, the default xfi_pcs assignment logic only works when in the device tree we have PHY_INTERFACE_MODE_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. Currently, it is impossible to make the s/xgmii/10gbase-r/ device tree conversion, because it would break forward compatibility (new device tree with old kernel). The only way to modify existing device trees to phy-interface-mode = "10gbase-r" is to fix stable kernels to accept this value and handle it properly. One reason why the conversion is desirable is because with pre-phylink kernels, the Aquantia PHY driver used to warn about the improper use of PHY_INTERFACE_MODE_XGMII [1]. It is best to have a single (latest) device tree that works with all supported stable kernel versions. Note that the blamed commit does not constitute a regression per se. Older stable kernels like 6.1 still do not work with "10gbase-r", but for a different reason. That is a battle for another time. [1] https://lore.kernel.org/netdev/20240214-ls1046-dts-use-10gbase-r-v1-1-8c2d68547393@concurrent-rt.com/ Fixes: 5d93cfcf7360 ("net: dpaa: Convert to phylink") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- .../net/ethernet/freescale/fman/fman_memac.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)