RE: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
Hello Wolfram, Thank you for your feedback! > -Original Message- > From: Wolfram Sang > Sent: 21 September 2018 16:48 > To: Geert Uytterhoeven > Cc: Fabrizio Castro ; Laurent Pinchart > ; Geert Uytterhoeven > ; Linus Walleij ; > Linux-Renesas ; open > list:GPIO SUBSYSTEM ; Simon Horman > ; Chris Paterson > ; Biju Das ; Wolfram > Sang > Subject: Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups > > > > > 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. > > I don't think this multiple RMW is a problem. > > > 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. > > I didn't fully get if this one bit controls only the CLK wire or all the > relevant wires? I assume it is the latter one. For that, option 2) is > totally fine with me. It is not super clear what the intended behaviour is from the HW User's manual indeed, but I have tested SDR50 and it seems to be working fine, which would indicate that the control bit in the POC Control Register is actually controlling all of the lines of the interface, and not just the CLK pin. Thanks, Fab Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
> > 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. I don't think this multiple RMW is a problem. > 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. I didn't fully get if this one bit controls only the CLK wire or all the relevant wires? I assume it is the latter one. For that, option 2) is totally fine with me. signature.asc Description: PGP signature
RE: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
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
> 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 = <_pins>, <_pins_clk>;
> > pinctrl-1 = <_pins>, <_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 --
> ge...@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
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End,
Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered
No. 04586709.
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
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 = <_pins>, <_pins_clk>;
> pinctrl-1 = <_pins>, <_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 -- ge...@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
RE: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
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
> wrote:
> > Add SDHI2 pin groups and functions to the R8A77470 SoC.
> >
> > Signed-off-by: Fabrizio Castro
> > Reviewed-by: Biju Das
>
> Reviewed-by: Geert Uytterhoeven
>
> 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 = <_pins>, <_pins_clk>;
pinctrl-1 = <_pins>, <_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 --
> ge...@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
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End,
Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered
No. 04586709.
Re: [PATCH 4/5] pinctrl: sh-pfc: r8a77470: Add SDHI2 pin groups
On Tue, Sep 18, 2018 at 3:48 PM Fabrizio Castro
wrote:
> Add SDHI2 pin groups and functions to the R8A77470 SoC.
>
> Signed-off-by: Fabrizio Castro
> Reviewed-by: Biju Das
Reviewed-by: Geert Uytterhoeven
But there are more SDHI channels?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
