| Message ID | 20230315160228.2362-5-cnsztl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rockchip: improve support for NanoPi R5 series | expand |
On Wed, Mar 15, 2023 at 9:02 AM Tianling Shen <cnsztl@gmail.com> wrote: > > - Changed phy-mode to rgmii. > > - Fixed pull type in pinctrl for gmac0. > > - Removed duplicate properties in mdio node. > These properties are defined in the gmac0 node already. > > Signed-off-by: Tianling Shen <cnsztl@gmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > index e9adf5e66529..2a1118f15c29 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > @@ -57,7 +57,7 @@ > assigned-clock-rates = <0>, <125000000>; > clock_in_out = "output"; > phy-handle = <&rgmii_phy0>; > - phy-mode = "rgmii-id"; > + phy-mode = "rgmii"; > pinctrl-names = "default"; > pinctrl-0 = <&gmac0_miim > &gmac0_tx_bus2 > @@ -79,9 +79,6 @@ > reg = <1>; > pinctrl-0 = <ð_phy0_reset_pin>; > pinctrl-names = "default"; > - reset-assert-us = <10000>; > - reset-deassert-us = <50000>; > - reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>; Hmm, I don't see RK_PC4 being used anywhere else. gmac0 has RK_PC5 as snsp,reset-gpio. So it essentially drops reset for the PHY. Is it expected? > }; > }; > > @@ -115,7 +112,7 @@ > &pinctrl { > gmac0 { > eth_phy0_reset_pin: eth-phy0-reset-pin { > - rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_down>; > + rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_up>; > }; > }; > > -- > 2.17.1 >
Hi Vasily, On Thu, Mar 16, 2023 at 8:16 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote: > > On Wed, Mar 15, 2023 at 9:02 AM Tianling Shen <cnsztl@gmail.com> wrote: > > > > - Changed phy-mode to rgmii. > > > > - Fixed pull type in pinctrl for gmac0. > > > > - Removed duplicate properties in mdio node. > > These properties are defined in the gmac0 node already. > > > > Signed-off-by: Tianling Shen <cnsztl@gmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > > index e9adf5e66529..2a1118f15c29 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > > @@ -57,7 +57,7 @@ > > assigned-clock-rates = <0>, <125000000>; > > clock_in_out = "output"; > > phy-handle = <&rgmii_phy0>; > > - phy-mode = "rgmii-id"; > > + phy-mode = "rgmii"; > > pinctrl-names = "default"; > > pinctrl-0 = <&gmac0_miim > > &gmac0_tx_bus2 > > @@ -79,9 +79,6 @@ > > reg = <1>; > > pinctrl-0 = <ð_phy0_reset_pin>; > > pinctrl-names = "default"; > > - reset-assert-us = <10000>; > > - reset-deassert-us = <50000>; > > - reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>; > > Hmm, I don't see RK_PC4 being used anywhere else. gmac0 has RK_PC5 as Yes, it's a typo, it should be RK_RC5. > snsp,reset-gpio. So it essentially drops reset for the PHY. Is it > expected? snsp,reset-gpio defined reset already, so we don't need to set it here again. --- snsp,reset-gpio is the legacy binding, but I still have no idea why reset-gpios doesn't work, the dwmac driver will fail to lookup phy: [ 10.398514] rk_gmac-dwmac fe2a0000.ethernet eth0: no phy found [ 10.399061] rk_gmac-dwmac fe2a0000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -19) Any ideas would be appreciated. Thanks, Tianling. > > > }; > > }; > > > > @@ -115,7 +112,7 @@ > > &pinctrl { > > gmac0 { > > eth_phy0_reset_pin: eth-phy0-reset-pin { > > - rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_down>; > > + rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_up>; > > }; > > }; > > > > -- > > 2.17.1 > >
Hi Tianling, On 2023-03-16 06:34, Tianling Shen wrote: > Hi Vasily, > > On Thu, Mar 16, 2023 at 8:16 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote: >> >> On Wed, Mar 15, 2023 at 9:02 AM Tianling Shen <cnsztl@gmail.com> wrote: >>> >>> - Changed phy-mode to rgmii. >>> >>> - Fixed pull type in pinctrl for gmac0. >>> >>> - Removed duplicate properties in mdio node. >>> These properties are defined in the gmac0 node already. >>> >>> Signed-off-by: Tianling Shen <cnsztl@gmail.com> >>> --- >>> arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts >>> index e9adf5e66529..2a1118f15c29 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts >>> @@ -57,7 +57,7 @@ >>> assigned-clock-rates = <0>, <125000000>; >>> clock_in_out = "output"; >>> phy-handle = <&rgmii_phy0>; >>> - phy-mode = "rgmii-id"; >>> + phy-mode = "rgmii"; >>> pinctrl-names = "default"; >>> pinctrl-0 = <&gmac0_miim >>> &gmac0_tx_bus2 >>> @@ -79,9 +79,6 @@ >>> reg = <1>; >>> pinctrl-0 = <ð_phy0_reset_pin>; >>> pinctrl-names = "default"; >>> - reset-assert-us = <10000>; >>> - reset-deassert-us = <50000>; >>> - reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>; >> >> Hmm, I don't see RK_PC4 being used anywhere else. gmac0 has RK_PC5 as > > Yes, it's a typo, it should be RK_RC5. > >> snsp,reset-gpio. So it essentially drops reset for the PHY. Is it >> expected? > > snsp,reset-gpio defined reset already, so we don't need to set it here again. > > --- > > snsp,reset-gpio is the legacy binding, but I still have no idea why > reset-gpios doesn't work, > the dwmac driver will fail to lookup phy: > > [ 10.398514] rk_gmac-dwmac fe2a0000.ethernet eth0: no phy found > [ 10.399061] rk_gmac-dwmac fe2a0000.ethernet eth0: __stmmac_open: > Cannot attach to PHY (error: -19) > > Any ideas would be appreciated. Generic ethernet phy driver is not resetting the phy in the same way that snsp,reset-gpio does, please see top two commits at [1]. I have been meaning to send that out as an RFC but I got stuck in a u-boot rabbit hole, and I also do not know what the correct way to fix this would be, so I played with both device tree and code changes. Will prioritize this and send out a RFC later today. [1] https://github.com/Kwiboo/linux-rockchip/commits/rk3568-eth-phy-reset Regards, Jonas > > Thanks, > Tianling. > >> >>> }; >>> }; >>> >>> @@ -115,7 +112,7 @@ >>> &pinctrl { >>> gmac0 { >>> eth_phy0_reset_pin: eth-phy0-reset-pin { >>> - rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_down>; >>> + rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_up>; >>> }; >>> }; >>> >>> -- >>> 2.17.1 >>>
Hi Jonas, On Thu, Mar 16, 2023 at 3:37 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > Hi Tianling, > On 2023-03-16 06:34, Tianling Shen wrote: > > Hi Vasily, > > > > On Thu, Mar 16, 2023 at 8:16 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote: > >> > >> On Wed, Mar 15, 2023 at 9:02 AM Tianling Shen <cnsztl@gmail.com> wrote: > >>> > >>> - Changed phy-mode to rgmii. > >>> > >>> - Fixed pull type in pinctrl for gmac0. > >>> > >>> - Removed duplicate properties in mdio node. > >>> These properties are defined in the gmac0 node already. > >>> > >>> Signed-off-by: Tianling Shen <cnsztl@gmail.com> > >>> --- > >>> arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 7 ++----- > >>> 1 file changed, 2 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > >>> index e9adf5e66529..2a1118f15c29 100644 > >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts > >>> @@ -57,7 +57,7 @@ > >>> assigned-clock-rates = <0>, <125000000>; > >>> clock_in_out = "output"; > >>> phy-handle = <&rgmii_phy0>; > >>> - phy-mode = "rgmii-id"; > >>> + phy-mode = "rgmii"; > >>> pinctrl-names = "default"; > >>> pinctrl-0 = <&gmac0_miim > >>> &gmac0_tx_bus2 > >>> @@ -79,9 +79,6 @@ > >>> reg = <1>; > >>> pinctrl-0 = <ð_phy0_reset_pin>; > >>> pinctrl-names = "default"; > >>> - reset-assert-us = <10000>; > >>> - reset-deassert-us = <50000>; > >>> - reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>; > >> > >> Hmm, I don't see RK_PC4 being used anywhere else. gmac0 has RK_PC5 as > > > > Yes, it's a typo, it should be RK_RC5. > > > >> snsp,reset-gpio. So it essentially drops reset for the PHY. Is it > >> expected? > > > > snsp,reset-gpio defined reset already, so we don't need to set it here again. > > > > --- > > > > snsp,reset-gpio is the legacy binding, but I still have no idea why > > reset-gpios doesn't work, > > the dwmac driver will fail to lookup phy: > > > > [ 10.398514] rk_gmac-dwmac fe2a0000.ethernet eth0: no phy found > > [ 10.399061] rk_gmac-dwmac fe2a0000.ethernet eth0: __stmmac_open: > > Cannot attach to PHY (error: -19) > > > > Any ideas would be appreciated. > > Generic ethernet phy driver is not resetting the phy in the same way > that snsp,reset-gpio does, please see top two commits at [1]. > > I have been meaning to send that out as an RFC but I got stuck in a > u-boot rabbit hole, and I also do not know what the correct way to fix > this would be, so I played with both device tree and code changes. > Will prioritize this and send out a RFC later today. > > [1] https://github.com/Kwiboo/linux-rockchip/commits/rk3568-eth-phy-reset Thanks for the hint! I will test your patches tonight. Thanks, Tianling. > > Regards, > Jonas > > > > > Thanks, > > Tianling. > > > >> > >>> }; > >>> }; > >>> > >>> @@ -115,7 +112,7 @@ > >>> &pinctrl { > >>> gmac0 { > >>> eth_phy0_reset_pin: eth-phy0-reset-pin { > >>> - rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_down>; > >>> + rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_up>; > >>> }; > >>> }; > >>> > >>> -- > >>> 2.17.1 > >>> >
Hi, On Thu, Mar 16, 2023 at 4:46 PM Tianling Shen <cnsztl@gmail.com> wrote: > > Hi Jonas, > > On Thu, Mar 16, 2023 at 3:37 PM Jonas Karlman <jonas@kwiboo.se> wrote: > > [...] > > > > Generic ethernet phy driver is not resetting the phy in the same way > > that snsp,reset-gpio does, please see top two commits at [1]. > > > > I have been meaning to send that out as an RFC but I got stuck in a > > u-boot rabbit hole, and I also do not know what the correct way to fix > > this would be, so I played with both device tree and code changes. > > Will prioritize this and send out a RFC later today. > > > > [1] https://github.com/Kwiboo/linux-rockchip/commits/rk3568-eth-phy-reset > > Thanks for the hint! I will test your patches tonight. I'm currently playing your patches at https://github.com/1715173329/imoutowrt/commits/master-rockchip-mdio I applied commit 8597fcfa0c5c792dabb44a2db7b283c56c99ec6a to NanoPi R5S, it worked perfectly. However, with commit c338ed260bfd87277c41aa0290f1f2aad8d629b1 + moving reset properties into the phy node, the driver still failed to lookup phy. I'm not sure if I missed / misunderstood anything. Thanks, Tianling. [...]
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts index e9adf5e66529..2a1118f15c29 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts @@ -57,7 +57,7 @@ assigned-clock-rates = <0>, <125000000>; clock_in_out = "output"; phy-handle = <&rgmii_phy0>; - phy-mode = "rgmii-id"; + phy-mode = "rgmii"; pinctrl-names = "default"; pinctrl-0 = <&gmac0_miim &gmac0_tx_bus2 @@ -79,9 +79,6 @@ reg = <1>; pinctrl-0 = <ð_phy0_reset_pin>; pinctrl-names = "default"; - reset-assert-us = <10000>; - reset-deassert-us = <50000>; - reset-gpios = <&gpio0 RK_PC4 GPIO_ACTIVE_LOW>; }; }; @@ -115,7 +112,7 @@ &pinctrl { gmac0 { eth_phy0_reset_pin: eth-phy0-reset-pin { - rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_down>; + rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_up>; }; };
- Changed phy-mode to rgmii. - Fixed pull type in pinctrl for gmac0. - Removed duplicate properties in mdio node. These properties are defined in the gmac0 node already. Signed-off-by: Tianling Shen <cnsztl@gmail.com> --- arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)