diff mbox series

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

Message ID 20231201160925.3136868-14-peter.griffin@linaro.org (mailing list archive)
State New
Headers show
Series Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand

Commit Message

Peter Griffin Dec. 1, 2023, 4:09 p.m. UTC
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

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(-)

Comments

William McVicker Dec. 1, 2023, 11:55 p.m. UTC | #1
On 12/01/2023, Peter Griffin 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
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>

Tested-by: Will McVicker <willmcvicker@google.com>

--

I verified boot and that the pinctrl driver is probing. When I tested

  echo mem > /sys/power/state

I wasn't able to re-wake the device (not sure how to send an eint gpio on
a phone form factor). I tried with no_console_suspend but that hits a SError
Interrupt (likely due to a PMU secure register access). Let me know if there's
anything else I should test out.

Thanks,
Will

> ---
>  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);
> +	}
> +}
> +
>  /*
>   * 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 */
> +		if (bank->fltcon_type != FLT_DEFAULT)
> +			exynos_eint_flt_config(d, bank,
> +					       EXYNOS9_FLTCON_DELAY);
>  	}
>  
>  	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);
> +
>  		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);
>  			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);
> +		}
>  }
>  
>  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
> +/* 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
>  
>  #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,
> +};
> +
>  /* 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
>
Sam Protsenko Dec. 2, 2023, 12:22 a.m. UTC | #2
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
>
Peter Griffin Dec. 5, 2023, 11:34 a.m. UTC | #3
Hi Sam,

Thanks for your review.

On Sat, 2 Dec 2023 at 00:22, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> 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.

Will fix

>
> >
> > 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?

Yes it does!
>
> 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).

I can refactor it along those lines.

>
> >  /*
> >   * 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().

Ok, I will update the function name to exynos_eint_set_filter() and
remove the comments.

>
> > +               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.

Will fix

>
> >         }
> >
> >         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'.

will fix

>
> > +
> >                 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'.

Will fix
>
> >                         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.

Will fix
>
> > +               }
> >  }
> >
> >  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?

Will fix
>
> > +/* 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?

Ok, will drop.

>
> >
> >  #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')?

I thought it made it clearer at the callee sites which filter was
being set, but I can update to a bool if that's what you prefer.

regards,

Peter.

>
> > +
> >  /* 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
> >
Peter Griffin Dec. 5, 2023, 7:10 p.m. UTC | #4
Hi Sam,

On Tue, 5 Dec 2023 at 11:34, Peter Griffin <peter.griffin@linaro.org> wrote:
>
> Hi Sam,
>
> Thanks for your review.
>
> On Sat, 2 Dec 2023 at 00:22, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> >
> > 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.
>
> Will fix
>
> >
> > >
> > > 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?
>
> Yes it does!
> >
> > 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).
>
> I can refactor it along those lines.
>
> >
> > >  /*
> > >   * 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().
>
> Ok, I will update the function name to exynos_eint_set_filter() and
> remove the comments.
>
> >
> > > +               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.
>
> Will fix
>
> >
> > >         }
> > >
> > >         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'.
>
> will fix
>
> >
> > > +
> > >                 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'.
>
> Will fix
> >
> > >                         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.
>
> Will fix
> >
> > > +               }
> > >  }
> > >
> > >  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?
>
> Will fix
> >
> > > +/* 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?
>
> Ok, will drop.
>
> >
> > >
> > >  #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')?
>
> I thought it made it clearer at the callee sites which filter was
> being set, but I can update to a bool if that's what you prefer.

Actually that was a previous version. We actually pass
EXYNOS9_FLTCON_DELAY and EXYNOS9_FLTCON_DIGITAL now so yes enum is no
longer required.

Peter
>
> regards,
>
> Peter.
>
> >
> > > +
> > >  /* 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
> > >
Peter Griffin Dec. 5, 2023, 7:51 p.m. UTC | #5
Hi Will,

Thanks for testing!

On Fri, 1 Dec 2023 at 23:55, William McVicker <willmcvicker@google.com> wrote:
>
> On 12/01/2023, Peter Griffin 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
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>
> Tested-by: Will McVicker <willmcvicker@google.com>
>
> --
>
> I verified boot and that the pinctrl driver is probing. When I tested
>
>   echo mem > /sys/power/state
>
> I wasn't able to re-wake the device (not sure how to send an eint gpio on
> a phone form factor). I tried with no_console_suspend but that hits a SError
> Interrupt (likely due to a PMU secure register access). Let me know if there's
> anything else I should test out.

Yes you're correct that SError is caused by the PMU secure register access.

Thanks,

Peter.
>
> Thanks,
> Will
>
> > ---
> >  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);
> > +     }
> > +}
> > +
> >  /*
> >   * 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 */
> > +             if (bank->fltcon_type != FLT_DEFAULT)
> > +                     exynos_eint_flt_config(d, bank,
> > +                                            EXYNOS9_FLTCON_DELAY);
> >       }
> >
> >       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);
> > +
> >               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);
> >                       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);
> > +             }
> >  }
> >
> >  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
> > +/* 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
> >
> >  #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,
> > +};
> > +
> >  /* 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
> >
diff mbox series

Patch

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);
+	}
+}
+
 /*
  * 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 */
+		if (bank->fltcon_type != FLT_DEFAULT)
+			exynos_eint_flt_config(d, bank,
+					       EXYNOS9_FLTCON_DELAY);
 	}
 
 	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);
+
 		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);
 			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);
+		}
 }
 
 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
+/* 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
 
 #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,
+};
+
 /* 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;