Re: [PATCH v2 13/13] pinctrl: renesas: pinctrl-rzg2l: Add support for RZ/V2H SoC

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 4:30 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@gmailcom> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add pinctrl driver support for RZ/V2H(P) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > 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