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

[Sander Vanheule]

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

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.

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

Best,
Sander

> +
>                 gmac0: mac at 0 {
>                         compatible = "mediatek,eth-mac";
>                         reg = <0>;