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

[Dragan Simic <dsimic@manjaro.org>]

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