[PATCH v3 3/3] ramips: mt7621-dts: add pinctrl properties for ethernet

[Arınç ÜNAL]

Hey Sander,

On 13/02/2022 15:31, Sander Vanheule wrote:
> Hi Arınç,
> 
> On Sun, 2022-02-13 at 11:15 +0300, Arınç ÜNAL wrote:
>> Add the missing pinctrl properties on the ethernet node.
>> GMAC1 will start working with this change.
>>
>> Link: https://lore.kernel.org/netdev/83a35aa3-6cb8-2bc4-2ff4-64278bbcd8c8@arinc9.com/
>>
>> Overwrite pinctrl-0 property without rgmii2_pins on devicetrees which use
>> the rgmii2 pins as GPIO (22 - 33).
>>
>> Add rgmii2 pin group to gpio function on mt7621_tplink_archer-x6-v3.dtsi
>> which uses GPIO 28.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal at arinc9.com>
>> ---
>>   target/linux/ramips/dts/mt7621.dtsi                         | 3 +++
>>   target/linux/ramips/dts/mt7621_alfa-network_quad-e4g.dts    | 4 ++++
>>   target/linux/ramips/dts/mt7621_buffalo_wsr-1166dhp.dts      | 4 ++++
>>   target/linux/ramips/dts/mt7621_buffalo_wsr-600dhp.dts       | 4 ++++
>>   target/linux/ramips/dts/mt7621_d-team_pbr-m1.dts            | 4 ++++
>>   target/linux/ramips/dts/mt7621_firefly_firewrt.dts          | 4 ++++
>>   target/linux/ramips/dts/mt7621_gnubee_gb-pc1.dts            | 4 ++++
>>   target/linux/ramips/dts/mt7621_gnubee_gb-pc2.dts            | 4 ++++
>>   target/linux/ramips/dts/mt7621_mediatek_ap-mt7621a-v60.dts  | 4 ++++
>>   .../linux/ramips/dts/mt7621_mediatek_mt7621-eval-board.dts  | 4 ++++
>>   .../linux/ramips/dts/mt7621_mikrotik_routerboard-m11g.dts   | 4 ++++
>>   target/linux/ramips/dts/mt7621_mtc_wr1201.dts               | 4 ++++
>>   target/linux/ramips/dts/mt7621_netgear_ex6150.dts           | 4 ++++
>>   target/linux/ramips/dts/mt7621_sercomm_na502.dts            | 4 ++++
>>   target/linux/ramips/dts/mt7621_telco-electronics_x1.dts     | 4 ++++
>>   target/linux/ramips/dts/mt7621_tplink_archer-x6-v3.dtsi     | 6 +++++-
>>   target/linux/ramips/dts/mt7621_tplink_re350-v1.dts          | 4 ++++
>>   target/linux/ramips/dts/mt7621_tplink_rexx0-v1.dtsi         | 4 ++++
>>   target/linux/ramips/dts/mt7621_ubnt_edgerouter-x.dtsi       | 4 ++++
>>   target/linux/ramips/dts/mt7621_wavlink_wl-wn531a6.dts       | 4 ++++
>>   target/linux/ramips/dts/mt7621_wevo_w2914ns-v2.dtsi         | 4 ++++
>>   target/linux/ramips/dts/mt7621_winstars_ws-wn583a6.dts      | 4 ++++
>>   target/linux/ramips/dts/mt7621_xzwifi_creativebox-v1.dts    | 4 ++++
>>   target/linux/ramips/dts/mt7621_zbtlink_zbt-wg1602.dtsi      | 4 ++++
>>   target/linux/ramips/dts/mt7621_zbtlink_zbt-wg2626.dts       | 4 ++++
>>   target/linux/ramips/dts/mt7621_zbtlink_zbt-wg3526.dtsi      | 4 ++++
>>   target/linux/ramips/dts/mt7621_zyxel_nr7101.dts             | 4 ++++
>>   27 files changed, 108 insertions(+), 1 deletion(-)
>>
> 
> Are you sure this is the only way to make this work? Overwriting a default in this many
> files doesn't make it look like a great default. This is probably happening because these

This is actually a minority of devicetrees. 26 opposed to 156 (minus 
devicetrees that #include any of the 26 devicetrees) which use all 3 pin 
groups on ethernet.

It's also upstreamed so I'd like to stick with it as much as I can.
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=0a93c0d75809582893e82039143591b9265b520e

> files already specify rgmii2 to be used with the GPIO function, and you're causing some
> sort of race as to what setting gets applied in the end, by adding rmgii2_pins to the
> default state for the &ethernet node.

Agreed:

[    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

That's why I overwrite the property without it for the 26 devicetrees.

> 
> What would make more sense to me, is if the rgmii settings could be enabled in the gmac-s
> themselves:
> 
>     &gmac0 {
>     	pinctrl-names = "default";
>     	pinctrl-0 = <&mdio_pins>, <&rgmii1_pins>;
>     }
>     
>     &gmac1 {
>     	/* Has state = "disabled" by default */
>     	pinctrl-names = "default";
>     	pinctrl-0 = <&mdio_pins>, <&rgmii2_pins>;
>     }
> 
> That way, if the pinctrl-s are processed properly, the rgmii2_pins setting would be
> applied automatically when gmac1 is enabled. mdio_pins would be applied when either (or
> both) gmac-s is (are) active.

This is also what I thought would be best but unfortunately that won't 
work. Please read the responses on this thread:
https://lore.kernel.org/netdev/83a35aa3-6cb8-2bc4-2ff4-64278bbcd8c8@arinc9.com/T/

> 
>> diff --git a/target/linux/ramips/dts/mt7621.dtsi b/target/linux/ramips/dts/mt7621.dtsi
>> index bfb66740a2..56799201c0 100644
>> --- a/target/linux/ramips/dts/mt7621.dtsi
>> +++ b/target/linux/ramips/dts/mt7621.dtsi
>> @@ -456,6 +456,9 @@
>>   
>>                  mediatek,ethsys = <&sysc>;
>>   
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&rgmii1_pins &rgmii2_pins &mdio_pins>;
> 
> Note that this should be a list of size-1 items like above. In binary form, both come down
> to the same thing, but this notation would be used on specifications with non-zero cells
> (cf. "#interrupts-cells = <1>" and "<&int_ctl 16>").

This is something I should address here and on upstream, thanks for 
reporting this!

Let me know what do you think of this patch, I don't want to send v4 
until we're on the same page.

Cheers.
Arınç