| Message ID | 20240116204103.29318-2-twoerner@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] arm64: dts: rockchip: rock-pi-e: adjust phy-handle name | expand |
On Wed, Jan 17, 2024 at 4:41 AM Trevor Woerner <twoerner@gmail.com> wrote: > > A number of snps (Synopsys) properties are not in their correct location. Nope. If you read the snps,dwmac.yaml binding file, you'll see that these properties have been deprecated. They are properties pertaining to the PHY and should be described under the PHY node. Support for reset GPIOs on PHY devices in phylib has been there since v4.16. The snps prefixed properties were deprecated in v5.3. ChenYu > Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E") > Signed-off-by: Trevor Woerner <twoerner@gmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > index 096cfa19036e..0739b8fec86e 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > @@ -150,8 +150,11 @@ &gmac2io { > phy-mode = "rgmii"; > phy-supply = <&vcc_io>; > pinctrl-names = "default"; > - pinctrl-0 = <&rgmiim1_pins>; > + pinctrl-0 = <&rgmiim1_pins>, <ð_phy_reset_pin>; > snps,aal; > + snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; > + snps,reset-active-low; > + snps,reset-delays-us = <0 10000 50000>; > snps,rxpbl = <0x4>; > snps,txpbl = <0x4>; > tx_delay = <0x26>; > @@ -165,13 +168,10 @@ mdio { > > rtl8211: ethernet-phy@1 { > reg = <1>; > - pinctrl-0 = <ð_phy_int_pin>, <ð_phy_reset_pin>; > + pinctrl-0 = <ð_phy_int_pin>; > pinctrl-names = "default"; > interrupt-parent = <&gpio1>; > interrupts = <24 IRQ_TYPE_LEVEL_LOW>; > - reset-assert-us = <10000>; > - reset-deassert-us = <50000>; > - reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; > }; > }; > }; > -- > 2.43.0.76.g1a87c842ece3 >
On Wed 2024-01-17 @ 12:38:39 PM, Chen-Yu Tsai wrote: > On Wed, Jan 17, 2024 at 4:41 AM Trevor Woerner <twoerner@gmail.com> wrote: > > > > A number of snps (Synopsys) properties are not in their correct location. > > Nope. If you read the snps,dwmac.yaml binding file, you'll see that these > properties have been deprecated. They are properties pertaining to the PHY > and should be described under the PHY node. Support for reset GPIOs on PHY > devices in phylib has been there since v4.16. The snps prefixed properties > were deprecated in v5.3. If that's the case, then the bindings and the drivers are out of sync in both U-Boot and Linux. I discovered this issue while working with older and newer revisions of the rock-pi-e board. The first three spins of the rock-pi-e have the rtl8211e PHY but the last one (v1.21) has the rtl8211f PHY. With the existing layout nothing works in U-Boot and in Linux the rtl8211e works but not the rtl8211f. With this patch both the rtl8211e and the rtl8211f PHYs work using the exact same device trees on both older and newer rock-pi-e boards in both U-Boot and Linux. Comparing the rock-pi-e's dts file with the one from the roc-rk3328-cc board, which also uses the layout in this patch, and which also uses the rtl8211e external PHY, is what led me in this direction. > > ChenYu > > > Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E") > > Signed-off-by: Trevor Woerner <twoerner@gmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > > index 096cfa19036e..0739b8fec86e 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > > @@ -150,8 +150,11 @@ &gmac2io { > > phy-mode = "rgmii"; > > phy-supply = <&vcc_io>; > > pinctrl-names = "default"; > > - pinctrl-0 = <&rgmiim1_pins>; > > + pinctrl-0 = <&rgmiim1_pins>, <ð_phy_reset_pin>; > > snps,aal; > > + snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; > > + snps,reset-active-low; > > + snps,reset-delays-us = <0 10000 50000>; > > snps,rxpbl = <0x4>; > > snps,txpbl = <0x4>; > > tx_delay = <0x26>; > > @@ -165,13 +168,10 @@ mdio { > > > > rtl8211: ethernet-phy@1 { > > reg = <1>; > > - pinctrl-0 = <ð_phy_int_pin>, <ð_phy_reset_pin>; > > + pinctrl-0 = <ð_phy_int_pin>; > > pinctrl-names = "default"; > > interrupt-parent = <&gpio1>; > > interrupts = <24 IRQ_TYPE_LEVEL_LOW>; > > - reset-assert-us = <10000>; > > - reset-deassert-us = <50000>; > > - reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; > > }; > > }; > > }; > > -- > > 2.43.0.76.g1a87c842ece3 > >
On 2024-01-17 06:47, Trevor Woerner wrote: > On Wed 2024-01-17 @ 12:38:39 PM, Chen-Yu Tsai wrote: >> On Wed, Jan 17, 2024 at 4:41 AM Trevor Woerner <twoerner@gmail.com> wrote: >>> >>> A number of snps (Synopsys) properties are not in their correct location. >> >> Nope. If you read the snps,dwmac.yaml binding file, you'll see that these >> properties have been deprecated. They are properties pertaining to the PHY >> and should be described under the PHY node. Support for reset GPIOs on PHY >> devices in phylib has been there since v4.16. The snps prefixed properties >> were deprecated in v5.3. > > If that's the case, then the bindings and the drivers are out of sync in both > U-Boot and Linux. I discovered this issue while working with older and newer > revisions of the rock-pi-e board. The first three spins of the rock-pi-e have > the rtl8211e PHY but the last one (v1.21) has the rtl8211f PHY. > > With the existing layout nothing works in U-Boot and in Linux the rtl8211e > works but not the rtl8211f. With this patch both the rtl8211e and the rtl8211f > PHYs work using the exact same device trees on both older and newer rock-pi-e > boards in both U-Boot and Linux. For linux this is probably related to the same chicken-and-egg reset issue outlined at [1]. The phy is not reset before it is probed and cannot be probed because it is not reset. As for U-Boot the designware/gmac_rockchip ethernet driver may need some adjustments to properly integrate with eth-phy uclass to properly reset the phy described in a ethernet-phy node before it can be probed. Following config options is disabled for rock-pi-e-rk3328 defconfig: # CONFIG_DM_ETH_PHY is not set # CONFIG_PHY_REALTEK is not set Also the driver needs to be updated to make use eth-phy uclass for it to work correctly. More similar to how the dwc_eth_qos driver works related to ethernet phy. [1] https://lore.kernel.org/linux-rockchip/47d55aca-bee6-810f-379f-9431649fefa6@kwiboo.se/ Regards, Jonas > > Comparing the rock-pi-e's dts file with the one from the roc-rk3328-cc board, > which also uses the layout in this patch, and which also uses the rtl8211e > external PHY, is what led me in this direction. > >> >> ChenYu >> >>> Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E") >>> Signed-off-by: Trevor Woerner <twoerner@gmail.com> >>> --- >>> arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts >>> index 096cfa19036e..0739b8fec86e 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts >>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts >>> @@ -150,8 +150,11 @@ &gmac2io { >>> phy-mode = "rgmii"; >>> phy-supply = <&vcc_io>; >>> pinctrl-names = "default"; >>> - pinctrl-0 = <&rgmiim1_pins>; >>> + pinctrl-0 = <&rgmiim1_pins>, <ð_phy_reset_pin>; >>> snps,aal; >>> + snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; >>> + snps,reset-active-low; >>> + snps,reset-delays-us = <0 10000 50000>; >>> snps,rxpbl = <0x4>; >>> snps,txpbl = <0x4>; >>> tx_delay = <0x26>; >>> @@ -165,13 +168,10 @@ mdio { >>> >>> rtl8211: ethernet-phy@1 { >>> reg = <1>; >>> - pinctrl-0 = <ð_phy_int_pin>, <ð_phy_reset_pin>; >>> + pinctrl-0 = <ð_phy_int_pin>; >>> pinctrl-names = "default"; >>> interrupt-parent = <&gpio1>; >>> interrupts = <24 IRQ_TYPE_LEVEL_LOW>; >>> - reset-assert-us = <10000>; >>> - reset-deassert-us = <50000>; >>> - reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; >>> }; >>> }; >>> }; >>> -- >>> 2.43.0.76.g1a87c842ece3 >>> > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On 2024-01-17 09:15, Jonas Karlman wrote: > On 2024-01-17 06:47, Trevor Woerner wrote: >> On Wed 2024-01-17 @ 12:38:39 PM, Chen-Yu Tsai wrote: >>> On Wed, Jan 17, 2024 at 4:41 AM Trevor Woerner <twoerner@gmail.com> wrote: >>>> >>>> A number of snps (Synopsys) properties are not in their correct location. >>> >>> Nope. If you read the snps,dwmac.yaml binding file, you'll see that these >>> properties have been deprecated. They are properties pertaining to the PHY >>> and should be described under the PHY node. Support for reset GPIOs on PHY >>> devices in phylib has been there since v4.16. The snps prefixed properties >>> were deprecated in v5.3. >> >> If that's the case, then the bindings and the drivers are out of sync in both >> U-Boot and Linux. I discovered this issue while working with older and newer >> revisions of the rock-pi-e board. The first three spins of the rock-pi-e have >> the rtl8211e PHY but the last one (v1.21) has the rtl8211f PHY. >> >> With the existing layout nothing works in U-Boot and in Linux the rtl8211e >> works but not the rtl8211f. With this patch both the rtl8211e and the rtl8211f >> PHYs work using the exact same device trees on both older and newer rock-pi-e >> boards in both U-Boot and Linux. > > For linux this is probably related to the same chicken-and-egg reset > issue outlined at [1]. The phy is not reset before it is probed and > cannot be probed because it is not reset. > > As for U-Boot the designware/gmac_rockchip ethernet driver may need some > adjustments to properly integrate with eth-phy uclass to properly reset > the phy described in a ethernet-phy node before it can be probed. > > Following config options is disabled for rock-pi-e-rk3328 defconfig: > > # CONFIG_DM_ETH_PHY is not set > # CONFIG_PHY_REALTEK is not set > > Also the driver needs to be updated to make use eth-phy uclass for it to > work correctly. More similar to how the dwc_eth_qos driver works related > to ethernet phy. I have just sent out a U-Boot series that fix ethernet on the v1.21 revision of the ROCK Pi E board, see [2]. Would expect that ethernet also starts to work in linux with those patches applied to U-Boot. I only tested and verified ethernet in U-Boot. Hopefully someone can take a closer look at the issue on linux side so that it does not need to depend on PHY being reset by the bootloader. [2] https://lore.kernel.org/u-boot/20240118071949.927089-1-jonas@kwiboo.se/T/ Regards, Jonas > > [1] https://lore.kernel.org/linux-rockchip/47d55aca-bee6-810f-379f-9431649fefa6@kwiboo.se/ > > Regards, > Jonas > >> >> Comparing the rock-pi-e's dts file with the one from the roc-rk3328-cc board, >> which also uses the layout in this patch, and which also uses the rtl8211e >> external PHY, is what led me in this direction. >> >>> >>> ChenYu >>> >>>> Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E") >>>> Signed-off-by: Trevor Woerner <twoerner@gmail.com> >>>> --- >>>> arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++----- >>>> 1 file changed, 5 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts >>>> index 096cfa19036e..0739b8fec86e 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts >>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts >>>> @@ -150,8 +150,11 @@ &gmac2io { >>>> phy-mode = "rgmii"; >>>> phy-supply = <&vcc_io>; >>>> pinctrl-names = "default"; >>>> - pinctrl-0 = <&rgmiim1_pins>; >>>> + pinctrl-0 = <&rgmiim1_pins>, <ð_phy_reset_pin>; >>>> snps,aal; >>>> + snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; >>>> + snps,reset-active-low; >>>> + snps,reset-delays-us = <0 10000 50000>; >>>> snps,rxpbl = <0x4>; >>>> snps,txpbl = <0x4>; >>>> tx_delay = <0x26>; >>>> @@ -165,13 +168,10 @@ mdio { >>>> >>>> rtl8211: ethernet-phy@1 { >>>> reg = <1>; >>>> - pinctrl-0 = <ð_phy_int_pin>, <ð_phy_reset_pin>; >>>> + pinctrl-0 = <ð_phy_int_pin>; >>>> pinctrl-names = "default"; >>>> interrupt-parent = <&gpio1>; >>>> interrupts = <24 IRQ_TYPE_LEVEL_LOW>; >>>> - reset-assert-us = <10000>; >>>> - reset-deassert-us = <50000>; >>>> - reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; >>>> }; >>>> }; >>>> }; >>>> -- >>>> 2.43.0.76.g1a87c842ece3 >>>>
On Thu 2024-01-18 @ 08:31:30 AM, Jonas Karlman wrote: > On 2024-01-17 09:15, Jonas Karlman wrote: > > On 2024-01-17 06:47, Trevor Woerner wrote: > >> On Wed 2024-01-17 @ 12:38:39 PM, Chen-Yu Tsai wrote: > >>> On Wed, Jan 17, 2024 at 4:41 AM Trevor Woerner <twoerner@gmail.com> wrote: > >>>> > >>>> A number of snps (Synopsys) properties are not in their correct location. > >>> > >>> Nope. If you read the snps,dwmac.yaml binding file, you'll see that these > >>> properties have been deprecated. They are properties pertaining to the PHY > >>> and should be described under the PHY node. Support for reset GPIOs on PHY > >>> devices in phylib has been there since v4.16. The snps prefixed properties > >>> were deprecated in v5.3. > >> > >> If that's the case, then the bindings and the drivers are out of sync in both > >> U-Boot and Linux. I discovered this issue while working with older and newer > >> revisions of the rock-pi-e board. The first three spins of the rock-pi-e have > >> the rtl8211e PHY but the last one (v1.21) has the rtl8211f PHY. > >> > >> With the existing layout nothing works in U-Boot and in Linux the rtl8211e > >> works but not the rtl8211f. With this patch both the rtl8211e and the rtl8211f > >> PHYs work using the exact same device trees on both older and newer rock-pi-e > >> boards in both U-Boot and Linux. > > > > For linux this is probably related to the same chicken-and-egg reset > > issue outlined at [1]. The phy is not reset before it is probed and > > cannot be probed because it is not reset. > > > > As for U-Boot the designware/gmac_rockchip ethernet driver may need some > > adjustments to properly integrate with eth-phy uclass to properly reset > > the phy described in a ethernet-phy node before it can be probed. > > > > Following config options is disabled for rock-pi-e-rk3328 defconfig: > > > > # CONFIG_DM_ETH_PHY is not set > > # CONFIG_PHY_REALTEK is not set > > > > Also the driver needs to be updated to make use eth-phy uclass for it to > > work correctly. More similar to how the dwc_eth_qos driver works related > > to ethernet phy. > > I have just sent out a U-Boot series that fix ethernet on the v1.21 > revision of the ROCK Pi E board, see [2]. Thank you! I have tested your patches both in U-Boot and Linux and they work perfectly. > Would expect that ethernet also starts to work in linux with those > patches applied to U-Boot. I only tested and verified ethernet in U-Boot. > > Hopefully someone can take a closer look at the issue on linux side > so that it does not need to depend on PHY being reset by the bootloader. Yes, as you suspected this is exactly what happened: providing a fix in U-Boot caused everything to work again in Linux. It would appear as though Linux is expecting the PHY to be reset/setup/configured by the bootloader in the case of this board/SoC/PHY. > [2] https://lore.kernel.org/u-boot/20240118071949.927089-1-jonas@kwiboo.se/T/ > > Regards, > Jonas > > > > > [1] https://lore.kernel.org/linux-rockchip/47d55aca-bee6-810f-379f-9431649fefa6@kwiboo.se/ > > > > Regards, > > Jonas > > > >> > >> Comparing the rock-pi-e's dts file with the one from the roc-rk3328-cc board, > >> which also uses the layout in this patch, and which also uses the rtl8211e > >> external PHY, is what led me in this direction. > >> > >>> > >>> ChenYu > >>> > >>>> Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E") > >>>> Signed-off-by: Trevor Woerner <twoerner@gmail.com> > >>>> --- > >>>> arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++----- > >>>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > >>>> index 096cfa19036e..0739b8fec86e 100644 > >>>> --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > >>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts > >>>> @@ -150,8 +150,11 @@ &gmac2io { > >>>> phy-mode = "rgmii"; > >>>> phy-supply = <&vcc_io>; > >>>> pinctrl-names = "default"; > >>>> - pinctrl-0 = <&rgmiim1_pins>; > >>>> + pinctrl-0 = <&rgmiim1_pins>, <ð_phy_reset_pin>; > >>>> snps,aal; > >>>> + snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; > >>>> + snps,reset-active-low; > >>>> + snps,reset-delays-us = <0 10000 50000>; > >>>> snps,rxpbl = <0x4>; > >>>> snps,txpbl = <0x4>; > >>>> tx_delay = <0x26>; > >>>> @@ -165,13 +168,10 @@ mdio { > >>>> > >>>> rtl8211: ethernet-phy@1 { > >>>> reg = <1>; > >>>> - pinctrl-0 = <ð_phy_int_pin>, <ð_phy_reset_pin>; > >>>> + pinctrl-0 = <ð_phy_int_pin>; > >>>> pinctrl-names = "default"; > >>>> interrupt-parent = <&gpio1>; > >>>> interrupts = <24 IRQ_TYPE_LEVEL_LOW>; > >>>> - reset-assert-us = <10000>; > >>>> - reset-deassert-us = <50000>; > >>>> - reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; > >>>> }; > >>>> }; > >>>> }; > >>>> -- > >>>> 2.43.0.76.g1a87c842ece3 > >>>> >
On Thu 2024-01-25 @ 09:12:46 PM, Heiko Stuebner wrote: > Hi, > > Am Freitag, 19. Januar 2024, 00:03:12 CET schrieb Trevor Woerner: > > On Thu 2024-01-18 @ 08:31:30 AM, Jonas Karlman wrote: > > > On 2024-01-17 09:15, Jonas Karlman wrote: > > > I have just sent out a U-Boot series that fix ethernet on the v1.21 > > > revision of the ROCK Pi E board, see [2]. > > > > Thank you! I have tested your patches both in U-Boot and Linux and they work > > perfectly. > > if I'm reading this correctly, this patch is not needed anymore, right? True, patch 2/2 is not needed (or correct). > I'm not yet sure about the first patch, as it really is > just a cosmetic ;-) I wouldn't say it's entirely cosmetic. Some rock-pi-e boards come with the 8211e PHY and others come with the 8211f PHY. Labelling the phandle as "rtl8211e" would be misleading/confusing for people who had boards whose boot logs would report otherwise.
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts index 096cfa19036e..0739b8fec86e 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts @@ -150,8 +150,11 @@ &gmac2io { phy-mode = "rgmii"; phy-supply = <&vcc_io>; pinctrl-names = "default"; - pinctrl-0 = <&rgmiim1_pins>; + pinctrl-0 = <&rgmiim1_pins>, <ð_phy_reset_pin>; snps,aal; + snps,reset-gpio = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; + snps,reset-active-low; + snps,reset-delays-us = <0 10000 50000>; snps,rxpbl = <0x4>; snps,txpbl = <0x4>; tx_delay = <0x26>; @@ -165,13 +168,10 @@ mdio { rtl8211: ethernet-phy@1 { reg = <1>; - pinctrl-0 = <ð_phy_int_pin>, <ð_phy_reset_pin>; + pinctrl-0 = <ð_phy_int_pin>; pinctrl-names = "default"; interrupt-parent = <&gpio1>; interrupts = <24 IRQ_TYPE_LEVEL_LOW>; - reset-assert-us = <10000>; - reset-deassert-us = <50000>; - reset-gpios = <&gpio1 RK_PC2 GPIO_ACTIVE_LOW>; }; }; };
A number of snps (Synopsys) properties are not in their correct location. Fixes: b918e81f2145 ("arm64: dts: rockchip: rk3328: Add Radxa ROCK Pi E") Signed-off-by: Trevor Woerner <twoerner@gmail.com> --- arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)