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

[Peter Geis]

On Tue, Dec 10, 2024 at 5:45 AM Dragan Simic <dsimic at 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 at 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";