Message-ID: <CADrjBPr3VVvY4axBhAEO4zVGhLMiDZ8jWHCf=uSfEBMcZSOa=Q@mail.gmail.com>
Date: Tue, 21 Jan 2025 15:39:04 +0000
From: Peter Griffin <peter.griffin@...aro.org>
To: André Draszik <andre.draszik@...aro.org>
Cc: Krzysztof Kozlowski <krzk@...nel.org>, Sylwester Nawrocki <s.nawrocki@...sung.com>, 
	Alim Akhtar <alim.akhtar@...sung.com>, Linus Walleij <linus.walleij@...aro.org>, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	tudor.ambarus@...aro.org, willmcvicker@...gle.com, semen.protsenko@...aro.org, 
	kernel-team@...roid.com, jaewon02.kim@...sung.com
Subject: Re: [PATCH 3/3] pinctrl: samsung: Add filter selection support for
 alive bank on gs101

Hi André,

Thanks for your review feedback.

On Tue, 21 Jan 2025 at 11:04, André Draszik <andre.draszik@...aro.org> 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@...aro.org>
> > ---
> >
> > 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  | 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

Powered by blists - more mailing lists