[RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

MT7621 is a SoC, so using "mediatek,mt7621" as its compatible is ambiguous.
Rename it to "mediatek,mt7621-gsw".

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/mt7530.txt | 8 ++++----
 drivers/net/dsa/mt7530.c                             | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
index 560369efad6c..a9c8492296b3 100644
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -4,7 +4,7 @@ Mediatek MT7530 Ethernet switch
 Required properties:
 
 - compatible: may be compatible = "mediatek,mt7530"
-	or compatible = "mediatek,mt7621"
+	or compatible = "mediatek,mt7621-gsw"
 	or compatible = "mediatek,mt7531"
 - #address-cells: Must be 1.
 - #size-cells: Must be 0.
@@ -35,7 +35,7 @@ Required properties for the child nodes within ports container:
 	user ports.
 - phy-mode: String, the following values are acceptable for port labeled
 	"cpu":
-	If compatible mediatek,mt7530 or mediatek,mt7621 is set,
+	If compatible mediatek,mt7530 or mediatek,mt7621-gsw is set,
 	must be either "trgmii" or "rgmii"
 	If compatible mediatek,mt7531 is set,
 	must be either "sgmii", "1000base-x" or "2500base-x"
@@ -168,7 +168,7 @@ Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
 		};
 
 		mt7530: switch@1f {
-			compatible = "mediatek,mt7621";
+			compatible = "mediatek,mt7621-gsw";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0x1f>;
@@ -251,7 +251,7 @@ Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
 		};
 
 		mt7530: switch@1f {
-			compatible = "mediatek,mt7621";
+			compatible = "mediatek,mt7621-gsw";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0x1f>;
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 55c8baa31e5d..347845d66671 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
 };
 
 static const struct of_device_id mt7530_of_match[] = {
-	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
+	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
 	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
 	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
 	{ /* sentinel */ },
-- 
2.25.1

[RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

MT7621 is a SoC, so using "mediatek,mt7621" as its compatible is ambiguous.
Rename it to "mediatek,mt7621-gsw".

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/mt7530.txt | 8 ++++----
 drivers/net/dsa/mt7530.c                             | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
index 560369efad6c..a9c8492296b3 100644
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -4,7 +4,7 @@ Mediatek MT7530 Ethernet switch
 Required properties:
 
 - compatible: may be compatible = "mediatek,mt7530"
-	or compatible = "mediatek,mt7621"
+	or compatible = "mediatek,mt7621-gsw"
 	or compatible = "mediatek,mt7531"
 - #address-cells: Must be 1.
 - #size-cells: Must be 0.
@@ -35,7 +35,7 @@ Required properties for the child nodes within ports container:
 	user ports.
 - phy-mode: String, the following values are acceptable for port labeled
 	"cpu":
-	If compatible mediatek,mt7530 or mediatek,mt7621 is set,
+	If compatible mediatek,mt7530 or mediatek,mt7621-gsw is set,
 	must be either "trgmii" or "rgmii"
 	If compatible mediatek,mt7531 is set,
 	must be either "sgmii", "1000base-x" or "2500base-x"
@@ -168,7 +168,7 @@ Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
 		};
 
 		mt7530: switch@1f {
-			compatible = "mediatek,mt7621";
+			compatible = "mediatek,mt7621-gsw";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0x1f>;
@@ -251,7 +251,7 @@ Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
 		};
 
 		mt7530: switch@1f {
-			compatible = "mediatek,mt7621";
+			compatible = "mediatek,mt7621-gsw";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			reg = <0x1f>;
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 55c8baa31e5d..347845d66671 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
 };
 
 static const struct of_device_id mt7530_of_match[] = {
-	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
+	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
 	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
 	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
 	{ /* sentinel */ },
-- 
2.25.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Andrew Lunn]

> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
>  };
>  
>  static const struct of_device_id mt7530_of_match[] = {
> -	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> +	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
>  	{ /* sentinel */ },

This will break backwards compatibility with existing DT blobs. You
need to keep the old "mediatek,mt7621", but please add a comment that
it is deprecated.

   Andrew

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Andrew Lunn]

> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
>  };
>  
>  static const struct of_device_id mt7530_of_match[] = {
> -	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> +	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
>  	{ /* sentinel */ },

This will break backwards compatibility with existing DT blobs. You
need to keep the old "mediatek,mt7621", but please add a comment that
it is deprecated.

   Andrew

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Florian Fainelli]

On 12/19/2020 8:26 AM, Andrew Lunn wrote:
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
>>  };
>>  
>>  static const struct of_device_id mt7530_of_match[] = {
>> -	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
>> +	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
>>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
>>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
>>  	{ /* sentinel */ },
> 
> This will break backwards compatibility with existing DT blobs. You
> need to keep the old "mediatek,mt7621", but please add a comment that
> it is deprecated.

Besides, adding -gsw would make it inconsistent with the existing
matching compatible strings. While it's not ideal to have the same
top-level SoC compatible and having another sub-node within that SoC's
DTS have the same compatible, given this would be break backwards
compatibility, cannot you stay with what is defined today?
-- 
Florian

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Florian Fainelli]

On 12/19/2020 8:26 AM, Andrew Lunn wrote:
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
>>  };
>>  
>>  static const struct of_device_id mt7530_of_match[] = {
>> -	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
>> +	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
>>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
>>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
>>  	{ /* sentinel */ },
> 
> This will break backwards compatibility with existing DT blobs. You
> need to keep the old "mediatek,mt7621", but please add a comment that
> it is deprecated.

Besides, adding -gsw would make it inconsistent with the existing
matching compatible strings. While it's not ideal to have the same
top-level SoC compatible and having another sub-node within that SoC's
DTS have the same compatible, given this would be break backwards
compatibility, cannot you stay with what is defined today?
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

Hi Andrew, Florian,

On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote:
> On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> >> --- a/drivers/net/dsa/mt7530.c
> >> +++ b/drivers/net/dsa/mt7530.c
> >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> >>  };
> >>  
> >>  static const struct of_device_id mt7530_of_match[] = {
> >> -	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> >> +	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> >>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> >>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> >>  	{ /* sentinel */ },
> > 
> > This will break backwards compatibility with existing DT blobs. You
> > need to keep the old "mediatek,mt7621", but please add a comment that
> > it is deprecated.
> 
> Besides, adding -gsw would make it inconsistent with the existing
> matching compatible strings. While it's not ideal to have the same
> top-level SoC compatible and having another sub-node within that SoC's
> DTS have the same compatible, given this would be break backwards
> compatibility, cannot you stay with what is defined today?

The MT7621 device tree is in staging. I suppose that some amount of
breaking changes could be tolerated?

But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi.

/ethernet@1e100000/mdio-bus {
	switch0: switch0@0 {
		compatible = "mediatek,mt7621";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0>;
		mediatek,mcm;
		resets = <&rstctrl 2>;
		reset-names = "mcm";

		ports {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0>;
			port@0 {
				status = "off";
				reg = <0>;
				label = "lan0";
			};
			port@1 {
				status = "off";
				reg = <1>;
				label = "lan1";
			};
			port@2 {
				status = "off";
				reg = <2>;
				label = "lan2";
			};
			port@3 {
				status = "off";
				reg = <3>;
				label = "lan3";
			};
			port@4 {
				status = "off";
				reg = <4>;
				label = "lan4";
			};
			port@6 {
				reg = <6>;
				label = "cpu";
				ethernet = <&gmac0>;
				phy-mode = "trgmii";
				fixed-link {
					speed = <1000>;
					full-duplex;
				};
			};
		};
	};
};

/ {
	gsw: gsw@1e110000 {
		compatible = "mediatek,mt7621-gsw";
		reg = <0x1e110000 0x8000>;
		interrupt-parent = <&gic>;
		interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
	};
};

What is the platform device at the memory address 1e110000?
There is no driver for it. The documentation only has me even more
confused:

Mediatek Gigabit Switch
=======================

The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621).

Required properties:
- compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw"
- reg: Address and length of the register set for the device
- interrupts: Should contain the gigabit switches interrupt
- resets: Should contain the gigabit switches resets
- reset-names: Should contain the reset names "gsw"

Example:

gsw@10110000 {
	compatible = "ralink,mt7620-gsw";     <- notice how even the example is bad and inconsistent
	reg = <0x10110000 8000>;

	resets = <&rstctrl 23>;
	reset-names = "gsw";

	interrupt-parent = <&intc>;
	interrupts = <17>;
};

Does the MT7621 contain two Ethernet switches, one accessed over MMIO
and another over MDIO? Or is it the same switch? I don't understand.
What is the relationship between the new compatible that you're
proposing, Qingfang, and the existing device tree bindings?

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

Hi Andrew, Florian,

On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote:
> On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> >> --- a/drivers/net/dsa/mt7530.c
> >> +++ b/drivers/net/dsa/mt7530.c
> >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> >>  };
> >>  
> >>  static const struct of_device_id mt7530_of_match[] = {
> >> -	{ .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> >> +	{ .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> >>  	{ .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> >>  	{ .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> >>  	{ /* sentinel */ },
> > 
> > This will break backwards compatibility with existing DT blobs. You
> > need to keep the old "mediatek,mt7621", but please add a comment that
> > it is deprecated.
> 
> Besides, adding -gsw would make it inconsistent with the existing
> matching compatible strings. While it's not ideal to have the same
> top-level SoC compatible and having another sub-node within that SoC's
> DTS have the same compatible, given this would be break backwards
> compatibility, cannot you stay with what is defined today?

The MT7621 device tree is in staging. I suppose that some amount of
breaking changes could be tolerated?

But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi.

/ethernet@1e100000/mdio-bus {
	switch0: switch0@0 {
		compatible = "mediatek,mt7621";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0>;
		mediatek,mcm;
		resets = <&rstctrl 2>;
		reset-names = "mcm";

		ports {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0>;
			port@0 {
				status = "off";
				reg = <0>;
				label = "lan0";
			};
			port@1 {
				status = "off";
				reg = <1>;
				label = "lan1";
			};
			port@2 {
				status = "off";
				reg = <2>;
				label = "lan2";
			};
			port@3 {
				status = "off";
				reg = <3>;
				label = "lan3";
			};
			port@4 {
				status = "off";
				reg = <4>;
				label = "lan4";
			};
			port@6 {
				reg = <6>;
				label = "cpu";
				ethernet = <&gmac0>;
				phy-mode = "trgmii";
				fixed-link {
					speed = <1000>;
					full-duplex;
				};
			};
		};
	};
};

/ {
	gsw: gsw@1e110000 {
		compatible = "mediatek,mt7621-gsw";
		reg = <0x1e110000 0x8000>;
		interrupt-parent = <&gic>;
		interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
	};
};

What is the platform device at the memory address 1e110000?
There is no driver for it. The documentation only has me even more
confused:

Mediatek Gigabit Switch
=======================

The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621).

Required properties:
- compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw"
- reg: Address and length of the register set for the device
- interrupts: Should contain the gigabit switches interrupt
- resets: Should contain the gigabit switches resets
- reset-names: Should contain the reset names "gsw"

Example:

gsw@10110000 {
	compatible = "ralink,mt7620-gsw";     <- notice how even the example is bad and inconsistent
	reg = <0x10110000 8000>;

	resets = <&rstctrl 23>;
	reset-names = "gsw";

	interrupt-parent = <&intc>;
	interrupts = <17>;
};

Does the MT7621 contain two Ethernet switches, one accessed over MMIO
and another over MDIO? Or is it the same switch? I don't understand.
What is the relationship between the new compatible that you're
proposing, Qingfang, and the existing device tree bindings?

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

Hi Vladimir,

On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Andrew, Florian,
>
> On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote:
> > On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> > >> --- a/drivers/net/dsa/mt7530.c
> > >> +++ b/drivers/net/dsa/mt7530.c
> > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> > >>  };
> > >>
> > >>  static const struct of_device_id mt7530_of_match[] = {
> > >> -  { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> > >> +  { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> > >>    { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> > >>    { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> > >>    { /* sentinel */ },
> > >
> > > This will break backwards compatibility with existing DT blobs. You
> > > need to keep the old "mediatek,mt7621", but please add a comment that
> > > it is deprecated.
> >
> > Besides, adding -gsw would make it inconsistent with the existing
> > matching compatible strings. While it's not ideal to have the same
> > top-level SoC compatible and having another sub-node within that SoC's
> > DTS have the same compatible, given this would be break backwards
> > compatibility, cannot you stay with what is defined today?
>
> The MT7621 device tree is in staging. I suppose that some amount of
> breaking changes could be tolerated?
>
> But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi.
>
> /ethernet@1e100000/mdio-bus {
>         switch0: switch0@0 {
>                 compatible = "mediatek,mt7621";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 reg = <0>;
>                 mediatek,mcm;
>                 resets = <&rstctrl 2>;
>                 reset-names = "mcm";
>
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         reg = <0>;
>                         port@0 {
>                                 status = "off";
>                                 reg = <0>;
>                                 label = "lan0";
>                         };
>                         port@1 {
>                                 status = "off";
>                                 reg = <1>;
>                                 label = "lan1";
>                         };
>                         port@2 {
>                                 status = "off";
>                                 reg = <2>;
>                                 label = "lan2";
>                         };
>                         port@3 {
>                                 status = "off";
>                                 reg = <3>;
>                                 label = "lan3";
>                         };
>                         port@4 {
>                                 status = "off";
>                                 reg = <4>;
>                                 label = "lan4";
>                         };
>                         port@6 {
>                                 reg = <6>;
>                                 label = "cpu";
>                                 ethernet = <&gmac0>;
>                                 phy-mode = "trgmii";
>                                 fixed-link {
>                                         speed = <1000>;
>                                         full-duplex;
>                                 };
>                         };
>                 };
>         };
> };
>
> / {
>         gsw: gsw@1e110000 {
>                 compatible = "mediatek,mt7621-gsw";
>                 reg = <0x1e110000 0x8000>;
>                 interrupt-parent = <&gic>;
>                 interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
>         };
> };
>
> What is the platform device at the memory address 1e110000?
> There is no driver for it. The documentation only has me even more
> confused:
>
> Mediatek Gigabit Switch
> =======================
>
> The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621).
>
> Required properties:
> - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw"
> - reg: Address and length of the register set for the device
> - interrupts: Should contain the gigabit switches interrupt
> - resets: Should contain the gigabit switches resets
> - reset-names: Should contain the reset names "gsw"
>
> Example:
>
> gsw@10110000 {
>         compatible = "ralink,mt7620-gsw";     <- notice how even the example is bad and inconsistent
>         reg = <0x10110000 8000>;
>
>         resets = <&rstctrl 23>;
>         reset-names = "gsw";
>
>         interrupt-parent = <&intc>;
>         interrupts = <17>;
> };
>
> Does the MT7621 contain two Ethernet switches, one accessed over MMIO
> and another over MDIO? Or is it the same switch? I don't understand.
> What is the relationship between the new compatible that you're
> proposing, Qingfang, and the existing device tree bindings?

The current dtsi is copied from OpenWrt, so the existing "mt7621-gsw"
/ "mt7620-gsw" compatible is for their swconfig driver.
MT7621 has only one switch, accessed over MDIO, so the reg property
has no effect.

Should this patch be accepted, the existing gsw nodes can be dropped.

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

Hi Vladimir,

On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Andrew, Florian,
>
> On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote:
> > On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> > >> --- a/drivers/net/dsa/mt7530.c
> > >> +++ b/drivers/net/dsa/mt7530.c
> > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> > >>  };
> > >>
> > >>  static const struct of_device_id mt7530_of_match[] = {
> > >> -  { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> > >> +  { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> > >>    { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> > >>    { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> > >>    { /* sentinel */ },
> > >
> > > This will break backwards compatibility with existing DT blobs. You
> > > need to keep the old "mediatek,mt7621", but please add a comment that
> > > it is deprecated.
> >
> > Besides, adding -gsw would make it inconsistent with the existing
> > matching compatible strings. While it's not ideal to have the same
> > top-level SoC compatible and having another sub-node within that SoC's
> > DTS have the same compatible, given this would be break backwards
> > compatibility, cannot you stay with what is defined today?
>
> The MT7621 device tree is in staging. I suppose that some amount of
> breaking changes could be tolerated?
>
> But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi.
>
> /ethernet@1e100000/mdio-bus {
>         switch0: switch0@0 {
>                 compatible = "mediatek,mt7621";
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 reg = <0>;
>                 mediatek,mcm;
>                 resets = <&rstctrl 2>;
>                 reset-names = "mcm";
>
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         reg = <0>;
>                         port@0 {
>                                 status = "off";
>                                 reg = <0>;
>                                 label = "lan0";
>                         };
>                         port@1 {
>                                 status = "off";
>                                 reg = <1>;
>                                 label = "lan1";
>                         };
>                         port@2 {
>                                 status = "off";
>                                 reg = <2>;
>                                 label = "lan2";
>                         };
>                         port@3 {
>                                 status = "off";
>                                 reg = <3>;
>                                 label = "lan3";
>                         };
>                         port@4 {
>                                 status = "off";
>                                 reg = <4>;
>                                 label = "lan4";
>                         };
>                         port@6 {
>                                 reg = <6>;
>                                 label = "cpu";
>                                 ethernet = <&gmac0>;
>                                 phy-mode = "trgmii";
>                                 fixed-link {
>                                         speed = <1000>;
>                                         full-duplex;
>                                 };
>                         };
>                 };
>         };
> };
>
> / {
>         gsw: gsw@1e110000 {
>                 compatible = "mediatek,mt7621-gsw";
>                 reg = <0x1e110000 0x8000>;
>                 interrupt-parent = <&gic>;
>                 interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
>         };
> };
>
> What is the platform device at the memory address 1e110000?
> There is no driver for it. The documentation only has me even more
> confused:
>
> Mediatek Gigabit Switch
> =======================
>
> The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621).
>
> Required properties:
> - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw"
> - reg: Address and length of the register set for the device
> - interrupts: Should contain the gigabit switches interrupt
> - resets: Should contain the gigabit switches resets
> - reset-names: Should contain the reset names "gsw"
>
> Example:
>
> gsw@10110000 {
>         compatible = "ralink,mt7620-gsw";     <- notice how even the example is bad and inconsistent
>         reg = <0x10110000 8000>;
>
>         resets = <&rstctrl 23>;
>         reset-names = "gsw";
>
>         interrupt-parent = <&intc>;
>         interrupts = <17>;
> };
>
> Does the MT7621 contain two Ethernet switches, one accessed over MMIO
> and another over MDIO? Or is it the same switch? I don't understand.
> What is the relationship between the new compatible that you're
> proposing, Qingfang, and the existing device tree bindings?

The current dtsi is copied from OpenWrt, so the existing "mt7621-gsw"
/ "mt7620-gsw" compatible is for their swconfig driver.
MT7621 has only one switch, accessed over MDIO, so the reg property
has no effect.

Should this patch be accepted, the existing gsw nodes can be dropped.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

On Sun, Dec 20, 2020 at 12:48:08PM +0800, DENG Qingfang wrote:
> Hi Vladimir,
> 
> On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Hi Andrew, Florian,
> >
> > On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote:
> > > On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> > > >> --- a/drivers/net/dsa/mt7530.c
> > > >> +++ b/drivers/net/dsa/mt7530.c
> > > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> > > >>  };
> > > >>
> > > >>  static const struct of_device_id mt7530_of_match[] = {
> > > >> -  { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> > > >> +  { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> > > >>    { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> > > >>    { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> > > >>    { /* sentinel */ },
> > > >
> > > > This will break backwards compatibility with existing DT blobs. You
> > > > need to keep the old "mediatek,mt7621", but please add a comment that
> > > > it is deprecated.
> > >
> > > Besides, adding -gsw would make it inconsistent with the existing
> > > matching compatible strings. While it's not ideal to have the same
> > > top-level SoC compatible and having another sub-node within that SoC's
> > > DTS have the same compatible, given this would be break backwards
> > > compatibility, cannot you stay with what is defined today?
> >
> > The MT7621 device tree is in staging. I suppose that some amount of
> > breaking changes could be tolerated?
> >
> > But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi.
> >
> > /ethernet@1e100000/mdio-bus {
> >         switch0: switch0@0 {
> >                 compatible = "mediatek,mt7621";
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >                 reg = <0>;
> >                 mediatek,mcm;
> >                 resets = <&rstctrl 2>;
> >                 reset-names = "mcm";
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         reg = <0>;
> >                         port@0 {
> >                                 status = "off";
> >                                 reg = <0>;
> >                                 label = "lan0";
> >                         };
> >                         port@1 {
> >                                 status = "off";
> >                                 reg = <1>;
> >                                 label = "lan1";
> >                         };
> >                         port@2 {
> >                                 status = "off";
> >                                 reg = <2>;
> >                                 label = "lan2";
> >                         };
> >                         port@3 {
> >                                 status = "off";
> >                                 reg = <3>;
> >                                 label = "lan3";
> >                         };
> >                         port@4 {
> >                                 status = "off";
> >                                 reg = <4>;
> >                                 label = "lan4";
> >                         };
> >                         port@6 {
> >                                 reg = <6>;
> >                                 label = "cpu";
> >                                 ethernet = <&gmac0>;
> >                                 phy-mode = "trgmii";
> >                                 fixed-link {
> >                                         speed = <1000>;
> >                                         full-duplex;
> >                                 };
> >                         };
> >                 };
> >         };
> > };
> >
> > / {
> >         gsw: gsw@1e110000 {
> >                 compatible = "mediatek,mt7621-gsw";
> >                 reg = <0x1e110000 0x8000>;
> >                 interrupt-parent = <&gic>;
> >                 interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
> >         };
> > };
> >
> > What is the platform device at the memory address 1e110000?
> > There is no driver for it. The documentation only has me even more
> > confused:
> >
> > Mediatek Gigabit Switch
> > =======================
> >
> > The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621).
> >
> > Required properties:
> > - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw"
> > - reg: Address and length of the register set for the device
> > - interrupts: Should contain the gigabit switches interrupt
> > - resets: Should contain the gigabit switches resets
> > - reset-names: Should contain the reset names "gsw"
> >
> > Example:
> >
> > gsw@10110000 {
> >         compatible = "ralink,mt7620-gsw";     <- notice how even the example is bad and inconsistent
> >         reg = <0x10110000 8000>;
> >
> >         resets = <&rstctrl 23>;
> >         reset-names = "gsw";
> >
> >         interrupt-parent = <&intc>;
> >         interrupts = <17>;
> > };
> >
> > Does the MT7621 contain two Ethernet switches, one accessed over MMIO
> > and another over MDIO? Or is it the same switch? I don't understand.
> > What is the relationship between the new compatible that you're
> > proposing, Qingfang, and the existing device tree bindings?
> 
> The current dtsi is copied from OpenWrt, so the existing "mt7621-gsw"
> / "mt7620-gsw" compatible is for their swconfig driver.
> MT7621 has only one switch, accessed over MDIO, so the reg property
> has no effect.
> 
> Should this patch be accepted, the existing gsw nodes can be dropped.

But still, what is at memory address 0x1e110000, if the switch is
accessed over MDIO?

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

On Sun, Dec 20, 2020 at 12:48:08PM +0800, DENG Qingfang wrote:
> Hi Vladimir,
> 
> On Sun, Dec 20, 2020 at 3:48 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Hi Andrew, Florian,
> >
> > On Sat, Dec 19, 2020 at 09:07:13AM -0800, Florian Fainelli wrote:
> > > On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> > > >> --- a/drivers/net/dsa/mt7530.c
> > > >> +++ b/drivers/net/dsa/mt7530.c
> > > >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> > > >>  };
> > > >>
> > > >>  static const struct of_device_id mt7530_of_match[] = {
> > > >> -  { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> > > >> +  { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> > > >>    { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> > > >>    { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> > > >>    { /* sentinel */ },
> > > >
> > > > This will break backwards compatibility with existing DT blobs. You
> > > > need to keep the old "mediatek,mt7621", but please add a comment that
> > > > it is deprecated.
> > >
> > > Besides, adding -gsw would make it inconsistent with the existing
> > > matching compatible strings. While it's not ideal to have the same
> > > top-level SoC compatible and having another sub-node within that SoC's
> > > DTS have the same compatible, given this would be break backwards
> > > compatibility, cannot you stay with what is defined today?
> >
> > The MT7621 device tree is in staging. I suppose that some amount of
> > breaking changes could be tolerated?
> >
> > But Qingfang, I'm confused when looking at drivers/staging/mt7621-dts/mt7621.dtsi.
> >
> > /ethernet@1e100000/mdio-bus {
> >         switch0: switch0@0 {
> >                 compatible = "mediatek,mt7621";
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >                 reg = <0>;
> >                 mediatek,mcm;
> >                 resets = <&rstctrl 2>;
> >                 reset-names = "mcm";
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         reg = <0>;
> >                         port@0 {
> >                                 status = "off";
> >                                 reg = <0>;
> >                                 label = "lan0";
> >                         };
> >                         port@1 {
> >                                 status = "off";
> >                                 reg = <1>;
> >                                 label = "lan1";
> >                         };
> >                         port@2 {
> >                                 status = "off";
> >                                 reg = <2>;
> >                                 label = "lan2";
> >                         };
> >                         port@3 {
> >                                 status = "off";
> >                                 reg = <3>;
> >                                 label = "lan3";
> >                         };
> >                         port@4 {
> >                                 status = "off";
> >                                 reg = <4>;
> >                                 label = "lan4";
> >                         };
> >                         port@6 {
> >                                 reg = <6>;
> >                                 label = "cpu";
> >                                 ethernet = <&gmac0>;
> >                                 phy-mode = "trgmii";
> >                                 fixed-link {
> >                                         speed = <1000>;
> >                                         full-duplex;
> >                                 };
> >                         };
> >                 };
> >         };
> > };
> >
> > / {
> >         gsw: gsw@1e110000 {
> >                 compatible = "mediatek,mt7621-gsw";
> >                 reg = <0x1e110000 0x8000>;
> >                 interrupt-parent = <&gic>;
> >                 interrupts = <GIC_SHARED 23 IRQ_TYPE_LEVEL_HIGH>;
> >         };
> > };
> >
> > What is the platform device at the memory address 1e110000?
> > There is no driver for it. The documentation only has me even more
> > confused:
> >
> > Mediatek Gigabit Switch
> > =======================
> >
> > The mediatek gigabit switch can be found on Mediatek SoCs (mt7620, mt7621).
> >
> > Required properties:
> > - compatible: Should be "mediatek,mt7620-gsw" or "mediatek,mt7621-gsw"
> > - reg: Address and length of the register set for the device
> > - interrupts: Should contain the gigabit switches interrupt
> > - resets: Should contain the gigabit switches resets
> > - reset-names: Should contain the reset names "gsw"
> >
> > Example:
> >
> > gsw@10110000 {
> >         compatible = "ralink,mt7620-gsw";     <- notice how even the example is bad and inconsistent
> >         reg = <0x10110000 8000>;
> >
> >         resets = <&rstctrl 23>;
> >         reset-names = "gsw";
> >
> >         interrupt-parent = <&intc>;
> >         interrupts = <17>;
> > };
> >
> > Does the MT7621 contain two Ethernet switches, one accessed over MMIO
> > and another over MDIO? Or is it the same switch? I don't understand.
> > What is the relationship between the new compatible that you're
> > proposing, Qingfang, and the existing device tree bindings?
> 
> The current dtsi is copied from OpenWrt, so the existing "mt7621-gsw"
> / "mt7620-gsw" compatible is for their swconfig driver.
> MT7621 has only one switch, accessed over MDIO, so the reg property
> has no effect.
> 
> Should this patch be accepted, the existing gsw nodes can be dropped.

But still, what is at memory address 0x1e110000, if the switch is
accessed over MDIO?

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

On Sun, Dec 20, 2020 at 3:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:

> But still, what is at memory address 0x1e110000, if the switch is
> accessed over MDIO?

It's "Ethernet GMAC", handled by mtk_eth_soc.

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[DENG Qingfang]

On Sun, Dec 20, 2020 at 3:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:

> But still, what is at memory address 0x1e110000, if the switch is
> accessed over MDIO?

It's "Ethernet GMAC", handled by mtk_eth_soc.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

On Sun, Dec 20, 2020 at 04:36:27PM +0800, DENG Qingfang wrote:
> On Sun, Dec 20, 2020 at 3:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > But still, what is at memory address 0x1e110000, if the switch is
> > accessed over MDIO?
>
> It's "Ethernet GMAC", handled by mtk_eth_soc.

I see. You have some work to do with that device tree.

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

On Sun, Dec 20, 2020 at 04:36:27PM +0800, DENG Qingfang wrote:
> On Sun, Dec 20, 2020 at 3:49 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > But still, what is at memory address 0x1e110000, if the switch is
> > accessed over MDIO?
>
> It's "Ethernet GMAC", handled by mtk_eth_soc.

I see. You have some work to do with that device tree.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

On Sun, Dec 20, 2020 at 12:21:53AM +0800, DENG Qingfang wrote:
> MT7621 is a SoC, so using "mediatek,mt7621" as its compatible is ambiguous.
> Rename it to "mediatek,mt7621-gsw".
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---

I would say that you need to resolve the situation with the docs at
Documentation/devicetree/bindings/net/mediatek,mt7620-gsw.txt
and with the bindings at drivers/staging/mt7621-dts/mt7621.dtsi first
(or in the same series). And still then, it would be nice if you could
preserve compatibility with the existing bindings at least for a while.

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Vladimir Oltean]

On Sun, Dec 20, 2020 at 12:21:53AM +0800, DENG Qingfang wrote:
> MT7621 is a SoC, so using "mediatek,mt7621" as its compatible is ambiguous.
> Rename it to "mediatek,mt7621-gsw".
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---

I would say that you need to resolve the situation with the docs at
Documentation/devicetree/bindings/net/mediatek,mt7620-gsw.txt
and with the bindings at drivers/staging/mt7621-dts/mt7621.dtsi first
(or in the same series). And still then, it would be nice if you could
preserve compatibility with the existing bindings at least for a while.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Chuanhong Guo]

Hi!

On Sun, Dec 20, 2020 at 1:10 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> >> --- a/drivers/net/dsa/mt7530.c
> >> +++ b/drivers/net/dsa/mt7530.c
> >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> >>  };
> >>
> >>  static const struct of_device_id mt7530_of_match[] = {
> >> -    { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> >> +    { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> >>      { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> >>      { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> >>      { /* sentinel */ },
> >
> > This will break backwards compatibility with existing DT blobs. You
> > need to keep the old "mediatek,mt7621", but please add a comment that
> > it is deprecated.
>
> Besides, adding -gsw would make it inconsistent with the existing
> matching compatible strings. While it's not ideal to have the same
> top-level SoC compatible and having another sub-node within that SoC's
> DTS have the same compatible, given this would be break backwards
> compatibility, cannot you stay with what is defined today?

U-boot for MT7621 doesn't support device tree, and the kernel image
is always packaged with dt. Do we need to maintain backward
compatibility at all in this situation?

-- 
Regards,
Chuanhong Guo

Re: [RFC PATCH net-next] net: dsa: mt7530: rename MT7621 compatible

[Chuanhong Guo]

Hi!

On Sun, Dec 20, 2020 at 1:10 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 12/19/2020 8:26 AM, Andrew Lunn wrote:
> >> --- a/drivers/net/dsa/mt7530.c
> >> +++ b/drivers/net/dsa/mt7530.c
> >> @@ -2688,7 +2688,7 @@ static const struct mt753x_info mt753x_table[] = {
> >>  };
> >>
> >>  static const struct of_device_id mt7530_of_match[] = {
> >> -    { .compatible = "mediatek,mt7621", .data = &mt753x_table[ID_MT7621], },
> >> +    { .compatible = "mediatek,mt7621-gsw", .data = &mt753x_table[ID_MT7621], },
> >>      { .compatible = "mediatek,mt7530", .data = &mt753x_table[ID_MT7530], },
> >>      { .compatible = "mediatek,mt7531", .data = &mt753x_table[ID_MT7531], },
> >>      { /* sentinel */ },
> >
> > This will break backwards compatibility with existing DT blobs. You
> > need to keep the old "mediatek,mt7621", but please add a comment that
> > it is deprecated.
>
> Besides, adding -gsw would make it inconsistent with the existing
> matching compatible strings. While it's not ideal to have the same
> top-level SoC compatible and having another sub-node within that SoC's
> DTS have the same compatible, given this would be break backwards
> compatibility, cannot you stay with what is defined today?

U-boot for MT7621 doesn't support device tree, and the kernel image
is always packaged with dt. Do we need to maintain backward
compatibility at all in this situation?

-- 
Regards,
Chuanhong Guo

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek