Hi André, Thanks for your review feedback. On Tue, 21 Jan 2025 at 11:04, André Draszik <andre.draszik@xxxxxxxxxx> wrote: > > Hi Peter, > > On Mon, 2025-01-20 at 22:34 +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@xxxxxxxxxx> > > --- > > > > 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@xxxxxxxxxx/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 | 60 ++++++++++++++++++++++++++++++- > > drivers/pinctrl/samsung/pinctrl-exynos.h | 9 +++++ > > drivers/pinctrl/samsung/pinctrl-samsung.c | 1 + > > drivers/pinctrl/samsung/pinctrl-samsung.h | 4 +++ > > 4 files changed, 73 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > > index ddc7245ec2e5..a0256715f8f6 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > > @@ -369,6 +369,60 @@ 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 &= ~(EXYNOS_FLTCON_MASK << shift); > > This is also clearing FLT_WIDTH. Is this intended? Should the > value be retained / restored if the digital filter mode is > selected? It seems in analog mode the width is ignored anyway, > so maybe it doesn't need to be cleared? Currently we don't support setting the FLT_WIDTH bitfield and the reset value is zero. But I guess it would be better to not clear the bitfield in case the bootloader does set a value in the future. I'll update to do that in the next version. > This might be more relevant if samsung-pinctrl implemented > PIN_CONFIG_INPUT_DEBOUNCE (which it doesn't at the moment), > but would still be good to allow that to work. > > > + 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 (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 = Delay filter, 0x1 Digital filter > > It's a delay filter filter either way, right? If so, I > think '0x0 = Delay filter' should instead be reworded to > '0x0 = Analog filter'. I see your point, and kind of agree that Analog is a better name. The rationale for going with "Digital filter" and "Delay filter" was that it matches the FLT_SEL bitfield description in the datasheet. I thought it might confuse people using a different name. The info about it being Analog filter came via a bug from Samsung. But if folks prefer Analog I can use that instead. @Krzysztof any thoughts on the above naming? > > > + * FLT_WIDTH Filtering width. Valid when FLT_SEL is 0x1 > > This complete above register layout description would be > better suited right above the macro definition in > pinctrl-exynos.h I believe. I can move the comment in the next version. > > > + */ > > +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; > > + u8 n = bank->nr_pins; > > + > > + if (!bank->eint_flt_selectable) > > + 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. > > + */ > > This comment is a bit confusing now. There is no loop left here (as > it has moved). The loop is an implementation detail of > exynos_eint_update_flt_reg() and shouldn't be referred to here. Will fix. > > > + 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); > > + } > > How about something like this instead of if/else: > > for (int n = 0; n < bank->nr_pins; n += 4) > exynos_eint_update_flt_reg(reg + n, > min(bank->nr_pins - n, 4), con); Will update as you suggest. > > > > +} > > + > > /* > > * exynos_eint_gpio_init() - setup handling of external gpio interrupts. > > * @d: driver data of samsung pinctrl driver. > > @@ -420,7 +474,7 @@ __init int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d) > > ret = -ENOMEM; > > goto err_domains; > > } > > - > > + exynos_eint_set_filter(bank, EXYNOS_FLTCON_DELAY); > > } > > > > return 0; > > @@ -833,6 +887,8 @@ void gs101_pinctrl_suspend(struct samsung_pin_bank *bank) > > bank->name, save->eint_fltcon1); > > pr_debug("%s: save mask %#010x\n", > > bank->name, save->eint_mask); > > + } else if (bank->eint_type == EINT_TYPE_WKUP) { > > + exynos_eint_set_filter(bank, EXYNOS_FLTCON_DELAY); > > } > > } > > > > @@ -888,6 +944,8 @@ void gs101_pinctrl_resume(struct samsung_pin_bank *bank) > > writel(save->eint_fltcon1, eint_fltcfg0 + 4); > > writel(save->eint_mask, regs + bank->irq_chip->eint_mask > > + bank->eint_offset); > > + } else if (bank->eint_type == EINT_TYPE_WKUP) { > > + exynos_eint_set_filter(bank, EXYNOS_FLTCON_DIGITAL); > > } > > } > > > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h > > index 773f161a82a3..4f2dc6a2e5c7 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-exynos.h > > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h > > @@ -52,6 +52,13 @@ > > #define EXYNOS_EINT_MAX_PER_BANK 8 > > #define EXYNOS_EINT_NR_WKUP_EINT > > > > +/* EINT filter configuration */ > > +#define EXYNOS_FLTCON_EN BIT(7) > > +#define EXYNOS_FLTCON_DIGITAL BIT(6) > > +#define EXYNOS_FLTCON_DELAY (0 << 6) > > should EXYNOS_FLTCON_DELAY be EXYNOS_FLTCON_ANALOG? Same comment as above, I used DELAY because it matches the datasheet bitfield description but I agree that ANALOG is a better name and would make code readability better. The downside is we don't match the datasheet description. Thanks, Peter
