netdev - Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy configuration for som revision 1.9

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]

Message-ID: <YmFWFzYz/iV4t2cW@shell.armlinux.org.uk>
Date:   Thu, 21 Apr 2022 14:03:19 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Josua Mayer <josua@...id-run.com>, netdev@...r.kernel.org,
        alvaro.karsz@...id-run.com, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>
Subject: Re: [PATCH v2 3/3] ARM: dts: imx6qdl-sr-som: update phy
 configuration for som revision 1.9

On Thu, Apr 21, 2022 at 02:27:16PM +0200, Andrew Lunn wrote:
> On Tue, Apr 19, 2022 at 01:27:09PM +0300, Josua Mayer wrote:
> > Since SoM revision 1.9 the PHY has been replaced with an ADIN1300,
> > add an entry for it next to the original.
> > 
> > Co-developed-by: Alvaro Karsz <alvaro.karsz@...id-run.com>
> > Signed-off-by: Alvaro Karsz <alvaro.karsz@...id-run.com>
> > Signed-off-by: Josua Mayer <josua@...id-run.com>
> > ---
> > V1 -> V2: changed dts property name
> > 
> >  arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> > index f86efd0ccc40..d46182095d79 100644
> > --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi
> > @@ -83,6 +83,12 @@ ethernet-phy@4 {
> >  			qca,clk-out-frequency = <125000000>;
> >  			qca,smarteee-tw-us-1g = <24>;
> >  		};
> > +
> > +		/* ADIN1300 (som rev 1.9 or later) */
> > +		ethernet-phy@1 {
> > +			reg = <1>;
> > +			adi,phy-output-clock = "125mhz-free-running";
> > +		};
> 
> There is currently the comment:
> 
>                  * The PHY can appear at either address 0 or 4 due to the
>                  * configuration (LED) pin not being pulled sufficiently.
>                  */
> 
> It would be good to add another comment about this PHY at address 1.

There is an issue with this approach. Listing the "possible" PHYs in DT
so we can configure them leads to the kernel complaining at boot time
with:

mdio_bus 2188000.ethernet-1: MDIO device at address 4 is missing.

So with this patch, we'll also get:

mdio_bus 2188000.ethernet-1: MDIO device at address 1 is missing.

which is not great for the user to see. Arguably though, it's down to
broken hardware design in the case of the AR8035, since this is caused
by insufficient pull on the LED pin to ensure the hardware address is
reliable configured.

However, adding this for a rev 1.9 uSOM where we know that the PHY is
different matter. I think we should be aiming for a new revision of
DT for the uSOM with the AR8035 nodes removed and the ADIN added,
rather than trying to stuff them all into a single DT and have the
kernel complain about not just one missing PHY, but now two.

The only other ways around this that I can see would be to have some
way to flag in DT that the PHYs are "optional" - if they're not found
while probing the hardware, then don't whinge about them. Or have
u-boot discover which address the PHY is located, and update the DT
blob passed to the kernel to disable the PHY addresses that aren't
present. Or edit the DT to update the node name and reg property. Or
something along those lines.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!