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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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 <[email protected]>
[Embedded World 2024, SECO SpA]<https://www.messe-ticket.de/Nuernberg/embeddedworld2024/Register/ew24517689>
> -----Original Message-----
> From: Vladimir Oltean <[email protected]>
> Sent: Wednesday, February 21, 2024 12:35 AM
> To: [email protected]
> Cc: David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni
> <[email protected]>; Madalin Bucur <[email protected]>; Sean
> Anderson <[email protected]>; Russell King <[email protected]>;
> Zachary Goldstein <[email protected]>; linux-
> [email protected]
> 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-
> [email protected]/
>
> Fixes: 5d93cfcf7360 ("net: dpaa: Convert to phylink")
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> .../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 <[email protected]>
Hello:
This patch was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html