Date: Fri, 20 Oct 2023 18:12:40 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Arınç ÜNAL <arinc.unal@...nc9.com>,
	Marek Behún <kabel@...nel.org>,
	Andrew Lunn <andrew@...n.ch>,
	Gregory Clement <gregory.clement@...tlin.com>,
	Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	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>,
	Christian Marangi <ansuelsmth@...il.com>,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 6/7] dt-bindings: marvell: Rewrite MV88E6xxx
 in schema

On Fri, Oct 20, 2023 at 02:47:20PM +0200, Linus Walleij wrote:
> On Thu, Oct 19, 2023 at 5:35 PM Vladimir Oltean <olteanv@...il.com> wrote:
> 
> > Yikes, both these examples are actually broken,
> 
> As you can see from the patch, they are just carried over from
> Documentation/devicetree/bindings/net/dsa/marvell.txt
> 
> +/- fixes to make them pass schema checks.

(...)

> These examples are already in the kernel. Migrating them
> from marvell.txt to marvell,mv88e6xxx.yaml doesn't make
> the situation worse, it's not like people magically start trusting
> the examples more because they are in YAML than in .txt.
> 
> But sure let's try to put in better examples!

You are not correct here. The examples from
Documentation/devicetree/bindings/net/dsa/marvell.txt don't have ports,
and the way in which you added the ports is wrong (at least relative to
the way in which you kept the mdio node).

> > What you have now is exactly what won't work, i.e. an OF-based
> > slave_mii_bus with a non-OF-based phy_connect().
> 
> Yeah when I run check_dtbs I get a few (not many) warnings
> like this on aarch64 and armv7_multi:
> 
> arch/arm/boot/dts/nxp/imx/imx6q-b450v3.dtb: switch@0: ports:port@4:
> 'phy-mode' is a required property
>     from schema $id:
> http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml#

Ok, the warning is valid, but I don't know what phy-mode to put there.
It is unrelated anyway. Some warnings will be expected after the schema
conversion, and they are not all mechanical to fix. When we put schema
validation in place for checking that CPU ports have valid link
descriptions that phylink can use, we decided to be lax in the kernel,
but strict in the dt-schema. Hmm, not sure what were we thinking.
We didn't have a schema for Marvell, so we weren't even seeing many of
the validation errors that you're now uncovering.

> Isn't there some in-kernel DTS file with a *good* example of how
> a Marvell mv88e6xxx switch is supposed to look I can just
> copy instead? We shouldn't conjure synthetic examples.

(...)

> I'm game. Point out the DTS file and I will take that.

You can use https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/imx6qdl-gw5904.dtsi#L211
(optionally renaming switch to ethernet-switch, and ports to ethernet-ports).
That uses the subset of the mv88e6xxx kernel bindings that U-Boot also
understands, so taking a U-Boot example is actually preferable.

> > One other thing I see as a deal breaker for this schema conversion is
> > that $nodename for Marvell needs to allow basically anything (invalidating
> > the constraint from ethernet-switch.yaml), because we can't change node
> > names in the case of some boards, otherwise we risk breaking them
> > (see MOX). If the schema starts emitting warnings for those node names,
> > then it's inevitable that some pixie in the future will eventually break
> > them by "fixing" the node name.
> 
> I already did a bit of hippo-in-china-porcelain store in the patches
> in this series mostly renaming things like "switch0@0" to "switch@0"
> (yeah that's all).
> 
> Is this part of the problem or something else?

Yes, for most of the switches, renaming their OF nodes should not be a problem.

For Marvell, I'd exercise extra caution and only rename those OF nodes
where I can confirm that doing so won't break anything. Marvell is one
of the oldest DSA drivers, and you can tell that the bindings have gone
through a lot before becoming more or less uniform.

Anyway, for the $nodename constraint, it _looks_ all mechanical and trivial
to fix (unlike the missing phy-mode that you point to, above), so someone
will jump to fix it. I would like to avoid that, because boot testing
will be key, and a board is not always available.

Powered by blists - more mailing lists