Re: [PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc

[Dragan Simic <dsimic@manjaro.org>]

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.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip