| Message ID | 20240423175900.702640-7-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add PFC support for Renesas RZ/V2H(P) SoC | expand |
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
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
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
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
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
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
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, }; static const struct of_device_id rzg2l_pinctrl_of_table[] = {