From: Zachary Goldstein <[email protected]>
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 <[email protected]>
---
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
Hi Zachary,
On Wed, Feb 14, 2024 at 05:21:54PM -0500, Zachary Goldstein via B4 Relay wrote:
> From: Zachary Goldstein <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> 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 <[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
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
> [email protected]
> 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/[email protected]/
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/[email protected]/
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> 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 <[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
>
> 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.