Re: [PATCH net-next v7 5/7] ARM64: dts: marvell: Fix some common switch mistakes

[Linus Walleij]

Hi Vladimir,

thanks for paging in the right maintainers to look at the respective boards,
much appreciated!

On Tue, Oct 24, 2023 at 8:28 PM Vladimir Oltean <olteanv@xxxxxxxxx> wrote:

> I looked at U-Boot's ft_board_setup() from board/Marvell/mvebu_armada-37xx/board.c
> and it doesn't appear to do anything with the switch. But after the MOX precedent
> (which is _still_ problematic, more below), I still think we are way too
> trigger-happy with this, and it would be good to ask someone who has the
> Espressobin to test.

Yeah that would be great.

> > -     /* switch nodes are enabled by U-Boot if modules are present */
> > +     /*
> > +      * NOTE: switch nodes are enabled by U-Boot if modules are present
> > +      * DO NOT change this node name (switch0@10) even if it is not following
> > +      * conventions! Deployed U-Boot binaries are explicitly looking for
> > +      * this node in order to augment the device tree!
> > +      */
>
> Not "this node", but all switch nodes!
(...)
> It's funny that you add a comment TO NOT rename switch nodes, then you
> proceed to do just that.

Yeah it's a stupid mistake on my behalf. :( too sleepy or something.

I fixed it up, and put a small comment above each of them not
to change the node name.

> > -             ports {
> > +             ethernet-ports {
>
> U-Boot code does this, so you can't rename "ports":
>
>         /*
>          * now if there are more switches or a SFP module coming after,
>          * enable corresponding ports
>          */
>         if (id < peridot + topaz - 1) {
>                 res = fdt_status_okay_by_pathf(blob,
>                                                "%s/switch%i@%x/ports/port@a",
>                                                mdio_path, id, addr);
>         } else if (id == peridot - 1 && !topaz && sfp) {
>                 res = fdt_status_okay_by_pathf(blob,
>                                                "%s/switch%i@%x/ports/port-sfp@a",
>                                                mdio_path, id, addr);
>         } else {
>                 res = 0;
>         }
>
> >                       #address-cells = <1>;
> >                       #size-cells = <0>;
> >
> > -                     port@1 {
> > +                     ethernet-port@1 {
>
> or "port@.*", or "port-sfp@a", for the same reason. Here and everywhere
> in this device tree. Basically only the ethernet-phy rename seems safe.

Fair, reverted it all.

> Having that said, we need to suppress these warnings for the Marvell
> schema only:
>
> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: switch0@10: $nodename:0: 'switch0@10' does not match '^(ethernet-)?switch(@.*)?$'
>         from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml#
> arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dtb: ethernet-switch@12: ethernet-ports: 'port-sfp@a' does not match any of the regexes: '^(ethernet-)?port@[0-9]+$', 'pinctrl-[0-9]+'
>         from schema $id: http://devicetree.org/schemas/net/dsa/marvell,mv88e6xxx.yaml#
>
> because someone _will_ fix them and break the boot in the process.

Really? I think you will stop them from doing that every single time ;)

Jokes aside, we certainly need a way to suppress this warning.

> Rob, Krzysztof, Conor, do you have any suggestion on how to achieve that?

What we can do easily is to override the $nodename requirement for
a certain compatible with one of those - if: constructions, but that would
unfortunately make us be lax on every other board as well.

What we want to achieve is:

1. Match on the top level compatible (under '/') with contains: const:
cznic,turris-mox

2. Then relax requirements on the switch nodes if that is true.

I assume I would have to go into
Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
and put hard requirement on node names from there. I'm not sure
this would work or that it's even possible, or desireable.

But...

We  *COULD* add a second over-specified compatible to the switch
node. Such as:

      switch0@10 {
                compatible = "marvell,turris-mox-mv88e6190-switch",
"marvell,mv88e6190";

(and the same for the 6085 version)

And use that to relax the requirement for that variant with an - if:
statemement.

This should work fine since U-Boot is only looking for nodenames, not
compatible strings. I think I will try this approach.

Yours,
Linus Walleij