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

[Matthias Schiffer <matthias.schiffer@ew.tq-group.com>]

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)


> 
> The MAC driver should return -EINVAL with phy-mode rgmii, or
> rmgii-rxid, because the MAC driver is physically incapable of being
> used on a board which has extra long TX clock lines, which 'rmgii' or
> rgmii-rxid would indicate.
> 
> Since the MAC driver is forcing the TX delay, it needs to take the
> value returned from of_get_phy_mode() and mask out the TX bit before
> passing it to the PHY.

Hmm okay, this is what the similar ICSSG/PRUETH driver does. I've always found
that solution to be particularly confusing, but if that's how it's supposed to
work, I'll have to accept that.

In my opinion the documentation Documentation/networking/phy.rst is not very
clear on this matter - the whole section "(RG)MII/electrical interface
considerations" talks about whether the PHY inserts the delay or not, so my
assumption was that phy-mode describes the PHY side of things and only that.

It gets even more confusing when taking into account
Documentation/devicetree/bindings/net/ethernet-controller.yaml, which contains
comments like "RGMII with internal RX delay provided by the PHY, the MAC should
not add an RX delay in this case", which sounds like there are only the cases
"delay is added by the PHY" and "delay is added by the MAC" - the case "delay is
part of the board design, neither MAC nor PHY add it" doesn't even appear.

> 
> Now, it could be that history has got in the way. There are boards out
> there which have broken DT but work. Fixing the MAC driver to do the
> correct thing will break those boards. Vendors with low quality code
> which works, but not really.
> 
> ~/linux/arch/arm64/boot/dts/ti$ grep rgmii k3-am625-*
> k3-am625-beagleplay.dts:	phy-mode = "rgmii-rxid";
> k3-am625-sk.dts:	phy-mode = "rgmii-rxid";
> 
> Yep, these two have broken DT, they don't describe the board
> correctly.
> 
> O.K. Can we fix this for you board? Yes, i think we can. If you take
> rmgii-rxid, aka PHY_INTERFACE_MODE_RGMII_RXID, and mask out the TX,
> you still get PHY_INTERFACE_MODE_RGMII_RXID. If you take rgmii-id,
> a.k.a. PHY_INTERFACE_MODE_RGMII_ID and mask out the TX, you get
> PHY_INTERFACE_MODE_RGMII_RXID, which is what you want.
> 
> Please produce a patch to the MAC driver, explaining the horrible mess
> the vendor made, and how this fixes it, but should also not break
> other boards.

I can make this change, but "am65-cpsw-nuss" currently supports 6 different
compatible strings, many of which are used for multiple SoC families.

Maybe someone from TI could chime in and say whether all of these have the fixed
TXC delay, or at least the current compatible strings are already specific
enough to tell whether the SoC adds a delay?


> 
> > No such defaults exist in the DP83867 driver. If any rgmii-*id mode is used, the
> > corresponding delays *must* be specified in the DTB:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/phy/dp83867.c#n532
> 
> That is bad, different to pretty every other PHY driver :-(
> 
> If you want, you could patch this driver as well, make it default to
> 2ns if delays are asked for.

Makes sense, I'll write a patch for that as well.

Best regards,
Matthias


> 
>     Andrew
> 
> ---
> pw-bot: cr

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