All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] staging: mt7621-dts: fix pinctrl-0 items to be size-1 items on ethernet
@ 2022-02-15  8:17 Arınç ÜNAL
  2022-02-15  8:17 ` [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1 Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-02-15  8:17 UTC (permalink / raw)
  To: Greg KH, Sergio Paracuellos, Sander Vanheule, NeilBrown,
	DENG Qingfang, Andrew Lunn, erkin.bozoglu
  Cc: linux-staging, Arınç ÜNAL

Fix pinctrl-0 items under the ethernet node to be size-1 items.
Current notation would be used on specifications with non-zero cells.

Fixes: 0a93c0d75809 ("staging: mt7621-dts: fix pinctrl properties for ethernet")
Reported-by: Sander Vanheule <sander@svanheule.net>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 4ae744b96464..4da20da243e6 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -323,7 +323,7 @@ ethernet: ethernet@1e100000 {
 		mediatek,ethsys = <&sysc>;
 
 		pinctrl-names = "default";
-		pinctrl-0 = <&rgmii1_pins &rgmii2_pins &mdio_pins>;
+		pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>, <&rgmii2_pins>;
 
 		gmac0: mac@0 {
 			compatible = "mediatek,eth-mac";
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15  8:17 [PATCH 1/2] staging: mt7621-dts: fix pinctrl-0 items to be size-1 items on ethernet Arınç ÜNAL
@ 2022-02-15  8:17 ` Arınç ÜNAL
  2022-02-15  8:35   ` Sergio Paracuellos
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-02-15  8:17 UTC (permalink / raw)
  To: Greg KH, Sergio Paracuellos, Sander Vanheule, NeilBrown,
	DENG Qingfang, Andrew Lunn, erkin.bozoglu
  Cc: linux-staging, Arınç ÜNAL

GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
rgmii2 bus cannot be used on this device.
Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
the GB-PC1 devicetree.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/mt7621-dts/gbpc1.dts b/drivers/staging/mt7621-dts/gbpc1.dts
index e38a083811e5..1b5175e6ccf3 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -114,6 +114,10 @@ default_gpio: gpio {
 	};
 };
 
+&ethernet {
+	pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
+};
+
 &switch0 {
 	ports {
 		port@0 {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15  8:17 ` [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1 Arınç ÜNAL
@ 2022-02-15  8:35   ` Sergio Paracuellos
  2022-02-15  8:49     ` Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Sergio Paracuellos @ 2022-02-15  8:35 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

Hi Arinc,

On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
> rgmii2 bus cannot be used on this device.
> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
> the GB-PC1 devicetree.
>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>  1 file changed, 4 insertions(+)

No issues in GB-PC1. So:

Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks,
    Sergio Paracuellos

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15  8:35   ` Sergio Paracuellos
@ 2022-02-15  8:49     ` Arınç ÜNAL
  2022-02-15  9:09       ` Sergio Paracuellos
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-02-15  8:49 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

Hey Sergio,

On 15/02/2022 11:35, Sergio Paracuellos wrote:
> Hi Arinc,
> 
> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
>> rgmii2 bus cannot be used on this device.
>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
>> the GB-PC1 devicetree.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>>   1 file changed, 4 insertions(+)
> 
> No issues in GB-PC1. So:
> 
> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks for testing it so quickly!

I was wondering if you got pinctrl errors on the bootlog before applying 
this patch series.

rgmii2 pin group is given gpio function so calling it from ethernet node 
would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins 
as GPIO.

[    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by 
pinctrl; cannot claim for 1e100000.ethernet
[    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
[    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22) 
from group rgmii2  on device rt2880-pinmux
[    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting, 
reverse things back
[    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22

Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15  8:49     ` Arınç ÜNAL
@ 2022-02-15  9:09       ` Sergio Paracuellos
  2022-02-15 14:11         ` Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Sergio Paracuellos @ 2022-02-15  9:09 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

Hi Arinc,

On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> Hey Sergio,
>
> On 15/02/2022 11:35, Sergio Paracuellos wrote:
> > Hi Arinc,
> >
> > On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
> >> rgmii2 bus cannot be used on this device.
> >> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
> >> the GB-PC1 devicetree.
> >>
> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> >> ---
> >>   drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >
> > No issues in GB-PC1. So:
> >
> > Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>
> Thanks for testing it so quickly!
>
> I was wondering if you got pinctrl errors on the bootlog before applying
> this patch series.
>
> rgmii2 pin group is given gpio function so calling it from ethernet node
> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
> as GPIO.
>
> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
> pinctrl; cannot claim for 1e100000.ethernet
> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
> from group rgmii2  on device rt2880-pinmux
> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
> reverse things back
> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22

No, I was not getting any kind of error since when I test your last
patch series I was not experimenting any kind of regression. I don't
have any issues now also with your new patch series. Your new changes
make sense since as you have said "rgmii2" pins are requested as GPIO
but it seems are not really being requested? I don't have time to
check the datasheet now but will try to get time to see what is
happening there.

Best regards,
   Sergio Paracuellos
>
> Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15  9:09       ` Sergio Paracuellos
@ 2022-02-15 14:11         ` Arınç ÜNAL
  2022-02-15 15:16           ` Sergio Paracuellos
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-02-15 14:11 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

On 15/02/2022 12:09, Sergio Paracuellos wrote:
> Hi Arinc,
> 
> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> Hey Sergio,
>>
>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
>>> Hi Arinc,
>>>
>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
>>>> rgmii2 bus cannot be used on this device.
>>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
>>>> the GB-PC1 devicetree.
>>>>
>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>> ---
>>>>    drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>
>>> No issues in GB-PC1. So:
>>>
>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>
>> Thanks for testing it so quickly!
>>
>> I was wondering if you got pinctrl errors on the bootlog before applying
>> this patch series.
>>
>> rgmii2 pin group is given gpio function so calling it from ethernet node
>> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
>> as GPIO.
>>
>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
>> pinctrl; cannot claim for 1e100000.ethernet
>> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
>> from group rgmii2  on device rt2880-pinmux
>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
>> reverse things back
>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
> 
> No, I was not getting any kind of error since when I test your last
> patch series I was not experimenting any kind of regression. I don't
> have any issues now also with your new patch series. Your new changes
> make sense since as you have said "rgmii2" pins are requested as GPIO
> but it seems are not really being requested? I don't have time to
> check the datasheet now but will try to get time to see what is
> happening there.

I think this must have something to do with pinctrl on newer kernels as 
the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).

I hacked together something to run 5.17-rc1 for RE650 on OpenWrt. Here's 
what I found:

When I still have rgmii2_pins on the ethernet node, I don't get any of 
the warnings above but this happens:

- failsafe button lights_toggle was pressed -
- failsafe -

When I apply this patch and test again I don't get into failsafe anymore.

The key for LIGHTS_TOGGLE is on GPIO 30 which is one of the rgmii2 pins.
https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621_tplink_rexx0-v1.dtsi#L34

What I make of this is, with newer kernels, pinctrl doesn't care and 
just claims the pin group for ethernet anyway. This would explain why 
the lights_toggle key GPIO goes from active low to high. So this patch 
is still a necessity.

Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15 14:11         ` Arınç ÜNAL
@ 2022-02-15 15:16           ` Sergio Paracuellos
  2022-02-27 15:11             ` Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Sergio Paracuellos @ 2022-02-15 15:16 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

Hi Arinc,

On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> On 15/02/2022 12:09, Sergio Paracuellos wrote:
> > Hi Arinc,
> >
> > On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> Hey Sergio,
> >>
> >> On 15/02/2022 11:35, Sergio Paracuellos wrote:
> >>> Hi Arinc,
> >>>
> >>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
> >>>> rgmii2 bus cannot be used on this device.
> >>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
> >>>> the GB-PC1 devicetree.
> >>>>
> >>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>> ---
> >>>>    drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
> >>>>    1 file changed, 4 insertions(+)
> >>>
> >>> No issues in GB-PC1. So:
> >>>
> >>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>
> >> Thanks for testing it so quickly!
> >>
> >> I was wondering if you got pinctrl errors on the bootlog before applying
> >> this patch series.
> >>
> >> rgmii2 pin group is given gpio function so calling it from ethernet node
> >> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
> >> as GPIO.
> >>
> >> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
> >> pinctrl; cannot claim for 1e100000.ethernet
> >> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
> >> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
> >> from group rgmii2  on device rt2880-pinmux
> >> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
> >> reverse things back
> >> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
> >
> > No, I was not getting any kind of error since when I test your last
> > patch series I was not experimenting any kind of regression. I don't
> > have any issues now also with your new patch series. Your new changes
> > make sense since as you have said "rgmii2" pins are requested as GPIO
> > but it seems are not really being requested? I don't have time to
> > check the datasheet now but will try to get time to see what is
> > happening there.
>
> I think this must have something to do with pinctrl on newer kernels as
> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).

I think is this commit which I did according to a review after moving
the driver from staging into 'drivers/pincrtl/ralink':

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b

You can also check other changes from where the driver was moved:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next

>
> I hacked together something to run 5.17-rc1 for RE650 on OpenWrt. Here's
> what I found:
>
> When I still have rgmii2_pins on the ethernet node, I don't get any of
> the warnings above but this happens:
>
> - failsafe button lights_toggle was pressed -
> - failsafe -
>
> When I apply this patch and test again I don't get into failsafe anymore.
>
> The key for LIGHTS_TOGGLE is on GPIO 30 which is one of the rgmii2 pins.
> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621_tplink_rexx0-v1.dtsi#L34
>
> What I make of this is, with newer kernels, pinctrl doesn't care and
> just claims the pin group for ethernet anyway. This would explain why
> the lights_toggle key GPIO goes from active low to high. So this patch
> is still a necessity.

Agreed, applying this patch is the correct thing to do.

Best regards,
    Sergio Paracuellos
>
> Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-15 15:16           ` Sergio Paracuellos
@ 2022-02-27 15:11             ` Arınç ÜNAL
  2022-02-28  7:01               ` Sergio Paracuellos
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-02-27 15:11 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

[-- Attachment #1: Type: text/plain, Size: 4361 bytes --]

On 15/02/2022 18:16, Sergio Paracuellos wrote:
> Hi Arinc,
> 
> On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> On 15/02/2022 12:09, Sergio Paracuellos wrote:
>>> Hi Arinc,
>>>
>>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> Hey Sergio,
>>>>
>>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
>>>>> Hi Arinc,
>>>>>
>>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>
>>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
>>>>>> rgmii2 bus cannot be used on this device.
>>>>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
>>>>>> the GB-PC1 devicetree.
>>>>>>
>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>> ---
>>>>>>     drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>
>>>>> No issues in GB-PC1. So:
>>>>>
>>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>>
>>>> Thanks for testing it so quickly!
>>>>
>>>> I was wondering if you got pinctrl errors on the bootlog before applying
>>>> this patch series.
>>>>
>>>> rgmii2 pin group is given gpio function so calling it from ethernet node
>>>> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
>>>> as GPIO.
>>>>
>>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
>>>> pinctrl; cannot claim for 1e100000.ethernet
>>>> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
>>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
>>>> from group rgmii2  on device rt2880-pinmux
>>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
>>>> reverse things back
>>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
>>>
>>> No, I was not getting any kind of error since when I test your last
>>> patch series I was not experimenting any kind of regression. I don't
>>> have any issues now also with your new patch series. Your new changes
>>> make sense since as you have said "rgmii2" pins are requested as GPIO
>>> but it seems are not really being requested? I don't have time to
>>> check the datasheet now but will try to get time to see what is
>>> happening there.
>>
>> I think this must have something to do with pinctrl on newer kernels as
>> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).
> 
> I think is this commit which I did according to a review after moving
> the driver from staging into 'drivers/pincrtl/ralink':
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b
> 
> You can also check other changes from where the driver was moved:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next

I realised current mt7621.dtsi does not apply the functions we specify 
for the pin groups on device-specific devicetrees. I believe we need to 
add this like on OpenWrt's mt7621.dtsi.

https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153

Can you apply the patch below to see if you get the error like above?
I also put it in attachments in case of space characters replacing tab 
on the mail.

Arınç

---
diff --git a/drivers/staging/mt7621-dts/gbpc1.dts 
b/drivers/staging/mt7621-dts/gbpc1.dts
index 1b5175e6ccf3..e38a083811e5 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -114,10 +114,6 @@ default_gpio: gpio {
  	};
  };

-&ethernet {
-	pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
-};
-
  &switch0 {
  	ports {
  		port@0 {
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi 
b/drivers/staging/mt7621-dts/mt7621.dtsi
index 4da20da243e6..8e181d6f70ae 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -152,6 +152,11 @@ spi0: spi@b00 {

  	pinctrl: pinctrl {
  		compatible = "ralink,rt2880-pinmux";
+		pinctrl-names = "default";
+		pinctrl-0 = <&state_default>;
+
+		state_default: pinctrl0 {
+		};

  		i2c_pins: i2c0-pins {
  			pinmux {

[-- Attachment #2: test.patch --]
[-- Type: text/plain, Size: 828 bytes --]

diff --git a/drivers/staging/mt7621-dts/gbpc1.dts b/drivers/staging/mt7621-dts/gbpc1.dts
index 1b5175e6ccf3..e38a083811e5 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -114,10 +114,6 @@ default_gpio: gpio {
 	};
 };
 
-&ethernet {
-	pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
-};
-
 &switch0 {
 	ports {
 		port@0 {
diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 4da20da243e6..8e181d6f70ae 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -152,6 +152,11 @@ spi0: spi@b00 {
 
 	pinctrl: pinctrl {
 		compatible = "ralink,rt2880-pinmux";
+		pinctrl-names = "default";
+		pinctrl-0 = <&state_default>;
+
+		state_default: pinctrl0 {
+		};
 
 		i2c_pins: i2c0-pins {
 			pinmux {

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-27 15:11             ` Arınç ÜNAL
@ 2022-02-28  7:01               ` Sergio Paracuellos
  2022-02-28 14:53                 ` Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Sergio Paracuellos @ 2022-02-28  7:01 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

Hi Arinc,

On Sun, Feb 27, 2022 at 4:12 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> On 15/02/2022 18:16, Sergio Paracuellos wrote:
> > Hi Arinc,
> >
> > On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> On 15/02/2022 12:09, Sergio Paracuellos wrote:
> >>> Hi Arinc,
> >>>
> >>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> Hey Sergio,
> >>>>
> >>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
> >>>>> Hi Arinc,
> >>>>>
> >>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>>>
> >>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
> >>>>>> rgmii2 bus cannot be used on this device.
> >>>>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
> >>>>>> the GB-PC1 devicetree.
> >>>>>>
> >>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>>>> ---
> >>>>>>     drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
> >>>>>>     1 file changed, 4 insertions(+)
> >>>>>
> >>>>> No issues in GB-PC1. So:
> >>>>>
> >>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>>>
> >>>> Thanks for testing it so quickly!
> >>>>
> >>>> I was wondering if you got pinctrl errors on the bootlog before applying
> >>>> this patch series.
> >>>>
> >>>> rgmii2 pin group is given gpio function so calling it from ethernet node
> >>>> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
> >>>> as GPIO.
> >>>>
> >>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
> >>>> pinctrl; cannot claim for 1e100000.ethernet
> >>>> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
> >>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
> >>>> from group rgmii2  on device rt2880-pinmux
> >>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
> >>>> reverse things back
> >>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
> >>>
> >>> No, I was not getting any kind of error since when I test your last
> >>> patch series I was not experimenting any kind of regression. I don't
> >>> have any issues now also with your new patch series. Your new changes
> >>> make sense since as you have said "rgmii2" pins are requested as GPIO
> >>> but it seems are not really being requested? I don't have time to
> >>> check the datasheet now but will try to get time to see what is
> >>> happening there.
> >>
> >> I think this must have something to do with pinctrl on newer kernels as
> >> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).
> >
> > I think is this commit which I did according to a review after moving
> > the driver from staging into 'drivers/pincrtl/ralink':
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b
> >
> > You can also check other changes from where the driver was moved:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next
>
> I realised current mt7621.dtsi does not apply the functions we specify
> for the pin groups on device-specific devicetrees. I believe we need to
> add this like on OpenWrt's mt7621.dtsi.
>
> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153
>
> Can you apply the patch below to see if you get the error like above?
> I also put it in attachments in case of space characters replacing tab
> on the mail.
>
> Arınç
>
> ---
> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> b/drivers/staging/mt7621-dts/gbpc1.dts
> index 1b5175e6ccf3..e38a083811e5 100644
> --- a/drivers/staging/mt7621-dts/gbpc1.dts
> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> @@ -114,10 +114,6 @@ default_gpio: gpio {
>         };
>   };
>
> -&ethernet {
> -       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
> -};
> -
>   &switch0 {
>         ports {
>                 port@0 {
> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> b/drivers/staging/mt7621-dts/mt7621.dtsi
> index 4da20da243e6..8e181d6f70ae 100644
> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> @@ -152,6 +152,11 @@ spi0: spi@b00 {
>
>         pinctrl: pinctrl {
>                 compatible = "ralink,rt2880-pinmux";
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&state_default>;
> +
> +               state_default: pinctrl0 {
> +               };

This should not be in the pinctrl node. I was told to remove it when
bindings were reviewed since these properties must be in consumer
nodes [0]. See binding documentation [1]:

[0]: https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg102634.html
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml?h=staging-linus&id=b6821b0d9b56386d2bf14806f90ec401468c799f

Best regards,
    Sergio Paracuellos

>
>                 i2c_pins: i2c0-pins {
>                         pinmux {

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-28  7:01               ` Sergio Paracuellos
@ 2022-02-28 14:53                 ` Arınç ÜNAL
  2022-02-28 15:11                   ` Sergio Paracuellos
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-02-28 14:53 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

On 28/02/2022 10:01, Sergio Paracuellos wrote:
> Hi Arinc,
> 
> On Sun, Feb 27, 2022 at 4:12 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> On 15/02/2022 18:16, Sergio Paracuellos wrote:
>>> Hi Arinc,
>>>
>>> On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> On 15/02/2022 12:09, Sergio Paracuellos wrote:
>>>>> Hi Arinc,
>>>>>
>>>>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>
>>>>>> Hey Sergio,
>>>>>>
>>>>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
>>>>>>> Hi Arinc,
>>>>>>>
>>>>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>>>
>>>>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
>>>>>>>> rgmii2 bus cannot be used on this device.
>>>>>>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
>>>>>>>> the GB-PC1 devicetree.
>>>>>>>>
>>>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>>>> ---
>>>>>>>>      drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>>>>>>>>      1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> No issues in GB-PC1. So:
>>>>>>>
>>>>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>>>>
>>>>>> Thanks for testing it so quickly!
>>>>>>
>>>>>> I was wondering if you got pinctrl errors on the bootlog before applying
>>>>>> this patch series.
>>>>>>
>>>>>> rgmii2 pin group is given gpio function so calling it from ethernet node
>>>>>> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
>>>>>> as GPIO.
>>>>>>
>>>>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
>>>>>> pinctrl; cannot claim for 1e100000.ethernet
>>>>>> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
>>>>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
>>>>>> from group rgmii2  on device rt2880-pinmux
>>>>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
>>>>>> reverse things back
>>>>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
>>>>>
>>>>> No, I was not getting any kind of error since when I test your last
>>>>> patch series I was not experimenting any kind of regression. I don't
>>>>> have any issues now also with your new patch series. Your new changes
>>>>> make sense since as you have said "rgmii2" pins are requested as GPIO
>>>>> but it seems are not really being requested? I don't have time to
>>>>> check the datasheet now but will try to get time to see what is
>>>>> happening there.
>>>>
>>>> I think this must have something to do with pinctrl on newer kernels as
>>>> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).
>>>
>>> I think is this commit which I did according to a review after moving
>>> the driver from staging into 'drivers/pincrtl/ralink':
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b
>>>
>>> You can also check other changes from where the driver was moved:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next
>>
>> I realised current mt7621.dtsi does not apply the functions we specify
>> for the pin groups on device-specific devicetrees. I believe we need to
>> add this like on OpenWrt's mt7621.dtsi.
>>
>> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153
>>
>> Can you apply the patch below to see if you get the error like above?
>> I also put it in attachments in case of space characters replacing tab
>> on the mail.
>>
>> Arınç
>>
>> ---
>> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
>> b/drivers/staging/mt7621-dts/gbpc1.dts
>> index 1b5175e6ccf3..e38a083811e5 100644
>> --- a/drivers/staging/mt7621-dts/gbpc1.dts
>> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
>> @@ -114,10 +114,6 @@ default_gpio: gpio {
>>          };
>>    };
>>
>> -&ethernet {
>> -       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
>> -};
>> -
>>    &switch0 {
>>          ports {
>>                  port@0 {
>> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
>> b/drivers/staging/mt7621-dts/mt7621.dtsi
>> index 4da20da243e6..8e181d6f70ae 100644
>> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
>> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
>> @@ -152,6 +152,11 @@ spi0: spi@b00 {
>>
>>          pinctrl: pinctrl {
>>                  compatible = "ralink,rt2880-pinmux";
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&state_default>;
>> +
>> +               state_default: pinctrl0 {
>> +               };
> 
> This should not be in the pinctrl node. I was told to remove it when
> bindings were reviewed since these properties must be in consumer
> nodes [0]. See binding documentation [1]:
> 
> [0]: https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg102634.html
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml?h=staging-linus&id=b6821b0d9b56386d2bf14806f90ec401468c799f

I'm not sure what a consumer node would be in this context so we can 
claim the pin groups with the given function under it.

Is the ethernet node a consumer node since I claim the rgmii2_pins pin 
group under it for example?

Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-28 14:53                 ` Arınç ÜNAL
@ 2022-02-28 15:11                   ` Sergio Paracuellos
  2022-03-03 15:16                     ` Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Sergio Paracuellos @ 2022-02-28 15:11 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, linux-staging

On Mon, Feb 28, 2022 at 3:53 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> On 28/02/2022 10:01, Sergio Paracuellos wrote:
> > Hi Arinc,
> >
> > On Sun, Feb 27, 2022 at 4:12 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> On 15/02/2022 18:16, Sergio Paracuellos wrote:
> >>> Hi Arinc,
> >>>
> >>> On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> On 15/02/2022 12:09, Sergio Paracuellos wrote:
> >>>>> Hi Arinc,
> >>>>>
> >>>>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>>>
> >>>>>> Hey Sergio,
> >>>>>>
> >>>>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
> >>>>>>> Hi Arinc,
> >>>>>>>
> >>>>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>>>>>
> >>>>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
> >>>>>>>> rgmii2 bus cannot be used on this device.
> >>>>>>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
> >>>>>>>> the GB-PC1 devicetree.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>>>>>> ---
> >>>>>>>>      drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
> >>>>>>>>      1 file changed, 4 insertions(+)
> >>>>>>>
> >>>>>>> No issues in GB-PC1. So:
> >>>>>>>
> >>>>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>>>>>
> >>>>>> Thanks for testing it so quickly!
> >>>>>>
> >>>>>> I was wondering if you got pinctrl errors on the bootlog before applying
> >>>>>> this patch series.
> >>>>>>
> >>>>>> rgmii2 pin group is given gpio function so calling it from ethernet node
> >>>>>> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
> >>>>>> as GPIO.
> >>>>>>
> >>>>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
> >>>>>> pinctrl; cannot claim for 1e100000.ethernet
> >>>>>> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
> >>>>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
> >>>>>> from group rgmii2  on device rt2880-pinmux
> >>>>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
> >>>>>> reverse things back
> >>>>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
> >>>>>
> >>>>> No, I was not getting any kind of error since when I test your last
> >>>>> patch series I was not experimenting any kind of regression. I don't
> >>>>> have any issues now also with your new patch series. Your new changes
> >>>>> make sense since as you have said "rgmii2" pins are requested as GPIO
> >>>>> but it seems are not really being requested? I don't have time to
> >>>>> check the datasheet now but will try to get time to see what is
> >>>>> happening there.
> >>>>
> >>>> I think this must have something to do with pinctrl on newer kernels as
> >>>> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).
> >>>
> >>> I think is this commit which I did according to a review after moving
> >>> the driver from staging into 'drivers/pincrtl/ralink':
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b
> >>>
> >>> You can also check other changes from where the driver was moved:
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next
> >>
> >> I realised current mt7621.dtsi does not apply the functions we specify
> >> for the pin groups on device-specific devicetrees. I believe we need to
> >> add this like on OpenWrt's mt7621.dtsi.
> >>
> >> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153
> >>
> >> Can you apply the patch below to see if you get the error like above?
> >> I also put it in attachments in case of space characters replacing tab
> >> on the mail.
> >>
> >> Arınç
> >>
> >> ---
> >> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> >> b/drivers/staging/mt7621-dts/gbpc1.dts
> >> index 1b5175e6ccf3..e38a083811e5 100644
> >> --- a/drivers/staging/mt7621-dts/gbpc1.dts
> >> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> >> @@ -114,10 +114,6 @@ default_gpio: gpio {
> >>          };
> >>    };
> >>
> >> -&ethernet {
> >> -       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
> >> -};
> >> -
> >>    &switch0 {
> >>          ports {
> >>                  port@0 {
> >> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> >> b/drivers/staging/mt7621-dts/mt7621.dtsi
> >> index 4da20da243e6..8e181d6f70ae 100644
> >> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> >> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> >> @@ -152,6 +152,11 @@ spi0: spi@b00 {
> >>
> >>          pinctrl: pinctrl {
> >>                  compatible = "ralink,rt2880-pinmux";
> >> +               pinctrl-names = "default";
> >> +               pinctrl-0 = <&state_default>;
> >> +
> >> +               state_default: pinctrl0 {
> >> +               };
> >
> > This should not be in the pinctrl node. I was told to remove it when
> > bindings were reviewed since these properties must be in consumer
> > nodes [0]. See binding documentation [1]:
> >
> > [0]: https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg102634.html
> > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml?h=staging-linus&id=b6821b0d9b56386d2bf14806f90ec401468c799f
>
> I'm not sure what a consumer node would be in this context so we can
> claim the pin groups with the given function under it.
>
> Is the ethernet node a consumer node since I claim the rgmii2_pins pin
> group under it for example?

That is my understanding, yes.

Best regards,
    Sergio Paracuellos
>
> Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-02-28 15:11                   ` Sergio Paracuellos
@ 2022-03-03 15:16                     ` Arınç ÜNAL
  2022-03-07 22:25                       ` Arınç ÜNAL
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-03-03 15:16 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, Rob Herring, linux-staging

On 28/02/2022 18:11, Sergio Paracuellos wrote:
> On Mon, Feb 28, 2022 at 3:53 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>
>> On 28/02/2022 10:01, Sergio Paracuellos wrote:
>>> Hi Arinc,
>>>
>>> On Sun, Feb 27, 2022 at 4:12 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>
>>>> On 15/02/2022 18:16, Sergio Paracuellos wrote:
>>>>> Hi Arinc,
>>>>>
>>>>> On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>
>>>>>> On 15/02/2022 12:09, Sergio Paracuellos wrote:
>>>>>>> Hi Arinc,
>>>>>>>
>>>>>>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>>>
>>>>>>>> Hey Sergio,
>>>>>>>>
>>>>>>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
>>>>>>>>> Hi Arinc,
>>>>>>>>>
>>>>>>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>>>>>>>>
>>>>>>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
>>>>>>>>>> rgmii2 bus cannot be used on this device.
>>>>>>>>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
>>>>>>>>>> the GB-PC1 devicetree.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>>>>>>>>>>       1 file changed, 4 insertions(+)
>>>>>>>>>
>>>>>>>>> No issues in GB-PC1. So:
>>>>>>>>>
>>>>>>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>>>>>>
>>>>>>>> Thanks for testing it so quickly!
>>>>>>>>
>>>>>>>> I was wondering if you got pinctrl errors on the bootlog before applying
>>>>>>>> this patch series.
>>>>>>>>
>>>>>>>> rgmii2 pin group is given gpio function so calling it from ethernet node
>>>>>>>> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
>>>>>>>> as GPIO.
>>>>>>>>
>>>>>>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
>>>>>>>> pinctrl; cannot claim for 1e100000.ethernet
>>>>>>>> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
>>>>>>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
>>>>>>>> from group rgmii2  on device rt2880-pinmux
>>>>>>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
>>>>>>>> reverse things back
>>>>>>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
>>>>>>>
>>>>>>> No, I was not getting any kind of error since when I test your last
>>>>>>> patch series I was not experimenting any kind of regression. I don't
>>>>>>> have any issues now also with your new patch series. Your new changes
>>>>>>> make sense since as you have said "rgmii2" pins are requested as GPIO
>>>>>>> but it seems are not really being requested? I don't have time to
>>>>>>> check the datasheet now but will try to get time to see what is
>>>>>>> happening there.
>>>>>>
>>>>>> I think this must have something to do with pinctrl on newer kernels as
>>>>>> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).
>>>>>
>>>>> I think is this commit which I did according to a review after moving
>>>>> the driver from staging into 'drivers/pincrtl/ralink':
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b
>>>>>
>>>>> You can also check other changes from where the driver was moved:
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next
>>>>
>>>> I realised current mt7621.dtsi does not apply the functions we specify
>>>> for the pin groups on device-specific devicetrees. I believe we need to
>>>> add this like on OpenWrt's mt7621.dtsi.
>>>>
>>>> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153
>>>>
>>>> Can you apply the patch below to see if you get the error like above?
>>>> I also put it in attachments in case of space characters replacing tab
>>>> on the mail.
>>>>
>>>> Arınç
>>>>
>>>> ---
>>>> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
>>>> b/drivers/staging/mt7621-dts/gbpc1.dts
>>>> index 1b5175e6ccf3..e38a083811e5 100644
>>>> --- a/drivers/staging/mt7621-dts/gbpc1.dts
>>>> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
>>>> @@ -114,10 +114,6 @@ default_gpio: gpio {
>>>>           };
>>>>     };
>>>>
>>>> -&ethernet {
>>>> -       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
>>>> -};
>>>> -
>>>>     &switch0 {
>>>>           ports {
>>>>                   port@0 {
>>>> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
>>>> b/drivers/staging/mt7621-dts/mt7621.dtsi
>>>> index 4da20da243e6..8e181d6f70ae 100644
>>>> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
>>>> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
>>>> @@ -152,6 +152,11 @@ spi0: spi@b00 {
>>>>
>>>>           pinctrl: pinctrl {
>>>>                   compatible = "ralink,rt2880-pinmux";
>>>> +               pinctrl-names = "default";
>>>> +               pinctrl-0 = <&state_default>;
>>>> +
>>>> +               state_default: pinctrl0 {
>>>> +               };
>>>
>>> This should not be in the pinctrl node. I was told to remove it when
>>> bindings were reviewed since these properties must be in consumer
>>> nodes [0]. See binding documentation [1]:
>>>
>>> [0]: https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg102634.html
>>> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml?h=staging-linus&id=b6821b0d9b56386d2bf14806f90ec401468c799f
>>
>> I'm not sure what a consumer node would be in this context so we can
>> claim the pin groups with the given function under it.
>>
>> Is the ethernet node a consumer node since I claim the rgmii2_pins pin
>> group under it for example?
> 
> That is my understanding, yes.

Adding Rob on Cc.

Solutions that come to my mind:

We can overwrite the pin group node with the function we want to set it.
Then claim it under a node where pins from the pin group is used.

Example:

diff --git a/drivers/staging/mt7621-dts/gbpc1.dts 
b/drivers/staging/mt7621-dts/gbpc1.dts
index 360ac98de3f1..709105b2822b 100644
--- a/drivers/staging/mt7621-dts/gbpc1.dts
+++ b/drivers/staging/mt7621-dts/gbpc1.dts
@@ -38,6 +38,9 @@ reset {
  	gpio-leds {
  		compatible = "gpio-leds";

+		pinctrl-names = "default";
+		pinctrl-0 = <&uart3_pins>;
+
  		power {
  			label = "green:power";
  			gpios = <&gpio 6 GPIO_ACTIVE_LOW>;
@@ -103,6 +106,12 @@ gpio {
  	};
  };

+&uart3_pins {
+	pinmux {
+		function = "gpio";
+	};
+};
+
  &ethernet {
  	pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
  };

A few issues with this:

- Some used pins might not be defined on the devicetree. We cannot claim 
these pin groups because the consumer node doesn't exist.

For example, check JTAG pins of GB-PC2 on page 15:
https://github.com/ngiger/GnuBee_Docs/blob/master/GB-PCx/Documents/GB-PC2_V1.1_schematic.pdf

- Pins on the same pin group can be used under different nodes. We 
cannot claim a pin group under multiple nodes.

So we can get back to this:

Treat pinctrl node as a consumer node. We might not need to refer to the 
pinctrl-0 & pinctrl-names properties on the rt2880-pinmux documentation 
since they're generic pinctrl properties.

Example:

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
b/drivers/staging/mt7621-dts/mt7621.dtsi
index 4da20da243e6..8e181d6f70ae 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -152,6 +152,12 @@ spi0: spi@b00 {

   	pinctrl: pinctrl {
   		compatible = "ralink,rt2880-pinmux";
+		pinctrl-names = "default";
+		pinctrl-0 = <&state_default>;
+
+		state_default: state-default {
+
+		};

   		i2c_pins: i2c0-pins {
   			pinmux {

On any mt7621 devicetree:

&state_default {
	gpio-pinmux {
		groups = "rgmii2", "uart3", "wdt";
		function = "gpio";
	};
};

By the way, is there a reason why wdt and jtag pin groups are missing 
from the documentation and mt7621.dtsi? If they are used as GPIO by 
default we can explicitly define them on mt7621.dtsi and mention them on 
the documentation.

Arınç

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-03-03 15:16                     ` Arınç ÜNAL
@ 2022-03-07 22:25                       ` Arınç ÜNAL
  2022-03-08 12:52                         ` Sergio Paracuellos
  0 siblings, 1 reply; 14+ messages in thread
From: Arınç ÜNAL @ 2022-03-07 22:25 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, Rob Herring, linux-staging

On 03/03/2022 18:16, Arınç ÜNAL wrote:
> On 28/02/2022 18:11, Sergio Paracuellos wrote:
>> On Mon, Feb 28, 2022 at 3:53 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>>>
>>> On 28/02/2022 10:01, Sergio Paracuellos wrote:
>>>> Hi Arinc,
>>>>
>>>> On Sun, Feb 27, 2022 at 4:12 PM Arınç ÜNAL <arinc.unal@arinc9.com> 
>>>> wrote:
>>>>>
>>>>> On 15/02/2022 18:16, Sergio Paracuellos wrote:
>>>>>> Hi Arinc,
>>>>>>
>>>>>> On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> 
>>>>>> wrote:
>>>>>>>
>>>>>>> On 15/02/2022 12:09, Sergio Paracuellos wrote:
>>>>>>>> Hi Arinc,
>>>>>>>>
>>>>>>>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL 
>>>>>>>> <arinc.unal@arinc9.com> wrote:
>>>>>>>>>
>>>>>>>>> Hey Sergio,
>>>>>>>>>
>>>>>>>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
>>>>>>>>>> Hi Arinc,
>>>>>>>>>>
>>>>>>>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL 
>>>>>>>>>> <arinc.unal@arinc9.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. 
>>>>>>>>>>> Therefore, the
>>>>>>>>>>> rgmii2 bus cannot be used on this device.
>>>>>>>>>>> Overwrite pinctrl-0 property under the ethernet node without 
>>>>>>>>>>> rgmii2_pins on
>>>>>>>>>>> the GB-PC1 devicetree.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
>>>>>>>>>>>       1 file changed, 4 insertions(+)
>>>>>>>>>>
>>>>>>>>>> No issues in GB-PC1. So:
>>>>>>>>>>
>>>>>>>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>>>>>>>>>
>>>>>>>>> Thanks for testing it so quickly!
>>>>>>>>>
>>>>>>>>> I was wondering if you got pinctrl errors on the bootlog before 
>>>>>>>>> applying
>>>>>>>>> this patch series.
>>>>>>>>>
>>>>>>>>> rgmii2 pin group is given gpio function so calling it from 
>>>>>>>>> ethernet node
>>>>>>>>> would cause this on my TP-Link RE650 v1 which also uses the 
>>>>>>>>> rgmii2_pins
>>>>>>>>> as GPIO.
>>>>>>>>>
>>>>>>>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already 
>>>>>>>>> requested by
>>>>>>>>> pinctrl; cannot claim for 1e100000.ethernet
>>>>>>>>> [    1.196966] rt2880-pinmux pinctrl: pin-22 
>>>>>>>>> (1e100000.ethernet) status -22
>>>>>>>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 
>>>>>>>>> (io22)
>>>>>>>>> from group rgmii2  on device rt2880-pinmux
>>>>>>>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying 
>>>>>>>>> setting,
>>>>>>>>> reverse things back
>>>>>>>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed 
>>>>>>>>> with error -22
>>>>>>>>
>>>>>>>> No, I was not getting any kind of error since when I test your last
>>>>>>>> patch series I was not experimenting any kind of regression. I 
>>>>>>>> don't
>>>>>>>> have any issues now also with your new patch series. Your new 
>>>>>>>> changes
>>>>>>>> make sense since as you have said "rgmii2" pins are requested as 
>>>>>>>> GPIO
>>>>>>>> but it seems are not really being requested? I don't have time to
>>>>>>>> check the datasheet now but will try to get time to see what is
>>>>>>>> happening there.
>>>>>>>
>>>>>>> I think this must have something to do with pinctrl on newer 
>>>>>>> kernels as
>>>>>>> the TP-Link RE650 that I tested uses the OpenWrt master branch 
>>>>>>> (Linux 5.10).
>>>>>>
>>>>>> I think is this commit which I did according to a review after moving
>>>>>> the driver from staging into 'drivers/pincrtl/ralink':
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b 
>>>>>>
>>>>>>
>>>>>> You can also check other changes from where the driver was moved:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next 
>>>>>>
>>>>>
>>>>> I realised current mt7621.dtsi does not apply the functions we specify
>>>>> for the pin groups on device-specific devicetrees. I believe we 
>>>>> need to
>>>>> add this like on OpenWrt's mt7621.dtsi.
>>>>>
>>>>> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253 
>>>>>
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153 
>>>>>
>>>>>
>>>>> Can you apply the patch below to see if you get the error like above?
>>>>> I also put it in attachments in case of space characters replacing tab
>>>>> on the mail.
>>>>>
>>>>> Arınç
>>>>>
>>>>> ---
>>>>> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
>>>>> b/drivers/staging/mt7621-dts/gbpc1.dts
>>>>> index 1b5175e6ccf3..e38a083811e5 100644
>>>>> --- a/drivers/staging/mt7621-dts/gbpc1.dts
>>>>> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
>>>>> @@ -114,10 +114,6 @@ default_gpio: gpio {
>>>>>           };
>>>>>     };
>>>>>
>>>>> -&ethernet {
>>>>> -       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
>>>>> -};
>>>>> -
>>>>>     &switch0 {
>>>>>           ports {
>>>>>                   port@0 {
>>>>> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
>>>>> b/drivers/staging/mt7621-dts/mt7621.dtsi
>>>>> index 4da20da243e6..8e181d6f70ae 100644
>>>>> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
>>>>> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
>>>>> @@ -152,6 +152,11 @@ spi0: spi@b00 {
>>>>>
>>>>>           pinctrl: pinctrl {
>>>>>                   compatible = "ralink,rt2880-pinmux";
>>>>> +               pinctrl-names = "default";
>>>>> +               pinctrl-0 = <&state_default>;
>>>>> +
>>>>> +               state_default: pinctrl0 {
>>>>> +               };
>>>>
>>>> This should not be in the pinctrl node. I was told to remove it when
>>>> bindings were reviewed since these properties must be in consumer
>>>> nodes [0]. See binding documentation [1]:
>>>>
>>>> [0]: 
>>>> https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg102634.html 
>>>>
>>>> [1]: 
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml?h=staging-linus&id=b6821b0d9b56386d2bf14806f90ec401468c799f 
>>>>
>>>
>>> I'm not sure what a consumer node would be in this context so we can
>>> claim the pin groups with the given function under it.
>>>
>>> Is the ethernet node a consumer node since I claim the rgmii2_pins pin
>>> group under it for example?
>>
>> That is my understanding, yes.
> 
> Adding Rob on Cc.
> 
> Solutions that come to my mind:
> 
> We can overwrite the pin group node with the function we want to set it.
> Then claim it under a node where pins from the pin group is used.
> 
> Example:
> 
> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts 
> b/drivers/staging/mt7621-dts/gbpc1.dts
> index 360ac98de3f1..709105b2822b 100644
> --- a/drivers/staging/mt7621-dts/gbpc1.dts
> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> @@ -38,6 +38,9 @@ reset {
>       gpio-leds {
>           compatible = "gpio-leds";
> 
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&uart3_pins>;
> +
>           power {
>               label = "green:power";
>               gpios = <&gpio 6 GPIO_ACTIVE_LOW>;
> @@ -103,6 +106,12 @@ gpio {
>       };
>   };
> 
> +&uart3_pins {
> +    pinmux {
> +        function = "gpio";
> +    };
> +};
> +
>   &ethernet {
>       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
>   };
> 
> A few issues with this:
> 
> - Some used pins might not be defined on the devicetree. We cannot claim 
> these pin groups because the consumer node doesn't exist.
> 
> For example, check JTAG pins of GB-PC2 on page 15:
> https://github.com/ngiger/GnuBee_Docs/blob/master/GB-PCx/Documents/GB-PC2_V1.1_schematic.pdf 
> 
> 
> - Pins on the same pin group can be used under different nodes. We 
> cannot claim a pin group under multiple nodes.
> 
> So we can get back to this:
> 
> Treat pinctrl node as a consumer node. We might not need to refer to the 
> pinctrl-0 & pinctrl-names properties on the rt2880-pinmux documentation 
> since they're generic pinctrl properties.
> 
> Example:
> 
> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> b/drivers/staging/mt7621-dts/mt7621.dtsi
> index 4da20da243e6..8e181d6f70ae 100644
> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> @@ -152,6 +152,12 @@ spi0: spi@b00 {
> 
>        pinctrl: pinctrl {
>            compatible = "ralink,rt2880-pinmux";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&state_default>;
> +
> +        state_default: state-default {
> +
> +        };
> 
>            i2c_pins: i2c0-pins {
>                pinmux {
> 
> On any mt7621 devicetree:
> 
> &state_default {
>      gpio-pinmux {
>          groups = "rgmii2", "uart3", "wdt";
>          function = "gpio";
>      };
> };

I think a better solution would be to claim default_state under a device 
specific devicetree so we don't touch mt7621.dtsi at all. An mt7621 
device might not need to use any pin groups as gpio so putting a 
state_default node under mt7621.dtsi doesn't make sense.

I'll send patches for GB-PC1 & GB-PC2 for you to comment on.

Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
  2022-03-07 22:25                       ` Arınç ÜNAL
@ 2022-03-08 12:52                         ` Sergio Paracuellos
  0 siblings, 0 replies; 14+ messages in thread
From: Sergio Paracuellos @ 2022-03-08 12:52 UTC (permalink / raw)
  To: Arınç ÜNAL
  Cc: Greg KH, Sander Vanheule, NeilBrown, DENG Qingfang, Andrew Lunn,
	erkin.bozoglu, Rob Herring, linux-staging

On Mon, Mar 7, 2022 at 11:25 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> On 03/03/2022 18:16, Arınç ÜNAL wrote:
> > On 28/02/2022 18:11, Sergio Paracuellos wrote:
> >> On Mon, Feb 28, 2022 at 3:53 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>
> >>> On 28/02/2022 10:01, Sergio Paracuellos wrote:
> >>>> Hi Arinc,
> >>>>
> >>>> On Sun, Feb 27, 2022 at 4:12 PM Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>> wrote:
> >>>>>
> >>>>> On 15/02/2022 18:16, Sergio Paracuellos wrote:
> >>>>>> Hi Arinc,
> >>>>>>
> >>>>>> On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> On 15/02/2022 12:09, Sergio Paracuellos wrote:
> >>>>>>>> Hi Arinc,
> >>>>>>>>
> >>>>>>>> On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL
> >>>>>>>> <arinc.unal@arinc9.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Hey Sergio,
> >>>>>>>>>
> >>>>>>>>> On 15/02/2022 11:35, Sergio Paracuellos wrote:
> >>>>>>>>>> Hi Arinc,
> >>>>>>>>>>
> >>>>>>>>>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL
> >>>>>>>>>> <arinc.unal@arinc9.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO.
> >>>>>>>>>>> Therefore, the
> >>>>>>>>>>> rgmii2 bus cannot be used on this device.
> >>>>>>>>>>> Overwrite pinctrl-0 property under the ethernet node without
> >>>>>>>>>>> rgmii2_pins on
> >>>>>>>>>>> the GB-PC1 devicetree.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>       drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
> >>>>>>>>>>>       1 file changed, 4 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> No issues in GB-PC1. So:
> >>>>>>>>>>
> >>>>>>>>>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>>>>>>>>
> >>>>>>>>> Thanks for testing it so quickly!
> >>>>>>>>>
> >>>>>>>>> I was wondering if you got pinctrl errors on the bootlog before
> >>>>>>>>> applying
> >>>>>>>>> this patch series.
> >>>>>>>>>
> >>>>>>>>> rgmii2 pin group is given gpio function so calling it from
> >>>>>>>>> ethernet node
> >>>>>>>>> would cause this on my TP-Link RE650 v1 which also uses the
> >>>>>>>>> rgmii2_pins
> >>>>>>>>> as GPIO.
> >>>>>>>>>
> >>>>>>>>> [    1.177349] rt2880-pinmux pinctrl: pin io22 already
> >>>>>>>>> requested by
> >>>>>>>>> pinctrl; cannot claim for 1e100000.ethernet
> >>>>>>>>> [    1.196966] rt2880-pinmux pinctrl: pin-22
> >>>>>>>>> (1e100000.ethernet) status -22
> >>>>>>>>> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22
> >>>>>>>>> (io22)
> >>>>>>>>> from group rgmii2  on device rt2880-pinmux
> >>>>>>>>> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying
> >>>>>>>>> setting,
> >>>>>>>>> reverse things back
> >>>>>>>>> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed
> >>>>>>>>> with error -22
> >>>>>>>>
> >>>>>>>> No, I was not getting any kind of error since when I test your last
> >>>>>>>> patch series I was not experimenting any kind of regression. I
> >>>>>>>> don't
> >>>>>>>> have any issues now also with your new patch series. Your new
> >>>>>>>> changes
> >>>>>>>> make sense since as you have said "rgmii2" pins are requested as
> >>>>>>>> GPIO
> >>>>>>>> but it seems are not really being requested? I don't have time to
> >>>>>>>> check the datasheet now but will try to get time to see what is
> >>>>>>>> happening there.
> >>>>>>>
> >>>>>>> I think this must have something to do with pinctrl on newer
> >>>>>>> kernels as
> >>>>>>> the TP-Link RE650 that I tested uses the OpenWrt master branch
> >>>>>>> (Linux 5.10).
> >>>>>>
> >>>>>> I think is this commit which I did according to a review after moving
> >>>>>> the driver from staging into 'drivers/pincrtl/ralink':
> >>>>>>
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b
> >>>>>>
> >>>>>>
> >>>>>> You can also check other changes from where the driver was moved:
> >>>>>>
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next
> >>>>>>
> >>>>>
> >>>>> I realised current mt7621.dtsi does not apply the functions we specify
> >>>>> for the pin groups on device-specific devicetrees. I believe we
> >>>>> need to
> >>>>> add this like on OpenWrt's mt7621.dtsi.
> >>>>>
> >>>>> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621.dtsi#L249-L253
> >>>>>
> >>>>>
> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/drivers/staging/mt7621-dts/mt7621.dtsi?h=staging-testing#n153
> >>>>>
> >>>>>
> >>>>> Can you apply the patch below to see if you get the error like above?
> >>>>> I also put it in attachments in case of space characters replacing tab
> >>>>> on the mail.
> >>>>>
> >>>>> Arınç
> >>>>>
> >>>>> ---
> >>>>> diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> >>>>> b/drivers/staging/mt7621-dts/gbpc1.dts
> >>>>> index 1b5175e6ccf3..e38a083811e5 100644
> >>>>> --- a/drivers/staging/mt7621-dts/gbpc1.dts
> >>>>> +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> >>>>> @@ -114,10 +114,6 @@ default_gpio: gpio {
> >>>>>           };
> >>>>>     };
> >>>>>
> >>>>> -&ethernet {
> >>>>> -       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
> >>>>> -};
> >>>>> -
> >>>>>     &switch0 {
> >>>>>           ports {
> >>>>>                   port@0 {
> >>>>> diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> >>>>> b/drivers/staging/mt7621-dts/mt7621.dtsi
> >>>>> index 4da20da243e6..8e181d6f70ae 100644
> >>>>> --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> >>>>> +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> >>>>> @@ -152,6 +152,11 @@ spi0: spi@b00 {
> >>>>>
> >>>>>           pinctrl: pinctrl {
> >>>>>                   compatible = "ralink,rt2880-pinmux";
> >>>>> +               pinctrl-names = "default";
> >>>>> +               pinctrl-0 = <&state_default>;
> >>>>> +
> >>>>> +               state_default: pinctrl0 {
> >>>>> +               };
> >>>>
> >>>> This should not be in the pinctrl node. I was told to remove it when
> >>>> bindings were reviewed since these properties must be in consumer
> >>>> nodes [0]. See binding documentation [1]:
> >>>>
> >>>> [0]:
> >>>> https://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg102634.html
> >>>>
> >>>> [1]:
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/tree/Documentation/devicetree/bindings/pinctrl/ralink,rt2880-pinmux.yaml?h=staging-linus&id=b6821b0d9b56386d2bf14806f90ec401468c799f
> >>>>
> >>>
> >>> I'm not sure what a consumer node would be in this context so we can
> >>> claim the pin groups with the given function under it.
> >>>
> >>> Is the ethernet node a consumer node since I claim the rgmii2_pins pin
> >>> group under it for example?
> >>
> >> That is my understanding, yes.
> >
> > Adding Rob on Cc.
> >
> > Solutions that come to my mind:
> >
> > We can overwrite the pin group node with the function we want to set it.
> > Then claim it under a node where pins from the pin group is used.
> >
> > Example:
> >
> > diff --git a/drivers/staging/mt7621-dts/gbpc1.dts
> > b/drivers/staging/mt7621-dts/gbpc1.dts
> > index 360ac98de3f1..709105b2822b 100644
> > --- a/drivers/staging/mt7621-dts/gbpc1.dts
> > +++ b/drivers/staging/mt7621-dts/gbpc1.dts
> > @@ -38,6 +38,9 @@ reset {
> >       gpio-leds {
> >           compatible = "gpio-leds";
> >
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&uart3_pins>;
> > +
> >           power {
> >               label = "green:power";
> >               gpios = <&gpio 6 GPIO_ACTIVE_LOW>;
> > @@ -103,6 +106,12 @@ gpio {
> >       };
> >   };
> >
> > +&uart3_pins {
> > +    pinmux {
> > +        function = "gpio";
> > +    };
> > +};
> > +
> >   &ethernet {
> >       pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
> >   };
> >
> > A few issues with this:
> >
> > - Some used pins might not be defined on the devicetree. We cannot claim
> > these pin groups because the consumer node doesn't exist.
> >
> > For example, check JTAG pins of GB-PC2 on page 15:
> > https://github.com/ngiger/GnuBee_Docs/blob/master/GB-PCx/Documents/GB-PC2_V1.1_schematic.pdf
> >
> >
> > - Pins on the same pin group can be used under different nodes. We
> > cannot claim a pin group under multiple nodes.
> >
> > So we can get back to this:
> >
> > Treat pinctrl node as a consumer node. We might not need to refer to the
> > pinctrl-0 & pinctrl-names properties on the rt2880-pinmux documentation
> > since they're generic pinctrl properties.
> >
> > Example:
> >
> > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi
> > b/drivers/staging/mt7621-dts/mt7621.dtsi
> > index 4da20da243e6..8e181d6f70ae 100644
> > --- a/drivers/staging/mt7621-dts/mt7621.dtsi
> > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi
> > @@ -152,6 +152,12 @@ spi0: spi@b00 {
> >
> >        pinctrl: pinctrl {
> >            compatible = "ralink,rt2880-pinmux";
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&state_default>;
> > +
> > +        state_default: state-default {
> > +
> > +        };
> >
> >            i2c_pins: i2c0-pins {
> >                pinmux {
> >
> > On any mt7621 devicetree:
> >
> > &state_default {
> >      gpio-pinmux {
> >          groups = "rgmii2", "uart3", "wdt";
> >          function = "gpio";
> >      };
> > };
>
> I think a better solution would be to claim default_state under a device
> specific devicetree so we don't touch mt7621.dtsi at all. An mt7621
> device might not need to use any pin groups as gpio so putting a
> state_default node under mt7621.dtsi doesn't make sense.
>
> I'll send patches for GB-PC1 & GB-PC2 for you to comment on.

Please, do. Thanks,
    Sergio Paracuellos

>
> Arınç

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-03-08 12:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-15  8:17 [PATCH 1/2] staging: mt7621-dts: fix pinctrl-0 items to be size-1 items on ethernet Arınç ÜNAL
2022-02-15  8:17 ` [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1 Arınç ÜNAL
2022-02-15  8:35   ` Sergio Paracuellos
2022-02-15  8:49     ` Arınç ÜNAL
2022-02-15  9:09       ` Sergio Paracuellos
2022-02-15 14:11         ` Arınç ÜNAL
2022-02-15 15:16           ` Sergio Paracuellos
2022-02-27 15:11             ` Arınç ÜNAL
2022-02-28  7:01               ` Sergio Paracuellos
2022-02-28 14:53                 ` Arınç ÜNAL
2022-02-28 15:11                   ` Sergio Paracuellos
2022-03-03 15:16                     ` Arınç ÜNAL
2022-03-07 22:25                       ` Arınç ÜNAL
2022-03-08 12:52                         ` Sergio Paracuellos

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.