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
