[PATCH 0/2] pinctrl: sh-pfc: tidyup IPSR/MOD_SEL/GPSR settings order

[PATCH 0/2] pinctrl: sh-pfc: tidyup IPSR/MOD_SEL/GPSR settings order

[Kuninori Morimoto]

Hi Geert, Laurent

Gen2 / Gen3 next datasheet will has below note.
Our current PFC settings order is
	MOD_SEL -> GPSR -> IPSR
But, this fixes it to
	MOD_SEL -> IPSR -> GPSR

------------
IPSRx and MOD_SELx registers shall be set before setting GPSRx
registers in case that they need to be configured.
MOD_SELx registers can be set either earlier or later than setting
IPSRx registers.
------------

1) patch make it easy to check
2) patch fixes above

Kuninori Morimoto (2):
      1) pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug
      2) pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

 drivers/pinctrl/sh-pfc/core.c   |  3 ++-
 drivers/pinctrl/sh-pfc/sh_pfc.h | 11 +++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)



Best regards
---
Kuninori Morimoto

[PATCH 1/2] pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current sh_pfc can't indicate GPSR/IPSR/MOD_SEL name for debug.
Of course we can get it from indicated register address, but it
is not convenient. This patch enables to indicate these.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/pinctrl/sh-pfc/core.c   | 3 ++-
 drivers/pinctrl/sh-pfc/sh_pfc.h | 9 ++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 0c2d14c..c59f858 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -204,8 +204,9 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
 
 	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
 
-	dev_dbg(pfc->dev, "write_reg addr = %x, value = 0x%x, field = %u, "
+	dev_dbg(pfc->dev, "%s: write_reg addr = %x, value = 0x%x, field = %u, "
 		"r_width = %u, f_width = %u\n",
+		crp->name,
 		crp->reg, value, field, crp->reg_width, crp->field_width);
 
 	mask = ~(mask << pos);
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index a490834..2b593fc 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -94,6 +94,7 @@ struct pinmux_func {
 };
 
 struct pinmux_cfg_reg {
+	const char *name;
 	u32 reg;
 	u8 reg_width, field_width;
 	const u16 *enum_ids;
@@ -110,7 +111,8 @@ struct pinmux_cfg_reg {
  * (from left to right, i.e. MSB to LSB), 2^f_width enum IDs must be specified,
  * one for each possible combination of the register field bit values.
  */
-#define PINMUX_CFG_REG(name, r, r_width, f_width) \
+#define PINMUX_CFG_REG(_name, r, r_width, f_width) \
+	.name = _name, \
 	.reg = r, .reg_width = r_width, .field_width = f_width,		\
 	.enum_ids = (const u16 [(r_width / f_width) * (1 << f_width)])
 
@@ -125,7 +127,8 @@ struct pinmux_cfg_reg {
  * (from left to right, i.e. MSB to LSB), 2^var_fwi enum IDs must be specified,
  * one for each possible combination of the register field bit values.
  */
-#define PINMUX_CFG_REG_VAR(name, r, r_width, var_fw0, var_fwn...) \
+#define PINMUX_CFG_REG_VAR(_name, r, r_width, var_fw0, var_fwn...) \
+	.name = _name, \
 	.reg = r, .reg_width = r_width,	\
 	.var_field_width = (const u8 [r_width]) \
 		{ var_fw0, var_fwn, 0 }, \
@@ -465,7 +468,7 @@ struct sh_pfc_soc_info {
  */
 #define PORTCR(nr, reg)							\
 	{								\
-		PINMUX_CFG_REG_VAR("PORT" nr "CR", reg, 8, 2, 2, 1, 3) {\
+		PINMUX_CFG_REG_VAR("PORT" #nr "CR", reg, 8, 2, 2, 1, 3) {\
 			/* PULMD[1:0], handled by .set_bias() */	\
 			0, 0, 0, 0,					\
 			/* IE and OE */					\
-- 
1.9.1

Re: [PATCH 1/2] pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, Mar 16, 2016 at 1:47 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Current sh_pfc can't indicate GPSR/IPSR/MOD_SEL name for debug.
> Of course we can get it from indicated register address, but it
> is not convenient. This patch enables to indicate these.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -94,6 +94,7 @@ struct pinmux_func {
>  };
>
>  struct pinmux_cfg_reg {
> +       const char *name;

Have you looked at the size increase, esp. for multi-platform kernels?

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 1/2] pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug

[Kuninori Morimoto]

Hi Geert

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > Current sh_pfc can't indicate GPSR/IPSR/MOD_SEL name for debug.
> > Of course we can get it from indicated register address, but it
> > is not convenient. This patch enables to indicate these.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> > @@ -94,6 +94,7 @@ struct pinmux_func {
> >  };
> >
> >  struct pinmux_cfg_reg {
> > +       const char *name;
> 
> Have you looked at the size increase, esp. for multi-platform kernels?

Indeed.
I checked its size by "ls -l", and the result is below.
zImage size was... reduced ??

without this patch

	10661888  Image
	 3544032  zImage

with this patch

	10661888  Image
	 3543432  zImage

Re: [PATCH 1/2] pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, Mar 17, 2016 at 1:30 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
>> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> >
>> > Current sh_pfc can't indicate GPSR/IPSR/MOD_SEL name for debug.
>> > Of course we can get it from indicated register address, but it
>> > is not convenient. This patch enables to indicate these.
>> >
>> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>
>> > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
>> > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
>> > @@ -94,6 +94,7 @@ struct pinmux_func {
>> >  };
>> >
>> >  struct pinmux_cfg_reg {
>> > +       const char *name;
>>
>> Have you looked at the size increase, esp. for multi-platform kernels?
>
> Indeed.
> I checked its size by "ls -l", and the result is below.
> zImage size was... reduced ??
>
> without this patch
>
>         10661888  Image
>          3544032  zImage
>
> with this patch
>
>         10661888  Image
>          3543432  zImage

Please use "size drivers/pinctrl/sh-pfc/built-in.o".

zImage size is affected by section alignment and crompression.

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 1/2] pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Wednesday 16 March 2016 00:47:48 Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Current sh_pfc can't indicate GPSR/IPSR/MOD_SEL name for debug.
> Of course we can get it from indicated register address, but it
> is not convenient. This patch enables to indicate these.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/pinctrl/sh-pfc/core.c   | 3 ++-
>  drivers/pinctrl/sh-pfc/sh_pfc.h | 9 ++++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index 0c2d14c..c59f858 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -204,8 +204,9 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
> 
>  	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
> 
> -	dev_dbg(pfc->dev, "write_reg addr = %x, value = 0x%x, field = %u, "
> +	dev_dbg(pfc->dev, "%s: write_reg addr = %x, value = 0x%x, field = %u, "
>  		"r_width = %u, f_width = %u\n",
> +		crp->name,
>  		crp->reg, value, field, crp->reg_width, crp->field_width);
> 
>  	mask = ~(mask << pos);
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index a490834..2b593fc 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -94,6 +94,7 @@ struct pinmux_func {
>  };
> 
>  struct pinmux_cfg_reg {
> +	const char *name;

This will increase the kernel size, I would remove the name field when 
compiling the kernel in non-debug mode. You could use CONFIG_DYNAMIC_DEBUG or 
CONFIG_DEBUG_PINCTRL.

>  	u32 reg;
>  	u8 reg_width, field_width;
>  	const u16 *enum_ids;
> @@ -110,7 +111,8 @@ struct pinmux_cfg_reg {
>   * (from left to right, i.e. MSB to LSB), 2^f_width enum IDs must be
> specified, * one for each possible combination of the register field bit
> values. */
> -#define PINMUX_CFG_REG(name, r, r_width, f_width) \
> +#define PINMUX_CFG_REG(_name, r, r_width, f_width) \
> +	.name = _name, \
>  	.reg = r, .reg_width = r_width, .field_width = f_width,		\
>  	.enum_ids = (const u16 [(r_width / f_width) * (1 << f_width)])
> 
> @@ -125,7 +127,8 @@ struct pinmux_cfg_reg {
>   * (from left to right, i.e. MSB to LSB), 2^var_fwi enum IDs must be
> specified, * one for each possible combination of the register field bit
> values. */
> -#define PINMUX_CFG_REG_VAR(name, r, r_width, var_fw0, var_fwn...) \
> +#define PINMUX_CFG_REG_VAR(_name, r, r_width, var_fw0, var_fwn...) \
> +	.name = _name, \
>  	.reg = r, .reg_width = r_width,	\
>  	.var_field_width = (const u8 [r_width]) \
>  		{ var_fw0, var_fwn, 0 }, \
> @@ -465,7 +468,7 @@ struct sh_pfc_soc_info {
>   */
>  #define PORTCR(nr, reg)							\
>  	{								\
> -		PINMUX_CFG_REG_VAR("PORT" nr "CR", reg, 8, 2, 2, 1, 3) {\
> +		PINMUX_CFG_REG_VAR("PORT" #nr "CR", reg, 8, 2, 2, 1, 3) {\
>  			/* PULMD[1:0], handled by .set_bias() */	\
>  			0, 0, 0, 0,					\
>  			/* IE and OE */					\

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] pinctrl: sh-pfc: enable to indicate GPSR/IPSR/MOD_SEL for debug

[Kuninori Morimoto]

Hi Laurent, Geert

> >  struct pinmux_cfg_reg {
> > +	const char *name;
> 
> This will increase the kernel size, I would remove the name field when 
> compiling the kernel in non-debug mode. You could use CONFIG_DYNAMIC_DEBUG or 
> CONFIG_DEBUG_PINCTRL.

OK, [1/2] patch is not a big deal.
I'm happy if [2/2] was accepted.

[PATCH 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Gen2 / Gen3 datasheet will have below note in next version.
This patch follows this note.

IPSRx and MOD_SELx registers shall be set before setting GPSRx
registers in case that they need to be configured.
MOD_SELx registers can be set either earlier or later than setting
IPSRx registers.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/pinctrl/sh-pfc/sh_pfc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index 2b593fc..f174d61 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -279,7 +279,7 @@ struct sh_pfc_soc_info {
  *   - msel: Module selector
  */
 #define PINMUX_IPSR_MSEL(ipsr, fn, msel)				\
-	PINMUX_DATA(fn##_MARK, FN_##msel, FN_##ipsr, FN_##fn)
+	PINMUX_DATA(fn##_MARK, FN_##msel, FN_##fn, FN_##ipsr)
 
 /*
  * Describe a pinmux configuration for a single-function pin with GPIO
-- 
1.9.1

Re: [PATCH 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, Mar 16, 2016 at 1:48 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Gen2 / Gen3 datasheet will have below note in next version.
> This patch follows this note.
>
> IPSRx and MOD_SELx registers shall be set before setting GPSRx
> registers in case that they need to be configured.
> MOD_SELx registers can be set either earlier or later than setting
> IPSRx registers.

Does this note apply to R-Car Gen1 and SH7734, too?
Both use the PINMUX_IPSR_MSEL() macro.

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/pinctrl/sh-pfc/sh_pfc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
> index 2b593fc..f174d61 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -279,7 +279,7 @@ struct sh_pfc_soc_info {
>   *   - msel: Module selector
>   */
>  #define PINMUX_IPSR_MSEL(ipsr, fn, msel)                               \
> -       PINMUX_DATA(fn##_MARK, FN_##msel, FN_##ipsr, FN_##fn)
> +       PINMUX_DATA(fn##_MARK, FN_##msel, FN_##fn, FN_##ipsr)

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 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Kuninori Morimoto]

Hi Geert

> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > Gen2 / Gen3 datasheet will have below note in next version.
> > This patch follows this note.
> >
> > IPSRx and MOD_SELx registers shall be set before setting GPSRx
> > registers in case that they need to be configured.
> > MOD_SELx registers can be set either earlier or later than setting
> > IPSRx registers.
> 
> Does this note apply to R-Car Gen1 and SH7734, too?
> Both use the PINMUX_IPSR_MSEL() macro.

Yes, this note can be match for these.
But I'm not sure whether these datasheet will be updated.

Re: [PATCH 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Wednesday 16 March 2016 00:48:11 Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Gen2 / Gen3 datasheet will have below note in next version.
> This patch follows this note.
> 
> IPSRx and MOD_SELx registers shall be set before setting GPSRx
> registers in case that they need to be configured.
> MOD_SELx registers can be set either earlier or later than setting
> IPSRx registers.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/pinctrl/sh-pfc/sh_pfc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index 2b593fc..f174d61 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -279,7 +279,7 @@ struct sh_pfc_soc_info {
>   *   - msel: Module selector
>   */
>  #define PINMUX_IPSR_MSEL(ipsr, fn, msel)				\
> -	PINMUX_DATA(fn##_MARK, FN_##msel, FN_##ipsr, FN_##fn)
> +	PINMUX_DATA(fn##_MARK, FN_##msel, FN_##fn, FN_##ipsr)
> 
>  /*
>   * Describe a pinmux configuration for a single-function pin with GPIO

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Linus Walleij]

On Wed, Mar 16, 2016 at 1:48 AM, Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Gen2 / Gen3 datasheet will have below note in next version.
> This patch follows this note.
>
> IPSRx and MOD_SELx registers shall be set before setting GPSRx
> registers in case that they need to be configured.
> MOD_SELx registers can be set either earlier or later than setting
> IPSRx registers.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

I wait for Geert to either queue this for his first v4.7 pull request
or tell me to apply it for fixes. Is it a regression?

Yours,
Linus Walleij

Re: [PATCH 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Geert Uytterhoeven]

Hi Linus,

On Tue, Mar 22, 2016 at 2:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Mar 16, 2016 at 1:48 AM, Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
>
>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>
>> Gen2 / Gen3 datasheet will have below note in next version.
>> This patch follows this note.
>>
>> IPSRx and MOD_SELx registers shall be set before setting GPSRx
>> registers in case that they need to be configured.
>> MOD_SELx registers can be set either earlier or later than setting
>> IPSRx registers.
>>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> I wait for Geert to either queue this for his first v4.7 pull request
> or tell me to apply it for fixes. Is it a regression?

I'm not aware of any issues due to this.

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 2/2] pinctrl: sh-pfc: IPSRx and MOD_SELx should be set before GPSRx

[Linus Walleij]

On Tue, Mar 22, 2016 at 3:30 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Mar 22, 2016 at 2:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> I wait for Geert to either queue this for his first v4.7 pull request
>> or tell me to apply it for fixes. Is it a regression?
>
> I'm not aware of any issues due to this.

OK just queue it with the rest.

I assume there will still be lots of action in sh-pfc for this merge
window.

Yours,
Linus Walleij