diff mbox series

[RFC,v2,4/4] arm64: dts: rockchip: Add mt7531 dsa node to BPI-R2-Pro board

Message ID 20220430130347.15190-5-linux@fw-web.de (mailing list archive)
State Superseded
Headers show
Series Support mt7531 on BPI-R2 Pro | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Frank Wunderlich April 30, 2022, 1:03 p.m. UTC
From: Frank Wunderlich <frank-w@public-files.de>

Add Device Tree node for mt7531 switch connected to gmac0.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
v2:
- drop status=disabled
---
 .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts   | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Heiko Stübner April 30, 2022, 2:05 p.m. UTC | #1
Am Samstag, 30. April 2022, 15:03:47 CEST schrieb Frank Wunderlich:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add Device Tree node for mt7531 switch connected to gmac0.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v2:
> - drop status=disabled
> ---
>  .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts   | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> index ed2f2bd9a016..f0ffbe818170 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> @@ -437,6 +437,54 @@ &i2c5 {
>  	status = "disabled";
>  };
>  
> +&mdio0 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	switch@0 {
> +		compatible = "mediatek,mt7531";
> +		reg = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@1 {
> +				reg = <1>;
> +				label = "lan0";
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				label = "lan1";
> +			};
> +
> +			port@3 {
> +				reg = <3>;
> +				label = "lan2";
> +			};
> +
> +			port@4 {
> +				reg = <4>;
> +				label = "lan3";
> +			};
> +
> +			port@5 {
> +				reg = <5>;
> +				label = "cpu";
> +				ethernet = <&gmac0>;
> +				phy-mode = "rgmii";
- phy-mode: String, the following values are acceptable for port labeled
	"cpu":
	If compatible mediatek,mt7530 or mediatek,mt7621 is set,
	must be either "trgmii" or "rgmii"
	If compatible mediatek,mt7531 is set,
	must be either "sgmii", "1000base-x" or "2500base-x"

So I guess the phy-mode needs to change?


Heiko

> +
> +				fixed-link {
> +					speed = <1000>;
> +					full-duplex;
> +					pause;
> +				};
> +			};
> +		};
> +	};
> +};
> +
>  &mdio1 {
>  	rgmii_phy1: ethernet-phy@0 {
>  		compatible = "ethernet-phy-ieee802.3-c22";
>
Frank Wunderlich April 30, 2022, 2:17 p.m. UTC | #2
Am 30. April 2022 16:05:06 MESZ schrieb Heiko Stuebner <heiko@sntech.de>:
>Am Samstag, 30. April 2022, 15:03:47 CEST schrieb Frank Wunderlich:

>> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
>> @@ -437,6 +437,54 @@ &i2c5 {
>>  	status = "disabled";
>>  };
>>  

>> +			port@5 {
>> +				reg = <5>;
>> +				label = "cpu";
>> +				ethernet = <&gmac0>;
>> +				phy-mode = "rgmii";
>- phy-mode: String, the following values are acceptable for port
>labeled
>	"cpu":
>	If compatible mediatek,mt7530 or mediatek,mt7621 is set,
>	must be either "trgmii" or "rgmii"
>	If compatible mediatek,mt7531 is set,
>	must be either "sgmii", "1000base-x" or "2500base-x"
>
>So I guess the phy-mode needs to change?

This results from current (before my cpu-port patches) implementation in driver where cpu-port is fixed to port 6.

On Mt7530 port 6 supports rgmii and trgmii. On mt7531 port6 only supports sgmii (which is basicly 2500base-x). Afaik it does not support 1G. Port 5 on mt7531 supports rgmii or sgmii (dual sgmii mode) but seems not reflected yet in txt.

On thing more to change when converting txt to yaml.

regards Frank
Vladimir Oltean May 4, 2022, 3:24 p.m. UTC | #3
On Sat, Apr 30, 2022 at 03:03:47PM +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add Device Tree node for mt7531 switch connected to gmac0.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
> v2:
> - drop status=disabled
> ---
>  .../boot/dts/rockchip/rk3568-bpi-r2-pro.dts   | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> index ed2f2bd9a016..f0ffbe818170 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
> @@ -437,6 +437,54 @@ &i2c5 {
>  	status = "disabled";
>  };
>  
> +&mdio0 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	switch@0 {

I think the preferable names are the newer "ethernet-switch@0",
"ethernet-ports", "ethernet-port@0".

Otherwise

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

> +		compatible = "mediatek,mt7531";
> +		reg = <0>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@1 {
> +				reg = <1>;
> +				label = "lan0";
> +			};
> +
> +			port@2 {
> +				reg = <2>;
> +				label = "lan1";
> +			};
> +
> +			port@3 {
> +				reg = <3>;
> +				label = "lan2";
> +			};
> +
> +			port@4 {
> +				reg = <4>;
> +				label = "lan3";
> +			};
> +
> +			port@5 {
> +				reg = <5>;
> +				label = "cpu";
> +				ethernet = <&gmac0>;
> +				phy-mode = "rgmii";
> +
> +				fixed-link {
> +					speed = <1000>;
> +					full-duplex;
> +					pause;
> +				};
> +			};
> +		};
> +	};
> +};
> +
>  &mdio1 {
>  	rgmii_phy1: ethernet-phy@0 {
>  		compatible = "ethernet-phy-ieee802.3-c22";
> -- 
> 2.25.1
>
Frank Wunderlich May 4, 2022, 3:33 p.m. UTC | #4
Hi,

thanks for review

> Gesendet: Mittwoch, 04. Mai 2022 um 17:24 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>

> > +&mdio0 {
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	switch@0 {
>
> I think the preferable names are the newer "ethernet-switch@0",
> "ethernet-ports", "ethernet-port@0".
>
> Otherwise
>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

current device-tree nodes using "switch" and "ports"

see discussioon here about make it fixed to "ports" property instead of PatternProperties including optional "ethernet-"

https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155

>
> > +		compatible = "mediatek,mt7531";
> > +		reg = <0>;
> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			port@1 {
> > +				reg = <1>;
> > +				label = "lan0";
> > +			};
> > +
> > +			port@2 {
> > +				reg = <2>;
> > +				label = "lan1";
> > +			};
> > +
> > +			port@3 {
> > +				reg = <3>;
> > +				label = "lan2";
> > +			};
> > +
> > +			port@4 {
> > +				reg = <4>;
> > +				label = "lan3";
> > +			};
> > +
> > +			port@5 {
> > +				reg = <5>;
> > +				label = "cpu";
> > +				ethernet = <&gmac0>;
> > +				phy-mode = "rgmii";
> > +
> > +				fixed-link {
> > +					speed = <1000>;
> > +					full-duplex;
> > +					pause;
> > +				};
> > +			};
> > +		};
> > +	};
Vladimir Oltean May 4, 2022, 3:47 p.m. UTC | #5
On Wed, May 04, 2022 at 05:33:20PM +0200, Frank Wunderlich wrote:
> Hi,
> 
> thanks for review
> 
> > Gesendet: Mittwoch, 04. Mai 2022 um 17:24 Uhr
> > Von: "Vladimir Oltean" <olteanv@gmail.com>
> 
> > > +&mdio0 {
> > > +	#address-cells = <1>;
> > > +	#size-cells = <0>;
> > > +
> > > +	switch@0 {
> >
> > I think the preferable names are the newer "ethernet-switch@0",
> > "ethernet-ports", "ethernet-port@0".
> >
> > Otherwise
> >
> > Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> 
> current device-tree nodes using "switch" and "ports"
> 
> see discussioon here about make it fixed to "ports" property instead of PatternProperties including optional "ethernet-"
> 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155

Hmm, I don't get why Krzysztof said to just keep what is used in
existing device trees. The schema validator should describe what is
valid, and since the mt7530 driver does not care one way or another
(some drivers do explicitly parse the "ports"/"ethernet-ports" node),
then whatever is valid for the DSA core is also valid for the mt7530
bindings. And "ethernet-ports" is valid too, so I think it should be
accepted by mediatek.yaml...
Krzysztof Kozlowski May 5, 2022, 8:46 a.m. UTC | #6
On 04/05/2022 17:47, Vladimir Oltean wrote:
>>
>> current device-tree nodes using "switch" and "ports"
>>
>> see discussioon here about make it fixed to "ports" property instead of PatternProperties including optional "ethernet-"
>>
>> https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155
> 
> Hmm, I don't get why Krzysztof said to just keep what is used in
> existing device trees. The schema validator should describe what is
> valid, 

These were talks about bindings which describe hardware. The node name,
except Devicetree spec asking for generic names, does not matter here
actually.

> and since the mt7530 driver does not care one way or another
> (some drivers do explicitly parse the "ports"/"ethernet-ports" node),
> then whatever is valid for the DSA core is also valid for the mt7530
> bindings. And "ethernet-ports" is valid too, so I think it should be
> accepted by mediatek.yaml...

You can make it "(ethernet-)?ports" as well. My comment was purely to
make it simpler, for bindings (goes into properties, not
patternProperties) and for us. If you prefer to keep it like DSA core,
also fine.

Best regards,
Krzysztof
Frank Wunderlich May 5, 2022, 9:06 a.m. UTC | #7
Am 5. Mai 2022 10:46:55 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>On 04/05/2022 17:47, Vladimir Oltean wrote:
>>>
>>> current device-tree nodes using "switch" and "ports"
>>>
>>> see discussioon here about make it fixed to "ports" property instead
>of PatternProperties including optional "ethernet-"
>>>
>>>
>https://patchwork.kernel.org/project/linux-mediatek/patch/20220502153238.85090-1-linux@fw-web.de/#24843155
>> 
>> Hmm, I don't get why Krzysztof said to just keep what is used in
>> existing device trees. The schema validator should describe what is
>> valid, 
>
>These were talks about bindings which describe hardware. The node name,
>except Devicetree spec asking for generic names, does not matter here
>actually.
>
>> and since the mt7530 driver does not care one way or another
>> (some drivers do explicitly parse the "ports"/"ethernet-ports" node),
>> then whatever is valid for the DSA core is also valid for the mt7530
>> bindings. And "ethernet-ports" is valid too, so I think it should be
>> accepted by mediatek.yaml...
>
>You can make it "(ethernet-)?ports" as well. My comment was purely to
>make it simpler, for bindings (goes into properties, not
>patternProperties) and for us. If you prefer to keep it like DSA core,
>also fine.

Ok, i'm also thinking, the dsa-definition will be the right way (pattern-properties with optional "ethernet-") in binding.

Should i use "ethernet-ports" instead of "ports" here? Current dts with mt7530/mt7531 switches using "ports" so i would use it here too. If dsa prefer ethernet-ports now it should be changed in other files too.

>Best regards,
>Krzysztof

Hi,
regards Frank
Krzysztof Kozlowski May 5, 2022, 9:22 a.m. UTC | #8
On 05/05/2022 11:06, Frank Wunderlich wrote:
>> You can make it "(ethernet-)?ports" as well. My comment was purely to
>> make it simpler, for bindings (goes into properties, not
>> patternProperties) and for us. If you prefer to keep it like DSA core,
>> also fine.
> 
> Ok, i'm also thinking, the dsa-definition will be the right way (pattern-properties with optional "ethernet-") in binding.
> 
> Should i use "ethernet-ports" instead of "ports" here? Current dts with mt7530/mt7531 switches using "ports" so i would use it here too. If dsa prefer ethernet-ports now it should be changed in other files too.

I think bindings allow both, so choose something consistent with
existing DTS sources.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
index ed2f2bd9a016..f0ffbe818170 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts
@@ -437,6 +437,54 @@  &i2c5 {
 	status = "disabled";
 };
 
+&mdio0 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	switch@0 {
+		compatible = "mediatek,mt7531";
+		reg = <0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@1 {
+				reg = <1>;
+				label = "lan0";
+			};
+
+			port@2 {
+				reg = <2>;
+				label = "lan1";
+			};
+
+			port@3 {
+				reg = <3>;
+				label = "lan2";
+			};
+
+			port@4 {
+				reg = <4>;
+				label = "lan3";
+			};
+
+			port@5 {
+				reg = <5>;
+				label = "cpu";
+				ethernet = <&gmac0>;
+				phy-mode = "rgmii";
+
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+					pause;
+				};
+			};
+		};
+	};
+};
+
 &mdio1 {
 	rgmii_phy1: ethernet-phy@0 {
 		compatible = "ethernet-phy-ieee802.3-c22";