lkml.org 
[lkml]   [2024]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2 06/13] pinctrl: renesas: pinctrl-rzg2l: Add function pointers for locking/unlocking the PFC register
    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

    \
     
     \ /
      Last update: 2024-04-25 13:41    [W:5.482 / U:0.564 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site