netdev - Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: document a phy link status inversion property

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] [thread-next>] [day] [month] [year] [list]

Date: Thu, 1 Jun 2023 16:45:57 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Atsushi Nemoto <atsushi.nemoto@...d.co.jp>
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org,
	Michael Hennerich <michael.hennerich@...log.com>,
	Alexandru Tachici <alexandru.tachici@...log.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>
Subject: Re: [PATCH net-next v2 1/2] dt-bindings: net: adin: document a phy
 link status inversion property

On Thu, Jun 01, 2023 at 11:17:52PM +0900, Atsushi Nemoto wrote:
> On 2023/06/01 22:36, Andrew Lunn wrote:
> >> But in this case, LINK_ST is also connected to MII_RXLINK pin of
> >> the MAC module in SoC. MII_RXLINK also expects low-active signal input.
> >> (though I only wrote about LED, sorry)
> > 
> > This is why the Commit message should contain the answer to 'Why?'.
> > The code tells us 'What', but without knowing the 'Why', it is hard to
> > say if you are doing the right or wrong thing.
> 
> Yes, then how about this?
> ---
> The LINK_ST is active-high by default. This can be inverted by the
> GE_LNK_STAT_INV_REG register, meaning that link up is indicated by
> setting LINK_ST low.

O.K. So far. But the rest still does not explain why. So i suggest
adding....

One application of this PHY is connecting it to the PRU in TI Sitara
SoC. This MAC requires a hardware indication of link, rather than the
usual phylib callback via adjust link. And this input is active-low.
Add a property to indicate that LINK_ST be configured active low.

> Please look at a block diagram in this page:
> https://software-dl.ti.com/processor-sdk-linux/esd/docs/latest/linux/Foundational_Components/PRU-ICSS/Linux_Drivers/PRU-ICSS_Ethernet.html

We are now getting close to having all the pieces of the puzzle to
decide if this is the right or wrong way to do this.

This appears to be the 'Vendor Crap' driver. You are patching mainline
here, so it is the mainline PRU driver we care about:

https://lore.kernel.org/netdev/20230424053233.2338782-1-danishanwar@ti.com/T/

Looking at the device tree binding, it uses the standard phy-handle,
phy-mode properties. There is also emac_adjust_link() which is used by
phylib to tell the MAC driver the link is down.

So you now need to convince me this change is actually needed in
mainline.

Thanks
	Andrew