Re: [PATCH 0/5] arm: dts: mt7623: relocate gmacs, mt7530 switch, and mux phy

Re: [PATCH 0/5] arm: dts: mt7623: relocate gmacs, mt7530 switch, and mux phy

[Matthias Brugger]

On 20/01/2023 21:53, Arınç ÜNAL wrote:
> Yeia sou!
> 

γεια σας!

Whole series applied, thanks!

> I initially just wanted to submit the phy muxing patch for BPI-R2 but after
> looking around the bindings, I decided to do a bit of a cleanup.
> 
> The main takeaway of the cleanup is that the MT7530 switch is actually a
> part of the multi-chip module on the MT7623AI SoC. I'm moving the bindings
> for this switch to mt7623a.dtsi so they don't need to be defined on every
> device with this SoC. This should ease it up for supporting more devices
> with MT7623AI SoC on mainline.
> 
> I don't know if there's a pin wired for interrupt. The switch is going to
> work with polling for the time being. Folks from MediaTek, feel free to
> fill me in on this.
> 
> I've tested phy muxing on my BPI-R2. DTs compile fine.
> 
> Arınç ÜNAL (5):
>    arm: dts: mt7623: add gmacs to mt7623.dtsi
>    arm: dts: mt7623: add mt7530 switch to mt7623a.dtsi
>    arm: dts: mt7623: change mt7530 switch address
>    arm: dts: mt7623: mux phy0 on Bananapi BPI-R2
>    arm: dts: mt7623: enable flow control on port@6
> 
>   arch/arm/boot/dts/mt7623.dtsi                 | 14 ++++
>   arch/arm/boot/dts/mt7623a-rfb-emmc.dts        | 86 ++++++----------------
>   arch/arm/boot/dts/mt7623a-rfb-nand.dts        | 86 ++++++----------------
>   arch/arm/boot/dts/mt7623a.dtsi                | 74 +++++++++++++++++++
>   arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts | 22 +++---
>   arch/arm/boot/dts/mt7623n-rfb-emmc.dts        | 13 ++--
>   6 files changed, 147 insertions(+), 148 deletions(-)
> 
> 

Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

[Frank Wunderlich]

Am 20. Januar 2023 21:53:17 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:

> 		switch@1f {
> 			compatible = "mediatek,mt7530";
> 			reg = <0x1f>;
>@@ -199,11 +208,6 @@ ports {
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
>-				port@0 {
>-					reg = <0>;
>-					label = "wan";
>-				};
>-

This will break existing userspace setups using wan as interface name.

> 				port@1 {
> 					reg = <1>;
> 					label = "lan0";


regards Frank

Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

[Arınç ÜNAL]

On 25.01.2023 19:41, Frank Wunderlich wrote:
> Am 20. Januar 2023 21:53:17 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:
> 
>> 		switch@1f {
>> 			compatible = "mediatek,mt7530";
>> 			reg = <0x1f>;
>> @@ -199,11 +208,6 @@ ports {
>> 				#address-cells = <1>;
>> 				#size-cells = <0>;
>>
>> -				port@0 {
>> -					reg = <0>;
>> -					label = "wan";
>> -				};
>> -
> 
> This will break existing userspace setups using wan as interface name.

Yup. The OS being used will have to either rename the interface or adapt 
to the new name, eth1.

Arınç

Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

[Matthias Brugger]

On 25/01/2023 17:54, Arınç ÜNAL wrote:
> On 25.01.2023 19:41, Frank Wunderlich wrote:
>> Am 20. Januar 2023 21:53:17 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:
>>
>>>         switch@1f {
>>>             compatible = "mediatek,mt7530";
>>>             reg = <0x1f>;
>>> @@ -199,11 +208,6 @@ ports {
>>>                 #address-cells = <1>;
>>>                 #size-cells = <0>;
>>>
>>> -                port@0 {
>>> -                    reg = <0>;
>>> -                    label = "wan";
>>> -                };
>>> -
>>
>> This will break existing userspace setups using wan as interface name.
> 
> Yup. The OS being used will have to either rename the interface or adapt to the 
> new name, eth1.
> 

In that case please explain in the commit message why this is needed. We are 
trying really hard to not break userspace by a device-tree update.

Regards,
Matthias

Aw: Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

[Frank Wunderlich]

Hi
> Gesendet: Dienstag, 31. Januar 2023 um 12:55 Uhr
> Von: "Matthias Brugger" <matthias.bgg@gmail.com>
> An: "Arınç ÜNAL" <arinc.unal@arinc9.com>, frank-w@public-files.de, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>
> Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, erkin.bozoglu@xeront.com, "Sean Wang" <sean.wang@mediatek.com>, "DENG Qingfang" <dqfext@gmail.com>
> Betreff: Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2
>
> 
> 
> On 25/01/2023 17:54, Arınç ÜNAL wrote:
> > On 25.01.2023 19:41, Frank Wunderlich wrote:
> >> Am 20. Januar 2023 21:53:17 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:
> >>
> >>>         switch@1f {
> >>>             compatible = "mediatek,mt7530";
> >>>             reg = <0x1f>;
> >>> @@ -199,11 +208,6 @@ ports {
> >>>                 #address-cells = <1>;
> >>>                 #size-cells = <0>;
> >>>
> >>> -                port@0 {
> >>> -                    reg = <0>;
> >>> -                    label = "wan";
> >>> -                };
> >>> -
> >>
> >> This will break existing userspace setups using wan as interface name.
> > 
> > Yup. The OS being used will have to either rename the interface or adapt to the 
> > new name, eth1.
> > 
> 
> In that case please explain in the commit message why this is needed. We are 
> trying really hard to not break userspace by a device-tree update.

I understand the reason why Arınç wants to change it:

currently all ports including wan-port are routed via dsa over gmac0 (trgmii), Arınç enables the second gmac and port5 of switch to route wan-traffic over the other gmac. But unfortunately there is no mainline-way to rename the gmac1 interface (eth1) to wan...this would be a way to not break the userspace.

i had an older patch for mtk_eth_soc [1] which do it at driver level for mediatek ethernet driver, but afair a generic way to (re)name interfaces in dts (vendor independ) was requested as the right way.

regards Frank

[1] https://github.com/frank-w/BPI-Router-Linux/commit/5088c9c81e90014622a950fca38cfb29e0421155

Re: Aw: Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2

[Arınç ÜNAL]

On 31.01.2023 19:39, Frank Wunderlich wrote:
> Hi
>> Gesendet: Dienstag, 31. Januar 2023 um 12:55 Uhr
>> Von: "Matthias Brugger" <matthias.bgg@gmail.com>
>> An: "Arınç ÜNAL" <arinc.unal@arinc9.com>, frank-w@public-files.de, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>
>> Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, erkin.bozoglu@xeront.com, "Sean Wang" <sean.wang@mediatek.com>, "DENG Qingfang" <dqfext@gmail.com>
>> Betreff: Re: [PATCH 4/5] arm: dts: mt7623: mux phy0 on Bananapi BPI-R2
>>
>>
>>
>> On 25/01/2023 17:54, Arınç ÜNAL wrote:
>>> On 25.01.2023 19:41, Frank Wunderlich wrote:
>>>> Am 20. Januar 2023 21:53:17 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:
>>>>
>>>>>          switch@1f {
>>>>>              compatible = "mediatek,mt7530";
>>>>>              reg = <0x1f>;
>>>>> @@ -199,11 +208,6 @@ ports {
>>>>>                  #address-cells = <1>;
>>>>>                  #size-cells = <0>;
>>>>>
>>>>> -                port@0 {
>>>>> -                    reg = <0>;
>>>>> -                    label = "wan";
>>>>> -                };
>>>>> -
>>>>
>>>> This will break existing userspace setups using wan as interface name.
>>>
>>> Yup. The OS being used will have to either rename the interface or adapt to the
>>> new name, eth1.
>>>
>>
>> In that case please explain in the commit message why this is needed. We are
>> trying really hard to not break userspace by a device-tree update.
> 
> I understand the reason why Arınç wants to change it:
> 
> currently all ports including wan-port are routed via dsa over gmac0 (trgmii), Arınç enables the second gmac and port5 of switch to route wan-traffic over the other gmac. But unfortunately there is no mainline-way to rename the gmac1 interface (eth1) to wan...this would be a way to not break the userspace.
> 
> i had an older patch for mtk_eth_soc [1] which do it at driver level for mediatek ethernet driver, but afair a generic way to (re)name interfaces in dts (vendor independ) was requested as the right way.
> 
> regards Frank
> 
> [1] https://github.com/frank-w/BPI-Router-Linux/commit/5088c9c81e90014622a950fca38cfb29e0421155

I tried this [0] before. It's not going to happen on mainline, and 
that's fine. I'll just clearly mention on the patch log that the 
interface name will change with this patch.

[0] 
https://lore.kernel.org/netdev/20220404114000.3549-1-arinc.unal@arinc9.com/t/#u

Arınç

Re: [PATCH 1/5] arm: dts: mt7623: add gmacs to mt7623.dtsi

[Frank Wunderlich]

Am 20. Januar 2023 21:53:14 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:

>index 25d31e40a553..5eb698a90d34 100644
>--- a/arch/arm/boot/dts/mt7623.dtsi
>+++ b/arch/arm/boot/dts/mt7623.dtsi
>@@ -981,6 +981,20 @@ eth: ethernet@1b100000 {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		status = "disabled";
>+
>+		gmac0: mac@0 {
>+			compatible = "mediatek,eth-mac";
>+			reg = <0>;
>+			phy-mode = "trgmii";
>+			status = "disabled";
>+		};
>+
>+		gmac1: mac@1 {
>+			compatible = "mediatek,eth-mac";
>+			reg = <1>;
>+			phy-mode = "rgmii";
>+			status = "disabled";
>+		};
> 	};
> 

>diff --git a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>index 5008115d2494..a5800a524302 100644
>--- a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>+++ b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>@@ -175,9 +175,7 @@ &eth {
> 	status = "okay";
> 
> 	gmac0: mac@0 {

Should node not be accessed with label (&gmac0) instead of defining it again and shadow the one from dtsi?

>-		compatible = "mediatek,eth-mac";
>-		reg = <0>;
>-		phy-mode = "trgmii";
>+		status = "okay";
> 
> 		fixed-link {
> 			speed = <1000>;

Same for other boards.

regards Frank

Re: [PATCH 1/5] arm: dts: mt7623: add gmacs to mt7623.dtsi

[Arınç ÜNAL]

On 25.01.2023 19:45, Frank Wunderlich wrote:
> Am 20. Januar 2023 21:53:14 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:
> 
>> index 25d31e40a553..5eb698a90d34 100644
>> --- a/arch/arm/boot/dts/mt7623.dtsi
>> +++ b/arch/arm/boot/dts/mt7623.dtsi
>> @@ -981,6 +981,20 @@ eth: ethernet@1b100000 {
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>> 		status = "disabled";
>> +
>> +		gmac0: mac@0 {
>> +			compatible = "mediatek,eth-mac";
>> +			reg = <0>;
>> +			phy-mode = "trgmii";
>> +			status = "disabled";
>> +		};
>> +
>> +		gmac1: mac@1 {
>> +			compatible = "mediatek,eth-mac";
>> +			reg = <1>;
>> +			phy-mode = "rgmii";
>> +			status = "disabled";
>> +		};
>> 	};
>>
> 
>> diff --git a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>> index 5008115d2494..a5800a524302 100644
>> --- a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>> +++ b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>> @@ -175,9 +175,7 @@ &eth {
>> 	status = "okay";
>>
>> 	gmac0: mac@0 {
> 
> Should node not be accessed with label (&gmac0) instead of defining it again and shadow the one from dtsi?

I think that's up to preference. I kept it the current way as it's 
cleaner than taking it out of &eth.

Arınç

Re: [PATCH 1/5] arm: dts: mt7623: add gmacs to mt7623.dtsi

[Matthias Brugger]

On 25/01/2023 17:52, Arınç ÜNAL wrote:
> On 25.01.2023 19:45, Frank Wunderlich wrote:
>> Am 20. Januar 2023 21:53:14 MEZ schrieb "Arınç ÜNAL" <arinc.unal@arinc9.com>:
>>
>>> index 25d31e40a553..5eb698a90d34 100644
>>> --- a/arch/arm/boot/dts/mt7623.dtsi
>>> +++ b/arch/arm/boot/dts/mt7623.dtsi
>>> @@ -981,6 +981,20 @@ eth: ethernet@1b100000 {
>>>         #address-cells = <1>;
>>>         #size-cells = <0>;
>>>         status = "disabled";
>>> +
>>> +        gmac0: mac@0 {
>>> +            compatible = "mediatek,eth-mac";
>>> +            reg = <0>;
>>> +            phy-mode = "trgmii";
>>> +            status = "disabled";
>>> +        };
>>> +
>>> +        gmac1: mac@1 {
>>> +            compatible = "mediatek,eth-mac";
>>> +            reg = <1>;
>>> +            phy-mode = "rgmii";
>>> +            status = "disabled";
>>> +        };
>>>     };
>>>
>>
>>> diff --git a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts 
>>> b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>>> index 5008115d2494..a5800a524302 100644
>>> --- a/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>>> +++ b/arch/arm/boot/dts/mt7623n-bananapi-bpi-r2.dts
>>> @@ -175,9 +175,7 @@ &eth {
>>>     status = "okay";
>>>
>>>     gmac0: mac@0 {
>>
>> Should node not be accessed with label (&gmac0) instead of defining it again 
>> and shadow the one from dtsi?
> 
> I think that's up to preference. I kept it the current way as it's cleaner than 
> taking it out of &eth.
> 

I'd prefer to do use the label, so that we stay consistent in the source tree.
I'll remove the whole series for now. Please provide a v2.

Thanks,
Matthias