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

[Andrew Lunn]

> 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.

Yes it is, you are reading it wrong. First off, as said a number of
times in the last couple of months, this description is from the
perspective of the PHY. It is not the board perspective. So:

      # RX and TX delays are added by the MAC when required
      - rgmii

>From the perspective of the PHY, "when required" the MAC adds the
delay. It might not be required, because the PCB adds the delay.

      # RGMII with internal RX and TX delays provided by the PHY,
      # the MAC should not add the RX or TX delays in this case
      - rgmii-id

The MAC should not add delays because the PHY does. But this implies
the PCB cannot also be adding delays because you would end up with two
sets of delays.

> 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.

You need to look at it broken driver by broken driver. I _think_ the
Aspeed case can be fixed. Others we need to look at the details. Maybe
in some cases we do need to add a warning to the device tree binding
the driver is FUBAR and has special, broken meaning for 'rgmii'.

> As a first step, I'd update the docs to describe the intended behavior, but
> mention that some drivers implement it wrong.

Feel free to submit patches. However, please think about it. DT is OS
agnostic. DT describes the hardware, not configuration. Other OSes
might decide on a different policy about which of the MAC/PHY pair
implements the delay, since that is configuration, not a hardware
description.

> 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...

Won't help anyway. If developers are getting it wrong now, why should
they suddenly start getting it right when all you are changing is the
name.

The real issue is that you can combine two bugs to get a working
system. It seems like few developers actually understand RGMII
delays. They don't spend time to understand it, they just try random
things until it works. As a result, they get systems where the MAC is
unknowingly adding delays, and 'rgmii' works, so job done. They don't
know or care it is wrong.

> - Introduce an additional DTS flag next to phy-mode to express that the
>   phy-mode is supposed to be interpreted correctly

If you are going to do add any sort of property, it should be a bool
indicating the opposite, 'phy-mode-is-fubar'. Something which
developers will think about before copying into their DT.

> Do you have any suggestions?

Add a checkpatch.pl check. Any patch which adds a phy-mode !=
'rgmii-id' needs to have a comment on the previous line containing the
word PCB. That should at least help stop more broken DT being added.

	Andrew