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

From: Dragan Simic <dsimic@manjaro.org>
To: Peter Geis <pgwipeout@gmail.com>
Cc: Heiko Stuebner <heiko@sntech.de>,
	Conor Dooley <conor+dt@kernel.org>,
	Diederik de Haas <didi.debian@cknow.org>,
	Johan Jonker <jbx6244@gmail.com>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Rob Herring <robh@kernel.org>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org
Subject: Re: [PATCH 6/6] arm64: dts: rockchip: Remove address aligned beats from rk3328-roc
Date: Tue, 10 Dec 2024 11:45:49 +0100	[thread overview]
Message-ID: <2b68c2dd3618e5904a4eac1ec87d29a7@manjaro.org> (raw)
In-Reply-To: <20241210013010.81257-7-pgwipeout@gmail.com>

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";

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