Message-ID: <19574942-d06b-44b0-8b6c-d3ddd94db89f@cherry.de>
Date: Thu, 22 May 2025 10:18:12 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Andrew Lunn <andrew@...n.ch>, Quentin Schulz <foss+kernel@...il.net>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Jakob Unterwurzacher <jakob.unterwurzacher@...rry.de>,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Kever Yang <kever.yang@...k-chips.com>
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: support Ethernet Switch adapter
 for RK3588 Jaguar

Hi Andrew,

On 5/21/25 6:25 PM, Andrew Lunn wrote:
>> +&gmac1 {
>> +	clock_in_out = "output";
>> +	phy-mode = "rgmii";
> 
> Does the PCB have extra long clock lines to implement the 2ns delays?
> 

Not that I am aware no.

The issue here is that I believe the Linux driver actually got the whole 
phy-mode thing wrong?

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

First, tx_delay defaults to 0x30 if absent, rx_delay to 0x10 if absent, 
which seems a bit odd but why not.

Then you have rk_gmac_powerup() handling the delays.

If RGMII, then use rx_delay and tx_delay. If RGMII-ID, use neither. If 
RGMII-RXID use tx_delay. If RGMII-TXID use rx_delay.

This is the complete opposite of what I was expecting? I would like to 
use rgmii-id because this should better fit the HW matching the 
documentation in the dt-binding here 
(https://elixir.bootlin.com/linux/v6.15-rc6/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287), 
but this would actually disable the delays on the MAC if I read that 
correctly.

>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&gmac1_rx_bus2
>> +		     &gmac1_tx_bus2
>> +		     &gmac1_rgmii_clk
>> +		     &gmac1_rgmii_bus
>> +		     &eth1_pins>;
>> +	rx_delay = <0x30>;
>> +	tx_delay = <0x30>;
> 
> Since this has a switch on the other end, its a bit more complicated
> with RGMII delays. Normally, the MAC does nothing and passed rgmii-id
> to the PHY, and the PHY then does the delays. However, here you don't
> have a PHY. So you have the MAC add the delays. This looks O.K. I

The switch actually supports adding delays on the port used for DSA conduit.

https://eu.mouser.com/datasheet/2/268/KSZ9896C_Data_Sheet_DS00002390C-3443638.pdf 
5.2.3.2 XMII Port Control 1 Register bits 3 and 4. But the granularity 
is essentially: 0 delay or at least 1.5ns...

With the SoC MAC we actually have a much (assumed) more precise granularity.

https://github.com/FanX-Tek/rk3588-TRM-and-Datasheet/blob/master/Rockchip%20RK3588%20TRM%20V1.0-Part1-20220309.pdf

25.6.11 Clock Architecture

"""
In order to dynamically adjust the timing between TX/RX clocks with data,
delayline is integrated in TX and RX clock path. Register 
SYS_GRF_SOC_CON7[5:2] can
enable the delayline and SYS_GRF_SOC_CON8[15:0]/SYS_GRF_SOC_CON9[15:0] 
is used to
determine the delay length. There are 200 delay elements in each delayline.
"""

No information on the meaning of the number we put in rx_delay/tx_delay. 
Then you'll ask how we figure out the value to put there :) We initially 
port on the downstream kernel where they have a debugfs entry to 
dynamically modify the delay and return an eye of which you get the center.

> would prefer that the driver used the standardized
> rx-internal-delay-ps & tx-internal-delay-ps rather than these vendor

I would prefer too. We would need to know what the value we put in 
rx_delay/tx_delay actually mean in terms of picoseconds? (adding Kever 
in Cc) The worry I have is whether this is stable across all SoCs using 
that IP and/or if the delay is based off the MAC clock or something like 
that?

Then, in order to NOT break new kernel Image with old DT, I assume we 
need something like:

If RGMII, then use rx_delay and tx_delay, no 
rx-internal-delay-ps/tx-internal-delay-ps.

If RGMII-ID, no rx_delay/tx_delay, both 
rx-internal-delay-ps/tx-internal-delay-ps.
If RGMII-RXID use tx_delay and rx-internal-delay-ps.
If RGMII-TXID use rx_delay and tx-internal-delay-ps.

Fail if there's a mix of tx_delay/rx_delay and 
tx-internal-delay-ps/rx-internal-delay-ps properties? (possibly even on 
dt-binding level).

> properties. But that is probably out of scope for this patchset.
> 
>> +			port@5 {
>> +				reg = <5>;
>> +				ethernet = <&gmac1>;
>> +				label = "CPU";
>> +				phy-mode = "rgmii";
> 
> Again, this probably should be rgmii-id to correctly describe the PCB,
> but i don't know if the switch takes any notice of it.
> 

It does something based on DT:

drivers/net/dsa/microchip/ksz_common.c:ksz_parse_rgmii_delay()

If neither rx-internal-delay-ps/tx-internal-delay-ps properties are 
present, set rx_delay to enabled (2000 is arbitrary there) when in RGMII 
or RGMII_RXID mode, set tx_delay to enabled (2000 is arbitrary there) 
when in RGMII or RGMII_TXID mode. If one property is missing, it is set 
to disabled.

Now I do have a question. Shouldn't phy-mode from the MAC match the one 
from the PHY (here I assume the switch contains a PHY on the CPU port)?

I'm a bit confused by the following sentence:

"""
Normally, the MAC does nothing and passed rgmii-id
"""

is this something that the MAC driver is supposed to do or is the 
subsystem handling that somehow? How do I know how/when to rewrite the 
phy-mode passed to the PHY?

Cheers,
Quentin

Powered by blists - more mailing lists