* Re: [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names [not found] <20230623080135.15696-1-fabrizio.castro.jz@renesas.com> @ 2023-06-23 8:40 ` Geert Uytterhoeven 2023-06-23 9:16 ` Fabrizio Castro 2023-06-24 19:11 ` Linus Walleij 0 siblings, 2 replies; 4+ messages in thread From: Geert Uytterhoeven @ 2023-06-23 8:40 UTC (permalink / raw) To: Fabrizio Castro Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Magnus Damm, linux-renesas-soc, devicetree, linux-kernel, Chris Paterson, Biju Das, Lad Prabhakar, Linus Walleij, open list:GPIO SUBSYSTEM Hi Fabrizio, CC pinctrl Thanks for your patch! On Fri, Jun 23, 2023 at 10:01 AM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > The original commit uses the same names ("data" and "ctrl") > for the subnodes of pinctrl definitions for both eMMC and > SDHI0 (emmc_pins, sdhi0_pins, and sdhi0_pins_uhs) leading to That should be fine, as the parents of these subnodes do have different names? > the below issue: > > 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 To me, that sounds like a bug in the pinctrl core. Or am I missing something? > This commit fixes the problem by making the names for the > pinctrl subnodes of eMMC and SDHI0 unique. > > Fixes: b6c0be722b0c ("arm64: dts: renesas: rzv2mevk2: Add uSD card and eMMC support") > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > .../arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > index 39fe3f94991e..11c5cffea5a5 100644 > --- a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > +++ b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > @@ -167,7 +167,7 @@ &i2c2 { > > &pinctrl { > emmc_pins: emmc { > - data { > + emmc_data { > pinmux = <RZV2M_PORT_PINMUX(0, 0, 2)>, /* MMDAT0 */ > <RZV2M_PORT_PINMUX(0, 1, 2)>, /* MMDAT1 */ > <RZV2M_PORT_PINMUX(0, 2, 2)>, /* MMDAT2 */ > @@ -179,7 +179,7 @@ data { > power-source = <1800>; > }; > > - ctrl { > + emmc_ctrl { > pinmux = <RZV2M_PORT_PINMUX(0, 10, 2)>, /* MMCMD */ > <RZV2M_PORT_PINMUX(0, 11, 2)>; /* MMCLK */ > power-source = <1800>; > @@ -197,7 +197,7 @@ i2c2_pins: i2c2 { > }; > > sdhi0_pins: sd0 { > - data { > + sd0_data { > pinmux = <RZV2M_PORT_PINMUX(8, 2, 1)>, /* SD0DAT0 */ > <RZV2M_PORT_PINMUX(8, 3, 1)>, /* SD0DAT1 */ > <RZV2M_PORT_PINMUX(8, 4, 1)>, /* SD0DAT2 */ > @@ -205,20 +205,20 @@ data { > power-source = <3300>; > }; > > - ctrl { > + sd0_ctrl { > pinmux = <RZV2M_PORT_PINMUX(8, 0, 1)>, /* SD0CMD */ > <RZV2M_PORT_PINMUX(8, 1, 1)>; /* SD0CLK */ > power-source = <3300>; > }; > > - cd { > + sd0_cd { > pinmux = <RZV2M_PORT_PINMUX(8, 7, 1)>; /* SD0CD */ > power-source = <3300>; > }; > }; > > sdhi0_pins_uhs: sd0-uhs { > - data { > + sd0_uhs_data { > pinmux = <RZV2M_PORT_PINMUX(8, 2, 1)>, /* SD0DAT0 */ > <RZV2M_PORT_PINMUX(8, 3, 1)>, /* SD0DAT1 */ > <RZV2M_PORT_PINMUX(8, 4, 1)>, /* SD0DAT2 */ > @@ -226,13 +226,13 @@ data { > power-source = <1800>; > }; > > - ctrl { > + sd0_uhs_ctrl { > pinmux = <RZV2M_PORT_PINMUX(8, 0, 1)>, /* SD0CMD */ > <RZV2M_PORT_PINMUX(8, 1, 1)>; /* SD0CLK */ > power-source = <1800>; > }; > > - cd { > + sd0_uhs_cd { > pinmux = <RZV2M_PORT_PINMUX(8, 7, 1)>; /* SD0CD */ > power-source = <1800>; > }; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names 2023-06-23 8:40 ` [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names Geert Uytterhoeven @ 2023-06-23 9:16 ` Fabrizio Castro 2023-06-24 19:11 ` Linus Walleij 1 sibling, 0 replies; 4+ messages in thread From: Fabrizio Castro @ 2023-06-23 9:16 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Magnus Damm, linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Chris Paterson, Biju Das, Prabhakar Mahadev Lad, Linus Walleij, open list:GPIO SUBSYSTEM Hi Geert, Thanks for your reply. > From: Geert Uytterhoeven <geert@linux-m68k.org> > Subject: Re: [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI > pinctrl names > > Hi Fabrizio, > > CC pinctrl > > Thanks for your patch! > > On Fri, Jun 23, 2023 at 10:01 AM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > The original commit uses the same names ("data" and "ctrl") > > for the subnodes of pinctrl definitions for both eMMC and > > SDHI0 (emmc_pins, sdhi0_pins, and sdhi0_pins_uhs) leading to > > That should be fine, as the parents of these subnodes do have > different > names? That's what I originally thought as well. > > > the below issue: > > > > 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 > > To me, that sounds like a bug in the pinctrl core. > Or am I missing something? I am not sure if this behaviour is intended or not, I'll probably have a deeper look into it later on if I have time, but right now (with the current state of things) this patch is necessary. It doesn't hurt to have unique names for subnodes, and looking through Renesas device trees, the names for the subnodes are unique for the other platforms. By the way, I didn't see the problem when I tested the original patch (I still have the logs of the original testing, that was based 6.2.0-rc3+, and all was working well), therefore it'd be good to look further into this later on, for future reference. Cheers, Fab > > > This commit fixes the problem by making the names for the > > pinctrl subnodes of eMMC and SDHI0 unique. > > > > Fixes: b6c0be722b0c ("arm64: dts: renesas: rzv2mevk2: Add uSD card > and eMMC support") > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > --- > > .../arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts | 16 ++++++++----- > --- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > > index 39fe3f94991e..11c5cffea5a5 100644 > > --- a/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > > +++ b/arch/arm64/boot/dts/renesas/r9a09g011-v2mevk2.dts > > @@ -167,7 +167,7 @@ &i2c2 { > > > > &pinctrl { > > emmc_pins: emmc { > > - data { > > + emmc_data { > > pinmux = <RZV2M_PORT_PINMUX(0, 0, 2)>, /* > MMDAT0 */ > > <RZV2M_PORT_PINMUX(0, 1, 2)>, /* > MMDAT1 */ > > <RZV2M_PORT_PINMUX(0, 2, 2)>, /* > MMDAT2 */ > > @@ -179,7 +179,7 @@ data { > > power-source = <1800>; > > }; > > > > - ctrl { > > + emmc_ctrl { > > pinmux = <RZV2M_PORT_PINMUX(0, 10, 2)>, /* > MMCMD */ > > <RZV2M_PORT_PINMUX(0, 11, 2)>; /* > MMCLK */ > > power-source = <1800>; > > @@ -197,7 +197,7 @@ i2c2_pins: i2c2 { > > }; > > > > sdhi0_pins: sd0 { > > - data { > > + sd0_data { > > pinmux = <RZV2M_PORT_PINMUX(8, 2, 1)>, /* > SD0DAT0 */ > > <RZV2M_PORT_PINMUX(8, 3, 1)>, /* > SD0DAT1 */ > > <RZV2M_PORT_PINMUX(8, 4, 1)>, /* > SD0DAT2 */ > > @@ -205,20 +205,20 @@ data { > > power-source = <3300>; > > }; > > > > - ctrl { > > + sd0_ctrl { > > pinmux = <RZV2M_PORT_PINMUX(8, 0, 1)>, /* > SD0CMD */ > > <RZV2M_PORT_PINMUX(8, 1, 1)>; /* > SD0CLK */ > > power-source = <3300>; > > }; > > > > - cd { > > + sd0_cd { > > pinmux = <RZV2M_PORT_PINMUX(8, 7, 1)>; /* > SD0CD */ > > power-source = <3300>; > > }; > > }; > > > > sdhi0_pins_uhs: sd0-uhs { > > - data { > > + sd0_uhs_data { > > pinmux = <RZV2M_PORT_PINMUX(8, 2, 1)>, /* > SD0DAT0 */ > > <RZV2M_PORT_PINMUX(8, 3, 1)>, /* > SD0DAT1 */ > > <RZV2M_PORT_PINMUX(8, 4, 1)>, /* > SD0DAT2 */ > > @@ -226,13 +226,13 @@ data { > > power-source = <1800>; > > }; > > > > - ctrl { > > + sd0_uhs_ctrl { > > pinmux = <RZV2M_PORT_PINMUX(8, 0, 1)>, /* > SD0CMD */ > > <RZV2M_PORT_PINMUX(8, 1, 1)>; /* > SD0CLK */ > > power-source = <1800>; > > }; > > > > - cd { > > + sd0_uhs_cd { > > pinmux = <RZV2M_PORT_PINMUX(8, 7, 1)>; /* > SD0CD */ > > power-source = <1800>; > > }; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names 2023-06-23 8:40 ` [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names Geert Uytterhoeven 2023-06-23 9:16 ` Fabrizio Castro @ 2023-06-24 19:11 ` Linus Walleij 2023-07-03 14:49 ` Geert Uytterhoeven 1 sibling, 1 reply; 4+ messages in thread From: Linus Walleij @ 2023-06-24 19:11 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Fabrizio Castro, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Magnus Damm, linux-renesas-soc, devicetree, linux-kernel, Chris Paterson, Biju Das, Lad Prabhakar, open list:GPIO SUBSYSTEM On Fri, Jun 23, 2023 at 10:40 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Jun 23, 2023 at 10:01 AM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > the below issue: > > > > 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 > > 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. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names 2023-06-24 19:11 ` Linus Walleij @ 2023-07-03 14:49 ` Geert Uytterhoeven 0 siblings, 0 replies; 4+ messages in thread From: Geert Uytterhoeven @ 2023-07-03 14:49 UTC (permalink / raw) To: Linus Walleij, Fabrizio Castro Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Magnus Damm, linux-renesas-soc, devicetree, linux-kernel, Chris Paterson, Biju Das, Lad Prabhakar, open list:GPIO SUBSYSTEM Hi Linus, Fabrizio, On Sat, Jun 24, 2023 at 9:12 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Jun 23, 2023 at 10:40 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Fri, Jun 23, 2023 at 10:01 AM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> 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@linux-m68k.org 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-03 14:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230623080135.15696-1-fabrizio.castro.jz@renesas.com>
2023-06-23 8:40 ` [PATCH] arm64: dts: renesas: rzv2mevk2: Fix eMMC/SDHI pinctrl names Geert Uytterhoeven
2023-06-23 9:16 ` Fabrizio Castro
2023-06-24 19:11 ` Linus Walleij
2023-07-03 14:49 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).