Message-ID: <e4db4e6f0a5a42ceacacc925adbe13747a6f948e.camel@icenowy.me>
Date: Wed, 04 Jun 2025 18:52:06 +0800
From: Icenowy Zheng <uwu@...nowy.me>
To: Andrew Lunn <andrew@...n.ch>, Rob Herring <robh@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>,  Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,  Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Chaoyi Chen
 <chaoyi.chen@...k-chips.com>, Matthias Schiffer
 <matthias.schiffer@...tq-group.com>, "Russell King (Oracle)"
 <linux@...linux.org.uk>, Heiner Kallweit <hkallweit1@...il.com>, 
 netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] dt-bindings: net: ethernet-controller: Add
 informative text about RGMII delays

在 2025-04-30星期三的 11:21 -0500，Andrew Lunn写道：
> Device Tree and Ethernet MAC driver writers often misunderstand RGMII
> delays. Rewrite the Normative section in terms of the PCB, is the PCB
> adding the 2ns delay. This meaning was previous implied by the
> definition, but often wrongly interpreted due to the ambiguous
> wording
> and looking at the definition from the wrong perspective. The new
> definition concentrates clearly on the hardware, and should be less
> ambiguous.
> 
> Add an Informative section to the end of the binding describing in
> detail what the four RGMII delays mean. This expands on just the PCB
> meaning, adding in the implications for the MAC and PHY.
> 
> Additionally, when the MAC or PHY needs to add a delay, which is
> software configuration, describe how Linux does this, in the hope of
> reducing errors. Make it clear other users of device tree binding may
> implement the software configuration in other ways while still
> conforming to the binding.

Sorry for my late disturbance (so late that this patch is already
included in released version), but I have some questions about this...

> 
> Fixes: 9d3de3c58347 ("dt-bindings: net: Add YAML schemas for the
> generic Ethernet options")
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> Acked-by: Conor Dooley <conor.dooley@...rochip.com>
> ---
> Changes in v2:
> Reword Normative section
> manor->manner
> add when using phylib/phylink
> request details in the commit message and .dts comments
> clarify PHY -internal-delay-ps values being depending on rgmii-X
> mode.
> Link to v1:
> https://lore.kernel.org/r/20250429-v6-15-rc3-net-rgmii-delays-v1-1-f52664945741@lunn.ch
> ---
>  .../bindings/net/ethernet-controller.yaml          | 97
> ++++++++++++++++++++--
>  1 file changed, 90 insertions(+), 7 deletions(-)
> 
> 
> ---
> base-commit: d4cb1ecc22908ef46f2885ee2978a4f22e90f365
> change-id: 20250429-v6-15-rc3-net-rgmii-delays-8a00c4788fa7
> 
> Best regards,
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-
> controller.yaml b/Documentation/devicetree/bindings/net/ethernet-
> controller.yaml
> index
> 45819b2358002bc75e876eddb4b2ca18017c04bd..a2d4c626f659a57fc7dcd39301f
> 322c28afed69d 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -74,19 +74,17 @@ properties:
>        - rev-rmii
>        - moca
>  
> -      # RX and TX delays are added by the MAC when required
> +      # RX and TX delays are provided by the PCB. See below

This really sounds like a breaking change that changes the meaning of
the definition of this item instead of simply rewording.

Everything written according to the original description is broken by
this change.

In addition, considering this is set as a property of the MAC device
tree node, it's more natural to represents the perspective of MAC,
instead of only describing what's on the PCB.

>        - rgmii
>  
> -      # RGMII with internal RX and TX delays provided by the PHY,
> -      # the MAC should not add the RX or TX delays in this case
> +      # RX and TX delays are not provided by the PCB. This is the
> most
> +      # frequent case. See below
>        - rgmii-id
>  
> -      # RGMII with internal RX delay provided by the PHY, the MAC
> -      # should not add an RX delay in this case
> +      # TX delay is provided by the PCB. See below
>        - rgmii-rxid
>  
> -      # RGMII with internal TX delay provided by the PHY, the MAC
> -      # should not add an TX delay in this case
> +      # RX delay is provided by the PCB. See below
>        - rgmii-txid
>        - rtbi
>        - smii
> @@ -286,4 +284,89 @@ allOf:
>  
>  additionalProperties: true
>  
> +# Informative
> +# ===========
> +#
> +# 'phy-modes' & 'phy-connection-type' properties 'rgmii', 'rgmii-
> id',
> +# 'rgmii-rxid', and 'rgmii-txid' are frequently used wrongly by
> +# developers. This informative section clarifies their usage.
> +#
> +# The RGMII specification requires a 2ns delay between the data and
> +# clock signals on the RGMII bus. How this delay is implemented is
> not
> +# specified.
> +#
> +# One option is to make the clock traces on the PCB longer than the
> +# data traces. A sufficiently difference in length can provide the
> 2ns
> +# delay. If both the RX and TX delays are implemented in this
> manner,
> +# 'rgmii' should be used, so indicating the PCB adds the delays.
> +#
> +# If the PCB does not add these delays via extra long traces,
> +# 'rgmii-id' should be used. Here, 'id' refers to 'internal delay',
> +# where either the MAC or PHY adds the delay.
> +#
> +# If only one of the two delays are implemented via extra long clock
> +# lines, either 'rgmii-rxid' or 'rgmii-txid' should be used,
> +# indicating the MAC or PHY should implement one of the delays
> +# internally, while the PCB implements the other delay.
> +#
> +# Device Tree describes hardware, and in this case, it describes the
> +# PCB between the MAC and the PHY, if the PCB implements delays or
> +# not.
> +#
> +# In practice, very few PCBs make use of extra long clock lines.
> Hence
> +# any RGMII phy mode other than 'rgmii-id' is probably wrong, and is
> +# unlikely to be accepted during review without details provided in
> +# the commit description and comments in the .dts file.
> +#
> +# 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.

Usually, in this case, this isn't pure software configuration but
already described as '?x-internal-delay-ps' (or in some legacy cases,
'vendor,?x-delay-ps') property in the MAC.

> +#
> +# The following describes how Linux implements the configuration of
> +# the MAC and PHY to add these delays when the PCB does not. As
> stated
> +# above, developers often get this wrong, and the aim of this
> section
> +# is reduce the frequency of these errors by Linux developers. Other
> +# users of the Device Tree may implement it differently, and still
> be
> +# consistent with both the normative and informative description
> +# above.
> +#
> +# By default in Linux, when using phylib/phylink, the MAC is
> expected
> +# to read the 'phy-mode' from Device Tree, not implement any delays,
> +# and pass the value to the PHY. The PHY will then implement delays
> as
> +# specified by the 'phy-mode'. The PHY should always be reconfigured
> +# to implement the needed delays, replacing any setting performed by
> +# strapping or the bootloader, etc.
> +#
> +# Experience to date is that all PHYs which implement RGMII also
> +# implement the ability to add or not add the needed delays. Hence
> +# this default is expected to work in all cases. Ignoring this
> default
> +# is likely to be questioned by Reviews, and require a strong
> argument
> +# to be accepted.

Although these PHYs are able to implement (or not to implement) the
delay, it's not promised that this could be overriden by the kernel
instead of being set up as strap pins.

Take Realtek GbE PHYs, which I am most familiar with, as examples. The
Linux realtek netphy driver supports overriding delay configuration
only for RTL8211E/F. From Internet, RTL8211B/C/E/F datasheets could be
found. All of them contain RXDLY/TXDLY strap pins, but neither of them
mention any register to config the delays, which means that on
RTL8211B/C there's no known way to override them (the only known , and
on RTL8211E/F the way to override them are undocumented registers from
unknown origin (which means no support from Realtek themselves are
available). I do partly participate a dramatic case about RTL8211E:
there were some Pine A64+ boards with broken RTL8211E that have some
delay-chain related issues, and Pine64 only got magic numbers from
Realtek to fix them (the magic numbers happen to match current RTL8211E
delay configuration code in realtek driver and show that RXDLY is
overriden to disabled).

In addition, the Linux kernel contains a "Generic PHY" driver for any
802.1 c22 PHYs to work, without setting any delays. With this driver
it's impossible to let the PHY "always be reconfigured", and
enabling/disabling the PHY-specific driver may lead to behavior change
(when enabling the specific driver, the configuration is overriden;
when disabling it, the strap pin configuration is used). Well
personally I think the driver should give out a warning when it can
read out strip pin status and it does not match the DT configuration).

The DT binding (and phylib) currently also lacks any way to describe
"respect the hardware strapping delay configuration of PHY".

> +#
> +# There are a small number of cases where the MAC has hard coded
> +# delays which cannot be disabled. The 'phy-mode' only describes the
> +# PCB.  The inability to disable the delays in the MAC does not
> change
> +# the meaning of 'phy-mode'. It does however mean that a 'phy-mode'
> of
> +# 'rgmii' is now invalid, it cannot be supported, since both the PCB
> +# and the MAC and PHY adding delays cannot result in a functional
> +# link. Thus the MAC should report a fatal error for any modes which

Considering compatibilty, should this be just a warning (which usually
means a wrong phy-mode setup) instead of a fatal error?

> +# cannot be supported. When the MAC implements the delay, it must
> +# ensure that the PHY does not also implement the same delay. So it
> +# must modify the phy-mode it passes to the PHY, removing the delay
> it
> +# has added. Failure to remove the delay will result in a
> +# non-functioning link.

In case of the MAC implementing a '?x-internal-delay-ps' property, when
should the MAC stripping delays from phy-mode? Should the phy-mode be
changed when MAC delaying 100ps? Should it be changed when MAC delaying
1000ps? And if the PHY could be reprogrammed to do the delay and the
property contains a big value that is higher than 2000ps, should the
software switch to do the delay in PHY and decrease the MAC side delay
by 2000ps?

Besides this, do we have any existing MAC driver that implements
stripping the delay from passing to PHY? How should we maintain
compatibilty between a DT that does not consider MAC driver delay
stripping and a driver that considers this? DTs are expected to be
backward compatible, and if this problem could not be solved, we can
never implement this intended MAC delay stripping.

My personal idea here, which reflects the current practice of most
drivers/DTs I read and leads to no known breaking change, is to make
the "phy-mode" property of the MAC node represents how the MAC expects
external components (PCB+PHY), and, assuming no significant PCB delay,
just pass this value to PHY instead. The MAC side delay shouldn't be
considered and represented in this property, but extra properties, like
'?x-internal-delay-ps' should be used instead.

> +#
> +# Sometimes there is a need to fine tune the delays. Often the MAC
> or
> +# PHY can perform this fine tuning. In the MAC node, the Device Tree
> +# properties 'rx-internal-delay-ps' and 'tx-internal-delay-ps'
> should
> +# be used to indicate fine tuning performed by the MAC. The values
> +# expected here are small. A value of 2000ps, i.e 2ns, and a phy-
> mode
> +# of 'rgmii' will not be accepted by Reviewers.

BTW the MAC might be very powerful on introducing delays, e.g. it can
have delay lines on data pins in addition to clock pins (in case of
data pins are delayed, this could be considered as a "negative clock
delay"), or some hardware might be able to do per-data-pin data delay
(to compensate HW trace length inconsistency). Should these be
considered by the standard binding, or should these be left to vendored
properties?

> +#
> +# If the PHY is to perform fine tuning, the properties
> +# 'rx-internal-delay-ps' and 'tx-internal-delay-ps' in the PHY node
> +# should be used. When the PHY is implementing delays, e.g. 'rgmii-
> id'
> +# these properties should have a value near to 2000ps. If the PCB is
> +# implementing delays, e.g. 'rgmii', a small value can be used to
> fine
> +# tune the delay added by the PCB.
>  ...

Powered by blists - more mailing lists