linux-kernel - Re: [PATCH] arm64: dts: rockchip: ROCK3B: Correct clock rates for Ethernet PHYs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3c6fd86-1fd8-4540-a4f4-0c6e3a0e2583@lunn.ch>
Date: Fri, 20 Dec 2024 18:21:23 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Alicja Michalska <alicja.michalska@...ements.com>
Cc: heiko@...ech.de, linux-rockchip@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: rockchip: ROCK3B: Correct clock rates for
 Ethernet PHYs

On Fri, Dec 20, 2024 at 05:32:27PM +0100, Alicja Michalska wrote:
> Built-in ethernet PHYs did not work on mainline kernel:

Builtin, as within the SoC?
> 
> fe010000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
> fe010000.ethernet eth0: __stmmac_open: Cannot attach to PHY

Not being able to find the PHY has nothing to do with RGMII delays.
If the RGMII delays are wrong, packets will not be
received/transmitted, or they will be corrupt and thrown away with
checksum errors. But the PHY will be found and you can attach to it.

So please leave everything to do with RGMII delays alone, at least
until you have a patch which causes the PHY to be found and attached.
It is likely not broken.

> According to the board design, they need to be configured as output with
> static TX/RX delay. This patch sets it accordingly.
> 
> Signed-off-by: Alicja Michalska <alicja.michalska@...ements.com>
> ---
>  .../boot/dts/rockchip/rk3568-rock-3b.dts      | 68 +++++++++++--------
>  1 file changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> index 3d0c1ccfaa79..5350158302e4 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> @@ -183,37 +183,65 @@ &cpu3 {
>  &gmac0 {
>  	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
>  	assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>, <&cru CLK_MAC0_2TOP>;
> -	clock_in_out = "input";
> -	phy-handle = <&rgmii_phy0>;
> -	phy-mode = "rgmii-id";
> +	assigned-clock-rates = <0>, <125000000>;
> +	clock_in_out = "output";
>  	phy-supply = <&vcc_3v3>;
> +	phy-mode = "rgmii";

This is wrong. RGMII requires a 2ns delay between the data and clock
lines. One way you can achieve this is by having extra long clock
lines on the PCB. If you have that, you use phy-mode rgmii. If there
are not extra long clock lines, you use rgmii-id, which indicates the
MAC/PHY pair need to insert the delays.

Now vendors very frequently get this wrong, monkeys typing Shakespeare
until something works, with no idea why it works, or if it is correct.
Using the wrong phy-mode, combined with wrong tx_delay and rx_delay
settings cancel each other out and it works. But its just "Two wrongs
make a right". We don't want mainline kernel code working like this.

> +	phy-handle = <&rgmii_phy0>;
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&gmac0_miim
>  		     &gmac0_tx_bus2
>  		     &gmac0_rx_bus2
>  		     &gmac0_rgmii_clk
> -		     &gmac0_rgmii_bus
> -		     &gmac0_clkinout>;
> +		     &gmac0_rgmii_bus>;
> +	snps,reset-gpio = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
> +	snps,reset-active-low;
> +	snps,reset-delays-us = <0 20000 100000>;

You are adding this here and removing the equivalent from the PHY
node. Using the PHY node properties is the recommended way to do this,
since it is the same for all PHYs, where as the vendor snps,
properties only work for this device.

So, now you have something that works, slowly undo all the changes,
and find what actually made it work. But you need to undo stuff in
pairs. Remove all the RGMII delay related changes and test. Remove the
reset GPIO changes and test, etc.

	Andrew