Re: [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Linus, Fabrizio,

On Sat, Jun 24, 2023 at 9:12 PM Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Fri, Jun 23, 2023 at 10:40 AM Geert Uytterhoeven
> <geert@xxxxxxxxxxxxxx> wrote:
> > On Fri, Jun 23, 2023 at 10:01 AM Fabrizio Castro
> > <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> > > pinctrl-rzv2m b6250000.pinctrl: pin P8_2 already requested by 85000000.mmc; cannot claim for 85020000.mmc
> > > pinctrl-rzv2m b6250000.pinctrl: pin-130 (85020000.mmc) status -22
> > > renesas_sdhi_internal_dmac 85020000.mmc: Error applying setting, reverse things back

I managed to reproduce the issue[*], and devised a fix, which I will
send shortly.

> > To me, that sounds like a bug in the pinctrl core.
> > Or am I missing something?
>
> The pin control core tracks on a per-pin basis, it has no clue about
> the name of certain dt nodes.
>
> This bug would be in the DT parsing code for the different states
> I think, and rzv2m is not using the core helpers for this but
> rather rzv2m_dt_subnode_to_map() etc.

Indeed, there's an issue in rzv2m_dt_subnode_to_map(): it registers
groups and functions using just the subnode names, which may not
be unique.  When this happens, they are ignored silently.

      $ cat /sys/kernel/debug/pinctrl/b6250000.pinctrl/pingroups
      registered pin groups:
      group: data
      pin 130 (P8_2)
      pin 131 (P8_3)
      pin 132 (P8_4)
      pin 133 (P8_5)

      group: ctrl
      pin 128 (P8_0)
      pin 129 (P8_1)

      group: cd
      pin 135 (P8_7)

      $ cat /sys/kernel/debug/pinctrl/b6250000.pinctrl/pinmux-functions
      function 0: data, groups = [ data ]
      function 1: ctrl, groups = [ ctrl ]
      function 2: cd, groups = [ cd ]

You can see this by adding checks in the pin control core:

--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -639,8 +639,10 @@ int pinctrl_generic_add_group(struct pinctrl_dev
*pctldev, const char *name,
                return -EINVAL;

        selector = pinctrl_generic_group_name_to_selector(pctldev, name);
-       if (selector >= 0)
+       if (selector >= 0) {
+               pr_err("Duplicate group name %s (selector %d)\n",
name, selector);
                return selector;
+       }

        selector = pctldev->num_groups;

--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -878,8 +878,10 @@ int pinmux_generic_add_function(struct
pinctrl_dev *pctldev,
                return -EINVAL;

        selector = pinmux_func_name_to_selector(pctldev, name);
-       if (selector >= 0)
+       if (selector >= 0) {
+pr_err("Duplicate function name %s (selector %d)\n", name, selector);
                return selector;
+       }

        selector = pctldev->num_functions;

Is there any special reason why such duplicates are just ignored,
and not flagged as an error?

The RZ/G2L and RZ/N1 pin control drivers have the same issue, but it
is not triggered on these platforms (yet), as their DTS files either
do not use subnodes, or use unique subnode names.

The RZ/A1 and RZ/A2 pin control drivers are not affected, as they do
not support subnodes.

The StarFive JH7100 pin control driver does it right, by constructing
group/function names from both the node and the subnode names.

[*] As I do not have an RZ/V2M board, I added pin control, eMMC, and
    SDHI snippets from RZ/V2M to the Koelsch DTS, and modified the
    drivers to not touch the non-existing hardware.




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