Re: [PATCH v2 5/5] arm64: dts: ti: Add TQ-Systems TQMa62xx SoM and MBa62xx carrier board Device Trees

[Matthias Schiffer]

On Tue, 2024-12-10 at 20:54 +0100, Andrew Lunn wrote:
> 
> On Tue, Dec 10, 2024 at 10:56:41AM +0100, Matthias Schiffer wrote:
> > On Mon, 2024-12-09 at 17:14 +0100, Andrew Lunn wrote:
> > > 
> > > > Not our board, but the AM62 SoC. From the datasheet:
> > > > 
> > > > "TXC is delayed internally before being driven to the RGMII[x]_TXC pin. This
> > > > internal delay is always enabled." So enabling the TX delay on the PHY side
> > > > would result in a double delay.
> > > 
> > > phy-mode describes the board. If the board does not have extra long
> > > clock lines, phy-mode should be rgmii-id.
> > > 
> > > The fact the MAC is doing something which no other MAC does should be
> > > hidden away in the MAC driver, as much as possible.
> > 
> > Isn't it kind of a philosophical question whether a delay added by the SoC
> > integration is part of the MAC or not? One could also argue that the MAC IP core
> > is always the same, with some SoCs adding the delay and others not. (I don't
> > know if there are actually SoCs with the same IP core that don't add a delay;
> > I'm just not a big fan of hiding details in the driver that could easily be
> > described by the Device Tree, thus making the driver more generic)
> 
> It is more about, what does phy-mode = "rgmii"; mean? It means the
> board provides the delay via extra long clock lines. Except for when
> some random MAC driver has a completely different meaning, it is not
> documented it means something else, you have to read the sources and
> the mailing lists, to find out what this particularly MAC driver is
> doing for phy-mode = "rgmii".
> 
> Do we really want that. Or should we define that phy-mode = "rgmii"
> means the PCB provides the delay. End of story, no exceptions. And
> that "rgmii-id" means the MAC/PHY pair need to provide the delay? End
> of story, no exceptions.


Hi Andrew,

I've just thought about this issue again. As mentioned, a number of MAC
drivers(*) implement what is described here:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml?h=v6.13#n77


That is, the delay is either added by the PHY or the MAC; having a delay on the
PCB is simply not supported. Fixing MAC drivers to interpret the values without
"id" to mean that there is a delay on the PCB will break existing Device Trees,
so that's no good.

As a first step, I'd update the docs to describe the intended behavior, but
mention that some drivers implement it wrong. The question is how to deal with
the wrong behavior going forward. I see the following options, but none spark
joy for me...

- Keep current driver behavior forever where fixing it would break existing
  Device Trees
- Deprecate all existing "rgmii*" values because of their inconsistent
  implementation, come up with new ones. Seems like a huge pain to add support
  for in all MAC drivers and other code that deals with PHY modes...
- Introduce an additional DTS flag next to phy-mode to express that the
  phy-mode is supposed to be interpreted correctly

Do you have any suggestions?

Best,
Matthias



(*) At a glance, at least the following MAC drivers appear to configure a delay,
making it impossible to support delays on the PCB with the current
implementation:
- renesas/ravb_main.c
- stmicro/stmmac/dwmac-* (some variants)
- ti/cpsw-phy-sel.c (incomplete; does not distinguish between RX and TX delays) 
- Some other MAC drivers check for specific PHY_INTERFACE_MODE_RGMII* values,
  which also doesn't seem completely correct



-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/