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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2953e10c-0a57-4d49-b831-3864a07eefd5@lunn.ch>
Date: Tue, 10 Dec 2024 20:54:19 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
Cc: Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
	Tero Kristo <kristo@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kees Cook <kees@...nel.org>, Tony Luck <tony.luck@...el.com>,
	"Guilherme G. Piccoli" <gpiccoli@...lia.com>,
	Felipe Balbi <balbi@...nel.org>,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-hardening@...r.kernel.org, Devarsh Thakkar <devarsht@...com>,
	Hari Nagalla <hnagalla@...com>, linux@...tq-group.com
Subject: Re: [PATCH v2 5/5] arm64: dts: ti: Add TQ-Systems TQMa62xx SoM and
 MBa62xx carrier board Device Trees

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.

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