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

[Arınç ÜNAL]

On 13/02/2022 17:14, Sander Vanheule wrote:

[snip]

>>> 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.
> 
> An alternative would be to add the rgmii2 configuration to &state_default node only on the
> devices where it is actually required.
> 
>     &state_default {
>     	gpio {
>     		groups = /* gpio groups */
>     		function = "gpio";
>     	};
>     
>     	gmac1 {
>     		groups = "rgmii2";
>     		function = "rgmii2";
>     	};
>     };

That would mean I'd have to do this on ~70 devicetrees (instead of 26 as 
originally proposed) which I'm going to mux PHY 0 or 4 to GMAC1 with my 
next patch.

Secondly, the default mode for the rgmii2 pin group is the SoC function 
(which is "rgmii2") and not gpio. To add to this, the majority of the 
devicetrees use the SoC function for rgmii2 so we're already doing the 
most minimal changes with the current patch.

> 
>>
>>>
>>> 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/
> 
> Right, I think you've pointed this out already a few times.Sorry for the noise, I should've re-read that thread before replying. :)
> 
> Since these gmac-s are always internal to the SoC, pinctrl properties should make sense
> there. Did you investigate why these settings don't get applied?

No, and I don't think I'm capable of doing so (not much of a code reader 
I'm not).

Arınç