Hi Sam,
On Tue, 5 Dec 2023 at 11:34, Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
>
> Hi Sam,
>
> Thanks for your review.
>
> On Sat, 2 Dec 2023 at 00:22, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
> >
> > On Fri, Dec 1, 2023 at 10:11 AM Peter Griffin <peter.griffin@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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
> > >
