RE: [PATCH] arm64: dts: ls1046ardb: Set aqr106 phy mode to usxgmii

-----Original Message----- From: Sascha Hauer s.hauer@pengutronix.de Sent: Thursday, April 23, 2020 1:22 PM To: linux-arm-kernel@lists.infradead.org Cc: Madalin Bucur madalin.bucur@nxp.com; Shawn Guo shawnguo@kernel.org; Leo Li leoyang.li@nxp.com; Sascha Hauer s.hauer@pengutronix.de Subject: [PATCH] arm64: dts: ls1046ardb: Set aqr106 phy mode to usxgmii
The AQR107 family of phy devices do not support xgmii, but usxgmii instead. Since ce64c1f77a9d ("net: phy: aquantia: add USXGMII support and warn if XGMII mode is set") the kernel warns about xgmii being used. Change device tree to usxgmii.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
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 d53ccc56bb63..02fbef92b96a 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts @@ -151,7 +151,7 @@ ethernet@ea000 {
ethernet@f0000 { /* 10GEC1 */ phy-handle = <&aqr106_phy>;
phy-connection-type = "xgmii";
phy-connection-type = "usxgmii";
};
ethernet@f2000 { /* 10GEC2 */
-- 2.26.1
Hi Sascha,
thank you for trying to correct this problem. Unfortunately "usxgmii" here is incorrect too, as that mode is not supported by the LS1046A SoC. The connection mode used, as documented by the SoC and PHY datasheets, is XFI. Unfortunately there was resistance against including this connection type in the list supported by the kernel (please note the distinction between connection type and connection mode). At a certain moment the two were aliased and the kernel uses connection mode, not connection type. While we should describe here the hardware, the board connection type (XFI), in the kernel the connection mode was lately preferred (10G-BaseR). So, today we cannot use "xfi" here, as the hardware description property should read. The closest thing we can use is "10gbase-r". Unfortunately, in u-boot support for "xfi" is already in place [1] and the device tree should be different for the two for this reason - this goes against the spirit of the device tree that should not depend on the software using it...
I had on my agenda to fix this problem, had to stop when "xfi" was rejected, at the time not even "10gbase-r" was an option. Also worth noting here is that, while we change "xgmii" to a correct/better value, we should also tolerate the old variant, as there are users in the wild unable/unwilling to update the device tree and backwards compatibility should be ensured, further complicating the matter.
Regards, Madalin
[1] I mention u-boot here because it's the default boot loader used by LS1046ARDB and there are some interdependences with it, making things even more complicated than they seem: u-boot currently performs a fix-up for this device tree field, based on RCW (reset configuration word), resulting in an override of the value provided to the booting kernel. Some relevant u-boot commits:
https://github.com/u-boot/u-boot/commit/17285fc2833e0db04a2bd3d411cdf1a3e387... https://github.com/u-boot/u-boot/commit/8a141d6e9cc1841082e4c996703eafb037ec...

Hi Madalin,
On Thu, Apr 23, 2020 at 12:59:16PM +0000, Madalin Bucur wrote:
-----Original Message----- From: Sascha Hauer s.hauer@pengutronix.de Sent: Thursday, April 23, 2020 1:22 PM To: linux-arm-kernel@lists.infradead.org Cc: Madalin Bucur madalin.bucur@nxp.com; Shawn Guo shawnguo@kernel.org; Leo Li leoyang.li@nxp.com; Sascha Hauer s.hauer@pengutronix.de Subject: [PATCH] arm64: dts: ls1046ardb: Set aqr106 phy mode to usxgmii
The AQR107 family of phy devices do not support xgmii, but usxgmii instead. Since ce64c1f77a9d ("net: phy: aquantia: add USXGMII support and warn if XGMII mode is set") the kernel warns about xgmii being used. Change device tree to usxgmii.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
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 d53ccc56bb63..02fbef92b96a 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a-rdb.dts @@ -151,7 +151,7 @@ ethernet@ea000 {
ethernet@f0000 { /* 10GEC1 */ phy-handle = <&aqr106_phy>;
phy-connection-type = "xgmii";
phy-connection-type = "usxgmii";
};
ethernet@f2000 { /* 10GEC2 */
-- 2.26.1
Hi Sascha,
thank you for trying to correct this problem. Unfortunately "usxgmii" here is incorrect too, as that mode is not supported by the LS1046A SoC. The connection mode used, as documented by the SoC and PHY datasheets, is XFI. Unfortunately there was resistance against including this connection type in the list supported by the kernel (please note the distinction between connection type and connection mode). At a certain moment the two were aliased and the kernel uses connection mode, not connection type. While we should describe here the hardware, the board connection type (XFI), in the kernel the connection mode was lately preferred (10G-BaseR). So, today we cannot use "xfi" here, as the hardware description property should read. The closest thing we can use is "10gbase-r". Unfortunately, in u-boot support for "xfi" is already in place [1] and the device tree should be different for the two for this reason - this goes against the spirit of the device tree that should not depend on the software using it...
I had on my agenda to fix this problem, had to stop when "xfi" was rejected, at the time not even "10gbase-r" was an option. Also worth noting here is that, while we change "xgmii" to a correct/better value, we should also tolerate the old variant, as there are users in the wild unable/unwilling to update the device tree and backwards compatibility should be ensured, further complicating the matter.
Thanks for the explanation. It looked like one of those simple patch opportunities at first, apparently it isn't.
Sascha
participants (2)
-
Madalin Bucur
-
Sascha Hauer