Message-ID: <9e8f659d-9116-46a6-b7e7-3d4705f57ac6@cherry.de>
Date: Thu, 22 May 2025 16:12:14 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Quentin Schulz <foss+kernel@...il.net>, 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/22/25 2:50 PM, Andrew Lunn wrote:
> On Thu, May 22, 2025 at 10:18:12AM +0200, Quentin Schulz wrote:
>> 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.
> 
> So 'rgmii-id' describes the hardware.
> 
>>
>> The issue here is that I believe the Linux driver actually got the whole
>> phy-mode thing wrong?
> 
> Quite possible, a few drivers do.
> 
>> 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?
> 
> This driver, and the aspeed driver cause a lot of problems....
> 

Would the suggested change in the previous answer be acceptable to you?

@Kever, is there any way to know what the register values for 
rx_delay/tx_delay actually mean in terms of picoseconds delay added by 
the MAC maybe?

>>> 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.
> 
> That actually looks to be the simplest and correct solution. Set the
> MAC to 'rgmii-id', rx_delay and tx_delay to 0, even if they are
> ignored. And in the switch, also 'rgmii-id' and let it insert the 2ns
> delay. You can use rx-internal-delay-ps and tx-internal-delay-ps if
> you want, but it seems to default to sensible values.
> 

I don't have control on how much is inserted by the PHY as opposed to 
the MAC, so I'm wary of using a much less precise (on paper) delay. I 
have no clue if doing this in the PHY is going to put us in the center 
of the eye or not. Thanks to Rockchip's kernel tool, we are expecting to 
be in the center of the eye right now.

>> 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?
> 
> A small number of MACs have hard coded delays. You cannot turn the
> delay off. So the MAC has no choice but to do the delay. 'rgmii' is
> simply not possible, so -EINVAL. For 'rgmii-id', if you pass that to
> the PHY, it will also add a delay, and 4ns in total does not work. So
> when the MAC is adding delays, it needs to mask out the delays it is
> adding before calling phy_attach() passing an rgmii mode.
> 

If I understand correctly, if phy-mode is rgmii-id in DT and the MAC is 
adding the delay, I need to force PHY_INTERFACE_MODE_RGMII phy-mode when 
attaching the PHY in the MAC device. This seems to indicate that the 
meaning of phy-mode = "rgmii" in the DT differs from phy_mode = 
PHY_INTERFACE_MODE_RGMII. The former represents the link, so rgmii means 
no delay added by either IC (MAC or PHY), but the latter is for the 
specific device with this phy_mode set and means "no delay for this 
particular device, but maybe the other end of the link adds it".

Does this also mean you cannot have mixed addition of delay? E.g. having 
1ns from MAC and 1ns from PHY? It has to be only on one of the IC?

In the comment at the bottom of the dt binding there's this following 
sentence:

# When the PCB does not implement the delays, the MAC or PHY must.  As
# such, this is software configuration, and so not described in Device
# Tree.

Why do we have two possible locations for rx-internal-delay-ps: PHY and 
MAC? This means to me the location is in some way software configuration 
since the MAC won't read the property from the PHY and vice-versa. Why 
isn't the MAC responsible for providing the delay to add to the PHY when 
attaching as well? Or are we supposed to have the same values and 
presence of the properties in the MAC and PHY? The KSZ DSA switch seems 
to be handling the delay based on the phy-mode from the DT for the DSA 
conduit and not the one it gets when being attached to the MAC, c.f. 
ksz_parse_rgmii_delay() called in ksz_switch_register() for each port in 
DT with port mode gotten with of_get_phy_mode().

Cheers,
Quentin

Powered by blists - more mailing lists