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

Sun Feb 13 06:14:38 PST 2022

On Sun, 2022-02-13 at 16:06 +0300, Arınç ÜNAL wrote:
> 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.

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";
   	};
   };

> 
> > 
> > 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?

> > 
> > > 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.

I think it's rather large and intrusive, but if nobody can see any proper alternatives
otherwise, I'm not the one to stop this from being merged.

Best,
Sander