Hi Prabhakar, On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> 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 */ > +#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@xxxxxxxxxxxxxx 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