[v2,00/13] Add PFC support for Renesas RZ/V2H(P) SoC

Message ID	20240423175900.702640-1-prabhakar.mahadev-lad.rj@bp.renesas.com
Headers	show Return-Path: <devicetree+bounces-61994-incoming-dt=patchwork.ozlabs.org@vger.kernel.org> From: Prabhakar <prabhakar.csengg@gmail.com> To: Geert Uytterhoeven <geert+renesas@glider.be>, Linus Walleij <linus.walleij@linaro.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, Magnus Damm <magnus.damm@gmail.com>, linux-renesas-soc@vger.kernel.org Cc: linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Prabhakar <prabhakar.csengg@gmail.com>, Biju Das <biju.das.jz@bp.renesas.com>, Fabrizio Castro <fabrizio.castro.jz@renesas.com>, Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Subject: [PATCH v2 00/13] Add PFC support for Renesas RZ/V2H(P) SoC Date: Tue, 23 Apr 2024 18:58:47 +0100 Message-Id: <20240423175900.702640-1-prabhakar.mahadev-lad.rj@bp.renesas.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	Add PFC support for Renesas RZ/V2H(P) SoC \| expand [v2,00/13] Add PFC support for Renesas RZ/V2H(P) SoC [v2,01/13] dt-bindings: pinctrl: renesas,rzg2l-pinctrl: Remove the check from the object [v2,02/13] dt-bindings: pinctrl: renesas: Document RZ/V2H(P) SoC

Lad, Prabhakar April 23, 2024, 5:58 p.m. UTC

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series aims to add PFC (Pin Function Controller) support for
Renesas RZ/V2H(P) SoC. The PFC block on RZ/V2H(P) is almost similar to
one found on the RZ/G2L family with couple of differences. To able to
re-use the use the existing driver for RZ/V2H(P) SoC function pointers
are introduced based on the SoC changes.


RFC->v2
- Fixed review comments pointed by Rob
- Incorporated changes suggested by Claudiu
- Fixed build error reported for m68K
- Dropped IOLH groups as we will be passing register values
- Fixed configs for dedicated pins
- Added support for slew-rate and bias settings
- Added support for OEN

RFC: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20240326222844.1422948-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (13):
  dt-bindings: pinctrl: renesas,rzg2l-pinctrl: Remove the check from the
    object
  dt-bindings: pinctrl: renesas: Document RZ/V2H(P) SoC
  pinctrl: renesas: pinctrl-rzg2l: Allow more bits for pin configuration
  pinctrl: renesas: pinctrl-rzg2l: Allow parsing of variable
    configuration for all architectures
  pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and
    ETH
  pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
    locking/unlocking the PFC register
  pinctrl: renesas: pinctrl-rzg2l: Add function pointer for writing to
    PMC register
  pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
    reading/writing OEN register
  pinctrl: renesas: pinctrl-rzg2l: Add support to configure the
    slew-rate
  pinctrl: renesas: pinctrl-rzg2l: Add support to set pulling up/down
    the pins
  pinctrl: renesas: pinctrl-rzg2l: Pass pincontrol device pointer to
    pinconf_generic_parse_dt_config()
  pinctrl: renesas: pinctrl-rzg2l: Add support for custom parameters
  pinctrl: renesas: pinctrl-rzg2l: Add support for RZ/V2H SoC

 .../pinctrl/renesas,rzg2l-pinctrl.yaml        |  40 +-
 drivers/pinctrl/renesas/pinctrl-rzg2l.c       | 640 ++++++++++++++++--
 2 files changed, 617 insertions(+), 63 deletions(-)

Biju Das April 23, 2024, 6:12 p.m. UTC | #1

Hi Prabhakar,

Thanks for the patch.

> -----Original Message-----
> From: Prabhakar <prabhakar.csengg@gmail.com>
> Sent: Tuesday, April 23, 2024 6:59 PM
> Subject: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
> locking/unlocking the PFC register
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On the RZ/G2L SoC, the PFCWE bit controls writing to PFC registers.
> However, on the RZ/V2H(P) SoC, the PFCWE (REGWE_A on RZ/V2H) bit controls writing to both PFC and
> PMC registers. Additionally, BIT(7) B0WI is undocumented for the PWPR register on RZ/V2H(P) SoC. To
> accommodate these differences across SoC variants, introduce the set_pfc_mode() and
> pm_set_pfc() function pointers.
> 
> Note, in rzg2l_pinctrl_set_pfc_mode() the pwpr_pfc_unlock() call is now called before PMC
> read/write and pwpr_pfc_lock() call is now called after PMC read/write this is to keep changes
> minimal for RZ/V2H(P).
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - Introduced function pointer for (un)lock
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 51 ++++++++++++++++---------
>  1 file changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index bec4685b4681..0840fda7ca69 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -246,6 +246,8 @@ struct rzg2l_variable_pin_cfg {
>  	u64 pin:3;
>  };
> 
> +struct rzg2l_pinctrl;
> +
>  struct rzg2l_pinctrl_data {
>  	const char * const *port_pins;
>  	const u64 *port_pin_configs;
> @@ -256,6 +258,8 @@ struct rzg2l_pinctrl_data {
>  	const struct rzg2l_hwcfg *hwcfg;
>  	const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
>  	unsigned int n_variable_pin_cfg;
> +	void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
> +	void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
>  };
> 
>  /**
> @@ -462,7 +466,6 @@ static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] =
> {  static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
>  				       u8 pin, u8 off, u8 func)
>  {
> -	const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
>  	unsigned long flags;
>  	u32 reg;
> 
> @@ -473,27 +476,23 @@ static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
>  	reg &= ~(PM_MASK << (pin * 2));
>  	writew(reg, pctrl->base + PM(off));
> 
> +	pctrl->data->pwpr_pfc_unlock(pctrl);
> +
>  	/* Temporarily switch to GPIO mode with PMC register */
>  	reg = readb(pctrl->base + PMC(off));
>  	writeb(reg & ~BIT(pin), pctrl->base + PMC(off));
> 
> -	/* Set the PWPR register to allow PFC register to write */
> -	writel(0x0, pctrl->base + regs->pwpr);		/* B0WI=0, PFCWE=0 */
> -	writel(PWPR_PFCWE, pctrl->base + regs->pwpr);	/* B0WI=0, PFCWE=1 */
> -
>  	/* Select Pin function mode with PFC register */
>  	reg = readl(pctrl->base + PFC(off));
>  	reg &= ~(PFC_MASK << (pin * 4));
>  	writel(reg | (func << (pin * 4)), pctrl->base + PFC(off));
> 
> -	/* Set the PWPR register to be write-protected */
> -	writel(0x0, pctrl->base + regs->pwpr);		/* B0WI=0, PFCWE=0 */
> -	writel(PWPR_B0WI, pctrl->base + regs->pwpr);	/* B0WI=1, PFCWE=0 */
> -
>  	/* Switch to Peripheral pin function with PMC register */
>  	reg = readb(pctrl->base + PMC(off));
>  	writeb(reg | BIT(pin), pctrl->base + PMC(off));
> 
> +	pctrl->data->pwpr_pfc_lock(pctrl);
> +
>  	spin_unlock_irqrestore(&pctrl->lock, flags);  };
> 
> @@ -2519,12 +2518,8 @@ static void rzg2l_pinctrl_pm_setup_dedicated_regs(struct rzg2l_pinctrl
> *pctrl, b  static void rzg2l_pinctrl_pm_setup_pfc(struct rzg2l_pinctrl *pctrl)  {
>  	u32 nports = pctrl->data->n_port_pins / RZG2L_PINS_PER_PORT;
> -	const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> -	const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> 
> -	/* Set the PWPR register to allow PFC register to write. */
> -	writel(0x0, pctrl->base + regs->pwpr);		/* B0WI=0, PFCWE=0 */
> -	writel(PWPR_PFCWE, pctrl->base + regs->pwpr);	/* B0WI=0, PFCWE=1 */
> +	pctrl->data->pwpr_pfc_unlock(pctrl);
> 
>  	/* Restore port registers. */
>  	for (u32 port = 0; port < nports; port++) { @@ -2567,9 +2562,7 @@ static void
> rzg2l_pinctrl_pm_setup_pfc(struct rzg2l_pinctrl *pctrl)
>  		}
>  	}
> 
> -	/* Set the PWPR register to be write-protected. */
> -	writel(0x0, pctrl->base + regs->pwpr);		/* B0WI=0, PFCWE=0 */
> -	writel(PWPR_B0WI, pctrl->base + regs->pwpr);	/* B0WI=1, PFCWE=0 */
> +	pctrl->data->pwpr_pfc_lock(pctrl);
>  }
> 
>  static int rzg2l_pinctrl_suspend_noirq(struct device *dev) @@ -2631,6 +2624,24 @@ static int
> rzg2l_pinctrl_resume_noirq(struct device *dev)
>  	return 0;
>  }
> 
> +static void rzg2l_pwpr_pfc_unlock(struct rzg2l_pinctrl *pctrl) {
> +	const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> +
> +	/* Set the PWPR register to allow PFC register to write */
> +	writel(0x0, pctrl->base + regs->pwpr);		/* B0WI=0, PFCWE=0 */
> +	writel(PWPR_PFCWE, pctrl->base + regs->pwpr);	/* B0WI=0, PFCWE=1 */
> +}
> +
> +static void rzg2l_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl) {
> +	const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> +
> +	/* Set the PWPR register to be write-protected */
> +	writel(0x0, pctrl->base + regs->pwpr);		/* B0WI=0, PFCWE=0 */
> +	writel(PWPR_B0WI, pctrl->base + regs->pwpr);	/* B0WI=1, PFCWE=0 */
> +}
> +
>  static const struct rzg2l_hwcfg rzg2l_hwcfg = {
>  	.regs = {
>  		.pwpr = 0x3014,
> @@ -2688,6 +2699,8 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
>  	.variable_pin_cfg = r9a07g043f_variable_pin_cfg,
>  	.n_variable_pin_cfg = ARRAY_SIZE(r9a07g043f_variable_pin_cfg),
>  #endif
> +	.pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> +	.pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
>  };
> 
>  static struct rzg2l_pinctrl_data r9a07g044_data = { @@ -2699,6 +2712,8 @@ static struct
> rzg2l_pinctrl_data r9a07g044_data = {
>  	.n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +
>  		ARRAY_SIZE(rzg2l_dedicated_pins.rzg2l_pins),
>  	.hwcfg = &rzg2l_hwcfg,
> +	.pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> +	.pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
>  };
> 
>  static struct rzg2l_pinctrl_data r9a08g045_data = { @@ -2709,6 +2724,8 @@ static struct
> rzg2l_pinctrl_data r9a08g045_data = {
>  	.n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT,
>  	.n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins),
>  	.hwcfg = &rzg3s_hwcfg,
> +	.pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> +	.pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,

Some memory can be saved by avoiding duplication of data by using
a single pointer for structure containing function pointers??

struct rzg2l_pinctrl_fns {
	void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
	void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
}

Cheers,
Biju

>  };
> 
>  static const struct of_device_id rzg2l_pinctrl_of_table[] = {
> --
> 2.34.1

Lad, Prabhakar April 25, 2024, 11:40 a.m. UTC | #2

Hi Biju,

Thank you for the review.

On Tue, Apr 23, 2024 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch.
>
> > -----Original Message-----
> > From: Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: Tuesday, April 23, 2024 6:59 PM
> > Subject: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
> > locking/unlocking the PFC register
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the RZ/G2L SoC, the PFCWE bit controls writing to PFC registers.
> > However, on the RZ/V2H(P) SoC, the PFCWE (REGWE_A on RZ/V2H) bit controls writing to both PFC and
> > PMC registers. Additionally, BIT(7) B0WI is undocumented for the PWPR register on RZ/V2H(P) SoC. To
> > accommodate these differences across SoC variants, introduce the set_pfc_mode() and
> > pm_set_pfc() function pointers.
> >
> > Note, in rzg2l_pinctrl_set_pfc_mode() the pwpr_pfc_unlock() call is now called before PMC
> > read/write and pwpr_pfc_lock() call is now called after PMC read/write this is to keep changes
> > minimal for RZ/V2H(P).
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - Introduced function pointer for (un)lock
> > ---
> >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 51 ++++++++++++++++---------
> >  1 file changed, 34 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index bec4685b4681..0840fda7ca69 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -246,6 +246,8 @@ struct rzg2l_variable_pin_cfg {
> >       u64 pin:3;
> >  };
> >
> > +struct rzg2l_pinctrl;
> > +
> >  struct rzg2l_pinctrl_data {
> >       const char * const *port_pins;
> >       const u64 *port_pin_configs;
> > @@ -256,6 +258,8 @@ struct rzg2l_pinctrl_data {
> >       const struct rzg2l_hwcfg *hwcfg;
> >       const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
> >       unsigned int n_variable_pin_cfg;
> > +     void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
> > +     void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
> >  };
> >
> >  /**
> > @@ -462,7 +466,6 @@ static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] =
> > {  static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> >                                      u8 pin, u8 off, u8 func)
> >  {
> > -     const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> >       unsigned long flags;
> >       u32 reg;
> >
> > @@ -473,27 +476,23 @@ static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> >       reg &= ~(PM_MASK << (pin * 2));
> >       writew(reg, pctrl->base + PM(off));
> >
> > +     pctrl->data->pwpr_pfc_unlock(pctrl);
> > +
> >       /* Temporarily switch to GPIO mode with PMC register */
> >       reg = readb(pctrl->base + PMC(off));
> >       writeb(reg & ~BIT(pin), pctrl->base + PMC(off));
> >
> > -     /* Set the PWPR register to allow PFC register to write */
> > -     writel(0x0, pctrl->base + regs->pwpr);          /* B0WI=0, PFCWE=0 */
> > -     writel(PWPR_PFCWE, pctrl->base + regs->pwpr);   /* B0WI=0, PFCWE=1 */
> > -
> >       /* Select Pin function mode with PFC register */
> >       reg = readl(pctrl->base + PFC(off));
> >       reg &= ~(PFC_MASK << (pin * 4));
> >       writel(reg | (func << (pin * 4)), pctrl->base + PFC(off));
> >
> > -     /* Set the PWPR register to be write-protected */
> > -     writel(0x0, pctrl->base + regs->pwpr);          /* B0WI=0, PFCWE=0 */
> > -     writel(PWPR_B0WI, pctrl->base + regs->pwpr);    /* B0WI=1, PFCWE=0 */
> > -
> >       /* Switch to Peripheral pin function with PMC register */
> >       reg = readb(pctrl->base + PMC(off));
> >       writeb(reg | BIT(pin), pctrl->base + PMC(off));
> >
> > +     pctrl->data->pwpr_pfc_lock(pctrl);
> > +
> >       spin_unlock_irqrestore(&pctrl->lock, flags);  };
> >
> > @@ -2519,12 +2518,8 @@ static void rzg2l_pinctrl_pm_setup_dedicated_regs(struct rzg2l_pinctrl
> > *pctrl, b  static void rzg2l_pinctrl_pm_setup_pfc(struct rzg2l_pinctrl *pctrl)  {
> >       u32 nports = pctrl->data->n_port_pins / RZG2L_PINS_PER_PORT;
> > -     const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> > -     const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> >
> > -     /* Set the PWPR register to allow PFC register to write. */
> > -     writel(0x0, pctrl->base + regs->pwpr);          /* B0WI=0, PFCWE=0 */
> > -     writel(PWPR_PFCWE, pctrl->base + regs->pwpr);   /* B0WI=0, PFCWE=1 */
> > +     pctrl->data->pwpr_pfc_unlock(pctrl);
> >
> >       /* Restore port registers. */
> >       for (u32 port = 0; port < nports; port++) { @@ -2567,9 +2562,7 @@ static void
> > rzg2l_pinctrl_pm_setup_pfc(struct rzg2l_pinctrl *pctrl)
> >               }
> >       }
> >
> > -     /* Set the PWPR register to be write-protected. */
> > -     writel(0x0, pctrl->base + regs->pwpr);          /* B0WI=0, PFCWE=0 */
> > -     writel(PWPR_B0WI, pctrl->base + regs->pwpr);    /* B0WI=1, PFCWE=0 */
> > +     pctrl->data->pwpr_pfc_lock(pctrl);
> >  }
> >
> >  static int rzg2l_pinctrl_suspend_noirq(struct device *dev) @@ -2631,6 +2624,24 @@ static int
> > rzg2l_pinctrl_resume_noirq(struct device *dev)
> >       return 0;
> >  }
> >
> > +static void rzg2l_pwpr_pfc_unlock(struct rzg2l_pinctrl *pctrl) {
> > +     const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> > +
> > +     /* Set the PWPR register to allow PFC register to write */
> > +     writel(0x0, pctrl->base + regs->pwpr);          /* B0WI=0, PFCWE=0 */
> > +     writel(PWPR_PFCWE, pctrl->base + regs->pwpr);   /* B0WI=0, PFCWE=1 */
> > +}
> > +
> > +static void rzg2l_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl) {
> > +     const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> > +
> > +     /* Set the PWPR register to be write-protected */
> > +     writel(0x0, pctrl->base + regs->pwpr);          /* B0WI=0, PFCWE=0 */
> > +     writel(PWPR_B0WI, pctrl->base + regs->pwpr);    /* B0WI=1, PFCWE=0 */
> > +}
> > +
> >  static const struct rzg2l_hwcfg rzg2l_hwcfg = {
> >       .regs = {
> >               .pwpr = 0x3014,
> > @@ -2688,6 +2699,8 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
> >       .variable_pin_cfg = r9a07g043f_variable_pin_cfg,
> >       .n_variable_pin_cfg = ARRAY_SIZE(r9a07g043f_variable_pin_cfg),
> >  #endif
> > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> >  };
> >
> >  static struct rzg2l_pinctrl_data r9a07g044_data = { @@ -2699,6 +2712,8 @@ static struct
> > rzg2l_pinctrl_data r9a07g044_data = {
> >       .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +
> >               ARRAY_SIZE(rzg2l_dedicated_pins.rzg2l_pins),
> >       .hwcfg = &rzg2l_hwcfg,
> > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> >  };
> >
> >  static struct rzg2l_pinctrl_data r9a08g045_data = { @@ -2709,6 +2724,8 @@ static struct
> > rzg2l_pinctrl_data r9a08g045_data = {
> >       .n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT,
> >       .n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins),
> >       .hwcfg = &rzg3s_hwcfg,
> > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
>
> Some memory can be saved by avoiding duplication of data by using
> a single pointer for structure containing function pointers??
>
> struct rzg2l_pinctrl_fns {
>         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
>         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
> }
>
Ok makes sense, I will do that in the next version.

Cheers,
Prabhakar

Lad, Prabhakar May 16, 2024, 8:02 a.m. UTC | #3

On Tue, Apr 23, 2024 at 6:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Hi All,
>
> This patch series aims to add PFC (Pin Function Controller) support for
> Renesas RZ/V2H(P) SoC. The PFC block on RZ/V2H(P) is almost similar to
> one found on the RZ/G2L family with couple of differences. To able to
> re-use the use the existing driver for RZ/V2H(P) SoC function pointers
> are introduced based on the SoC changes.
>
>
> RFC->v2
> - Fixed review comments pointed by Rob
> - Incorporated changes suggested by Claudiu
> - Fixed build error reported for m68K
> - Dropped IOLH groups as we will be passing register values
> - Fixed configs for dedicated pins
> - Added support for slew-rate and bias settings
> - Added support for OEN
>
> RFC: https://patchwork.kernel.org/project/linux-renesas-soc/cover/20240326222844.1422948-1-prabhakar.mahadev-lad.rj@bp.renesas.com/
>
> Cheers,
> Prabhakar
>
> Lad Prabhakar (13):
>   dt-bindings: pinctrl: renesas,rzg2l-pinctrl: Remove the check from the
>     object
>   dt-bindings: pinctrl: renesas: Document RZ/V2H(P) SoC
>   pinctrl: renesas: pinctrl-rzg2l: Allow more bits for pin configuration
>   pinctrl: renesas: pinctrl-rzg2l: Allow parsing of variable
>     configuration for all architectures
>   pinctrl: renesas: pinctrl-rzg2l: Validate power registers for SD and
>     ETH
>   pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
>     locking/unlocking the PFC register
>   pinctrl: renesas: pinctrl-rzg2l: Add function pointer for writing to
>     PMC register
>   pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
>     reading/writing OEN register
>   pinctrl: renesas: pinctrl-rzg2l: Add support to configure the
>     slew-rate
>   pinctrl: renesas: pinctrl-rzg2l: Add support to set pulling up/down
>     the pins
>   pinctrl: renesas: pinctrl-rzg2l: Pass pincontrol device pointer to
>     pinconf_generic_parse_dt_config()
>   pinctrl: renesas: pinctrl-rzg2l: Add support for custom parameters
>   pinctrl: renesas: pinctrl-rzg2l: Add support for RZ/V2H SoC
>
Gentle ping.

Cheers,
Prabhakar

>  .../pinctrl/renesas,rzg2l-pinctrl.yaml        |  40 +-
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c       | 640 ++++++++++++++++--
>  2 files changed, 617 insertions(+), 63 deletions(-)
>
> --
> 2.34.1
>

Geert Uytterhoeven May 22, 2024, 10:19 a.m. UTC | #4

Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> The pin configuration bits have been growing for every new SoCs being
> added for the pinctrl-rzg2l driver which would mean updating the macros
> every time for each new configuration. To avoid this allocate additional
> bits for pin configuration by relocating the known fixed bits to the very
> end of the configuration.
>
> Also update the size of 'cfg' to 'u64' to allow more configuration bits in
> the 'struct rzg2l_variable_pin_cfg'.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - Merged the macros and rzg2l_variable_pin_cfg changes into single patch
> - Updated types for the config changes

Thanks for the update!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -78,9 +78,9 @@
>                                          PIN_CFG_FILNUM | \
>                                          PIN_CFG_FILCLKSEL)
>
> -#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(35, 28)
> -#define PIN_CFG_PIN_REG_MASK           GENMASK(27, 20)
> -#define PIN_CFG_MASK                   GENMASK(19, 0)
> +#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(62, 55)
> +#define PIN_CFG_PIN_REG_MASK           GENMASK_ULL(54, 47)
> +#define PIN_CFG_MASK                   GENMASK_ULL(46, 0)
>
>  /*
>   * m indicates the bitmap of supported pins, a is the register index

> @@ -241,9 +241,9 @@ struct rzg2l_dedicated_configs {
>   * @pin: port pin
>   */
>  struct rzg2l_variable_pin_cfg {
> -       u32 cfg:20;
> -       u32 port:5;
> -       u32 pin:3;
> +       u64 cfg:46;

47, to match PIN_CFG_MASK()?

> +       u64 port:5;
> +       u64 pin:3;
>  };

To avoid such mistakes, and to increase uniformity, I think it would
be good to get rid of this structure, and replace it by masks, to be
used with FIELD_GET() and FIELD_PREP_CONST().

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

Geert Uytterhoeven May 22, 2024, 10:21 a.m. UTC | #5

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Enable parsing of variable configuration for all architectures. This patch
> is in preparation for adding support for the RZ/V2H SoC, which utilizes the
> ARM64 architecture and features port pins with variable configuration.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - No change

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

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 11:53 a.m. UTC | #6

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On RZ/V2H(P) SoC, the power registers for SD and ETH do not exist,
> resulting in invalid register offsets. Ensure that the register offsets
> are valid before any read/write operations are performed. If the power
> registers are not available, both SD and ETH will be set to '0'.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - Update check to != 0 instead of -EINVAL

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

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 12:05 p.m. UTC | #7

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> On the RZ/G2L SoC, the PFCWE bit controls writing to PFC registers.
> However, on the RZ/V2H(P) SoC, the PFCWE (REGWE_A on RZ/V2H) bit controls
> writing to both PFC and PMC registers. Additionally, BIT(7) B0WI is
> undocumented for the PWPR register on RZ/V2H(P) SoC. To accommodate these
> differences across SoC variants, introduce the set_pfc_mode() and
> pm_set_pfc() function pointers.
>
> Note, in rzg2l_pinctrl_set_pfc_mode() the pwpr_pfc_unlock() call is now
> called before PMC read/write and pwpr_pfc_lock() call is now called after
> PMC read/write this is to keep changes minimal for RZ/V2H(P).
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - Introduced function pointer for (un)lock

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

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

Geert Uytterhoeven May 22, 2024, 12:23 p.m. UTC | #8

Hi Biju,

On Tue, Apr 23, 2024 at 8:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > From: Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: Tuesday, April 23, 2024 6:59 PM
> > Subject: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
> > locking/unlocking the PFC register
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > On the RZ/G2L SoC, the PFCWE bit controls writing to PFC registers.
> > However, on the RZ/V2H(P) SoC, the PFCWE (REGWE_A on RZ/V2H) bit controls writing to both PFC and
> > PMC registers. Additionally, BIT(7) B0WI is undocumented for the PWPR register on RZ/V2H(P) SoC. To
> > accommodate these differences across SoC variants, introduce the set_pfc_mode() and
> > pm_set_pfc() function pointers.
> >
> > Note, in rzg2l_pinctrl_set_pfc_mode() the pwpr_pfc_unlock() call is now called before PMC
> > read/write and pwpr_pfc_lock() call is now called after PMC read/write this is to keep changes
> > minimal for RZ/V2H(P).
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - Introduced function pointer for (un)lock

> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -2688,6 +2699,8 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
> >       .variable_pin_cfg = r9a07g043f_variable_pin_cfg,
> >       .n_variable_pin_cfg = ARRAY_SIZE(r9a07g043f_variable_pin_cfg),
> >  #endif
> > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> >  };
> >
> >  static struct rzg2l_pinctrl_data r9a07g044_data = { @@ -2699,6 +2712,8 @@ static struct
> > rzg2l_pinctrl_data r9a07g044_data = {
> >       .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +
> >               ARRAY_SIZE(rzg2l_dedicated_pins.rzg2l_pins),
> >       .hwcfg = &rzg2l_hwcfg,
> > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> >  };
> >
> >  static struct rzg2l_pinctrl_data r9a08g045_data = { @@ -2709,6 +2724,8 @@ static struct
> > rzg2l_pinctrl_data r9a08g045_data = {
> >       .n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT,
> >       .n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins),
> >       .hwcfg = &rzg3s_hwcfg,
> > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
>
> Some memory can be saved by avoiding duplication of data by using
> a single pointer for structure containing function pointers??
>
> struct rzg2l_pinctrl_fns {
>         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
>         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
> }

So that would replace 3 (4 after adding RZ/V2H support) x 2 pointers in
rzg2l_pinctrl_data structures by 3 (4) pointers in rzg2l_pinctrl_data
structures + 1 (2) x 2 pointers in rzg2l_pinctrl_fns structures, and
code size would increase due to extra pointer dereferences before
each call.
Am I missing something?

Merging rzg2l_pwpr_pfc_{,un}lock() into a single function (taking a
"bool lock" flag) might be a better solution to reduce rzg2l_pinctrl_data size.

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 12:39 p.m. UTC | #9

Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> This patch introduces a function pointer, pmc_writeb(), in the
> struct rzg2l_pinctrl_data to facilitate writing to the PMC register. On
> the RZ/V2H(P) SoC, unlocking the PWPR.REGWE_A bit before writing to PMC
> registers is required, whereas this is not the case for the existing
> RZ/G2L family. This addition enables the reuse of existing code for
> RZ/V2H(P). Additionally, this patch populates this function pointer with
> appropriate data for existing SoCs.
>
> Note that this functionality is only handled in rzg2l_gpio_request(), as
> PMC unlock/lock during PFC setup will be taken care of in the
> pwpr_pfc_unlock/pwpr_pfc_lock.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - No change

Thanks for the update!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -463,6 +464,11 @@ static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] = {
>  };
>  #endif
>
> +static void rzg2l_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr)

Please pass the register offset instead of the virtual register address.
You do have pctrl->base here, and rzv2h_pmc_writeb() will need to use
pctrl->base for all other register writes anyway.

> +{
> +       writeb(val, addr);
> +}

Gr{oetje,eeting}s,

                        Geert

Biju Das May 22, 2024, 12:40 p.m. UTC | #10

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Wednesday, May 22, 2024 1:23 PM
> Subject: Re: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
> locking/unlocking the PFC register
> 
> Hi Biju,
> 
> On Tue, Apr 23, 2024 at 8:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > -----Original Message-----
> > > From: Prabhakar <prabhakar.csengg@gmail.com>
> > > Sent: Tuesday, April 23, 2024 6:59 PM
> > > Subject: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add
> > > function pointers for locking/unlocking the PFC register
> > >
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > On the RZ/G2L SoC, the PFCWE bit controls writing to PFC registers.
> > > However, on the RZ/V2H(P) SoC, the PFCWE (REGWE_A on RZ/V2H) bit
> > > controls writing to both PFC and PMC registers. Additionally, BIT(7)
> > > B0WI is undocumented for the PWPR register on RZ/V2H(P) SoC. To
> > > accommodate these differences across SoC variants, introduce the
> > > set_pfc_mode() and
> > > pm_set_pfc() function pointers.
> > >
> > > Note, in rzg2l_pinctrl_set_pfc_mode() the pwpr_pfc_unlock() call is
> > > now called before PMC read/write and pwpr_pfc_lock() call is now
> > > called after PMC read/write this is to keep changes minimal for RZ/V2H(P).
> > >
> > > Signed-off-by: Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > RFC->v2
> > > - Introduced function pointer for (un)lock
> 
> > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > @@ -2688,6 +2699,8 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
> > >       .variable_pin_cfg = r9a07g043f_variable_pin_cfg,
> > >       .n_variable_pin_cfg = ARRAY_SIZE(r9a07g043f_variable_pin_cfg),
> > >  #endif
> > > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> > >  };
> > >
> > >  static struct rzg2l_pinctrl_data r9a07g044_data = { @@ -2699,6
> > > +2712,8 @@ static struct rzg2l_pinctrl_data r9a07g044_data = {
> > >       .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +
> > >               ARRAY_SIZE(rzg2l_dedicated_pins.rzg2l_pins),
> > >       .hwcfg = &rzg2l_hwcfg,
> > > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> > >  };
> > >
> > >  static struct rzg2l_pinctrl_data r9a08g045_data = { @@ -2709,6
> > > +2724,8 @@ static struct rzg2l_pinctrl_data r9a08g045_data = {
> > >       .n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT,
> > >       .n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins),
> > >       .hwcfg = &rzg3s_hwcfg,
> > > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> >
> > Some memory can be saved by avoiding duplication of data by using a
> > single pointer for structure containing function pointers??
> >
> > struct rzg2l_pinctrl_fns {
> >         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
> >         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl); }
> 
> So that would replace 3 (4 after adding RZ/V2H support) x 2 pointers in rzg2l_pinctrl_data
> structures by 3 (4) pointers in rzg2l_pinctrl_data structures + 1 (2) x 2 pointers in
> rzg2l_pinctrl_fns structures, and code size would increase due to extra pointer dereferences before
> each call.
> Am I missing something?

Current case
3 * 2 pointers = 6 pointers

Suggestion
3 * 1 pointer + 1 * 2 pointer = 5 pointers

As you said,  code size would increase due to extra pointer dereferences before
each call.


> 
> Merging rzg2l_pwpr_pfc_{,un}lock() into a single function (taking a "bool lock" flag) might be a
> better solution to reduce rzg2l_pinctrl_data size.

I agree.

Cheers,
Biju

Geert Uytterhoeven May 22, 2024, 12:44 p.m. UTC | #11

Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> This patch introduces function pointers, read_oen() and write_oen(), in the
> struct rzg2l_pinctrl_data to facilitate reading and writing to the PFC_OEN
> register. On the RZ/V2H(P) SoC, unlocking the PWPR.REGWE_B bit before
> writing to the PFC_OEN register is necessary, and the PFC_OEN register has
> more bits compared to the RZ/G2L family. To handle these differences
> between RZ/G2L and RZ/V2H(P) and to reuse the existing code for RZ/V2H(P),
> these function pointers are introduced.
>
> Additionally, this patch populates these function pointers with appropriate
> data for existing SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - No change

Thanks for the update!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -261,6 +261,8 @@ struct rzg2l_pinctrl_data {
>         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
>         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
>         void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
> +       u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
> +       int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);

Please use consistent naming: "pmc_writeb" uses <noun>_<verb> ordering,
"read_oen" uses <verb>_<noun> ordering.

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 12:51 p.m. UTC | #12

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add support to configure slew-rate property of the pin.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - New patch

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

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 1:14 p.m. UTC | #13

Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add support to configure bias-disable, bias-pull-up and bias-pull-down
> properties of the pin.
>
> Two new function pointers get_bias_param() and get_bias_val() are
> introduced as the values in PUPD register differ when compared to
> RZ/G2L family and RZ/V2H(P) SoC,
>
> Value | RZ/G2L        | RZ/V2H
> ---------------------------------
> 00b:  | Bias Disabled | Pull up/down disabled
> 01b:  | Pull-up       | Pull up/down disabled
> 10b:  | Pull-down     | Pull-down
> 11b:  | Prohibited    | Pull-up
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - New patch

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -1139,6 +1175,25 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
>                 arg = rzg2l_read_pin_config(pctrl, SR(off), bit, SR_MASK);
>                 break;
>
> +       case PIN_CONFIG_BIAS_DISABLE:
> +       case PIN_CONFIG_BIAS_PULL_UP:
> +       case PIN_CONFIG_BIAS_PULL_DOWN: {
> +               if (!(cfg & PIN_CFG_PUPD))
> +                       return -EINVAL;
> +
> +               ret = pctrl->data->get_bias_param(rzg2l_read_pin_config(pctrl,
> +                                                                       PUPD(off),
> +                                                                       bit, PUPD_MASK));

This is rather long, so please split it in two parts, like is done in
other cases in this function:

    arg = rzg2l_read_pin_config(pctrl, PUPD(off), bit, PUPD_MASK);
    ret = pctrl->data->get_bias_param(arg);

> +               if (ret < 0)
> +                       return ret;
> +
> +               if (ret != param)
> +                       return -EINVAL;
> +               /* for PIN_CONFIG_BIAS_PULL_UP/DOWN when enabled we just return 1 */
> +               arg = 1;
> +               break;
> +       }
> +

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 1:17 p.m. UTC | #14

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Pass pincontrol device pointer to pinconf_generic_parse_dt_config() in
> prepration for passing custom params.

preparation

>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - No change

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

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 1:21 p.m. UTC | #15

Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> In preparation for passing custom params for RZ/V2H(P) SoC assign the
> custom params that is being passed via struct rzg2l_pinctrl_data.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - No change

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -262,6 +262,9 @@ struct rzg2l_pinctrl_data {
>         const struct rzg2l_hwcfg *hwcfg;
>         const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
>         unsigned int n_variable_pin_cfg;
> +       unsigned int num_custom_params;
> +       const struct pinconf_generic_params *custom_params;
> +       const struct pin_config_item *custom_conf_items;

Perhaps this should be protected by #ifdef CONFIG_DEBUG_FS, too?

>         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
>         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
>         void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
> @@ -2374,6 +2377,13 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
>         pctrl->desc.pmxops = &rzg2l_pinctrl_pmxops;
>         pctrl->desc.confops = &rzg2l_pinctrl_confops;
>         pctrl->desc.owner = THIS_MODULE;
> +       if (pctrl->data->num_custom_params) {
> +               pctrl->desc.num_custom_params = pctrl->data->num_custom_params;
> +               pctrl->desc.custom_params = pctrl->data->custom_params;
> +#ifdef CONFIG_DEBUG_FS
> +               pctrl->desc.custom_conf_items = pctrl->data->custom_conf_items;
> +#endif
> +       }
>
>         pins = devm_kcalloc(pctrl->dev, pctrl->desc.npins, sizeof(*pins), GFP_KERNEL);
>         if (!pins)

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 1:26 p.m. UTC | #16

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add support to configure bias-disable, bias-pull-up and bias-pull-down
> properties of the pin.
>
> Two new function pointers get_bias_param() and get_bias_val() are
> introduced as the values in PUPD register differ when compared to
> RZ/G2L family and RZ/V2H(P) SoC,
>
> Value | RZ/G2L        | RZ/V2H
> ---------------------------------
> 00b:  | Bias Disabled | Pull up/down disabled
> 01b:  | Pull-up       | Pull up/down disabled
> 10b:  | Pull-down     | Pull-down
> 11b:  | Prohibited    | Pull-up
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - New patch
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 73 +++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 102fa75c71d3..c144bf43522b 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -122,6 +122,7 @@
>  #define IOLH(off)              (0x1000 + (off) * 8)
>  #define SR(off)                        (0x1400 + (off) * 8)
>  #define IEN(off)               (0x1800 + (off) * 8)
> +#define PUPD(off)              (0x1C00 + (off) * 8)
>  #define ISEL(off)              (0x2C00 + (off) * 8)
>  #define SD_CH(off, ch)         ((off) + (ch) * 4)
>  #define ETH_POC(off, ch)       ((off) + (ch) * 4)
> @@ -140,6 +141,7 @@
>  #define IEN_MASK               0x01
>  #define IOLH_MASK              0x03
>  #define SR_MASK                        0x01
> +#define PUPD_MASK              0x03
>
>  #define PM_INPUT               0x1
>  #define PM_OUTPUT              0x2
> @@ -265,6 +267,8 @@ struct rzg2l_pinctrl_data {
>         void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
>         u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
>         int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
> +       int (*get_bias_param)(u8 val);
> +       int (*get_bias_val)(enum pin_config_param param);

Please use consistent naming: "pmc_writeb" uses <noun>_<verb> ordering,
"get_bias_pararm" uses <verb>_<noun> ordering.

Perhaps "hw_to_bias_param()" and "bias_param_to_hw()?"

Gr{oetje,eeting}s,

                        Geert

Geert Uytterhoeven May 22, 2024, 3:29 p.m. UTC | #17

Hi Prabhakar,

On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add pinctrl driver support for RZ/V2H(P) SoC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - Renamed renesas-rzv2h,output-impedance -> renesas,output-impedance
> - Dropped IOLH groups
> - Fixed dedicated pin configs
> - Updated r9a09g057_variable_pin_cfg
> - Added support OEN
> - Added support for bias settings
> - Added function pointers for pwpr (un)lock
> - Added support for slew-rate

Thanks for the update!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -59,6 +59,10 @@
>  #define PIN_CFG_OEN                    BIT(15)
>  #define PIN_CFG_VARIABLE               BIT(16)
>  #define PIN_CFG_NOGPIO_INT             BIT(17)
> +#define PIN_CFG_OPEN_DRAIN             BIT(18)

Or PIN_CFG_NOD, to match the docs?
You can always add a comment if the meaning is unclear:
/* N-ch Open Drain */

> +#define PIN_CFG_SCHMIT_CTRL            BIT(19)

SCHMITT (double T). Or just call it PIN_CFG_SMT, to match the docs?
/* Schmitt-trigger input control */

> +#define PIN_CFG_ELC                    BIT(20)

/* Event Link Control */

> +#define PIN_CFG_IOLH_RZV2H             BIT(21)
>
>  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
>                                         (PIN_CFG_IOLH_##group | \
> @@ -73,6 +77,10 @@
>  #define RZG3S_MPXED_PIN_FUNCS(group)   (RZG2L_MPXED_COMMON_PIN_FUNCS(group) | \
>                                          PIN_CFG_SOFT_PS)
>
> +#define RZV2H_MPXED_PIN_FUNCS          (RZG2L_MPXED_COMMON_PIN_FUNCS(RZV2H) | \
> +                                        PIN_CFG_OPEN_DRAIN | \
> +                                        PIN_CFG_SR)

I think you can include PIN_CFG_SCHMIT_CTRL here, and thus drop it
from all tables below?

> +
>  #define RZG2L_MPXED_ETH_PIN_FUNCS(x)   ((x) | \
>                                          PIN_CFG_FILONOFF | \
>                                          PIN_CFG_FILNUM | \
> @@ -128,13 +136,15 @@
>  #define ETH_POC(off, ch)       ((off) + (ch) * 4)
>  #define QSPI                   (0x3008)
>  #define ETH_MODE               (0x3018)
> +#define PFC_OEN                        (0x3C40) /* known on RZ/V2H(P) only */
>
>  #define PVDD_2500              2       /* I/O domain voltage 2.5V */
>  #define PVDD_1800              1       /* I/O domain voltage <= 1.8V */
>  #define PVDD_3300              0       /* I/O domain voltage >= 3.3V */
>
>  #define PWPR_B0WI              BIT(7)  /* Bit Write Disable */

FWIW, this should be PWPR_BOWI (O like in Oscar, not 0 = Zero).

> -#define PWPR_PFCWE             BIT(6)  /* PFC Register Write Enable */
> +#define              BIT(6)  /* PFC (and PMC on RZ/V2H) Register Write Enable */

As this bit is called differently on RZ/V2H, and there are different
code paths to handle PWPR on RZ/V2H vs. RZ/G2L, please add an extra
definition for PWPR_REGWE_A, and use that in RZ/V2H-specific
functions.

> +#define PWPR_REGWE_B           BIT(5)  /* OEN Register Write Enable, known only in RZ/V2H(P) */
>
>  #define PM_MASK                        0x03
>  #define PFC_MASK               0x07
                                   \
> @@ -330,6 +353,8 @@ struct rzg2l_pinctrl {
>         spinlock_t                      lock; /* lock read/write registers */
>         struct mutex                    mutex; /* serialize adding groups and functions */
>
> +       raw_spinlock_t                  pwpr_lock; /* serialize PWPR register access */

Do you need this lock?
I.e. can't you use the existing .lock above instead? (see below)

> +
>         struct rzg2l_pinctrl_pin_settings *settings;
>         struct rzg2l_pinctrl_reg_cache  *cache;
>         struct rzg2l_pinctrl_reg_cache  *dedicated_cache;

> @@ -480,6 +538,19 @@ static void rzg2l_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *
>         writeb(val, addr);
>  }
>
> +static void rzv2h_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr)
> +{
> +       const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> +       u8 pwpr;
> +
> +       raw_spin_lock(&pctrl->pwpr_lock);

rzg2l_pinctrl_data.pmc_write() is always called with rzg2l_pinctrl.lock
held.

> +       pwpr = readb(pctrl->base + regs->pwpr);
> +       writeb(pwpr | PWPR_PFCWE, pctrl->base + regs->pwpr);

PWPR_REGWE_A

> +       writeb(val, addr);
> +       writeb(pwpr & ~PWPR_PFCWE, pctrl->base + regs->pwpr);

PWPR_REGWE_A

> +       raw_spin_unlock(&pctrl->pwpr_lock);
> +}
> +
>  static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
>                                        u8 pin, u8 off, u8 func)
>  {

> +static u8 rzv2h_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, u32 offset)
> +{
> +       static const char * const pin_names[] = { "ET0_TXC_TXCLK", "ET1_TXC_TXCLK",
> +                                                 "XSPI0_RESET0N", "XSPI0_CS0N",
> +                                                 "XSPI0_CKN", "XSPI0_CKP" };

        static const char * const pin_names[] = {
                "ET0_TXC_TXCLK", "ET1_TXC_TXCLK", "XSPI0_RESET0N",
                "XSPI0_CS0N", "XSPI0_CKN", "XSPI0_CKP"
        };

> +       const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset];
> +       u8 bit_array[] = { 0, 1, 2, 3, 4, 5 };

Do you need this identity-transforming array? ;-)

> +       unsigned int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(bit_array); i++) {

ARRAY_SIZE(pin_names)

> +               if (!strcmp(pin_desc->name, pin_names[i]))
> +                       return bit_array[i];

return i;

> +       }
> +
> +       /* Should not happen. */
> +       return 0;
> +}
> +
> +static u32 rzv2h_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin)
> +{
> +       u8 bit;
> +
> +       if (!(caps & PIN_CFG_OEN))
> +               return 0;
> +
> +       bit = rzv2h_pin_to_oen_bit(pctrl, offset);
> +
> +       return !(readb(pctrl->base + PFC_OEN) & BIT(bit));
> +}
> +
> +static int rzv2h_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen)
> +{
> +       const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> +       const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> +       unsigned long flags;
> +       u8 val, bit;
> +       u8 pwpr;
> +
> +       if (!(caps & PIN_CFG_OEN))
> +               return -EINVAL;
> +
> +       bit = rzv2h_pin_to_oen_bit(pctrl, offset);
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       val = readb(pctrl->base + PFC_OEN);
> +       if (oen)
> +               val &= ~BIT(bit);
> +       else
> +               val |= BIT(bit);
> +
> +       raw_spin_lock(&pctrl->pwpr_lock);

rzg2l_pinctrl.lock is taken above.

> +       pwpr = readb(pctrl->base + regs->pwpr);
> +       writeb(pwpr | PWPR_REGWE_B, pctrl->base + regs->pwpr);
> +       writeb(val, pctrl->base + PFC_OEN);
> +       writeb(pwpr & ~PWPR_REGWE_B, pctrl->base + regs->pwpr);
> +       raw_spin_unlock(&pctrl->pwpr_lock);
> +       spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +       return 0;
> +}

> @@ -2747,6 +3098,32 @@ static void rzg2l_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl)
>         writel(PWPR_B0WI, pctrl->base + regs->pwpr);    /* B0WI=1, PFCWE=0 */
>  }
>
> +static void rzv2h_pwpr_pfc_unlock(struct rzg2l_pinctrl *pctrl)
> +{
> +       const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> +       u8 pwpr;
> +
> +       /*
> +        * lock is acquired in pfc unlock call back and then released in
> +        * pfc lock callback
> +        */
> +       raw_spin_lock(&pctrl->pwpr_lock);

Except for rzg2l_pinctrl_pm_setup_pfc(), this function is always
called while holding rzg2l_pinctrl.lock.  So I think you can just
take rzg2l_pinctrl.lock in rzg2l_pinctrl_pm_setup_pfc(), and get rid
of pwpr_lock?

> +       /* Set the PWPR register to allow PFC and PMC register to write */
> +       pwpr = readb(pctrl->base + regs->pwpr);
> +       writeb(PWPR_PFCWE | pwpr, pctrl->base + regs->pwpr);

PWPR_REGWE_A

> +}
> +
> +static void rzv2h_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl)
> +{
> +       const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> +       u8 pwpr;
> +
> +       /* Set the PWPR register to be write-protected */
> +       pwpr = readb(pctrl->base + regs->pwpr);
> +       writeb(pwpr & ~PWPR_PFCWE, pctrl->base + regs->pwpr);

PWPR_REGWE_A

> +       raw_spin_unlock(&pctrl->pwpr_lock);
> +}
> +
>  static const struct rzg2l_hwcfg rzg2l_hwcfg = {
>         .regs = {
>                 .pwpr = 0x3014,

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

Lad, Prabhakar May 28, 2024, 6:47 p.m. UTC | #18

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 11:19 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The pin configuration bits have been growing for every new SoCs being
> > added for the pinctrl-rzg2l driver which would mean updating the macros
> > every time for each new configuration. To avoid this allocate additional
> > bits for pin configuration by relocating the known fixed bits to the very
> > end of the configuration.
> >
> > Also update the size of 'cfg' to 'u64' to allow more configuration bits in
> > the 'struct rzg2l_variable_pin_cfg'.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - Merged the macros and rzg2l_variable_pin_cfg changes into single patch
> > - Updated types for the config changes
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -78,9 +78,9 @@
> >                                          PIN_CFG_FILNUM | \
> >                                          PIN_CFG_FILCLKSEL)
> >
> > -#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(35, 28)
> > -#define PIN_CFG_PIN_REG_MASK           GENMASK(27, 20)
> > -#define PIN_CFG_MASK                   GENMASK(19, 0)
> > +#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(62, 55)
> > +#define PIN_CFG_PIN_REG_MASK           GENMASK_ULL(54, 47)
> > +#define PIN_CFG_MASK                   GENMASK_ULL(46, 0)
> >
> >  /*
> >   * m indicates the bitmap of supported pins, a is the register index
>
> > @@ -241,9 +241,9 @@ struct rzg2l_dedicated_configs {
> >   * @pin: port pin
> >   */
> >  struct rzg2l_variable_pin_cfg {
> > -       u32 cfg:20;
> > -       u32 port:5;
> > -       u32 pin:3;
> > +       u64 cfg:46;
>
> 47, to match PIN_CFG_MASK()?
>
Oops, I missed that.

> > +       u64 port:5;
> > +       u64 pin:3;
> >  };
>
> To avoid such mistakes, and to increase uniformity, I think it would
> be good to get rid of this structure, and replace it by masks, to be
> used with FIELD_GET() and FIELD_PREP_CONST().
>
Agreed, I will make a patch on top of this patch (so that its easier
for review).

Cheers,
Prabhakar

Lad, Prabhakar May 28, 2024, 7:15 p.m. UTC | #19

Hi Biju, Geert,

Thank you for the review.

On Wed, May 22, 2024 at 1:40 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Geert,
>
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Wednesday, May 22, 2024 1:23 PM
> > Subject: Re: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add function pointers for
> > locking/unlocking the PFC register
> >
> > Hi Biju,
> >
> > On Tue, Apr 23, 2024 at 8:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > -----Original Message-----
> > > > From: Prabhakar <prabhakar.csengg@gmail.com>
> > > > Sent: Tuesday, April 23, 2024 6:59 PM
> > > > Subject: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add
> > > > function pointers for locking/unlocking the PFC register
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > On the RZ/G2L SoC, the PFCWE bit controls writing to PFC registers.
> > > > However, on the RZ/V2H(P) SoC, the PFCWE (REGWE_A on RZ/V2H) bit
> > > > controls writing to both PFC and PMC registers. Additionally, BIT(7)
> > > > B0WI is undocumented for the PWPR register on RZ/V2H(P) SoC. To
> > > > accommodate these differences across SoC variants, introduce the
> > > > set_pfc_mode() and
> > > > pm_set_pfc() function pointers.
> > > >
> > > > Note, in rzg2l_pinctrl_set_pfc_mode() the pwpr_pfc_unlock() call is
> > > > now called before PMC read/write and pwpr_pfc_lock() call is now
> > > > called after PMC read/write this is to keep changes minimal for RZ/V2H(P).
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > RFC->v2
> > > > - Introduced function pointer for (un)lock
> >
> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > @@ -2688,6 +2699,8 @@ static struct rzg2l_pinctrl_data r9a07g043_data = {
> > > >       .variable_pin_cfg = r9a07g043f_variable_pin_cfg,
> > > >       .n_variable_pin_cfg = ARRAY_SIZE(r9a07g043f_variable_pin_cfg),
> > > >  #endif
> > > > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > > > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> > > >  };
> > > >
> > > >  static struct rzg2l_pinctrl_data r9a07g044_data = { @@ -2699,6
> > > > +2712,8 @@ static struct rzg2l_pinctrl_data r9a07g044_data = {
> > > >       .n_dedicated_pins = ARRAY_SIZE(rzg2l_dedicated_pins.common) +
> > > >               ARRAY_SIZE(rzg2l_dedicated_pins.rzg2l_pins),
> > > >       .hwcfg = &rzg2l_hwcfg,
> > > > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > > > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> > > >  };
> > > >
> > > >  static struct rzg2l_pinctrl_data r9a08g045_data = { @@ -2709,6
> > > > +2724,8 @@ static struct rzg2l_pinctrl_data r9a08g045_data = {
> > > >       .n_port_pins = ARRAY_SIZE(r9a08g045_gpio_configs) * RZG2L_PINS_PER_PORT,
> > > >       .n_dedicated_pins = ARRAY_SIZE(rzg3s_dedicated_pins),
> > > >       .hwcfg = &rzg3s_hwcfg,
> > > > +     .pwpr_pfc_unlock = &rzg2l_pwpr_pfc_unlock,
> > > > +     .pwpr_pfc_lock = &rzg2l_pwpr_pfc_lock,
> > >
> > > Some memory can be saved by avoiding duplication of data by using a
> > > single pointer for structure containing function pointers??
> > >
> > > struct rzg2l_pinctrl_fns {
> > >         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
> > >         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl); }
> >
> > So that would replace 3 (4 after adding RZ/V2H support) x 2 pointers in rzg2l_pinctrl_data
> > structures by 3 (4) pointers in rzg2l_pinctrl_data structures + 1 (2) x 2 pointers in
> > rzg2l_pinctrl_fns structures, and code size would increase due to extra pointer dereferences before
> > each call.
> > Am I missing something?
>
> Current case
> 3 * 2 pointers = 6 pointers
>
> Suggestion
> 3 * 1 pointer + 1 * 2 pointer = 5 pointers
>
> As you said,  code size would increase due to extra pointer dereferences before
> each call.
>
>
> >
> > Merging rzg2l_pwpr_pfc_{,un}lock() into a single function (taking a "bool lock" flag) might be a
> > better solution to reduce rzg2l_pinctrl_data size.
>
> I agree.
>
OK, I will introduce a single function pointer (pwpr_pfc_lock_unlock)
in this patch.

Cheers,
Prabhakar

Lad, Prabhakar May 28, 2024, 7:33 p.m. UTC | #20

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 1:39 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > This patch introduces a function pointer, pmc_writeb(), in the
> > struct rzg2l_pinctrl_data to facilitate writing to the PMC register. On
> > the RZ/V2H(P) SoC, unlocking the PWPR.REGWE_A bit before writing to PMC
> > registers is required, whereas this is not the case for the existing
> > RZ/G2L family. This addition enables the reuse of existing code for
> > RZ/V2H(P). Additionally, this patch populates this function pointer with
> > appropriate data for existing SoCs.
> >
> > Note that this functionality is only handled in rzg2l_gpio_request(), as
> > PMC unlock/lock during PFC setup will be taken care of in the
> > pwpr_pfc_unlock/pwpr_pfc_lock.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - No change
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -463,6 +464,11 @@ static const struct rzg2l_variable_pin_cfg r9a07g043f_variable_pin_cfg[] = {
> >  };
> >  #endif
> >
> > +static void rzg2l_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr)
>
> Please pass the register offset instead of the virtual register address.
> You do have pctrl->base here, and rzv2h_pmc_writeb() will need to use
> pctrl->base for all other register writes anyway.
>
Agreed,  I will pass the register offset (u16 offset) instead of
virtual address.

Cheers,
Prabhakar

Lad, Prabhakar May 28, 2024, 7:42 p.m. UTC | #21

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 1:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > This patch introduces function pointers, read_oen() and write_oen(), in the
> > struct rzg2l_pinctrl_data to facilitate reading and writing to the PFC_OEN
> > register. On the RZ/V2H(P) SoC, unlocking the PWPR.REGWE_B bit before
> > writing to the PFC_OEN register is necessary, and the PFC_OEN register has
> > more bits compared to the RZ/G2L family. To handle these differences
> > between RZ/G2L and RZ/V2H(P) and to reuse the existing code for RZ/V2H(P),
> > these function pointers are introduced.
> >
> > Additionally, this patch populates these function pointers with appropriate
> > data for existing SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - No change
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -261,6 +261,8 @@ struct rzg2l_pinctrl_data {
> >         void (*pwpr_pfc_unlock)(struct rzg2l_pinctrl *pctrl);
> >         void (*pwpr_pfc_lock)(struct rzg2l_pinctrl *pctrl);
> >         void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
> > +       u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
> > +       int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
>
> Please use consistent naming: "pmc_writeb" uses <noun>_<verb> ordering,
> "read_oen" uses <verb>_<noun> ordering.
>
Ok, I'll rename them to oen_read() and oen_write().

Cheers,
Prabhakar

Lad, Prabhakar May 28, 2024, 8:01 p.m. UTC | #22

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 2:14 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support to configure bias-disable, bias-pull-up and bias-pull-down
> > properties of the pin.
> >
> > Two new function pointers get_bias_param() and get_bias_val() are
> > introduced as the values in PUPD register differ when compared to
> > RZ/G2L family and RZ/V2H(P) SoC,
> >
> > Value | RZ/G2L        | RZ/V2H
> > ---------------------------------
> > 00b:  | Bias Disabled | Pull up/down disabled
> > 01b:  | Pull-up       | Pull up/down disabled
> > 10b:  | Pull-down     | Pull-down
> > 11b:  | Prohibited    | Pull-up
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - New patch
>
> Thanks for your patch!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -1139,6 +1175,25 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >                 arg = rzg2l_read_pin_config(pctrl, SR(off), bit, SR_MASK);
> >                 break;
> >
> > +       case PIN_CONFIG_BIAS_DISABLE:
> > +       case PIN_CONFIG_BIAS_PULL_UP:
> > +       case PIN_CONFIG_BIAS_PULL_DOWN: {
> > +               if (!(cfg & PIN_CFG_PUPD))
> > +                       return -EINVAL;
> > +
> > +               ret = pctrl->data->get_bias_param(rzg2l_read_pin_config(pctrl,
> > +                                                                       PUPD(off),
> > +                                                                       bit, PUPD_MASK));
>
> This is rather long, so please split it in two parts, like is done in
> other cases in this function:
>
>     arg = rzg2l_read_pin_config(pctrl, PUPD(off), bit, PUPD_MASK);
>     ret = pctrl->data->get_bias_param(arg);
>
Agreed, I'll update it as per your suggestion.

Cheers,
Prabhakar

Lad, Prabhakar May 28, 2024, 8:01 p.m. UTC | #23

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support to configure bias-disable, bias-pull-up and bias-pull-down
> > properties of the pin.
> >
> > Two new function pointers get_bias_param() and get_bias_val() are
> > introduced as the values in PUPD register differ when compared to
> > RZ/G2L family and RZ/V2H(P) SoC,
> >
> > Value | RZ/G2L        | RZ/V2H
> > ---------------------------------
> > 00b:  | Bias Disabled | Pull up/down disabled
> > 01b:  | Pull-up       | Pull up/down disabled
> > 10b:  | Pull-down     | Pull-down
> > 11b:  | Prohibited    | Pull-up
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - New patch
> > ---
> >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 73 +++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index 102fa75c71d3..c144bf43522b 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -122,6 +122,7 @@
> >  #define IOLH(off)              (0x1000 + (off) * 8)
> >  #define SR(off)                        (0x1400 + (off) * 8)
> >  #define IEN(off)               (0x1800 + (off) * 8)
> > +#define PUPD(off)              (0x1C00 + (off) * 8)
> >  #define ISEL(off)              (0x2C00 + (off) * 8)
> >  #define SD_CH(off, ch)         ((off) + (ch) * 4)
> >  #define ETH_POC(off, ch)       ((off) + (ch) * 4)
> > @@ -140,6 +141,7 @@
> >  #define IEN_MASK               0x01
> >  #define IOLH_MASK              0x03
> >  #define SR_MASK                        0x01
> > +#define PUPD_MASK              0x03
> >
> >  #define PM_INPUT               0x1
> >  #define PM_OUTPUT              0x2
> > @@ -265,6 +267,8 @@ struct rzg2l_pinctrl_data {
> >         void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
> >         u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
> >         int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
> > +       int (*get_bias_param)(u8 val);
> > +       int (*get_bias_val)(enum pin_config_param param);
>
> Please use consistent naming: "pmc_writeb" uses <noun>_<verb> ordering,
> "get_bias_pararm" uses <verb>_<noun> ordering.
>
> Perhaps "hw_to_bias_param()" and "bias_param_to_hw()?"
>
Ok, I'll rename as suggested above.

Cheers,
Prabhakar

Lad, Prabhakar May 28, 2024, 8:07 p.m. UTC | #24

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 2:21 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > In preparation for passing custom params for RZ/V2H(P) SoC assign the
> > custom params that is being passed via struct rzg2l_pinctrl_data.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - No change
>
> Thanks for your patch!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -262,6 +262,9 @@ struct rzg2l_pinctrl_data {
> >         const struct rzg2l_hwcfg *hwcfg;
> >         const struct rzg2l_variable_pin_cfg *variable_pin_cfg;
> >         unsigned int n_variable_pin_cfg;
> > +       unsigned int num_custom_params;
> > +       const struct pinconf_generic_params *custom_params;
> > +       const struct pin_config_item *custom_conf_items;
>
> Perhaps this should be protected by #ifdef CONFIG_DEBUG_FS, too?
>
Agreed, I'll protect custom_conf_items by #ifdef CONFIG_DEBUG_FS.

Cheers,
Prabhakar

Lad, Prabhakar May 29, 2024, 8:32 p.m. UTC | #25

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 4:30 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add pinctrl driver support for RZ/V2H(P) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - Renamed renesas-rzv2h,output-impedance -> renesas,output-impedance
> > - Dropped IOLH groups
> > - Fixed dedicated pin configs
> > - Updated r9a09g057_variable_pin_cfg
> > - Added support OEN
> > - Added support for bias settings
> > - Added function pointers for pwpr (un)lock
> > - Added support for slew-rate
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -59,6 +59,10 @@
> >  #define PIN_CFG_OEN                    BIT(15)
> >  #define PIN_CFG_VARIABLE               BIT(16)
> >  #define PIN_CFG_NOGPIO_INT             BIT(17)
> > +#define PIN_CFG_OPEN_DRAIN             BIT(18)
>
> Or PIN_CFG_NOD, to match the docs?
> You can always add a comment if the meaning is unclear:
> /* N-ch Open Drain */
>
Agreed, I will update as above.

> > +#define PIN_CFG_SCHMIT_CTRL            BIT(19)
>
> SCHMITT (double T). Or just call it PIN_CFG_SMT, to match the docs?
> /* Schmitt-trigger input control */
>
Agreed, I will update as above.

> > +#define PIN_CFG_ELC                    BIT(20)
>
> /* Event Link Control */
>
> > +#define PIN_CFG_IOLH_RZV2H             BIT(21)
> >
> >  #define RZG2L_MPXED_COMMON_PIN_FUNCS(group) \
> >                                         (PIN_CFG_IOLH_##group | \
> > @@ -73,6 +77,10 @@
> >  #define RZG3S_MPXED_PIN_FUNCS(group)   (RZG2L_MPXED_COMMON_PIN_FUNCS(group) | \
> >                                          PIN_CFG_SOFT_PS)
> >
> > +#define RZV2H_MPXED_PIN_FUNCS          (RZG2L_MPXED_COMMON_PIN_FUNCS(RZV2H) | \
> > +                                        PIN_CFG_OPEN_DRAIN | \
> > +                                        PIN_CFG_SR)
>
> I think you can include PIN_CFG_SCHMIT_CTRL here, and thus drop it
> from all tables below?
>
Agreed.

> > +
> >  #define RZG2L_MPXED_ETH_PIN_FUNCS(x)   ((x) | \
> >                                          PIN_CFG_FILONOFF | \
> >                                          PIN_CFG_FILNUM | \
> > @@ -128,13 +136,15 @@
> >  #define ETH_POC(off, ch)       ((off) + (ch) * 4)
> >  #define QSPI                   (0x3008)
> >  #define ETH_MODE               (0x3018)
> > +#define PFC_OEN                        (0x3C40) /* known on RZ/V2H(P) only */
> >
> >  #define PVDD_2500              2       /* I/O domain voltage 2.5V */
> >  #define PVDD_1800              1       /* I/O domain voltage <= 1.8V */
> >  #define PVDD_3300              0       /* I/O domain voltage >= 3.3V */
> >
> >  #define PWPR_B0WI              BIT(7)  /* Bit Write Disable */
>
> FWIW, this should be PWPR_BOWI (O like in Oscar, not 0 = Zero).
>
Indeed, I'll make a seperate patch for this.

> > -#define PWPR_PFCWE             BIT(6)  /* PFC Register Write Enable */
> > +#define              BIT(6)  /* PFC (and PMC on RZ/V2H) Register Write Enable */
>
> As this bit is called differently on RZ/V2H, and there are different
> code paths to handle PWPR on RZ/V2H vs. RZ/G2L, please add an extra
> definition for PWPR_REGWE_A, and use that in RZ/V2H-specific
> functions.
>
Agreed, I will add PWPR_REGWE_A.

> > +#define PWPR_REGWE_B           BIT(5)  /* OEN Register Write Enable, known only in RZ/V2H(P) */
> >
> >  #define PM_MASK                        0x03
> >  #define PFC_MASK               0x07
>                                    \
> > @@ -330,6 +353,8 @@ struct rzg2l_pinctrl {
> >         spinlock_t                      lock; /* lock read/write registers */
> >         struct mutex                    mutex; /* serialize adding groups and functions */
> >
> > +       raw_spinlock_t                  pwpr_lock; /* serialize PWPR register access */
>
> Do you need this lock?
> I.e. can't you use the existing .lock above instead? (see below)
>
> > +
> >         struct rzg2l_pinctrl_pin_settings *settings;
> >         struct rzg2l_pinctrl_reg_cache  *cache;
> >         struct rzg2l_pinctrl_reg_cache  *dedicated_cache;
>
> > @@ -480,6 +538,19 @@ static void rzg2l_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *
> >         writeb(val, addr);
> >  }
> >
> > +static void rzv2h_pmc_writeb(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr)
> > +{
> > +       const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> > +       u8 pwpr;
> > +
> > +       raw_spin_lock(&pctrl->pwpr_lock);
>
> rzg2l_pinctrl_data.pmc_write() is always called with rzg2l_pinctrl.lock
> held.
>
> > +       pwpr = readb(pctrl->base + regs->pwpr);
> > +       writeb(pwpr | PWPR_PFCWE, pctrl->base + regs->pwpr);
>
> PWPR_REGWE_A
>
> > +       writeb(val, addr);
> > +       writeb(pwpr & ~PWPR_PFCWE, pctrl->base + regs->pwpr);
>
> PWPR_REGWE_A
>
> > +       raw_spin_unlock(&pctrl->pwpr_lock);
> > +}
> > +
> >  static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
> >                                        u8 pin, u8 off, u8 func)
> >  {
>
> > +static u8 rzv2h_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, u32 offset)
> > +{
> > +       static const char * const pin_names[] = { "ET0_TXC_TXCLK", "ET1_TXC_TXCLK",
> > +                                                 "XSPI0_RESET0N", "XSPI0_CS0N",
> > +                                                 "XSPI0_CKN", "XSPI0_CKP" };
>
>         static const char * const pin_names[] = {
>                 "ET0_TXC_TXCLK", "ET1_TXC_TXCLK", "XSPI0_RESET0N",
>                 "XSPI0_CS0N", "XSPI0_CKN", "XSPI0_CKP"
>         };
>
> > +       const struct pinctrl_pin_desc *pin_desc = &pctrl->desc.pins[offset];
> > +       u8 bit_array[] = { 0, 1, 2, 3, 4, 5 };
>
> Do you need this identity-transforming array? ;-)
>
Oops, it can be simplified.

> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(bit_array); i++) {
>
> ARRAY_SIZE(pin_names)
>
> > +               if (!strcmp(pin_desc->name, pin_names[i]))
> > +                       return bit_array[i];
>
> return i;
>
> > +       }
> > +
> > +       /* Should not happen. */
> > +       return 0;
> > +}
> > +
> > +static u32 rzv2h_read_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin)
> > +{
> > +       u8 bit;
> > +
> > +       if (!(caps & PIN_CFG_OEN))
> > +               return 0;
> > +
> > +       bit = rzv2h_pin_to_oen_bit(pctrl, offset);
> > +
> > +       return !(readb(pctrl->base + PFC_OEN) & BIT(bit));
> > +}
> > +
> > +static int rzv2h_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen)
> > +{
> > +       const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> > +       const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> > +       unsigned long flags;
> > +       u8 val, bit;
> > +       u8 pwpr;
> > +
> > +       if (!(caps & PIN_CFG_OEN))
> > +               return -EINVAL;
> > +
> > +       bit = rzv2h_pin_to_oen_bit(pctrl, offset);
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       val = readb(pctrl->base + PFC_OEN);
> > +       if (oen)
> > +               val &= ~BIT(bit);
> > +       else
> > +               val |= BIT(bit);
> > +
> > +       raw_spin_lock(&pctrl->pwpr_lock);
>
> rzg2l_pinctrl.lock is taken above.
>
> > +       pwpr = readb(pctrl->base + regs->pwpr);
> > +       writeb(pwpr | PWPR_REGWE_B, pctrl->base + regs->pwpr);
> > +       writeb(val, pctrl->base + PFC_OEN);
> > +       writeb(pwpr & ~PWPR_REGWE_B, pctrl->base + regs->pwpr);
> > +       raw_spin_unlock(&pctrl->pwpr_lock);
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > +       return 0;
> > +}
>
> > @@ -2747,6 +3098,32 @@ static void rzg2l_pwpr_pfc_lock(struct rzg2l_pinctrl *pctrl)
> >         writel(PWPR_B0WI, pctrl->base + regs->pwpr);    /* B0WI=1, PFCWE=0 */
> >  }
> >
> > +static void rzv2h_pwpr_pfc_unlock(struct rzg2l_pinctrl *pctrl)
> > +{
> > +       const struct rzg2l_register_offsets *regs = &pctrl->data->hwcfg->regs;
> > +       u8 pwpr;
> > +
> > +       /*
> > +        * lock is acquired in pfc unlock call back and then released in
> > +        * pfc lock callback
> > +        */
> > +       raw_spin_lock(&pctrl->pwpr_lock);
>
> Except for rzg2l_pinctrl_pm_setup_pfc(), this function is always
> called while holding rzg2l_pinctrl.lock.  So I think you can just
> take rzg2l_pinctrl.lock in rzg2l_pinctrl_pm_setup_pfc(), and get rid
> of pwpr_lock?
>
Agreed.

Cheers,
Prabhakar

Claudiu Beznea May 30, 2024, 7:48 a.m. UTC | #26

Hi, Prabhakar,

On 23.04.2024 20:58, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add support to configure bias-disable, bias-pull-up and bias-pull-down
> properties of the pin.
> 
> Two new function pointers get_bias_param() and get_bias_val() are
> introduced as the values in PUPD register differ when compared to
> RZ/G2L family and RZ/V2H(P) SoC,
> 
> Value | RZ/G2L        | RZ/V2H
> ---------------------------------
> 00b:  | Bias Disabled | Pull up/down disabled
> 01b:  | Pull-up       | Pull up/down disabled
> 10b:  | Pull-down     | Pull-down
> 11b:  | Prohibited    | Pull-up
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC->v2
> - New patch
> ---
>  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 73 +++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 102fa75c71d3..c144bf43522b 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -122,6 +122,7 @@
>  #define IOLH(off)		(0x1000 + (off) * 8)
>  #define SR(off)			(0x1400 + (off) * 8)
>  #define IEN(off)		(0x1800 + (off) * 8)
> +#define PUPD(off)		(0x1C00 + (off) * 8)
>  #define ISEL(off)		(0x2C00 + (off) * 8)
>  #define SD_CH(off, ch)		((off) + (ch) * 4)
>  #define ETH_POC(off, ch)	((off) + (ch) * 4)
> @@ -140,6 +141,7 @@
>  #define IEN_MASK		0x01
>  #define IOLH_MASK		0x03
>  #define SR_MASK			0x01
> +#define PUPD_MASK		0x03
>  
>  #define PM_INPUT		0x1
>  #define PM_OUTPUT		0x2
> @@ -265,6 +267,8 @@ struct rzg2l_pinctrl_data {
>  	void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
>  	u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
>  	int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
> +	int (*get_bias_param)(u8 val);
> +	int (*get_bias_val)(enum pin_config_param param);
>  };
>  
>  /**
> @@ -1081,6 +1085,38 @@ static int rzg2l_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8
>  	return 0;
>  }
>  
> +static int rzg2l_get_bias_param(u8 val)
> +{
> +	switch (val) {
> +	case 0:
> +		return PIN_CONFIG_BIAS_DISABLE;
> +	case 1:
> +		return PIN_CONFIG_BIAS_PULL_UP;
> +	case 2:
> +		return PIN_CONFIG_BIAS_PULL_DOWN;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int rzg2l_get_bias_val(enum pin_config_param param)
> +{
> +	switch (param) {
> +	case PIN_CONFIG_BIAS_DISABLE:
> +		return 0;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		return 1;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		return 2;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
>  				     unsigned int _pin,
>  				     unsigned long *config)
> @@ -1139,6 +1175,25 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
>  		arg = rzg2l_read_pin_config(pctrl, SR(off), bit, SR_MASK);
>  		break;
>  
> +	case PIN_CONFIG_BIAS_DISABLE:
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +	case PIN_CONFIG_BIAS_PULL_DOWN: {

Block { } can be removed here.

> +		if (!(cfg & PIN_CFG_PUPD))
> +			return -EINVAL;
> +
> +		ret = pctrl->data->get_bias_param(rzg2l_read_pin_config(pctrl,
> +									PUPD(off),
> +									bit, PUPD_MASK));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret != param)
> +			return -EINVAL;

Can this happen? Otherwise it can be removed.

> +		/* for PIN_CONFIG_BIAS_PULL_UP/DOWN when enabled we just return 1 */

What about bias disable? I haven't checked in detail, is it OK to do
arg = 1 here?

> +		arg = 1;
> +		break;
> +	}
> +
>  	case PIN_CONFIG_DRIVE_STRENGTH: {
>  		unsigned int index;
>  
> @@ -1254,6 +1309,20 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
>  			rzg2l_rmw_pin_config(pctrl, SR(off), bit, SR_MASK, arg);
>  			break;
>  
> +		case PIN_CONFIG_BIAS_DISABLE:
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +		case PIN_CONFIG_BIAS_PULL_DOWN: {

Block { } can be removed in this case.

> +			if (!(cfg & PIN_CFG_PUPD))
> +				return -EINVAL;
> +
> +			ret = pctrl->data->get_bias_val(param);
> +			if (ret < 0)
> +				return ret;
> +
> +			rzg2l_rmw_pin_config(pctrl, PUPD(off), bit, PUPD_MASK, ret);
> +			break;
> +		}
> +
>  		case PIN_CONFIG_DRIVE_STRENGTH:
>  			arg = pinconf_to_config_argument(_configs[i]);
>  
> @@ -2746,6 +2815,8 @@ static struct rzg2l_pinctrl_data r9a07g044_data = {
>  	.pmc_writeb = &rzg2l_pmc_writeb,
>  	.read_oen = &rzg2l_read_oen,
>  	.write_oen = &rzg2l_write_oen,
> +	.get_bias_param = &rzg2l_get_bias_param,
> +	.get_bias_val = &rzg2l_get_bias_val,
>  };
>  
>  static struct rzg2l_pinctrl_data r9a08g045_data = {
> @@ -2761,6 +2832,8 @@ static struct rzg2l_pinctrl_data r9a08g045_data = {
>  	.pmc_writeb = &rzg2l_pmc_writeb,
>  	.read_oen = &rzg2l_read_oen,
>  	.write_oen = &rzg2l_write_oen,
> +	.get_bias_param = &rzg2l_get_bias_param,
> +	.get_bias_val = &rzg2l_get_bias_val,
>  };
>  
>  static const struct of_device_id rzg2l_pinctrl_of_table[] = {

Lad, Prabhakar May 30, 2024, 10:15 a.m. UTC | #27

Hi Claudiu,

Thank you for the review.

On Thu, May 30, 2024 at 8:48 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Prabhakar,
>
> On 23.04.2024 20:58, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support to configure bias-disable, bias-pull-up and bias-pull-down
> > properties of the pin.
> >
> > Two new function pointers get_bias_param() and get_bias_val() are
> > introduced as the values in PUPD register differ when compared to
> > RZ/G2L family and RZ/V2H(P) SoC,
> >
> > Value | RZ/G2L        | RZ/V2H
> > ---------------------------------
> > 00b:  | Bias Disabled | Pull up/down disabled
> > 01b:  | Pull-up       | Pull up/down disabled
> > 10b:  | Pull-down     | Pull-down
> > 11b:  | Prohibited    | Pull-up
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - New patch
> > ---
> >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 73 +++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index 102fa75c71d3..c144bf43522b 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -122,6 +122,7 @@
> >  #define IOLH(off)            (0x1000 + (off) * 8)
> >  #define SR(off)                      (0x1400 + (off) * 8)
> >  #define IEN(off)             (0x1800 + (off) * 8)
> > +#define PUPD(off)            (0x1C00 + (off) * 8)
> >  #define ISEL(off)            (0x2C00 + (off) * 8)
> >  #define SD_CH(off, ch)               ((off) + (ch) * 4)
> >  #define ETH_POC(off, ch)     ((off) + (ch) * 4)
> > @@ -140,6 +141,7 @@
> >  #define IEN_MASK             0x01
> >  #define IOLH_MASK            0x03
> >  #define SR_MASK                      0x01
> > +#define PUPD_MASK            0x03
> >
> >  #define PM_INPUT             0x1
> >  #define PM_OUTPUT            0x2
> > @@ -265,6 +267,8 @@ struct rzg2l_pinctrl_data {
> >       void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
> >       u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
> >       int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
> > +     int (*get_bias_param)(u8 val);
> > +     int (*get_bias_val)(enum pin_config_param param);
> >  };
> >
> >  /**
> > @@ -1081,6 +1085,38 @@ static int rzg2l_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8
> >       return 0;
> >  }
> >
> > +static int rzg2l_get_bias_param(u8 val)
> > +{
> > +     switch (val) {
> > +     case 0:
> > +             return PIN_CONFIG_BIAS_DISABLE;
> > +     case 1:
> > +             return PIN_CONFIG_BIAS_PULL_UP;
> > +     case 2:
> > +             return PIN_CONFIG_BIAS_PULL_DOWN;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int rzg2l_get_bias_val(enum pin_config_param param)
> > +{
> > +     switch (param) {
> > +     case PIN_CONFIG_BIAS_DISABLE:
> > +             return 0;
> > +     case PIN_CONFIG_BIAS_PULL_UP:
> > +             return 1;
> > +     case PIN_CONFIG_BIAS_PULL_DOWN:
> > +             return 2;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> >  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >                                    unsigned int _pin,
> >                                    unsigned long *config)
> > @@ -1139,6 +1175,25 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >               arg = rzg2l_read_pin_config(pctrl, SR(off), bit, SR_MASK);
> >               break;
> >
> > +     case PIN_CONFIG_BIAS_DISABLE:
> > +     case PIN_CONFIG_BIAS_PULL_UP:
> > +     case PIN_CONFIG_BIAS_PULL_DOWN: {
>
> Block { } can be removed here.
>
Agreed, I will drop it.

> > +             if (!(cfg & PIN_CFG_PUPD))
> > +                     return -EINVAL;
> > +
> > +             ret = pctrl->data->get_bias_param(rzg2l_read_pin_config(pctrl,
> > +                                                                     PUPD(off),
> > +                                                                     bit, PUPD_MASK));
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             if (ret != param)
> > +                     return -EINVAL;
>
> Can this happen? Otherwise it can be removed.
>
> > +             /* for PIN_CONFIG_BIAS_PULL_UP/DOWN when enabled we just return 1 */
>
> What about bias disable? I haven't checked in detail, is it OK to do
> arg = 1 here?
>
For BIAS_DISABLE config there isn't any argument, hence the above
comment mentions only for UP/DOWN. Passing arg = 1 for BIAS_DISABLE
has no effect.

> > +             arg = 1;
> > +             break;
> > +     }
> > +
> >       case PIN_CONFIG_DRIVE_STRENGTH: {
> >               unsigned int index;
> >
> > @@ -1254,6 +1309,20 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> >                       rzg2l_rmw_pin_config(pctrl, SR(off), bit, SR_MASK, arg);
> >                       break;
> >
> > +             case PIN_CONFIG_BIAS_DISABLE:
> > +             case PIN_CONFIG_BIAS_PULL_UP:
> > +             case PIN_CONFIG_BIAS_PULL_DOWN: {
>
> Block { } can be removed in this case.
>
Agreed, I will drop it.

Cheers,
Prabhakar

Lad, Prabhakar May 30, 2024, 10:37 a.m. UTC | #28

On Thu, May 30, 2024 at 8:48 AM claudiu beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Prabhakar,
>
> On 23.04.2024 20:58, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add support to configure bias-disable, bias-pull-up and bias-pull-down
> > properties of the pin.
> >
> > Two new function pointers get_bias_param() and get_bias_val() are
> > introduced as the values in PUPD register differ when compared to
> > RZ/G2L family and RZ/V2H(P) SoC,
> >
> > Value | RZ/G2L        | RZ/V2H
> > ---------------------------------
> > 00b:  | Bias Disabled | Pull up/down disabled
> > 01b:  | Pull-up       | Pull up/down disabled
> > 10b:  | Pull-down     | Pull-down
> > 11b:  | Prohibited    | Pull-up
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC->v2
> > - New patch
> > ---
> >  drivers/pinctrl/renesas/pinctrl-rzg2l.c | 73 +++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > index 102fa75c71d3..c144bf43522b 100644
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -122,6 +122,7 @@
> >  #define IOLH(off)            (0x1000 + (off) * 8)
> >  #define SR(off)                      (0x1400 + (off) * 8)
> >  #define IEN(off)             (0x1800 + (off) * 8)
> > +#define PUPD(off)            (0x1C00 + (off) * 8)
> >  #define ISEL(off)            (0x2C00 + (off) * 8)
> >  #define SD_CH(off, ch)               ((off) + (ch) * 4)
> >  #define ETH_POC(off, ch)     ((off) + (ch) * 4)
> > @@ -140,6 +141,7 @@
> >  #define IEN_MASK             0x01
> >  #define IOLH_MASK            0x03
> >  #define SR_MASK                      0x01
> > +#define PUPD_MASK            0x03
> >
> >  #define PM_INPUT             0x1
> >  #define PM_OUTPUT            0x2
> > @@ -265,6 +267,8 @@ struct rzg2l_pinctrl_data {
> >       void (*pmc_writeb)(struct rzg2l_pinctrl *pctrl, u8 val, void __iomem *addr);
> >       u32 (*read_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin);
> >       int (*write_oen)(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8 pin, u8 oen);
> > +     int (*get_bias_param)(u8 val);
> > +     int (*get_bias_val)(enum pin_config_param param);
> >  };
> >
> >  /**
> > @@ -1081,6 +1085,38 @@ static int rzg2l_write_oen(struct rzg2l_pinctrl *pctrl, u32 caps, u32 offset, u8
> >       return 0;
> >  }
> >
> > +static int rzg2l_get_bias_param(u8 val)
> > +{
> > +     switch (val) {
> > +     case 0:
> > +             return PIN_CONFIG_BIAS_DISABLE;
> > +     case 1:
> > +             return PIN_CONFIG_BIAS_PULL_UP;
> > +     case 2:
> > +             return PIN_CONFIG_BIAS_PULL_DOWN;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int rzg2l_get_bias_val(enum pin_config_param param)
> > +{
> > +     switch (param) {
> > +     case PIN_CONFIG_BIAS_DISABLE:
> > +             return 0;
> > +     case PIN_CONFIG_BIAS_PULL_UP:
> > +             return 1;
> > +     case PIN_CONFIG_BIAS_PULL_DOWN:
> > +             return 2;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     return -EINVAL;
> > +}
> > +
> >  static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >                                    unsigned int _pin,
> >                                    unsigned long *config)
> > @@ -1139,6 +1175,25 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >               arg = rzg2l_read_pin_config(pctrl, SR(off), bit, SR_MASK);
> >               break;
> >
> > +     case PIN_CONFIG_BIAS_DISABLE:
> > +     case PIN_CONFIG_BIAS_PULL_UP:
> > +     case PIN_CONFIG_BIAS_PULL_DOWN: {
>
> Block { } can be removed here.
>
> > +             if (!(cfg & PIN_CFG_PUPD))
> > +                     return -EINVAL;
> > +
> > +             ret = pctrl->data->get_bias_param(rzg2l_read_pin_config(pctrl,
> > +                                                                     PUPD(off),
> > +                                                                     bit, PUPD_MASK));
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             if (ret != param)
> > +                     return -EINVAL;
>
> Can this happen? Otherwise it can be removed.
>
Yes this can happen (and is needed) as we want to report only the
current BIAS setting of the pin.

For example without this condition I get the below for ET1_RXD3 pin:
pin 173 (ET1_RXD3): input bias disabled, input bias pull down (0x1
ohms), input bias pull up (0x1 ohms)
with the check included:
pin 173 (ET1_RXD3): input bias disabled

Cheers,
Prabhakar

[v2,00/13] Add PFC support for Renesas RZ/V2H(P) SoC

Message

Comments