| Message ID | 20241210013010.81257-7-pgwipeout@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rockchip: rk3328 fixes in preparation for usb3-phy | expand |
Hello Peter, Thanks for the patch. Please, see some comments below. On 2024-12-10 02:30, Peter Geis wrote: > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have > been unnecessary in the separate device trees. There is also a > performance loss to using snps,aal. Remove these from the rk3328-roc > device tree. > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > index 6984387ff8b3..0d476cc2144d 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > @@ -155,12 +155,9 @@ &gmac2io { > phy-mode = "rgmii"; > pinctrl-names = "default"; > pinctrl-0 = <&rgmiim1_pins>; > - snps,aal; Huh, I see that quite a few RK3328 board dts files specify the snps,aal node. I wonder was it a "cargo cult" approach at play, :) or was there some real need for it? Actually, I see now that you added snps,aal to rk3328-roc- cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip: improve rk3328-roc-cc rgmii performance."), so I guess that your further research and testing showed that it actually isn't needed for Ethernet stability? > 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>; Unless I'm missing something, the commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add the snps,rxpbl node to the RK3328 SoC dtsi, and the respective driver does nothing about it when the snps,txpbl node is found. Though, I see that rk3328-rock-pi-e.dts is the only other RK3328 board dts file that specifies the snps,rxpbl node, so it seems that removing the snps,rxpbl node here should be safe, especially because it was you who added it in the same commit mentioned above. If there were some SoC-level issues, all RK3328 boards would've needed it. > tx_delay = <0x24>; > rx_delay = <0x18>; > status = "okay";
On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello Peter, > > Thanks for the patch. Please, see some comments below. > > On 2024-12-10 02:30, Peter Geis wrote: > > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for > > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have > > been unnecessary in the separate device trees. There is also a > > performance loss to using snps,aal. Remove these from the rk3328-roc > > device tree. > > > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > > > --- > > > > arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > > index 6984387ff8b3..0d476cc2144d 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi > > @@ -155,12 +155,9 @@ &gmac2io { > > phy-mode = "rgmii"; > > pinctrl-names = "default"; > > pinctrl-0 = <&rgmiim1_pins>; > > - snps,aal; > > Huh, I see that quite a few RK3328 board dts files specify > the snps,aal node. I wonder was it a "cargo cult" approach > at play, :) or was there some real need for it? > > Actually, I see now that you added snps,aal to rk3328-roc- > cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip: > improve rk3328-roc-cc rgmii performance."), so I guess that > your further research and testing showed that it actually > isn't needed for Ethernet stability? > > > 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>; > > Unless I'm missing something, the commit 8a469ee35606 ("arm64: > dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add > the snps,rxpbl node to the RK3328 SoC dtsi, and the respective > driver does nothing about it when the snps,txpbl node is found. > > Though, I see that rk3328-rock-pi-e.dts is the only other > RK3328 board dts file that specifies the snps,rxpbl node, so > it seems that removing the snps,rxpbl node here should be safe, > especially because it was you who added it in the same commit > mentioned above. If there were some SoC-level issues, all > RK3328 boards would've needed it. Good Morning, You'll notice the author of that patch was me. Setting aal, txpbl, and rxpbl was the original fix I came up with for the rk3328, which I applied to the only board I had. Someone else later on isolated it specifically isolated it to just the txpbl and applied it to both the rk3328 and rk3399 directly. This was just something that was left hanging after that result. Looking at how rk356x was done, I suspect there's an even more elegant solution. However I don't have the deep level knowledge nor documentation to implement it. Very Respectfully, Peter Geis > > > tx_delay = <0x24>; > > rx_delay = <0x18>; > > status = "okay";
Hello Peter, On 2024-12-10 12:29, Peter Geis wrote: > On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> Thanks for the patch. Please, see some comments below. >> >> On 2024-12-10 02:30, Peter Geis wrote: >> > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for >> > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have >> > been unnecessary in the separate device trees. There is also a >> > performance loss to using snps,aal. Remove these from the rk3328-roc >> > device tree. >> > >> > Signed-off-by: Peter Geis <pgwipeout@gmail.com> >> > >> > --- >> > >> > arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 --- >> > 1 file changed, 3 deletions(-) >> > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >> > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >> > index 6984387ff8b3..0d476cc2144d 100644 >> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >> > @@ -155,12 +155,9 @@ &gmac2io { >> > phy-mode = "rgmii"; >> > pinctrl-names = "default"; >> > pinctrl-0 = <&rgmiim1_pins>; >> > - snps,aal; >> >> Huh, I see that quite a few RK3328 board dts files specify >> the snps,aal node. I wonder was it a "cargo cult" approach >> at play, :) or was there some real need for it? >> >> Actually, I see now that you added snps,aal to rk3328-roc- >> cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip: >> improve rk3328-roc-cc rgmii performance."), so I guess that >> your further research and testing showed that it actually >> isn't needed for Ethernet stability? >> >> > 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>; >> >> Unless I'm missing something, the commit 8a469ee35606 ("arm64: >> dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add >> the snps,rxpbl node to the RK3328 SoC dtsi, and the respective >> driver does nothing about it when the snps,txpbl node is found. >> >> Though, I see that rk3328-rock-pi-e.dts is the only other >> RK3328 board dts file that specifies the snps,rxpbl node, so >> it seems that removing the snps,rxpbl node here should be safe, >> especially because it was you who added it in the same commit >> mentioned above. If there were some SoC-level issues, all >> RK3328 boards would've needed it. > > Good Morning, > > You'll notice the author of that patch was me. Setting aal, txpbl, and > rxpbl was the original fix I came up with for the rk3328, which I > applied to the only board I had. Someone else later on isolated it > specifically isolated it to just the txpbl and applied it to both the > rk3328 and rk3399 directly. > > This was just something that was left hanging after that result. > > Looking at how rk356x was done, I suspect there's an even more elegant > solution. However I don't have the deep level knowledge nor > documentation to implement it. Sure, I noticed that you authored the original Ethernet stability fix. :) With all this in mind, please feel free to include Reviewed-by: Dragan Simic <dsimic@manjaro.org> and I'll prepare a patch or two that clean up any and all leftovers in other board dts(i) files.
On 2024-12-10 14:44, Dragan Simic wrote: > On 2024-12-10 12:29, Peter Geis wrote: >> On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic@manjaro.org> >> wrote: >>> Thanks for the patch. Please, see some comments below. >>> >>> On 2024-12-10 02:30, Peter Geis wrote: >>> > Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for >>> > RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have >>> > been unnecessary in the separate device trees. There is also a >>> > performance loss to using snps,aal. Remove these from the rk3328-roc >>> > device tree. >>> > >>> > Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>> > >>> > --- >>> > >>> > arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 --- >>> > 1 file changed, 3 deletions(-) >>> > >>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >>> > b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >>> > index 6984387ff8b3..0d476cc2144d 100644 >>> > --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >>> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi >>> > @@ -155,12 +155,9 @@ &gmac2io { >>> > phy-mode = "rgmii"; >>> > pinctrl-names = "default"; >>> > pinctrl-0 = <&rgmiim1_pins>; >>> > - snps,aal; >>> >>> Huh, I see that quite a few RK3328 board dts files specify >>> the snps,aal node. I wonder was it a "cargo cult" approach >>> at play, :) or was there some real need for it? >>> >>> Actually, I see now that you added snps,aal to rk3328-roc- >>> cc.dts in the commit 393f3875c385 ("arm64: dts: rockchip: >>> improve rk3328-roc-cc rgmii performance."), so I guess that >>> your further research and testing showed that it actually >>> isn't needed for Ethernet stability? >>> >>> > 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>; >>> >>> Unless I'm missing something, the commit 8a469ee35606 ("arm64: >>> dts: rockchip: Add txpbl node for RK3399/RK3328") doesn't add >>> the snps,rxpbl node to the RK3328 SoC dtsi, and the respective >>> driver does nothing about it when the snps,txpbl node is found. >>> >>> Though, I see that rk3328-rock-pi-e.dts is the only other >>> RK3328 board dts file that specifies the snps,rxpbl node, so >>> it seems that removing the snps,rxpbl node here should be safe, >>> especially because it was you who added it in the same commit >>> mentioned above. If there were some SoC-level issues, all >>> RK3328 boards would've needed it. >> >> Good Morning, >> >> You'll notice the author of that patch was me. Setting aal, txpbl, and >> rxpbl was the original fix I came up with for the rk3328, which I >> applied to the only board I had. Someone else later on isolated it >> specifically isolated it to just the txpbl and applied it to both the >> rk3328 and rk3399 directly. >> >> This was just something that was left hanging after that result. >> >> Looking at how rk356x was done, I suspect there's an even more elegant >> solution. However I don't have the deep level knowledge nor >> documentation to implement it. > > Sure, I noticed that you authored the original Ethernet stability > fix. :) With all this in mind, please feel free to include > > Reviewed-by: Dragan Simic <dsimic@manjaro.org> > > and I'll prepare a patch or two that clean up any and all leftovers > in other board dts(i) files. For future reference, to help anyone going through the mailing-list archive, here's a link to the above-mentioned cleanup patch: https://lore.kernel.org/linux-rockchip/e00f08d2351e82d6acd56271a68c7ed05b3362e8.1733901896.git.dsimic@manjaro.org/T/#u
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi index 6984387ff8b3..0d476cc2144d 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi @@ -155,12 +155,9 @@ &gmac2io { phy-mode = "rgmii"; pinctrl-names = "default"; pinctrl-0 = <&rgmiim1_pins>; - 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 = <0x24>; rx_delay = <0x18>; status = "okay";
Since commit 8a469ee35606 ("arm64: dts: rockchip: Add txpbl node for RK3399/RK3328"), the snps,aal, snps,txpbl, and snps,rxpbl nodes have been unnecessary in the separate device trees. There is also a performance loss to using snps,aal. Remove these from the rk3328-roc device tree. Signed-off-by: Peter Geis <pgwipeout@gmail.com> --- arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 3 --- 1 file changed, 3 deletions(-)