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

[Arınç ÜNAL]

On 14/02/2022 12:30, Sander Vanheule wrote:
> On Sun, 2022-02-13 at 19:23 +0300, Arınç ÜNAL wrote:
>> 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.
> 
> That doesn't sound great either.
> 
> 
>> 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.
> 
> Not sure what you mean by this. So the "rgmii2" function is normally the
> default, but you want to make this explicit by adding it the the &ethernet node?

It's not explicitly giving it the rgmii2 function (since it's already 
there on mt7621.dtsi) but rather have the rgmii2 pin group called from 
the ethernet node so traffic can flow on the rgmii2 bus. I explained 
this in detail on my response to Bjørn here:

http://lists.openwrt.org/pipermail/openwrt-devel/2022-February/037939.html
http://lists.openwrt.org/pipermail/openwrt-devel/2022-February/037949.html

> 
> &state_default does not refer to the SoC's default HW state, but to the device's
> actual configuration. To be really 'clean', I would say the "default" state (as
> in 'pinctrl-names = "default"') of the GPIO node should be the one indicating
> which groups are to be configured as GPIO, as &state_default is the "default"
> state of the pin controller. But such a change is probably out of scope for this
> patch series.

Correct.

> 
>>
>>>
>>>>
>>>>>
>>>>> 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).
> 
> I had a brief look at the code (elixir.bootlin.org is very convenient for
> browsing the kernel), and it appears that while 'ethernet' is probed by the
> kernel as a proper device, the gmac nodes aren't. That means you don't get the
> automatic kernel machinery that comes associated with device probing, which
> includes pinctrl configuration. The gmac nodes are probed by the ethernet driver
> itself, and although that performs pinctrl operations on the main device
> ('ethernet' node), it doesn't do this for the gmac-s. It probably could do this,
> but I have no idea whether that would be acceptable, and is more a matter to
> discuss with upstream and the device driver maintainers.

Agreed.

Arınç