| 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
| ||
|
Message-ID: <CAJ+vNU0Up2tcpRuU2Erq+Y7HfiPigpzvCTKcEvyJohcRCKVEyg@mail.gmail.com>
Date: Mon, 22 Aug 2022 13:11:11 -0700
From: Tim Harvey <tharvey@...eworks.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
Vladimir Oltean <olteanv@...il.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Woojung Huh <woojung.huh@...rochip.com>,
"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Russell King <linux@...linux.org.uk>,
Michael Grzeschik <m.grzeschik@...gutronix.de>,
Oleksij Rempel <linux@...pel-privat.de>,
Thorsten Leemhuis <regressions@...mhuis.info>,
Alvin Šipraga <alsi@...g-olufsen.dk>,
Craig McQueen <craig@...ueen.id.au>
Subject: Re: [PATCH net] net: dsa: microchip: keep compatibility with device
tree blobs with no phy-mode
On Fri, Aug 19, 2022 at 9:42 AM Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Fri, Aug 19, 2022 at 01:47:51PM +0200, Rasmus Villemoes wrote:
> > > To give you an idea of how things work for user ports. If a user port
> > > has a phy-handle, DSA will connect to that, irrespective of what OF-based
> > > MDIO bus that is on. If not, DSA looks at whether ds->slave_mii_bus is
> > > populated with a struct mii_bus by the driver. If it is, it connects in
> > > a non-OF based way to a PHY address equal to the port number. If
> > > ds->slave_mii_bus doesn't exist but the driver provides
> > > ds->ops->phy_read and ds->ops->phy_write, DSA automatically creates
> > > ds->slave_mii_bus where its ops are the driver provided phy_read and
> > > phy_write, and it then does the same thing of connecting to the PHY in
> > > that non-OF based way.
> >
> > Thanks, that's quite useful. From quick grepping, it seems that ksz9567
> > currently falls into the latter category?
>
> So it would appear.
>
> > No, but it's actually easier for me to just do that rather than carry an
> > extra patch until the mainline fix hits 5.19.y.
>
> Whatever suits you best.
>
> > > there is something to be said about U-Boot compatibility. In U-Boot,
> > > with DM_DSA, I don't intend to support any unnecessary complexity and
> > > alternative ways of describing the same thing, so there, phy-mode and
> > > one of phy-handle or fixed-link are mandatory for all ports.
> >
> > OK. I suppose that means the linux driver for the ksz9477 family should
> > learn to check if there's an "mdio" subnode and if so populate
> > ds->slave_mii_bus, but unlike lan937x, absence of that node should not
> > be fatal?
>
> Yes.
>
> > > U-Boot can pass its own device tree to Linux, it means Linux DSA drivers
> > > might need to gradually gain support for OF-based phy-handle on user
> > > ports as well. So see what Tim Harvey has done in drivers/net/ksz9477.c
> > > in the U-Boot source code, and try to work with his device tree format,
> > > as a starting point.
> >
> > Hm. It does seem like that driver has the mdio bus optional (as in,
> > probe doesn't fail just because the subnode isn't present). But I'm
> > curious why ksz_probe_mdio() looks for a subnode called "mdios" rather
> > than "mdio". Tim, just a typo?
>
> No, definitely not a typo. I have to explain this for what seems like
> the millionth time already, but the idea with an "mdios" container was
> copied from the sja1105 driver, where there are actually multiple MDIO
> buses. Documentation/devicetree/bindings/net/mdio.yaml wants the $nodename
> to have the "^mdio(@.*)?" pattern, and:
> - if you use "mdio" you can have a single node
> - if you don't put the MDIO nodes under a container with an
> #address-cells != 0, you can't use the "mdio@...ething" syntax
>
> https://lore.kernel.org/all/20210621234930.espjau5l2t5dr75y@skbuf/T/#me43cf6b1976a3c3aec8357f19ab967f98eea1f73
>
> What I'm actually less clear about is whether ksz9477 actually needs this.
> Luckily U-Boot should have less compatibility issues to worry about,
> since the DT is appended to the bootloader image, so some corrections
> can be made if necessary.
Vladimir,
The linux ksz9477 driver does not need the 'mdios' subnode or
phy-handle's to function properly. I added these nodes to the U-Boot
dts for imx8mm-venice-gw7901.dts and imx8mp-venice-gw74xx.dts in order
for the U-Boot ksz9477 driver to work properly. So now I have
out-of-sync dts files for those that will cause an issue the next time
someone sync's dts from Linux back to U-Boot:
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts b/arch/arm64
/boot/dts/freescale/imx8mp-venice-gw74xx.dts
index 8469d5ddf960..f0bd67f12bad 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-venice-gw74xx.dts
@@ -500,6 +500,7 @@ ports {
lan1: port@0 {
reg = <0>;
label = "lan1";
+ phy-handle = <&sw_phy0>;
phy-mode = "internal";
local-mac-address = [00 00 00 00 00 00];
};
@@ -507,6 +508,7 @@ lan1: port@0 {
lan2: port@1 {
reg = <1>;
label = "lan2";
+ phy-handle = <&sw_phy1>;
phy-mode = "internal";
local-mac-address = [00 00 00 00 00 00];
};
@@ -514,6 +516,7 @@ lan2: port@1 {
lan3: port@2 {
reg = <2>;
label = "lan3";
+ phy-handle = <&sw_phy2>;
phy-mode = "internal";
local-mac-address = [00 00 00 00 00 00];
};
@@ -521,6 +524,7 @@ lan3: port@2 {
lan4: port@3 {
reg = <3>;
label = "lan4";
+ phy-handle = <&sw_phy3>;
phy-mode = "internal";
local-mac-address = [00 00 00 00 00 00];
};
@@ -528,6 +532,7 @@ lan4: port@3 {
lan5: port@4 {
reg = <4>;
label = "lan5";
+ phy-handle = <&sw_phy4>;
phy-mode = "internal";
local-mac-address = [00 00 00 00 00 00];
};
@@ -544,6 +549,38 @@ fixed-link {
};
};
};
+
+ mdios {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ mdio@0 {
+ reg = <0>;
+ compatible = "microchip,ksz-mdio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ sw_phy0: ethernet-phy@0 {
+ reg = <0x0>;
+ };
+
+ sw_phy1: ethernet-phy@1 {
+ reg = <0x1>;
+ };
+
+ sw_phy2: ethernet-phy@2 {
+ reg = <0x2>;
+ };
+
+ sw_phy3: ethernet-phy@3 {
+ reg = <0x3>;
+ };
+
+ sw_phy4: ethernet-phy@4 {
+ reg = <0x4>;
+ };
+ };
+ };
};
};
I believe you are still of the opinion that support for parsing the
mdios node above be added to the Linux DSA drivers and dt-bindings or
were you suggesting just the bindings be added? I don't think bindings
would get ack'd unless there was code actually using them for
something and its not clear to me exactly what you were asking for.
Best Regards,
Tim
Powered by blists - more mailing lists