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:

quoted

quoted

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.

quoted

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.

It has to be that why. If the MAC does the delay, the MAC needs to
ensure the PHY does not do the delays and well, and it achieves that
by setting PHY_INTERFACE_MODE to indicate the PHY should not add
delays. Do you have a better idea how this can be done?

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.

We have tried to improve the documentation. We have also been very
rigid in reviewing DT bindings, and what these things mean. But it
seems like many developers don't read reviews other developers get. Go
search the email archive. How many times have i had this very same
conversation?

Everybody gets pause wrong. Everybody gets EEE wrong. Everybody gets
RGMII delays wrong, not matter how many times we tell developers they
are getting it wrong.... phylink is helping with this, it takes it out
of developers hands so they cannot get pause or soon EEE wrong.

	Andrew