Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
References: <20231018-marvell-88e6152-wan-led-v4-0-3ee0c67383be@linaro.org>
 <20231018-marvell-88e6152-wan-led-v4-6-3ee0c67383be@linaro.org> <20231019153552.nndysafvblrkl2zn@skbuf>
In-Reply-To: <20231019153552.nndysafvblrkl2zn@skbuf>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Fri, 20 Oct 2023 14:47:20 +0200
Message-ID: <CACRpkdbskk22SLmopUTD78kMWL_gcOa=YWHLFtrkDAD5=W=HFw@mail.gmail.com>
Subject: Re: [PATCH net-next v4 6/7] dt-bindings: marvell: Rewrite MV88E6xxx
 in schema
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        =?UTF-8?B?TWFyZWsgQmVow7pu?= <kabel@kernel.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Christian Marangi <ansuelsmth@gmail.com>,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 19, 2023 at 5:35=E2=80=AFPM Vladimir Oltean <olteanv@gmail.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.

> So either:
>
> - you delete the "mdio" node and the ethernet-phys under it, or
> - you add all ethernet-phys under the mdio node, and put phy-handles
>   from ports to each of them, and phy-modes of "internal"
>
> 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#

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 don't want to see DT examples that are structurally broken, sorry,
> because then we wonder why users are confused.

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!

> Personally, I would opt for adding the more modern explicit phy-handle
> and phy-mode everywhere.

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

> Also, you seem to have duplicated some work also done by Ar=C4=B1n=C3=A7 =
but not
> finalized (the mv88e6xxx schema conversion, on which you were also
> copied). Let me add Marek here too, to make sure he's aware of 2
> previous attempts and doesn't start working on a 3rd one :)

Haha I forgot :D

> One other thing I see as a deal breaker for this schema conversion is
> that $nodename for Marvell needs to allow basically anything (invalidatin=
g
> 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?

I run the binding checks across all aarch64 and armv7_multi platforms
on this patch set without any major issues.

Yours,
Linus Walleij