Re: [PATCH 27/37] pinctrl: renesas: rzg2l: add support for different ds values on different groups

From: Geert Uytterhoeven
Date: Thu Sep 21 2023 - 18:56:05 EST

Next message: Mike Kravetz: "Re: [PATCH 0/2] hugetlbfs: close race between MADV_DONTNEED and page fault"
Previous message: Jason Gunthorpe: "Re: [PATCH v4 2/6] iommu/vt-d: Remove mm->pasid in intel_sva_bind_mm()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Claudiu,

Thanks for your patch!

On Tue, Sep 12, 2023 at 6:53 AM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> RZ/G3S supports different drive strenght values for different power sources

strength

> and pin groups (A, B, C). On each group there could be up to 4 drive
> strength values per power source. Available power sources are 1v8, 2v5,
> 3v3. Drive strength values are fine tuned than what was previously
> available on the driver thus the necessity of having micro-amp support.
> As drive strength and power source values are linked togheter the

together

> hardware setup for these was moved at the end of
> rzg2l_pinctrl_pinconf_set() to ensure proper validation of the new
> values.
>
> The drive strength values are expected to be initialized though SoC
> specific hardware configuration data structure.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c

> @@ -133,27 +135,40 @@ struct rzg2l_register_offsets {
> u16 sd_ch;
> };
>
> +/* Value to be passed on drive strength arrays as invalid value. */
> +#define RZG2L_INVALID_IOLH_VAL (0xffff)

I think you can do without this (see below).

> +
> /**
> * enum rzg2l_iolh_index - starting indexes in IOLH specific arrays
> + * @RZG2L_IOLH_IDX_1V8: starting index for 1V8 power source
> + * @RZG2L_IOLH_IDX_2V5: starting index for 2V5 power source
> * @RZG2L_IOLH_IDX_3V3: starting index for 3V3 power source
> * @RZG2L_IOLH_IDX_MAX: maximum index
> */
> enum rzg2l_iolh_index {
> - RZG2L_IOLH_IDX_3V3 = 0,
> - RZG2L_IOLH_IDX_MAX = 4,
> + RZG2L_IOLH_IDX_1V8 = 0,
> + RZG2L_IOLH_IDX_2V5 = 4,
> + RZG2L_IOLH_IDX_3V3 = 8,
> + RZG2L_IOLH_IDX_MAX = 12,
> };
>
> /**
> * struct rzg2l_hwcfg - hardware configuration data structure
> * @regs: hardware specific register offsets
> * @iolh_groupa_ua: IOLH group A micro amps specific values
> + * @iolh_groupb_ua: IOLH group B micro amps specific values
> + * @iolh_groupc_ua: IOLH group C micro amps specific values

uA

> * @iolh_groupb_oi: IOLH group B output impedance specific values
> + * @drive_strength_ua: driver strenght in ua is supported (otherwise mA is supported)

drive strength in uA

> * @func_base: base number for port function (see register PFC)
> */
> struct rzg2l_hwcfg {
> const struct rzg2l_register_offsets regs;
> u16 iolh_groupa_ua[RZG2L_IOLH_IDX_MAX];
> + u16 iolh_groupb_ua[RZG2L_IOLH_IDX_MAX];
> + u16 iolh_groupc_ua[RZG2L_IOLH_IDX_MAX];
> u16 iolh_groupb_oi[RZG2L_IOLH_IDX_MAX];
> + bool drive_strength_ua;
> u8 func_base;
> };
>

> @@ -555,6 +584,164 @@ static void rzg2l_rmw_pin_config(struct rzg2l_pinctrl *pctrl, u32 offset,
> spin_unlock_irqrestore(&pctrl->lock, flags);
> }
>
> +static int rzg2l_get_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps)
> +{
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> + unsigned long flags;
> + void __iomem *addr;
> + u32 pwr_reg;
> + u16 ps;
> +
> + if (caps & PIN_CFG_IO_VMC_SD0)
> + pwr_reg = SD_CH(regs->sd_ch, 0);
> + else if (caps & PIN_CFG_IO_VMC_SD1)
> + pwr_reg = SD_CH(regs->sd_ch, 1);
> + else if (caps & PIN_CFG_IO_VMC_QSPI)
> + pwr_reg = QSPI;
> + else if (!(caps & PIN_CFG_SOFT_PS))
> + return -EINVAL;
> +
> + spin_lock_irqsave(&pctrl->lock, flags);

No need to take this spinlock
(it was just moved, and wasn't needed before).

> + if (caps & PIN_CFG_SOFT_PS) {
> + ps = pctrl->settings[pin].power_source;
> + } else {
> + addr = pctrl->base + pwr_reg;
> + ps = (readl(addr) & PVDD_MASK) ? 1800 : 3300;
> + }
> + spin_unlock_irqrestore(&pctrl->lock, flags);

I think the above can be simplified using a new caps_to_pwr_reg()
helper:

if (caps & PIN_CFG_SOFT_PS)
return pctrl->settings[pin].power_source;

addr = pctrl->base + caps_to_pwr_reg(caps);
if (addr == (u32)-1)
return -EINVAL;

return (readl(addr) & PVDD_MASK) ? 1800 : 3300;

BTW, if it wasn't for the initialization of settings[pin].power_source
in rzg2l_pinctrl_register() using rzg2l_get_power_source() too, you
could always return the cached value.

> +
> + return ps;
> +}
> +
> +static int rzg2l_set_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps, u32 ps)
> +{
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> + unsigned long flags;
> + void __iomem *addr;
> + u32 pwr_reg;
> +
> + if (caps & PIN_CFG_IO_VMC_SD0)
> + pwr_reg = SD_CH(regs->sd_ch, 0);
> + else if (caps & PIN_CFG_IO_VMC_SD1)
> + pwr_reg = SD_CH(regs->sd_ch, 1);
> + else if (caps & PIN_CFG_IO_VMC_QSPI)
> + pwr_reg = QSPI;
> + else if (!(caps & PIN_CFG_SOFT_PS))
> + return -EINVAL;
> +
> + addr = pctrl->base + pwr_reg;
> + spin_lock_irqsave(&pctrl->lock, flags);
> + if (!(caps & PIN_CFG_SOFT_PS))
> + writel((ps == 1800) ? PVDD_1800 : PVDD_3300, addr);
> + pctrl->settings[pin].power_source = ps;
> + spin_unlock_irqrestore(&pctrl->lock, flags);

No need to take this spinlock
(it was just moved, and wasn't needed before).

> +
> + return 0;

This function can be simplified in a similar way.

> +}

> +static u16 rzg2l_iolh_ua_to_val(const struct rzg2l_hwcfg *hwcfg, u32 caps,
> + enum rzg2l_iolh_index ps_index, u16 ua)
> +{
> + const u16 *array = NULL;
> + u16 i;
> +
> + if (caps & PIN_CFG_IOLH_A)
> + array = &hwcfg->iolh_groupa_ua[ps_index];
> +
> + if (caps & PIN_CFG_IOLH_B)
> + array = &hwcfg->iolh_groupb_ua[ps_index];
> +
> + if (caps & PIN_CFG_IOLH_C)
> + array = &hwcfg->iolh_groupc_ua[ps_index];
> +
> + if (!array)
> + return RZG2L_INVALID_IOLH_VAL;

Just make the function return int, and return -EINVAL.

> +
> + for (i = 0; i < 4; i++) {
> + if (array[i] == ua)
> + return i;
> + }
> +
> + return RZG2L_INVALID_IOLH_VAL;
> +}
> +
> +static bool rzg2l_ds_supported(struct rzg2l_pinctrl *pctrl, u32 caps,

rzg2l_ds_is_supported(), for consistency with rzg2l_ps_is_supported()

> + enum rzg2l_iolh_index iolh_idx,
> + u16 ds)
> +{
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> + const u16 *array = NULL;
> + u16 i;
> +
> + if (caps & PIN_CFG_IOLH_A)
> + array = hwcfg->iolh_groupa_ua;
> +
> + if (caps & PIN_CFG_IOLH_B)
> + array = hwcfg->iolh_groupb_ua;
> +
> + if (caps & PIN_CFG_IOLH_C)
> + array = hwcfg->iolh_groupc_ua;
> +
> + /* Should not happen. */
> + if (!array)
> + return false;
> +
> + if (array[iolh_idx] == RZG2L_INVALID_IOLH_VAL)

If zero uA is considered an invalid value, this can be simplified to

if (!array[iolh_idx])

> + return false;
> +
> + for (i = 0; i < 4; i++) {
> + if (array[iolh_idx + i] == ds)
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> unsigned int _pin,
> unsigned long *config)

> @@ -594,40 +779,50 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> return -EINVAL;
> break;
>
> - case PIN_CONFIG_POWER_SOURCE: {
> - u32 pwr_reg = 0x0;
> -
> - if (cfg & PIN_CFG_IO_VMC_SD0)
> - pwr_reg = SD_CH(regs->sd_ch, 0);
> - else if (cfg & PIN_CFG_IO_VMC_SD1)
> - pwr_reg = SD_CH(regs->sd_ch, 1);
> - else if (cfg & PIN_CFG_IO_VMC_QSPI)
> - pwr_reg = QSPI;
> - else
> - return -EINVAL;
> -
> - spin_lock_irqsave(&pctrl->lock, flags);
> - addr = pctrl->base + pwr_reg;
> - arg = (readl(addr) & PVDD_MASK) ? 1800 : 3300;
> - spin_unlock_irqrestore(&pctrl->lock, flags);
> + case PIN_CONFIG_POWER_SOURCE:
> + ret = rzg2l_get_power_source(pctrl, _pin, cfg);
> + if (ret < 0)
> + return ret;
> + arg = ret;
> break;
> - }
>
> case PIN_CONFIG_DRIVE_STRENGTH: {
> unsigned int index;
>
> - if (!(cfg & PIN_CFG_IOLH_A))
> + if (!(cfg & PIN_CFG_IOLH_A) || hwcfg->drive_strength_ua)
> return -EINVAL;
>
> index = rzg2l_read_pin_config(pctrl, IOLH(off), bit, IOLH_MASK);
> + /*
> + * Drive strenght mA is supported only by group A and only
> + * for 3V3 port source.
> + */
> arg = hwcfg->iolh_groupa_ua[index + RZG2L_IOLH_IDX_3V3] / 1000;
> break;
> }
>
> + case PIN_CONFIG_DRIVE_STRENGTH_UA: {
> + enum rzg2l_iolh_index iolh_idx;
> + u8 val;
> +
> + if (!(cfg & (PIN_CFG_IOLH_A | PIN_CFG_IOLH_B | PIN_CFG_IOLH_C)) ||
> + !hwcfg->drive_strength_ua)
> + return -EINVAL;
> +
> + ret = rzg2l_get_power_source(pctrl, _pin, cfg);
> + if (ret < 0)
> + return ret;
> + iolh_idx = rzg2l_ps_to_iolh_idx(ret);
> + val = rzg2l_read_pin_config(pctrl, IOLH(off), bit, IOLH_MASK);
> + arg = rzg2l_iolh_val_to_ua(hwcfg, cfg, iolh_idx + val);
> + break;
> + }
> +
> case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
> unsigned int index;
>
> - if (!(cfg & PIN_CFG_IOLH_B))
> + if (!(cfg & PIN_CFG_IOLH_B) ||
> + hwcfg->iolh_groupb_oi[0] == RZG2L_INVALID_IOLH_VAL)

!hwcfg->iolh_groupb_oi[0]

> return -EINVAL;
>
> index = rzg2l_read_pin_config(pctrl, IOLH(off), bit, IOLH_MASK);

> @@ -730,11 +904,20 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> break;
> }
>
> + case PIN_CONFIG_DRIVE_STRENGTH_UA:
> + if (!(cfg & (PIN_CFG_IOLH_A | PIN_CFG_IOLH_B | PIN_CFG_IOLH_C)) ||
> + !hwcfg->drive_strength_ua)
> + return -EINVAL;
> +
> + settings.drive_strength_ua = pinconf_to_config_argument(_configs[i]);
> + break;
> +
> case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
> unsigned int arg = pinconf_to_config_argument(_configs[i]);
> unsigned int index;
>
> - if (!(cfg & PIN_CFG_IOLH_B))
> + if (!(cfg & PIN_CFG_IOLH_B) ||
> + hwcfg->iolh_groupb_oi[0] == RZG2L_INVALID_IOLH_VAL)

!iolh_groupb_oi[0]

> return -EINVAL;
>
> for (index = 0; index < ARRAY_SIZE(hwcfg->iolh_groupb_oi); index++) {
> @@ -753,6 +936,47 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> }
> }
>
> + /* Apply drive strength and power source. */
> + if (memcmp(&settings, &pctrl->settings[_pin], sizeof(settings))) {

I'd rather invert the logic and return early here, so you can decrease
indentation below...

> + enum rzg2l_iolh_index iolh_idx;
> + unsigned long flags;
> + int ret;
> + u16 val;
> +
> + if (settings.power_source == pctrl->settings[_pin].power_source)
> + goto apply_drive_strength;

... and invert the logic here to avoid the goto:

if (settings.power_source != pctrl->settings[_pin].power_source)) {
...
> +
> + ret = rzg2l_ps_is_supported(settings.power_source);
> + if (!ret)
> + return -EINVAL;
> +
> + /* Apply power source. */
> + ret = rzg2l_set_power_source(pctrl, _pin, cfg, settings.power_source);
> + if (ret)
> + return ret;
> +

}

> +apply_drive_strength:
> + if (settings.drive_strength_ua == pctrl->settings[_pin].drive_strength_ua)
> + return 0;

Same here:

if (settings.drive_strength_ua != pctrl->settings[_pin].drive_strength_ua) {
...

> +
> + iolh_idx = rzg2l_ps_to_iolh_idx(settings.power_source);
> + ret = rzg2l_ds_supported(pctrl, cfg, iolh_idx,
> + settings.drive_strength_ua);
> + if (!ret)
> + return -EINVAL;
> +
> + /* Get register value for this PS/DS tuple. */
> + val = rzg2l_iolh_ua_to_val(hwcfg, cfg, iolh_idx, settings.drive_strength_ua);
> + if (val == RZG2L_INVALID_IOLH_VAL)
> + return -EINVAL;

Make val int, and return val if it is a negative error code.

> +
> + /* Apply drive strength. */
> + rzg2l_rmw_pin_config(pctrl, IOLH(off), bit, IOLH_MASK, val);
> + spin_lock_irqsave(&pctrl->lock, flags);
> + pctrl->settings[_pin].drive_strength_ua = settings.drive_strength_ua;
> + spin_unlock_irqrestore(&pctrl->lock, flags);

No need to take the spinlock.

> + }
> +

And after that, you'll realize the memcmp() can just be dropped ;-)

> return 0;
> }
>
> @@ -1459,6 +1683,7 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
>
> static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
> {
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> struct pinctrl_pin_desc *pins;
> unsigned int i, j;
> u32 *pin_data;
> @@ -1501,6 +1726,22 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
> pins[index].drv_data = &pin_data[index];
> }
>
> + pctrl->settings = devm_kzalloc(pctrl->dev, sizeof(*pctrl->settings) * pctrl->desc.npins,
> + GFP_KERNEL);

devm_kcalloc()

> + if (!pctrl->settings)
> + return -ENOMEM;
> +
> + for (i = 0; hwcfg->drive_strength_ua && i < pctrl->desc.npins; i++) {
> + if (pin_data[i] & PIN_CFG_SOFT_PS) {
> + pctrl->settings[i].power_source = 3300;
> + } else {
> + ret = rzg2l_get_power_source(pctrl, i, pin_data[i]);
> + if (ret < 0)
> + continue;
> + pctrl->settings[i].power_source = ret;
> + }
> + }
> +
> ret = devm_pinctrl_register_and_init(pctrl->dev, &pctrl->desc, pctrl,
> &pctrl->pctl);
> if (ret) {
> @@ -1574,6 +1815,8 @@ static const struct rzg2l_hwcfg rzg2l_hwcfg = {
> .sd_ch = 0x3000,
> },
> .iolh_groupa_ua = {
> + /* 1v8, 2v5 power source */
> + [RZG2L_IOLH_IDX_1V8 ... RZG2L_IOLH_IDX_3V3 - 1] = RZG2L_INVALID_IOLH_VAL,

If zero uA is considered an invalid value, the initialization above can
be dropped.

> /* 3v3 power source */
> [RZG2L_IOLH_IDX_3V3] = 2000, 4000, 8000, 12000,
> },

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

Next message: Mike Kravetz: "Re: [PATCH 0/2] hugetlbfs: close race between MADV_DONTNEED and page fault"
Previous message: Jason Gunthorpe: "Re: [PATCH v4 2/6] iommu/vt-d: Remove mm->pasid in intel_sva_bind_mm()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]