[PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Ramesh Shanmugasundaram]

This patch adds DRIF[0-3] pinmux support for r8a7795 SoC.

Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
 drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121 +++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
index 33be5d56..6f246ec 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
@@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] = {
 	CANFD1_TX_MARK,         CANFD1_RX_MARK,
 };
 
+/* - DRIF --------------------------------------------------------------- */
+static const unsigned int drif0_data_a_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
+	RCAR_GP_PIN(6, 7),
+};
+static const unsigned int drif0_data_a_mux[] = {
+	RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK, RIF0_D1_A_MARK,
+};
+static const unsigned int drif0_data_b_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(5, 0), RCAR_GP_PIN(5, 4), RCAR_GP_PIN(5, 1),
+	RCAR_GP_PIN(5, 2),
+};
+static const unsigned int drif0_data_b_mux[] = {
+	RIF0_CLK_B_MARK, RIF0_SYNC_B_MARK, RIF0_D0_B_MARK, RIF0_D1_B_MARK,
+};
+static const unsigned int drif0_data_c_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(5, 12), RCAR_GP_PIN(5, 15), RCAR_GP_PIN(5, 13),
+	RCAR_GP_PIN(5, 14),
+};
+static const unsigned int drif0_data_c_mux[] = {
+	RIF0_CLK_C_MARK, RIF0_SYNC_C_MARK, RIF0_D0_C_MARK, RIF0_D1_C_MARK,
+};
+
+static const unsigned int drif1_data_a_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18), RCAR_GP_PIN(6, 19),
+	RCAR_GP_PIN(6, 20),
+};
+static const unsigned int drif1_data_a_mux[] = {
+	RIF1_CLK_A_MARK, RIF1_SYNC_A_MARK, RIF1_D0_A_MARK, RIF1_D1_A_MARK,
+};
+static const unsigned int drif1_data_b_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(5, 9), RCAR_GP_PIN(5, 3), RCAR_GP_PIN(5, 7),
+	RCAR_GP_PIN(5, 8),
+};
+static const unsigned int drif1_data_b_mux[] = {
+	RIF1_CLK_B_MARK, RIF1_SYNC_B_MARK, RIF1_D0_B_MARK, RIF1_D1_B_MARK,
+};
+static const unsigned int drif1_data_c_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(5, 5), RCAR_GP_PIN(5, 11), RCAR_GP_PIN(5, 6),
+	RCAR_GP_PIN(5, 10),
+};
+static const unsigned int drif1_data_c_mux[] = {
+	RIF1_CLK_C_MARK, RIF1_SYNC_C_MARK, RIF1_D0_C_MARK, RIF1_D1_C_MARK,
+};
+
+static const unsigned int drif2_data_a_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 7),
+	RCAR_GP_PIN(6, 10),
+};
+static const unsigned int drif2_data_a_mux[] = {
+	RIF2_CLK_A_MARK, RIF2_SYNC_A_MARK, RIF2_D0_A_MARK, RIF2_D1_A_MARK,
+};
+static const unsigned int drif2_data_b_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(6, 26), RCAR_GP_PIN(6, 27), RCAR_GP_PIN(6, 30),
+	RCAR_GP_PIN(6, 31),
+};
+static const unsigned int drif2_data_b_mux[] = {
+	RIF2_CLK_B_MARK, RIF2_SYNC_B_MARK, RIF2_D0_B_MARK, RIF2_D1_B_MARK,
+};
+
+static const unsigned int drif3_data_a_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18), RCAR_GP_PIN(6, 19),
+	RCAR_GP_PIN(6, 10),
+};
+static const unsigned int drif3_data_a_mux[] = {
+	RIF3_CLK_A_MARK, RIF3_SYNC_A_MARK, RIF3_D0_A_MARK, RIF3_D1_A_MARK,
+};
+static const unsigned int drif3_data_b_pins[] = {
+	/* CLK, SYNC, D0, D1 */
+	RCAR_GP_PIN(6, 24), RCAR_GP_PIN(6, 25), RCAR_GP_PIN(6, 28),
+	RCAR_GP_PIN(6, 29),
+};
+static const unsigned int drif3_data_b_mux[] = {
+	RIF3_CLK_B_MARK, RIF3_SYNC_B_MARK, RIF3_D0_B_MARK, RIF3_D1_B_MARK,
+};
+
 /* - HSCIF0 ----------------------------------------------------------------- */
 static const unsigned int hscif0_data_pins[] = {
 	/* RX, TX */
@@ -3350,6 +3435,16 @@ static const struct sh_pfc_pin_group pinmux_groups[] = {
 	SH_PFC_PIN_GROUP(canfd0_data_a),
 	SH_PFC_PIN_GROUP(canfd0_data_b),
 	SH_PFC_PIN_GROUP(canfd1_data),
+	SH_PFC_PIN_GROUP(drif0_data_a),
+	SH_PFC_PIN_GROUP(drif0_data_b),
+	SH_PFC_PIN_GROUP(drif0_data_c),
+	SH_PFC_PIN_GROUP(drif1_data_a),
+	SH_PFC_PIN_GROUP(drif1_data_b),
+	SH_PFC_PIN_GROUP(drif1_data_c),
+	SH_PFC_PIN_GROUP(drif2_data_a),
+	SH_PFC_PIN_GROUP(drif2_data_b),
+	SH_PFC_PIN_GROUP(drif3_data_a),
+	SH_PFC_PIN_GROUP(drif3_data_b),
 	SH_PFC_PIN_GROUP(hscif0_data),
 	SH_PFC_PIN_GROUP(hscif0_clk),
 	SH_PFC_PIN_GROUP(hscif0_ctrl),
@@ -3633,6 +3728,28 @@ static const char * const canfd1_groups[] = {
 	"canfd1_data",
 };
 
+static const char * const drif0_groups[] = {
+	"drif0_data_a",
+	"drif0_data_b",
+	"drif0_data_c",
+};
+
+static const char * const drif1_groups[] = {
+	"drif1_data_a",
+	"drif1_data_b",
+	"drif1_data_c",
+};
+
+static const char * const drif2_groups[] = {
+	"drif2_data_a",
+	"drif2_data_b",
+};
+
+static const char * const drif3_groups[] = {
+	"drif3_data_a",
+	"drif3_data_b",
+};
+
 static const char * const hscif0_groups[] = {
 	"hscif0_data",
 	"hscif0_clk",
@@ -3976,6 +4093,10 @@ static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(can_clk),
 	SH_PFC_FUNCTION(canfd0),
 	SH_PFC_FUNCTION(canfd1),
+	SH_PFC_FUNCTION(drif0),
+	SH_PFC_FUNCTION(drif1),
+	SH_PFC_FUNCTION(drif2),
+	SH_PFC_FUNCTION(drif3),
 	SH_PFC_FUNCTION(hscif0),
 	SH_PFC_FUNCTION(hscif1),
 	SH_PFC_FUNCTION(hscif2),
-- 
1.9.1

Re: [PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Geert Uytterhoeven]

Hi Ramesh,

On Fri, Jun 17, 2016 at 2:19 PM, Ramesh Shanmugasundaram
<ramesh.shanmugasundaram@bp.renesas.com> wrote:
> This patch adds DRIF[0-3] pinmux support for r8a7795 SoC.
>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>

Thanks for your patch!

> ---
>  drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121 +++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
>
> diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> index 33be5d56..6f246ec 100644
> --- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> @@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] = {
>         CANFD1_TX_MARK,         CANFD1_RX_MARK,
>  };
>
> +/* - DRIF --------------------------------------------------------------- */
> +static const unsigned int drif0_data_a_pins[] = {
> +       /* CLK, SYNC, D0, D1 */
> +       RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
> +       RCAR_GP_PIN(6, 7),
> +};
> +static const unsigned int drif0_data_a_mux[] = {
> +       RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK, RIF0_D1_A_MARK,
> +};

According to my information, each DRIF module consists of two sub-modules
(that's why there are 8 module clocks for 4 DRIF modules), each handling a data
pin, but sharing the CLK and SYNC signals.
Hence it's possible to receive using only one data pin. Is that correct?

Shouldn't the pinctrl data reflect that? The second unused data pin could be
used for something else.

One way to handle this is like SDHI and DU do: provide 2 sets of data, one for
single-bit, and one for dual-bit configurations:
  - "drif0_data1_a_pins" for CLK, SYNC, and D0,
  - "drif0_data2_a_pins" for CLK. SYNC, D0, and D1.

Alternative, it could be split in 3 groups:
  - "drif0_ctrl_a_pins" for CLK and SYNC,
  - "drif0_data0_a_pins for D0,
  - "drif0_data1_a_pins for D1.

I think the latter is preferable, as you can probably configure the DRIF to
receive data through D1 only, and leave D0 unused?

> +static const unsigned int drif3_data_a_pins[] = {
> +       /* CLK, SYNC, D0, D1 */
> +       RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18), RCAR_GP_PIN(6, 19),
> +       RCAR_GP_PIN(6, 10),

According to my datasheet, the above line should be

        RCAR_GP_PIN(6, 20),

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

RE: [PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Ramesh Shanmugasundaram]

Hi Geert,

Thanks for your time and review

> >  drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121
> > +++++++++++++++++++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >
> > diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > index 33be5d56..6f246ec 100644
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > @@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] = {
> >         CANFD1_TX_MARK,         CANFD1_RX_MARK,
> >  };
> >
> > +/* - DRIF
> > +--------------------------------------------------------------- */
> static const unsigned int drif0_data_a_pins[] = {
> > +       /* CLK, SYNC, D0, D1 */
> > +       RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
> > +       RCAR_GP_PIN(6, 7),
> > +};
> > +static const unsigned int drif0_data_a_mux[] = {
> > +       RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK,
> > +RIF0_D1_A_MARK, };
> 
> According to my information, each DRIF module consists of two sub-modules
> (that's why there are 8 module clocks for 4 DRIF modules), each handling a
> data pin, but sharing the CLK and SYNC signals.
> Hence it's possible to receive using only one data pin. Is that correct?

Yes, that is correct.

> 
> Shouldn't the pinctrl data reflect that? The second unused data pin could
> be used for something else.

Is that possible? For e.g. when MOD_SEL0(bit8 -> sel_drif2) is set to 0, all the 4 pins are owned by DRIF IP(RIF2_XXX_A set) even though one of the RIF2_D0_A or RIF2_D1_A may be unused depending on the master it interfaces with. Am I missing something?

> 
> One way to handle this is like SDHI and DU do: provide 2 sets of data, one
> for single-bit, and one for dual-bit configurations:
>   - "drif0_data1_a_pins" for CLK, SYNC, and D0,
>   - "drif0_data2_a_pins" for CLK. SYNC, D0, and D1.
> 
> Alternative, it could be split in 3 groups:
>   - "drif0_ctrl_a_pins" for CLK and SYNC,
>   - "drif0_data0_a_pins for D0,
>   - "drif0_data1_a_pins for D1.
> 
> I think the latter is preferable, as you can probably configure the DRIF
> to receive data through D1 only, and leave D0 unused?
> 
> > +static const unsigned int drif3_data_a_pins[] = {
> > +       /* CLK, SYNC, D0, D1 */
> > +       RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18), RCAR_GP_PIN(6, 19),
> > +       RCAR_GP_PIN(6, 10),
> 
> According to my datasheet, the above line should be
> 
>         RCAR_GP_PIN(6, 20),

You are right. Thanks again. I'll fix that typo :-(

Thanks,
Ramesh

Re: [PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Geert Uytterhoeven]

Hi Ramesh,

CC linux-gpio, LinusW, Laurent

On Fri, Jun 17, 2016 at 4:16 PM, Ramesh Shanmugasundaram
<ramesh.shanmugasundaram@bp.renesas.com> wrote:
>> >  drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121
>> > +++++++++++++++++++++++++++++++++++
>> >  1 file changed, 121 insertions(+)
>> >
>> > diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> > b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> > index 33be5d56..6f246ec 100644
>> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> > @@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] = {
>> >         CANFD1_TX_MARK,         CANFD1_RX_MARK,
>> >  };
>> >
>> > +/* - DRIF
>> > +--------------------------------------------------------------- */
>> static const unsigned int drif0_data_a_pins[] = {
>> > +       /* CLK, SYNC, D0, D1 */
>> > +       RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
>> > +       RCAR_GP_PIN(6, 7),
>> > +};
>> > +static const unsigned int drif0_data_a_mux[] = {
>> > +       RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK,
>> > +RIF0_D1_A_MARK, };
>>
>> According to my information, each DRIF module consists of two sub-modules
>> (that's why there are 8 module clocks for 4 DRIF modules), each handling a
>> data pin, but sharing the CLK and SYNC signals.
>> Hence it's possible to receive using only one data pin. Is that correct?
>
> Yes, that is correct.
>
>> Shouldn't the pinctrl data reflect that? The second unused data pin could
>> be used for something else.
>
> Is that possible? For e.g. when MOD_SEL0(bit8 -> sel_drif2) is set to 0, all the 4 pins are owned by DRIF IP(RIF2_XXX_A set) even though one of the RIF2_D0_A or RIF2_D1_A may be unused depending on the master it interfaces with. Am I missing something?

You're right, the whole block is switched.

The unused data pin can still be configured as a GPIO, as the GPSR register
(to select between GPIO and IP function) takes precedence.

Linus/Laurent: I assume the GPIO also takes precedence in the Linux pinctrl
subsystem, or would it return an error, as the pin is claimed by the IP group?

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

RE: [PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Ramesh Shanmugasundaram]

Hi Geert,

> CC linux-gpio, LinusW, Laurent
> 
> On Fri, Jun 17, 2016 at 4:16 PM, Ramesh Shanmugasundaram
> <ramesh.shanmugasundaram@bp.renesas.com> wrote:
> >> >  drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121
> >> > +++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 121 insertions(+)
> >> >
> >> > diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> >> > b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> >> > index 33be5d56..6f246ec 100644
> >> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> >> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> >> > @@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] =
> {
> >> >         CANFD1_TX_MARK,         CANFD1_RX_MARK,
> >> >  };
> >> >
> >> > +/* - DRIF
> >> > +--------------------------------------------------------------- */
> >> static const unsigned int drif0_data_a_pins[] = {
> >> > +       /* CLK, SYNC, D0, D1 */
> >> > +       RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
> >> > +       RCAR_GP_PIN(6, 7),
> >> > +};
> >> > +static const unsigned int drif0_data_a_mux[] = {
> >> > +       RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK,
> >> > +RIF0_D1_A_MARK, };
> >>
> >> According to my information, each DRIF module consists of two
> >> sub-modules (that's why there are 8 module clocks for 4 DRIF
> >> modules), each handling a data pin, but sharing the CLK and SYNC
> signals.
> >> Hence it's possible to receive using only one data pin. Is that
> correct?
> >
> > Yes, that is correct.
> >
> >> Shouldn't the pinctrl data reflect that? The second unused data pin
> >> could be used for something else.
> >
> > Is that possible? For e.g. when MOD_SEL0(bit8 -> sel_drif2) is set to 0,
> all the 4 pins are owned by DRIF IP(RIF2_XXX_A set) even though one of the
> RIF2_D0_A or RIF2_D1_A may be unused depending on the master it interfaces
> with. Am I missing something?
> 
> You're right, the whole block is switched.
> 
> The unused data pin can still be configured as a GPIO, as the GPSR
> register (to select between GPIO and IP function) takes precedence.

Is this precedence mentioned in h/w manual or is it based on current code & execution order? I assume it's the later?

> 
> Linus/Laurent: I assume the GPIO also takes precedence in the Linux
> pinctrl subsystem, or would it return an error, as the pin is claimed by
> the IP group?
> 

Looking at the code, pinctrl core does not allow multiple pin requests/claims when "strict" pmxops flag is set. However, Renesas pinctrl driver does not set this flag. So the request can proceed further in our case.

On my board, after IP claiming all the 4 pins I could still export one of the pins as GPIO (pinctrl subsystem is OK with it). From this point IP did not receive any data on this pin as expected. The other way around is around (GPIO->IP) is not possible because of this check in sh_pfc_func_set_mux.

359                 if (cfg->type != PINMUX_TYPE_NONE) {

I am happy with splitting the drif pins into three groups based on the above observation

  - "drif0_ctrl_a_pins" for CLK and SYNC,
  - "drif0_data0_a_pins for D0,
  - "drif0_data1_a_pins for D1.

If you are OK, I will send the reworked patch.

Thanks,
Ramesh

Re: [PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Geert Uytterhoeven]

Hi Ramesh,

On Wed, Jun 22, 2016 at 3:51 PM, Ramesh Shanmugasundaram
<ramesh.shanmugasundaram@bp.renesas.com> wrote:
>> CC linux-gpio, LinusW, Laurent
>>
>> On Fri, Jun 17, 2016 at 4:16 PM, Ramesh Shanmugasundaram
>> <ramesh.shanmugasundaram@bp.renesas.com> wrote:
>> >> >  drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121
>> >> > +++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 121 insertions(+)
>> >> >
>> >> > diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> >> > b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> >> > index 33be5d56..6f246ec 100644
>> >> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> >> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
>> >> > @@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] =
>> {
>> >> >         CANFD1_TX_MARK,         CANFD1_RX_MARK,
>> >> >  };
>> >> >
>> >> > +/* - DRIF
>> >> > +--------------------------------------------------------------- */
>> >> static const unsigned int drif0_data_a_pins[] = {
>> >> > +       /* CLK, SYNC, D0, D1 */
>> >> > +       RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
>> >> > +       RCAR_GP_PIN(6, 7),
>> >> > +};
>> >> > +static const unsigned int drif0_data_a_mux[] = {
>> >> > +       RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK,
>> >> > +RIF0_D1_A_MARK, };
>> >>
>> >> According to my information, each DRIF module consists of two
>> >> sub-modules (that's why there are 8 module clocks for 4 DRIF
>> >> modules), each handling a data pin, but sharing the CLK and SYNC
>> signals.
>> >> Hence it's possible to receive using only one data pin. Is that
>> correct?
>> >
>> > Yes, that is correct.
>> >
>> >> Shouldn't the pinctrl data reflect that? The second unused data pin
>> >> could be used for something else.
>> >
>> > Is that possible? For e.g. when MOD_SEL0(bit8 -> sel_drif2) is set to 0,
>> all the 4 pins are owned by DRIF IP(RIF2_XXX_A set) even though one of the
>> RIF2_D0_A or RIF2_D1_A may be unused depending on the master it interfaces
>> with. Am I missing something?
>>
>> You're right, the whole block is switched.
>>
>> The unused data pin can still be configured as a GPIO, as the GPSR
>> register (to select between GPIO and IP function) takes precedence.
>
> Is this precedence mentioned in h/w manual or is it based on current code & execution order? I assume it's the later?

Please see Figure 6.2 ("Procedure for Changing Pin Function from Peripheral
function to GPIO."), which says clearing a bit in GPSR is sufficient to
configure the corresponding pin as a GPIO.

>> Linus/Laurent: I assume the GPIO also takes precedence in the Linux
>> pinctrl subsystem, or would it return an error, as the pin is claimed by
>> the IP group?
>
> Looking at the code, pinctrl core does not allow multiple pin requests/claims when "strict" pmxops flag is set. However, Renesas pinctrl driver does not set this flag. So the request can proceed further in our case.
>
> On my board, after IP claiming all the 4 pins I could still export one of the pins as GPIO (pinctrl subsystem is OK with it). From this point IP did not receive any data on this pin as expected. The other way around is around (GPIO->IP) is not possible because of this check in sh_pfc_func_set_mux.
>
> 359                 if (cfg->type != PINMUX_TYPE_NONE) {

Thanks for verifying!

> I am happy with splitting the drif pins into three groups based on the above observation
>
>   - "drif0_ctrl_a_pins" for CLK and SYNC,
>   - "drif0_data0_a_pins for D0,
>   - "drif0_data1_a_pins for D1.
>
> If you are OK, I will send the reworked patch.

Thanks, that sounds great!

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

[PATCH v2] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Ramesh Shanmugasundaram]

This patch adds DRIF[0-3] pinmux support for r8a7795 SoC.

Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
---
Thanks Geert for the review.

This patch is based on renesas-drivers repo (tag:renesas-drivers-2016-06-21-v4.7-rc4)

Changes since v1:
 - Split DRIF data pins as separate groups so that unused data pins can be
   configured as GPIO (Geert)
 - Fixed DRIF3(A) D1 pin definition typo (Geert)
 - Rebased to latest tag renesas-drivers-2016-06-21-v4.7-rc4

Thanks,
Ramesh
---
 drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 291 +++++++++++++++++++++++++++++++++++
 1 file changed, 291 insertions(+)

diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
index 9f05157e..b74cdd3 100644
--- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
+++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
@@ -1656,6 +1656,221 @@ static const unsigned int canfd1_data_mux[] = {
 	CANFD1_TX_MARK,         CANFD1_RX_MARK,
 };
 
+/* - DRIF0 --------------------------------------------------------------- */
+static const unsigned int drif0_ctrl_a_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9),
+};
+static const unsigned int drif0_ctrl_a_mux[] = {
+	RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK,
+};
+static const unsigned int drif0_data0_a_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(6, 10),
+};
+static const unsigned int drif0_data0_a_mux[] = {
+	RIF0_D0_A_MARK,
+};
+static const unsigned int drif0_data1_a_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(6, 7),
+};
+static const unsigned int drif0_data1_a_mux[] = {
+	RIF0_D1_A_MARK,
+};
+static const unsigned int drif0_ctrl_b_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(5, 0), RCAR_GP_PIN(5, 4),
+};
+static const unsigned int drif0_ctrl_b_mux[] = {
+	RIF0_CLK_B_MARK, RIF0_SYNC_B_MARK,
+};
+static const unsigned int drif0_data0_b_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(5, 1),
+};
+static const unsigned int drif0_data0_b_mux[] = {
+	RIF0_D0_B_MARK,
+};
+static const unsigned int drif0_data1_b_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(5, 2),
+};
+static const unsigned int drif0_data1_b_mux[] = {
+	RIF0_D1_B_MARK,
+};
+static const unsigned int drif0_ctrl_c_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(5, 12), RCAR_GP_PIN(5, 15),
+};
+static const unsigned int drif0_ctrl_c_mux[] = {
+	RIF0_CLK_C_MARK, RIF0_SYNC_C_MARK,
+};
+static const unsigned int drif0_data0_c_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(5, 13),
+};
+static const unsigned int drif0_data0_c_mux[] = {
+	RIF0_D0_C_MARK,
+};
+static const unsigned int drif0_data1_c_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(5, 14),
+};
+static const unsigned int drif0_data1_c_mux[] = {
+	RIF0_D1_C_MARK,
+};
+/* - DRIF1 --------------------------------------------------------------- */
+static const unsigned int drif1_ctrl_a_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18),
+};
+static const unsigned int drif1_ctrl_a_mux[] = {
+	RIF1_CLK_A_MARK, RIF1_SYNC_A_MARK,
+};
+static const unsigned int drif1_data0_a_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(6, 19),
+};
+static const unsigned int drif1_data0_a_mux[] = {
+	RIF1_D0_A_MARK,
+};
+static const unsigned int drif1_data1_a_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(6, 20),
+};
+static const unsigned int drif1_data1_a_mux[] = {
+	RIF1_D1_A_MARK,
+};
+static const unsigned int drif1_ctrl_b_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(5, 9), RCAR_GP_PIN(5, 3),
+};
+static const unsigned int drif1_ctrl_b_mux[] = {
+	RIF1_CLK_B_MARK, RIF1_SYNC_B_MARK,
+};
+static const unsigned int drif1_data0_b_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(5, 7),
+};
+static const unsigned int drif1_data0_b_mux[] = {
+	RIF1_D0_B_MARK,
+};
+static const unsigned int drif1_data1_b_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(5, 8),
+};
+static const unsigned int drif1_data1_b_mux[] = {
+	RIF1_D1_B_MARK,
+};
+static const unsigned int drif1_ctrl_c_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(5, 5), RCAR_GP_PIN(5, 11),
+};
+static const unsigned int drif1_ctrl_c_mux[] = {
+	RIF1_CLK_C_MARK, RIF1_SYNC_C_MARK,
+};
+static const unsigned int drif1_data0_c_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(5, 6),
+};
+static const unsigned int drif1_data0_c_mux[] = {
+	RIF1_D0_C_MARK,
+};
+static const unsigned int drif1_data1_c_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(5, 10),
+};
+static const unsigned int drif1_data1_c_mux[] = {
+	RIF1_D1_C_MARK,
+};
+/* - DRIF2 --------------------------------------------------------------- */
+static const unsigned int drif2_ctrl_a_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9),
+};
+static const unsigned int drif2_ctrl_a_mux[] = {
+	RIF2_CLK_A_MARK, RIF2_SYNC_A_MARK,
+};
+static const unsigned int drif2_data0_a_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(6, 7),
+};
+static const unsigned int drif2_data0_a_mux[] = {
+	RIF2_D0_A_MARK,
+};
+static const unsigned int drif2_data1_a_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(6, 10),
+};
+static const unsigned int drif2_data1_a_mux[] = {
+	RIF2_D1_A_MARK,
+};
+static const unsigned int drif2_ctrl_b_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(6, 26), RCAR_GP_PIN(6, 27),
+};
+static const unsigned int drif2_ctrl_b_mux[] = {
+	RIF2_CLK_B_MARK, RIF2_SYNC_B_MARK,
+};
+static const unsigned int drif2_data0_b_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(6, 30),
+};
+static const unsigned int drif2_data0_b_mux[] = {
+	RIF2_D0_B_MARK,
+};
+static const unsigned int drif2_data1_b_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(6, 31),
+};
+static const unsigned int drif2_data1_b_mux[] = {
+	RIF2_D1_B_MARK,
+};
+/* - DRIF3 --------------------------------------------------------------- */
+static const unsigned int drif3_ctrl_a_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18),
+};
+static const unsigned int drif3_ctrl_a_mux[] = {
+	RIF3_CLK_A_MARK, RIF3_SYNC_A_MARK,
+};
+static const unsigned int drif3_data0_a_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(6, 19),
+};
+static const unsigned int drif3_data0_a_mux[] = {
+	RIF3_D0_A_MARK,
+};
+static const unsigned int drif3_data1_a_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(6, 20),
+};
+static const unsigned int drif3_data1_a_mux[] = {
+	RIF3_D1_A_MARK,
+};
+static const unsigned int drif3_ctrl_b_pins[] = {
+	/* CLK, SYNC */
+	RCAR_GP_PIN(6, 24), RCAR_GP_PIN(6, 25),
+};
+static const unsigned int drif3_ctrl_b_mux[] = {
+	RIF3_CLK_B_MARK, RIF3_SYNC_B_MARK,
+};
+static const unsigned int drif3_data0_b_pins[] = {
+	/* D0 */
+	RCAR_GP_PIN(6, 28),
+};
+static const unsigned int drif3_data0_b_mux[] = {
+	RIF3_D0_B_MARK,
+};
+static const unsigned int drif3_data1_b_pins[] = {
+	/* D1 */
+	RCAR_GP_PIN(6, 29),
+};
+static const unsigned int drif3_data1_b_mux[] = {
+	RIF3_D1_B_MARK,
+};
+
 /* - HSCIF0 ----------------------------------------------------------------- */
 static const unsigned int hscif0_data_pins[] = {
 	/* RX, TX */
@@ -3348,6 +3563,36 @@ static const struct sh_pfc_pin_group pinmux_groups[] = {
 	SH_PFC_PIN_GROUP(canfd0_data_a),
 	SH_PFC_PIN_GROUP(canfd0_data_b),
 	SH_PFC_PIN_GROUP(canfd1_data),
+	SH_PFC_PIN_GROUP(drif0_ctrl_a),
+	SH_PFC_PIN_GROUP(drif0_data0_a),
+	SH_PFC_PIN_GROUP(drif0_data1_a),
+	SH_PFC_PIN_GROUP(drif0_ctrl_b),
+	SH_PFC_PIN_GROUP(drif0_data0_b),
+	SH_PFC_PIN_GROUP(drif0_data1_b),
+	SH_PFC_PIN_GROUP(drif0_ctrl_c),
+	SH_PFC_PIN_GROUP(drif0_data0_c),
+	SH_PFC_PIN_GROUP(drif0_data1_c),
+	SH_PFC_PIN_GROUP(drif1_ctrl_a),
+	SH_PFC_PIN_GROUP(drif1_data0_a),
+	SH_PFC_PIN_GROUP(drif1_data1_a),
+	SH_PFC_PIN_GROUP(drif1_ctrl_b),
+	SH_PFC_PIN_GROUP(drif1_data0_b),
+	SH_PFC_PIN_GROUP(drif1_data1_b),
+	SH_PFC_PIN_GROUP(drif1_ctrl_c),
+	SH_PFC_PIN_GROUP(drif1_data0_c),
+	SH_PFC_PIN_GROUP(drif1_data1_c),
+	SH_PFC_PIN_GROUP(drif2_ctrl_a),
+	SH_PFC_PIN_GROUP(drif2_data0_a),
+	SH_PFC_PIN_GROUP(drif2_data1_a),
+	SH_PFC_PIN_GROUP(drif2_ctrl_b),
+	SH_PFC_PIN_GROUP(drif2_data0_b),
+	SH_PFC_PIN_GROUP(drif2_data1_b),
+	SH_PFC_PIN_GROUP(drif3_ctrl_a),
+	SH_PFC_PIN_GROUP(drif3_data0_a),
+	SH_PFC_PIN_GROUP(drif3_data1_a),
+	SH_PFC_PIN_GROUP(drif3_ctrl_b),
+	SH_PFC_PIN_GROUP(drif3_data0_b),
+	SH_PFC_PIN_GROUP(drif3_data1_b),
 	SH_PFC_PIN_GROUP(hscif0_data),
 	SH_PFC_PIN_GROUP(hscif0_clk),
 	SH_PFC_PIN_GROUP(hscif0_ctrl),
@@ -3631,6 +3876,48 @@ static const char * const canfd1_groups[] = {
 	"canfd1_data",
 };
 
+static const char * const drif0_groups[] = {
+	"drif0_ctrl_a",
+	"drif0_data0_a",
+	"drif0_data1_a",
+	"drif0_ctrl_b",
+	"drif0_data0_b",
+	"drif0_data1_b",
+	"drif0_ctrl_c",
+	"drif0_data0_c",
+	"drif0_data1_c",
+};
+
+static const char * const drif1_groups[] = {
+	"drif1_ctrl_a",
+	"drif1_data0_a",
+	"drif1_data1_a",
+	"drif1_ctrl_b",
+	"drif1_data0_b",
+	"drif1_data1_b",
+	"drif1_ctrl_c",
+	"drif1_data0_c",
+	"drif1_data1_c",
+};
+
+static const char * const drif2_groups[] = {
+	"drif2_ctrl_a",
+	"drif2_data0_a",
+	"drif2_data1_a",
+	"drif2_ctrl_b",
+	"drif2_data0_b",
+	"drif2_data1_b",
+};
+
+static const char * const drif3_groups[] = {
+	"drif3_ctrl_a",
+	"drif3_data0_a",
+	"drif3_data1_a",
+	"drif3_ctrl_b",
+	"drif3_data0_b",
+	"drif3_data1_b",
+};
+
 static const char * const hscif0_groups[] = {
 	"hscif0_data",
 	"hscif0_clk",
@@ -3974,6 +4261,10 @@ static const struct sh_pfc_function pinmux_functions[] = {
 	SH_PFC_FUNCTION(can_clk),
 	SH_PFC_FUNCTION(canfd0),
 	SH_PFC_FUNCTION(canfd1),
+	SH_PFC_FUNCTION(drif0),
+	SH_PFC_FUNCTION(drif1),
+	SH_PFC_FUNCTION(drif2),
+	SH_PFC_FUNCTION(drif3),
 	SH_PFC_FUNCTION(hscif0),
 	SH_PFC_FUNCTION(hscif1),
 	SH_PFC_FUNCTION(hscif2),
-- 
1.9.1

Re: [PATCH v2] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Geert Uytterhoeven]

On Thu, Jun 23, 2016 at 10:11 AM, Ramesh Shanmugasundaram
<ramesh.shanmugasundaram@bp.renesas.com> wrote:
> This patch adds DRIF[0-3] pinmux support for r8a7795 SoC.
>
> Signed-off-by: Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>
> ---
> Thanks Geert for the review.
>
> This patch is based on renesas-drivers repo (tag:renesas-drivers-2016-06-21-v4.7-rc4)
>
> Changes since v1:
>  - Split DRIF data pins as separate groups so that unused data pins can be
>    configured as GPIO (Geert)
>  - Fixed DRIF3(A) D1 pin definition typo (Geert)
>  - Rebased to latest tag renesas-drivers-2016-06-21-v4.7-rc4

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Queued in sh-pfc-for-v4.8.

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

Re: [PATCH] pinctrl: sh-pfc: r8a7795: Add DRIF support

[Linus Walleij]

On Tue, Jun 21, 2016 at 9:37 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> Linus/Laurent: I assume the GPIO also takes precedence in the Linux pinctrl
> subsystem, or would it return an error, as the pin is claimed by the IP group?

Complicated question. Depends on the .strict property is set on the pin
controller, see Documentation/pinctrl.txt

Yours,
Linus Walleij