Re: [PATCH v2 4/4] pinctrl: samsung: Add filter selection support for alive bank on gs101

["André Draszik" <andre.draszik@linaro.org>]

On Sat, 2025-03-01 at 11:43 +0000, Peter Griffin wrote:
> Newer Exynos based SoCs have a filter selection bitfield in the filter
> configuration registers on alive bank pins. This allows the selection of
> a digital or analog delay filter for each pin. Add support for selecting
> and enabling the filter.
> 
> On suspend we set 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
> a clock and has an adjustable filter delay flt_width 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>
> ---
> 
> Changes since v1:
> * Remove eint_flt_selectable bool as it can be deduced from EINT_TYPE_WKUP (Peter)
> * Move filter config comment to header (Andre)
> * Rename EXYNOS_FLTCON_DELAY to EXYNOS_FLTCON_ANALOG (Andre)
> * Remove misleading old comment (Andre)
> * Refactor exynos_eint_update_flt_reg() into a loop (Andre)
> 
> Note: this patch was previously sent as part of the initial gs101/ Pixel 6
> series and was dropped in v6. This new version incorporates the review
> feedback from Sam Protsenko here in v5.
> 
> Link: https://lore.kernel.org/all/20231201160925.3136868-1-peter.griffin@linaro.org/T/#m79ced98939e895c840d812c8b4c2b3f33ce604c8
> 
> Changes since previous version
> * Drop fltcon_type enum and use bool eint_flt_selectable (Sam)
> * Refactor and add exynos_eint_update_flt_reg() (Sam)
> * Rename function to exynos_eint_set_filter() for easier readability (Sam)
> * Remove comments and `if bank->fltcon_type != FLT_DEFAULT)` checks and indentation (Sam)
> ---
>  drivers/pinctrl/samsung/pinctrl-exynos.c | 35 ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/samsung/pinctrl-exynos.h | 21 +++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index ddc7245ec2e5..4c467651b034 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -369,6 +369,39 @@ struct exynos_eint_gpio_save {
>  	u32 eint_mask;
>  };
>  
> +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 * EXYNOS_FLTCON_LEN;
> +		val |= con << shift;

EXYNOS_FLTCON_ANALOG == 0, so this code still needs to clear
bit 6 to undo a previous enabling of digital filter:

    val &= ~(EXYNOS_FLTCON_DIGITAL << shift);
    val |= con << shift;


> +	}
> +	writel(val, reg);
> +}
> +
> +/*
> + * Set the desired filter (digital or analog delay) and enable it 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_set_filter(struct samsung_pin_bank *bank, int filter)
> +{
> +	unsigned int off = EXYNOS_GPIO_EFLTCON_OFFSET + bank->eint_fltcon_offset;
> +	void __iomem *reg = bank->drvdata->virt_base + off;
> +	unsigned int con = EXYNOS_FLTCON_EN | filter;
> +
> +	if (bank->eint_type != EINT_TYPE_WKUP)
> +		return;
> +
> +	for (int n = 0; n < bank->nr_pins; n += 4)
> +		exynos_eint_update_flt_reg(reg + n,
> +					   min(bank->nr_pins - n, 4), con);
> +}
> +
>  /*
>   * exynos_eint_gpio_init() - setup handling of external gpio interrupts.
>   * @d: driver data of samsung pinctrl driver.
> @@ -834,6 +867,7 @@ void gs101_pinctrl_suspend(struct samsung_pin_bank *bank)
>  		pr_debug("%s: save    mask %#010x\n",
>  			 bank->name, save->eint_mask);
>  	}
> +	exynos_eint_set_filter(bank, EXYNOS_FLTCON_ANALOG);

Similar to patch 2, might be nicer to have this all if / else if to
make it more obvious that set_filter() is not unconditional.


>  }
>  
>  void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
> @@ -889,6 +923,7 @@ void gs101_pinctrl_resume(struct samsung_pin_bank *bank)
>  		writel(save->eint_mask, regs + bank->irq_chip->eint_mask
>  		       + bank->eint_offset);
>  	}
> +	exynos_eint_set_filter(bank, EXYNOS_FLTCON_DIGITAL);

dito.

Cheers,
Andre'

>  }
>  
>  void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> index 773f161a82a3..203d4b76a956 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> @@ -52,6 +52,27 @@
>  #define EXYNOS_EINT_MAX_PER_BANK	8
>  #define EXYNOS_EINT_NR_WKUP_EINT
>  
> +/*
> + * EINT filter configuration register (on alive banks) 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 = Analog delay filter, 0x1 Digital filter (clock count)
> + * FLT_WIDTH	Filtering width. Valid when FLT_SEL is 0x1
> + */
> +
> +#define EXYNOS_FLTCON_EN		BIT(7)
> +#define EXYNOS_FLTCON_DIGITAL		BIT(6)
> +#define EXYNOS_FLTCON_ANALOG		(0 << 6)
> +#define EXYNOS_FLTCON_MASK		GENMASK(7, 0)
> +#define EXYNOS_FLTCON_LEN		8
> +
>  #define EXYNOS_PIN_BANK_EINTN(pins, reg, id)		\
>  	{						\
>  		.type		= &bank_type_off,	\
>