Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Fabrizio,

CC wolfram

On Wed, Sep 19, 2018 at 12:19 PM Fabrizio Castro
<fabrizio.castro@xxxxxxxxxxxxxx> wrote:
> Although this patch is pretty much standard, I would like to start a discussion as while testing SDHI2 (which goes on the uSD connector on the bottom side of the iwg23s) I have come across an issue. The POC Control Register (IOCTRL6) of the RZ/G1C is structured in a completely different way from the other members of the RZ/G1 family, only one bit is used to control the interface, as opposed to the usual one bit per pin layout.
>
> There are 3 possible ways to fix this:
> 1) keep the clk pin of the interface in a pin group on its own in the PFC driver (which means I would need to drop this patch or rework the pin groups with an additional patch), specify SH_PFC_PIN_CFG_IO_VOLTAGE for the clock line alone, keep the clk pin in a device tree node on its own in the board specific device tree and specify power-source only within the device tree node containing the clk line. The SD card device tree node in the board specific device tree would look like the following:
> ...
> pinctrl-0 = <&sdhi2_pins>, <&sdhi2_pins_clk>;
> pinctrl-1 = <&sdhi2_pins>, <&sdhi2_pins_clk_uhs>;
> pinctrl-names = "default", "state_uhs";
> ....

That matches the datasheet, which says the bit is for the CLK line,
but that can't
be true, as the voltage selection should affect other lines, too.

> 2) Specify SH_PFC_PIN_CFG_IO_VOLTAGE for every line that belongs to the interface, keep the SD card pin groups as specified by this patch, map all of the pins to the same bit in the POC register (as per pin_to_pocctrl is concerned), and the board specific device tree definitions would look like every other RZ/G1 or R-Car Gen2 boards that support SDR*
> The only downside would be that the kernel would read-modify-write the POC Control Register with the same value for every line in the interface.

This looks the most sensible solution to me: just map in your
.pin_to_pocctrl() method
all pins of the interface to the single bit.

> 3) specify SH_PFC_PIN_CFG_IO_VOLTAGE for the clock line alone, come up with another macro for the other lines, keep the pin groups as specified by this patch, modify the logic of sh_pfc_pinconf_set and sh_pfc_pinconf_validate, and the board specific device tree would look like any other RZ/G1 or R-Car Gen2 board that supports SDR*

This looks overly complex.
So .pin_to_pocctrl() would need to return "ignore" for the other pins,
which can work easily for the "set" case, but not for the "get" case.

> I am not particularly enthusiastic about option 3), but option 1) and option 2) seem equally sound to me.
>
> What do you think about this?

I'd go for option 2.

Wolfram: what do you think?

Thanks!

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