Re: [RFC] pinctrl: renesas: rzg2l: Add support to select MII/RGMII mode

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Biju,

CC Sergey

On Thu, Nov 18, 2021 at 3:56 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> RZ/G2L supports Ether MII/RGMII mode control which select
> the direction of the pins based on PHY mode.
>
> This patch adds support to select MII/RGMII mode based on
> the phy-mode property present in the ether node, as the
> register for configuring the same is located in pinctrl block
> rather than GbEthernet block.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

Thanks for your patch!

I can't really say I'm very enthusiastic about this. Looking up
Ethernet nodes like this is fragile, and doesn't work with dynamic
DT overlays (which are not supported upstream anyway, doh).
I think it would be better to configure this explicitly in DT,
especially since in RGMII mode, the user is supposed to configure
the TX_COL, TX_CRS, and RX_ERR in some specific way, too.

Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml does
not define anything like this, but perhaps we can do something
direction-based?
The documentation for the related bits in the ETHER_MODE register says:

    B'0 = RGMII: The Direction of the IO buffer is Output.
    B’1 = MII(Initial value): The Direction of the IO buffer is Input.

Below a few general coding comments (which hopefully become
irrelevant soon...).

> ---
> Current names on HW manual is based on pins which is going to
> change in next version like below.
>
> P20_0->ETH0_mode
> P29_0->ETH1_mode
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 70 +++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index ccee9c9e2e22..bc86119be01e 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -92,6 +92,7 @@
>  #define PWPR                   (0x3014)
>  #define SD_CH(n)               (0x3000 + (n) * 4)
>  #define QSPI                   (0x3008)
> +#define ETHER_MODE             (0x3018)
>
>  #define PVDD_1800              1       /* I/O domain voltage <= 1.8V */
>  #define PVDD_3300              0       /* I/O domain voltage >= 3.3V */
> @@ -104,6 +105,10 @@
>  #define PFC_MASK               0x07
>  #define IEN_MASK               0x01
>  #define IOLH_MASK              0x03
> +#define ETHER_MODE_ETH0_MASK   BIT(0)
> +#define ETHER_MODE_ETH1_MASK   BIT(1)
> +#define ETHER_MODE_ETH0_ADDR   0x11c20000
> +#define ETHER_MODE_ETH1_ADDR   0x11c30000
>
>  #define PM_INPUT               0x1
>  #define PM_OUTPUT              0x2
> @@ -1197,6 +1202,69 @@ static void rzg2l_pinctrl_clk_disable(void *data)
>         clk_disable_unprepare(data);
>  }
>
> +static bool rzg2l_pinctrl_is_rgmii_mode(struct rzg2l_pinctrl *pctrl,
> +                                       struct device_node *node)
> +{
> +       const char *mode = of_get_property(node, "phy-mode", NULL);
> +       bool ret = false;
> +
> +       if (!mode) {
> +               dev_err(pctrl->dev, "phy-mode missing, setting to mii mode");
> +               return ret;
> +       }
> +
> +       if ((!strcmp("rgmii", mode)) || (!strcmp("rgmii-id", mode)) ||
> +           (!strcmp("rgmii-rxid", mode)) || (!strcmp("rgmii-txid", mode)))

return !strncmp(mode, "rgmii", strlen("rgmii"));?

> +               ret = true;
> +
> +       return ret;
> +}
> +
> +static void rzg2l_pinctrl_set_eth_mode(struct rzg2l_pinctrl *pctrl,
> +                                      struct device_node *np)
> +{
> +       u8 reg = readb(pctrl->base + ETHER_MODE);
> +       u8 mask = ETHER_MODE_ETH0_MASK;
> +       const __be32 *prop;
> +       u64 addr;
> +
> +       prop = of_get_property(np, "reg", NULL);
> +       if (!prop)
> +               return;
> +
> +       addr = of_read_number(prop, of_n_addr_cells(np));

of_address_to_resource()

> +       if (addr == ETHER_MODE_ETH1_ADDR)
> +               mask = ETHER_MODE_ETH1_MASK;
> +
> +       if (rzg2l_pinctrl_is_rgmii_mode(pctrl, np))
> +               reg &= ~mask;
> +       else
> +               reg |= mask;
> +
> +       writeb(reg, pctrl->base + ETHER_MODE);
> +}
> +
> +static void rzg2l_pinctrl_set_ether_modes(struct rzg2l_pinctrl *pctrl)
> +{
> +       struct device_node *np, *np1 = NULL;
> +
> +       np = of_find_compatible_node(NULL, NULL, "renesas,rzg2l-gbeth");
> +       if (np) {
> +               np1 = of_find_compatible_node(np, NULL, "renesas,rzg2l-gbeth");
> +               if (of_device_is_available(np))
> +                       rzg2l_pinctrl_set_eth_mode(pctrl, np);
> +
> +               of_node_put(np);
> +       }
> +
> +       if (np1) {
> +               if (of_device_is_available(np1))
> +                       rzg2l_pinctrl_set_eth_mode(pctrl, np1);
> +
> +               of_node_put(np1);
> +       }

for_each_compatible_node(...) { ... }

> +}
> +
>  static int rzg2l_pinctrl_probe(struct platform_device *pdev)
>  {
>         struct rzg2l_pinctrl *pctrl;
> @@ -1246,6 +1314,8 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> +       rzg2l_pinctrl_set_ether_modes(pctrl);
> +
>         dev_info(pctrl->dev, "%s support registered\n", DRV_NAME);
>         return 0;
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds