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

[Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>]

Hello Geert,

> Subject: Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
>
> On Tue, Sep 18, 2018 at 3:48 PM Fabrizio Castro
> <fabrizio.castro@xxxxxxxxxxxxxx> wrote:
> > Add SDHI2 pin groups and functions to the R8A77470 SoC.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > Reviewed-by: Biju Das <biju.das@xxxxxxxxxxxxxx>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> But there are more SDHI channels?

We can't test SDHI0, and SDHI1 is used for the eMMC.

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

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*

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?

Thanks,
Fab

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



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.