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

From: Vladimir Oltean <olteanv@gmail.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "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>,
	"Marek Behún" <kabel@kernel.org>,
	"Christian Marangi" <ansuelsmth@gmail.com>,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	"Florian Fainelli" <florian.fainelli@broadcom.com>
Subject: Re: [PATCH net-next v8 5/9] ARM64: dts: marvell: Fix some common switch mistakes
Date: Wed, 29 Nov 2023 20:18:42 +0200	[thread overview]
Message-ID: <20231129181842.74zihcd642lx5zgo@skbuf> (raw)
In-Reply-To: <20231114-marvell-88e6152-wan-led-v8-5-50688741691b@linaro.org>

On Tue, Nov 14, 2023 at 12:36:00AM +0100, Linus Walleij wrote:
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> index 9eab2bb22134..66cd98b67744 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -304,7 +304,13 @@ phy1: ethernet-phy@1 {
>  		reg = <1>;
>  	};
>  
> -	/* 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!
> +	 * Also do not touch the "ports" or "port@n" nodes. These are also ABI.
> +	 */
>  	switch0@10 {
>  		compatible = "marvell,mv88e6190";
>  		reg = <0x10>;
> @@ -430,6 +436,7 @@ port-sfp@a {
>  		};
>  	};
>  
> +	/* NOTE: this node name is ABI, don't change it! */
>  	switch0@2 {
>  		compatible = "marvell,mv88e6085";
>  		reg = <0x2>;
> @@ -497,6 +504,7 @@ port@5 {
>  		};
>  	};
>  
> +	/* NOTE: this node name is ABI, don't change it! */
>  	switch1@11 {
>  		compatible = "marvell,mv88e6190";
>  		reg = <0x11>;
> @@ -622,6 +630,7 @@ port-sfp@a {
>  		};
>  	};
>  
> +	/* NOTE: this node name is ABI, don't change it! */
>  	switch1@2 {
>  		compatible = "marvell,mv88e6085";
>  		reg = <0x2>;
> @@ -689,6 +698,7 @@ port@5 {
>  		};
>  	};
>  
> +	/* NOTE: this node name is ABI, don't change it! */
>  	switch2@12 {
>  		compatible = "marvell,mv88e6190";
>  		reg = <0x12>;
> @@ -805,6 +815,7 @@ port-sfp@a {
>  		};
>  	};
>  
> +	/* NOTE: this node name is ABI, don't change it! */
>  	switch2@2 {
>  		compatible = "marvell,mv88e6085";
>  		reg = <0x2>;

I wouldn't spam the device tree with all these comments; doing so gives
a false sense of completeness. Code inspection shows that the "port-sfp@a"
node name is also established ABI, but there isn't any explicit comment
to point that out. I think a single comment that uses plural to refer to
all nodes should be enough.

Also, doesn't the comment go better along with the patch that changes
the switch compatible strings, rather than with the patch that actually
fixes what can be fixed (ethernet-phy node names)?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel