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

[Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>]

Hello Geert,

Thank you for your feedback!

> Subject: Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
>
> 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.

Thank you for your advice, I am going to send a series implementing "option 2" shortly.

Thanks,
Fab

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



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