Re: [PATCH v5 13/20] pinctrl: samsung: Add filter selection support for alive banks

[Sam Protsenko <semen.protsenko@linaro.org>]

On Fri, Dec 1, 2023 at 10:11 AM Peter Griffin <peter.griffin@linaro.org> wrote:
>
> Newer Exynos SoCs have a filter selection register on alive bank pins.
> This allows the selection of a digital or delay filter for each pin. If
> the filter selection register is not available then the default filter
> (digital) is applied.
>
> On suspend we apply the analog filter to all pins in the bank (as the
> digital filter relies on a clock). On resume the digital filter is
> reapplied to all pins in the bank. The digital filter is working via
> clock and has an adjustable filter delay register bitfield, whereas
> the analog filter uses a fixed delay.
>
> The filter determines to what extent signal fluctuations received through
> the pad are considered glitches.
>
> The code path can be exercised using
> echo mem > /sys/power/state
> And then wake the device using a eint gpio

Period.

>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c  | 89 ++++++++++++++++++++++-
>  drivers/pinctrl/samsung/pinctrl-exynos.h  |  7 ++
>  drivers/pinctrl/samsung/pinctrl-samsung.c |  2 +
>  drivers/pinctrl/samsung/pinctrl-samsung.h | 22 ++++++
>  4 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index 6b58ec84e34b..56fc11a1fe2f 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -269,6 +269,71 @@ struct exynos_eint_gpio_save {
>         u32 eint_mask;
>  };
>
> +/*
> + * Set the desired filter (digital or analog delay) to every pin in
> + * the bank. Note the filter selection bitfield is only found on alive
> + * banks. The filter determines to what extent signal fluctuations
> + * received through the pad are considered glitches.
> + */
> +static void exynos_eint_flt_config(struct samsung_pinctrl_drv_data *d,
> +                                  struct samsung_pin_bank *bank, int filter)
> +{
> +       unsigned int flt_reg, flt_con = 0;
> +       unsigned int val, shift;
> +       int i;
> +       int loop_cnt;
> +
> +       /*
> +        * The FLTCON register has the following layout
> +        *
> +        * BitfieldName[PinNum][Bit:Bit]
> +        * FLT_EN[3][31] FLT_SEL[3][30] FLT_WIDTH[3][29:24]
> +        * FLT_EN[2][23] FLT_SEL[2][22] FLT_WIDTH[2][21:16]
> +        * FLT_EN[1][15] FLT_SEL[1][14] FLT_WIDTH[1][13:8]
> +        * FLT_EN[0][7]  FLT_SEL[0][6]  FLT_WIDTH[0][5:0]
> +        *
> +        * FLT_EN       0x0 = Disable, 0x1=Enable
> +        * FLT_SEL      0x0 = Delay filter, 0x1 Digital filter
> +        * FLT_WIDTH    Filtering width. Valid when FLT_SEL is 0x1
> +        */
> +
> +       flt_con |= EXYNOS9_FLTCON_EN;
> +
> +       if (filter)
> +               flt_con |= EXYNOS9_FLTCON_DIGITAL;
> +
> +       flt_reg = EXYNOS_GPIO_EFLTCON_OFFSET + bank->fltcon_offset;
> +
> +       /*
> +        * If nr_pins > 4, we should set FLTCON0 register fully.
> +        * (pin0 ~ 3). So loop 4 times in case of FLTCON0.
> +        */
> +       if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN)
> +               loop_cnt = EXYNOS9_FLTCON_NR_PIN;
> +       else
> +               loop_cnt = bank->nr_pins;
> +
> +       val = readl(d->virt_base + flt_reg);
> +       for (i = 0; i < loop_cnt; i++) {
> +               shift = i * EXYNOS9_FLTCON_LEN;
> +               val &= ~(EXYNOS9_FLTCON_MASK << shift);
> +               val |= (flt_con << shift);
> +       }
> +       writel(val, d->virt_base + flt_reg);
> +
> +       /* Loop for FLTCON1 pin 4 ~ 7 */
> +       if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) {
> +               loop_cnt = (bank->nr_pins - EXYNOS9_FLTCON_NR_PIN);
> +               val = readl(d->virt_base + flt_reg + 0x4);
> +               for (i = 0; i < loop_cnt; i++) {
> +                       shift = i * EXYNOS9_FLTCON_LEN;
> +                       val &= ~(EXYNOS9_FLTCON_MASK << shift);
> +                       val |= (flt_con << shift);
> +               }
> +               writel(val, d->virt_base + flt_reg + 0x4);
> +       }
> +}
> +

This whole function needs a refactoring. Do you think below code looks better?

8<----------------------------------------------------------------->8
static void exynos_eint_update_flt_reg(void __iomem *reg, int cnt, int con)
{
    unsigned int val, shift;
    int i;

    val = readl(reg);
    for (i = 0; i < cnt; i++) {
        shift = i * EXYNOS9_FLTCON_LEN;
        val &= ~(EXYNOS9_FLTCON_MASK << shift);
        val |= con << shift;
    }
    writel(val, reg);
}

/*
 * Set the desired filter (digital or analog delay) to every pin in the bank.
 * Note the filter selection bitfield is only found on alive banks. The filter
 * determines to what extent signal fluctuations received through the pad are
 * considered glitches.
 *
 * The FLTCON register has the following layout:
 *
 *     BitfieldName[PinNum][Bit:Bit]
 *     FLT_EN[3][31] FLT_SEL[3][30] FLT_WIDTH[3][29:24]
 *     FLT_EN[2][23] FLT_SEL[2][22] FLT_WIDTH[2][21:16]
 *     FLT_EN[1][15] FLT_SEL[1][14] FLT_WIDTH[1][13:8]
 *     FLT_EN[0][7]  FLT_SEL[0][6]  FLT_WIDTH[0][5:0]
 *
 * FLT_EN    0x0 = Disable, 0x1 = Enable
 * FLT_SEL    0x0 = Delay filter, 0x1 = Digital filter
 * FLT_WIDTH    Filtering width. Valid when FLT_SEL is 0x1
 */
static void exynos_eint_flt_config(struct samsung_pinctrl_drv_data *d,
                   struct samsung_pin_bank *bank, int filter)
{
    unsigned int off = EXYNOS_GPIO_EFLTCON_OFFSET + bank->fltcon_offset;
    unsigned int con = EXYNOS9_FLTCON_EN | filter;
    void __iomem *reg = d->virt_base + off;
    u8 n = bank->nr_pins;

    if (bank->fltcon_type == FLT_DEFAULT)
        return;

    /*
     * If nr_pins > 4, we should set FLTCON0 register fully (pin0~3).
     * So loop 4 times in case of FLTCON0. Loop for FLTCON1 pin4~7.
     */
    if (n <= 4) {
        exynos_eint_update_flt_reg(reg, n, con);
    } else {
        exynos_eint_update_flt_reg(reg, 4, con);
        exynos_eint_update_flt_reg(reg + 0x4, n - 4, con);
    }
}
8<----------------------------------------------------------------->8

(the code is only to illustrate the idea, I never tested it).

>  /*
>   * exynos_eint_gpio_init() - setup handling of external gpio interrupts.
>   * @d: driver data of samsung pinctrl driver.
> @@ -321,6 +386,10 @@ __init int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
>                         goto err_domains;
>                 }
>
> +               /* Set Delay Analog Filter */

The code below looks quite self-explanatory to. Maybe remove all
comments like this? If you don't think exynos_eint_flt_config() is
clear, maybe rename it to exynos_eint_set_filter().

> +               if (bank->fltcon_type != FLT_DEFAULT)
> +                       exynos_eint_flt_config(d, bank,
> +                                              EXYNOS9_FLTCON_DELAY);

It fits the previous line just fine, no need to break the line.

Also, if you use the refactored version of exynos_eint_flt_config() I
mentioned above, you can drop all 'if' conditions like this.

>         }
>
>         return 0;
> @@ -555,6 +624,11 @@ __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
>                 if (bank->eint_type != EINT_TYPE_WKUP)
>                         continue;
>
> +               /* Set Digital Filter */
> +               if (bank->fltcon_type != FLT_DEFAULT)
> +                       exynos_eint_flt_config(d, bank,
> +                                              EXYNOS9_FLTCON_DIGITAL);

Ditto: no need to break the line, remove the comment. If you use the
refactored function, you can drop 'if'.

> +
>                 bank->irq_chip = devm_kmemdup(dev, irq_chip, sizeof(*irq_chip),
>                                               GFP_KERNEL);
>                 if (!bank->irq_chip) {
> @@ -658,6 +732,7 @@ static void exynos_pinctrl_suspend_bank(
>  void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
>  {
>         struct samsung_pin_bank *bank = drvdata->pin_banks;
> +       struct samsung_pinctrl_drv_data *d = bank->drvdata;
>         struct exynos_irq_chip *irq_chip = NULL;
>         int i;
>
> @@ -665,6 +740,10 @@ void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
>                 if (bank->eint_type == EINT_TYPE_GPIO)
>                         exynos_pinctrl_suspend_bank(drvdata, bank);
>                 else if (bank->eint_type == EINT_TYPE_WKUP) {
> +                       /* Setting Delay (Analog) Filter */
> +                       if (bank->fltcon_type != FLT_DEFAULT)
> +                               exynos_eint_flt_config(d, bank,
> +                                                      EXYNOS9_FLTCON_DELAY);

Ditto: no need to break the line, remove the comment. If you use the
refactored function, you can drop 'if'.

>                         if (!irq_chip) {
>                                 irq_chip = bank->irq_chip;
>                                 irq_chip->set_eint_wakeup_mask(drvdata,
> @@ -707,11 +786,19 @@ static void exynos_pinctrl_resume_bank(
>  void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
>  {
>         struct samsung_pin_bank *bank = drvdata->pin_banks;
> +       struct samsung_pinctrl_drv_data *d = bank->drvdata;
>         int i;
>
>         for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
> -               if (bank->eint_type == EINT_TYPE_GPIO)
> +               if (bank->eint_type == EINT_TYPE_GPIO) {
>                         exynos_pinctrl_resume_bank(drvdata, bank);
> +               } else if (bank->eint_type == EINT_TYPE_WKUP ||
> +                          bank->eint_type == EINT_TYPE_WKUP_MUX) {
> +                       /* Set Digital Filter */
> +                       if (bank->fltcon_type != FLT_DEFAULT)
> +                               exynos_eint_flt_config(d, bank,
> +                                                      EXYNOS9_FLTCON_DIGITAL);

Ditto: remove the comment, and if you use the refactored function, you
can drop 'if'; also there will be no need to break the line.

> +               }
>  }
>
>  static void exynos_retention_enable(struct samsung_pinctrl_drv_data *drvdata)
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> index 3ac52c2cf998..e2799ff1b5e9 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> @@ -50,6 +50,13 @@
>
>  #define EXYNOS_EINT_MAX_PER_BANK       8
>  #define EXYNOS_EINT_NR_WKUP_EINT

Maybe add an empty line here?

> +/* EINT filter configuration */
> +#define EXYNOS9_FLTCON_EN              BIT(7)
> +#define EXYNOS9_FLTCON_DIGITAL         BIT(6)
> +#define EXYNOS9_FLTCON_DELAY           (0 << 6)
> +#define EXYNOS9_FLTCON_MASK            0xff
> +#define EXYNOS9_FLTCON_LEN             8
> +#define EXYNOS9_FLTCON_NR_PIN          4

I'd say drop this one and just hard-code it where needed?

>
>  #define EXYNOS_PIN_BANK_EINTN(pins, reg, id)           \
>         {                                               \
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 79babbb39ced..50c360b4753a 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1105,6 +1105,8 @@ samsung_pinctrl_get_soc_data(struct samsung_pinctrl_drv_data *d,
>                 bank->eint_func = bdata->eint_func;
>                 bank->eint_type = bdata->eint_type;
>                 bank->eint_mask = bdata->eint_mask;
> +               bank->fltcon_type = bdata->fltcon_type;
> +               bank->fltcon_offset = bdata->fltcon_offset;
>                 bank->eint_offset = bdata->eint_offset;
>                 bank->name = bdata->name;
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index 9b3db50adef3..5fab3885a7d7 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -82,6 +82,20 @@ enum eint_type {
>         EINT_TYPE_WKUP_MUX,
>  };
>
> +/**
> + * enum fltcon_type - filter selection
> + * @FLT_DEFAULT: filter not selectable, default digital filter
> + * @FLT_SELECT: filter selectable (digital or delay)
> + *
> + * Some banks on newer Exynos based SoCs have a selectable filter on alive
> + * banks of 'analog/delay' or 'digital'. If the filter selection register is
> + * not available then the default filter is used (digital).
> + */
> +enum fltcon_type {
> +       FLT_DEFAULT,
> +       FLT_SELECTABLE,
> +};

Is there any benefit of having this enum over replacing it with just a
bool field (e.g. 'bool flt_selectable')?

> +
>  /* maximum length of a pin in pin descriptor (example: "gpa0-0") */
>  #define PIN_NAME_LENGTH        10
>
> @@ -122,6 +136,8 @@ struct samsung_pin_bank_type {
>   * @eint_type: type of the external interrupt supported by the bank.
>   * @eint_mask: bit mask of pins which support EINT function.
>   * @eint_offset: SoC-specific EINT register or interrupt offset of bank.
> + * @fltcon_type: whether the filter (delay/digital) is selectable
> + * @fltcon_offset: SoC-specific EINT filter control register offset of bank.
>   * @name: name to be prefixed for each pin in this pin bank.
>   */
>  struct samsung_pin_bank_data {
> @@ -133,6 +149,8 @@ struct samsung_pin_bank_data {
>         enum eint_type  eint_type;
>         u32             eint_mask;
>         u32             eint_offset;
> +       enum fltcon_type fltcon_type;
> +       u32             fltcon_offset;
>         const char      *name;
>  };
>
> @@ -147,6 +165,8 @@ struct samsung_pin_bank_data {
>   * @eint_type: type of the external interrupt supported by the bank.
>   * @eint_mask: bit mask of pins which support EINT function.
>   * @eint_offset: SoC-specific EINT register or interrupt offset of bank.
> + * @fltcon_type: whether the filter (delay/digital) is selectable
> + * @fltcon_offset: SoC-specific EINT filter control register offset of bank.
>   * @name: name to be prefixed for each pin in this pin bank.
>   * @id: id of the bank, propagated to the pin range.
>   * @pin_base: starting pin number of the bank.
> @@ -170,6 +190,8 @@ struct samsung_pin_bank {
>         enum eint_type  eint_type;
>         u32             eint_mask;
>         u32             eint_offset;
> +       enum fltcon_type fltcon_type;
> +       u32             fltcon_offset;
>         const char      *name;
>         u32             id;
>
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>