| Message ID | 20231011184823.443959-14-peter.griffin@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/20] dt-bindings: soc: samsung: exynos-pmu: Add gs101 compatible | expand |
On 10/11/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, and on > resume the digital filter is reapplied to all pins in the bank. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> Tested-by: Will McVicker <willmcvicker@google.com> Thanks, Will > --- > drivers/pinctrl/samsung/pinctrl-exynos.c | 82 ++++++++++++++++++++++- > drivers/pinctrl/samsung/pinctrl-exynos.h | 7 ++ > drivers/pinctrl/samsung/pinctrl-samsung.c | 2 + > drivers/pinctrl/samsung/pinctrl-samsung.h | 23 +++++++ > 4 files changed, 113 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > index a8212fc126bf..800831aa8357 100644 > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > @@ -269,6 +269,68 @@ struct exynos_eint_gpio_save { > u32 eint_mask; > }; > > +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; > + > + /* > + * This function sets the desired filter (digital or delay) to > + * every pin in the bank. Note the filter selection bitfield is > + * only found on alive banks. 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_con |= EXYNOS9_FLTCON_EN; > + > + if (filter) > + flt_con |= EXYNOS9_FLTCON_SEL_DIGITAL; > + > + flt_reg = EXYNOS_GPIO_EFLTCON_OFFSET + bank->fltcon_offset; > + > + if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) > + /* > + * if nr_pins > 4, we should set FLTCON0 register fully. > + * (pin0 ~ 3). So loop 4 times in case of FLTCON0. > + */ > + 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) { > + val = readl(d->virt_base + flt_reg + 0x4); > + loop_cnt = (bank->nr_pins - EXYNOS9_FLTCON_NR_PIN); > + > + 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 +383,9 @@ __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_SEL_DELAY); > } > > return 0; > @@ -555,6 +620,10 @@ __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_SEL_DIGITAL); > + > bank->irq_chip = devm_kmemdup(dev, irq_chip, sizeof(*irq_chip), > GFP_KERNEL); > if (!bank->irq_chip) { > @@ -658,6 +727,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 +735,9 @@ 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_SEL_DELAY); > if (!irq_chip) { > irq_chip = bank->irq_chip; > irq_chip->set_eint_wakeup_mask(drvdata, > @@ -707,11 +780,18 @@ 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_SEL_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 7bd6d82c9f36..63b2426ad5d6 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_SEL_DIGITAL BIT(6) > +#define EXYNOS9_FLTCON_SEL_DELAY 0 > +#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 e54847040b4a..449f8109d8b5 100644 > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c > @@ -1104,6 +1104,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 9af93e3d8d9f..de2ca8e8b378 100644 > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h > @@ -82,6 +82,21 @@ 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 some SoCs (gs101 and possibly others) have a selectable > + * filter on alive banks of 'delay/analog' or 'digital'. If the filter > + * selection 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 +137,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 +150,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 +166,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. > * @pin_base: starting pin number of the bank. > * @soc_priv: per-bank private data for SoC-specific code. > @@ -169,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 pin_base; > -- > 2.42.0.655.g421f12c284-goog >
On Wed, Oct 11, 2023 at 1:49 PM 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. > I wonder if that solves any particular issue. For Exynos850 I decided against adding this feature because I failed to find any benefits of it. Didn't even come up with the way to test it. Is it really needed for this SoC functioning? In case you have some more details on why it's needed and how it can be tested, please add that info to the commit message as well. > On suspend we apply the analog filter to all pins in the bank, and on > resume the digital filter is reapplied to all pins in the bank. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- Heads up: I noticed some merge warnings when applying this patch onto the most recent linux-next, like so: 8<---------------------------------------------------------------------->8 Applying: pinctrl: samsung: Add filter selection support for alive banks Using index info to reconstruct a base tree... M drivers/pinctrl/samsung/pinctrl-exynos.c M drivers/pinctrl/samsung/pinctrl-exynos.h M drivers/pinctrl/samsung/pinctrl-samsung.c M drivers/pinctrl/samsung/pinctrl-samsung.h Falling back to patching base and 3-way merge... Auto-merging drivers/pinctrl/samsung/pinctrl-samsung.h Auto-merging drivers/pinctrl/samsung/pinctrl-samsung.c Auto-merging drivers/pinctrl/samsung/pinctrl-exynos.h Auto-merging drivers/pinctrl/samsung/pinctrl-exynos.c 8<---------------------------------------------------------------------->8 It was still applied, but maybe if you are going to send v4 try to rebase your series on top of linux-next first. Below are pretty minor comments for the code. > drivers/pinctrl/samsung/pinctrl-exynos.c | 82 ++++++++++++++++++++++- > drivers/pinctrl/samsung/pinctrl-exynos.h | 7 ++ > drivers/pinctrl/samsung/pinctrl-samsung.c | 2 + > drivers/pinctrl/samsung/pinctrl-samsung.h | 23 +++++++ > 4 files changed, 113 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > index a8212fc126bf..800831aa8357 100644 > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > @@ -269,6 +269,68 @@ struct exynos_eint_gpio_save { > u32 eint_mask; > }; > > +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; > + > + /* > + * This function sets the desired filter (digital or delay) to > + * every pin in the bank. Note the filter selection bitfield is > + * only found on alive banks. 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] > + */ Maybe move this comment above this function? Or split it in two parts: function doc, and 'flt_con' variable doc. > + > + flt_con |= EXYNOS9_FLTCON_EN; > + > + if (filter) Different values are passed as a 'filter' param to this function. But here it's only used as a boolean value. Something doesn't feel right. > + flt_con |= EXYNOS9_FLTCON_SEL_DIGITAL; > + > + flt_reg = EXYNOS_GPIO_EFLTCON_OFFSET + bank->fltcon_offset; > + > + if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) > + /* > + * if nr_pins > 4, we should set FLTCON0 register fully. > + * (pin0 ~ 3). So loop 4 times in case of FLTCON0. > + */ Maybe move this comment above 'if' block? And start with capital letter, for consistency with other multi-line comments. > + loop_cnt = EXYNOS9_FLTCON_NR_PIN; > + else > + loop_cnt = bank->nr_pins; > + > + val = readl(d->virt_base + flt_reg); > + Maybe remove this empty line to make RMW block the whole? > + for (i = 0; i < loop_cnt; i++) { > + shift = i * EXYNOS9_FLTCON_LEN; > + val &= ~(EXYNOS9_FLTCON_MASK << shift); > + val |= (flt_con << shift); > + } > + Ditto. > + writel(val, d->virt_base + flt_reg); > + > + /* loop for FLTCON1 pin 4 ~ 7 */ Start with a capital letter for consistency. > + if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) { > + val = readl(d->virt_base + flt_reg + 0x4); > + loop_cnt = (bank->nr_pins - EXYNOS9_FLTCON_NR_PIN); > + > + for (i = 0; i < loop_cnt; i++) { > + shift = i * EXYNOS9_FLTCON_LEN; > + val &= ~(EXYNOS9_FLTCON_MASK << shift); > + val |= (flt_con << shift); > + } Code duplication, but it's minor. > + 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 +383,9 @@ __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_SEL_DELAY); > } > > return 0; > @@ -555,6 +620,10 @@ __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_SEL_DIGITAL); Please stick to 80 characters per line when possible. > + > bank->irq_chip = devm_kmemdup(dev, irq_chip, sizeof(*irq_chip), > GFP_KERNEL); > if (!bank->irq_chip) { > @@ -658,6 +727,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 +735,9 @@ 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_SEL_DELAY); Please stick to 80 characters per line when possible. > if (!irq_chip) { > irq_chip = bank->irq_chip; > irq_chip->set_eint_wakeup_mask(drvdata, > @@ -707,11 +780,18 @@ 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) { Indent it to be under the open bracket on the previous line. > + /* Set Digital Filter */ > + if (bank->fltcon_type != FLT_DEFAULT) > + exynos_eint_flt_config(d, bank, EXYNOS9_FLTCON_SEL_DIGITAL); Please stick to 80 characters per line when possible. > + } > } > > 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 7bd6d82c9f36..63b2426ad5d6 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_SEL_DIGITAL BIT(6) > +#define EXYNOS9_FLTCON_SEL_DELAY 0 > +#define EXYNOS9_FLTCON_MASK 0xff > +#define EXYNOS9_FLTCON_LEN 8 > +#define EXYNOS9_FLTCON_NR_PIN 4 > I guess we discussed using EXYNOS9 prefix during the review of Exynos850 initial submission, and decided against it. But in case of this SoC (which is obviously Exynos, but is called Google), I'm not even sure which name would be appropriate. I mean, if it's ok to use EXYNOS9 prefix, then maybe I should go ahead and rename existing EXYNOS850 definitions to EXYNOS9 too, as it belongs to the same platform family, to avoid any confusion. Krzysztof, what's your take on this? > #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 e54847040b4a..449f8109d8b5 100644 > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c > @@ -1104,6 +1104,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 9af93e3d8d9f..de2ca8e8b378 100644 > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h > @@ -82,6 +82,21 @@ 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 some SoCs (gs101 and possibly others) have a selectable > + * filter on alive banks of 'delay/analog' or 'digital'. If the filter > + * selection is not available then the default filter is used (digital). > + */ > + Maybe remove this empty line? > +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 +137,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 +150,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 +166,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. > * @pin_base: starting pin number of the bank. > * @soc_priv: per-bank private data for SoC-specific code. > @@ -169,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 pin_base; > -- > 2.42.0.655.g421f12c284-goog >
Hi Sam, Thanks for your review feedback. See my answers inline below. On Wed, 11 Oct 2023 at 23:47, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, Oct 11, 2023 at 1:49 PM 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. > > > > I wonder if that solves any particular issue. For Exynos850 I decided > against adding this feature because I failed to find any benefits of > it. Didn't even come up with the way to test it. Is it really needed > for this SoC functioning? In case you have some more details on why > it's needed and how it can be tested, please add that info to the > commit message as well. I can certainly add some more information to the commit message. The filter determines to what extent the signal fluctuations received through the pad on the GPIO are considered glitches. The downstream kernel used by Pixel 6 phones in production set this filter. If you want to test on e850 then I would issue echo mem > /sys/power/state And then try to wake the device using some eint gpio. The upstream kernel gs101/Oriole isn't functional enough currently to test suspend resume. The logic makes sense though that whilst suspended you don't want the digital filter enabled, as it is using a clock. Given this filter is about detecting glitches and signal fluctuations it looks like the sort of thing that may not deterministically fail but could lead to spurious wakeups, which is obviously not good for a battery based device. Additionally setting this filter is what is recommended by the SoC manufacturer, so it seems wise to set it. > > > On suspend we apply the analog filter to all pins in the bank, and on > > resume the digital filter is reapplied to all pins in the bank. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > Heads up: I noticed some merge warnings when applying this patch onto > the most recent linux-next, like so: > > 8<---------------------------------------------------------------------->8 > Applying: pinctrl: samsung: Add filter selection support for alive banks > Using index info to reconstruct a base tree... > M drivers/pinctrl/samsung/pinctrl-exynos.c > M drivers/pinctrl/samsung/pinctrl-exynos.h > M drivers/pinctrl/samsung/pinctrl-samsung.c > M drivers/pinctrl/samsung/pinctrl-samsung.h > Falling back to patching base and 3-way merge... > Auto-merging drivers/pinctrl/samsung/pinctrl-samsung.h > Auto-merging drivers/pinctrl/samsung/pinctrl-samsung.c > Auto-merging drivers/pinctrl/samsung/pinctrl-exynos.h > Auto-merging drivers/pinctrl/samsung/pinctrl-exynos.c > 8<---------------------------------------------------------------------->8 > > It was still applied, but maybe if you are going to send v4 try to > rebase your series on top of linux-next first. > > Below are pretty minor comments for the code. > > > drivers/pinctrl/samsung/pinctrl-exynos.c | 82 ++++++++++++++++++++++- > > drivers/pinctrl/samsung/pinctrl-exynos.h | 7 ++ > > drivers/pinctrl/samsung/pinctrl-samsung.c | 2 + > > drivers/pinctrl/samsung/pinctrl-samsung.h | 23 +++++++ > > 4 files changed, 113 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c > > index a8212fc126bf..800831aa8357 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > > @@ -269,6 +269,68 @@ struct exynos_eint_gpio_save { > > u32 eint_mask; > > }; > > > > +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; > > + > > + /* > > + * This function sets the desired filter (digital or delay) to > > + * every pin in the bank. Note the filter selection bitfield is > > + * only found on alive banks. 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] > > + */ > > Maybe move this comment above this function? Or split it in two parts: > function doc, and 'flt_con' variable doc. Ok will move/split it > > > + > > + flt_con |= EXYNOS9_FLTCON_EN; > > + > > + if (filter) > > Different values are passed as a 'filter' param to this function. But > here it's only used as a boolean value. Something doesn't feel right. That was done to aid readability at the callee sites > > > + flt_con |= EXYNOS9_FLTCON_SEL_DIGITAL; > > + > > + flt_reg = EXYNOS_GPIO_EFLTCON_OFFSET + bank->fltcon_offset; > > + > > + if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) > > + /* > > + * if nr_pins > 4, we should set FLTCON0 register fully. > > + * (pin0 ~ 3). So loop 4 times in case of FLTCON0. > > + */ > > Maybe move this comment above 'if' block? And start with capital > letter, for consistency with other multi-line comments. will fix > > > + loop_cnt = EXYNOS9_FLTCON_NR_PIN; > > + else > > + loop_cnt = bank->nr_pins; > > + > > + val = readl(d->virt_base + flt_reg); > > + > > Maybe remove this empty line to make RMW block the whole? will do > > > + for (i = 0; i < loop_cnt; i++) { > > + shift = i * EXYNOS9_FLTCON_LEN; > > + val &= ~(EXYNOS9_FLTCON_MASK << shift); > > + val |= (flt_con << shift); > > + } > > + > > Ditto. > > > + writel(val, d->virt_base + flt_reg); > > + > > + /* loop for FLTCON1 pin 4 ~ 7 */ > > Start with a capital letter for consistency. will fix > > > + if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) { > > + val = readl(d->virt_base + flt_reg + 0x4); > > + loop_cnt = (bank->nr_pins - EXYNOS9_FLTCON_NR_PIN); > > + > > + for (i = 0; i < loop_cnt; i++) { > > + shift = i * EXYNOS9_FLTCON_LEN; > > + val &= ~(EXYNOS9_FLTCON_MASK << shift); > > + val |= (flt_con << shift); > > + } > > Code duplication, but it's minor. > > > + 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 +383,9 @@ __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_SEL_DELAY); > > } > > > > return 0; > > @@ -555,6 +620,10 @@ __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_SEL_DIGITAL); > > Please stick to 80 characters per line when possible. will fix > > > + > > bank->irq_chip = devm_kmemdup(dev, irq_chip, sizeof(*irq_chip), > > GFP_KERNEL); > > if (!bank->irq_chip) { > > @@ -658,6 +727,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 +735,9 @@ 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_SEL_DELAY); > > Please stick to 80 characters per line when possible. will fix > > > if (!irq_chip) { > > irq_chip = bank->irq_chip; > > irq_chip->set_eint_wakeup_mask(drvdata, > > @@ -707,11 +780,18 @@ 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) { > > Indent it to be under the open bracket on the previous line. will fix > > > + /* Set Digital Filter */ > > + if (bank->fltcon_type != FLT_DEFAULT) > > + exynos_eint_flt_config(d, bank, EXYNOS9_FLTCON_SEL_DIGITAL); > > Please stick to 80 characters per line when possible. 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 7bd6d82c9f36..63b2426ad5d6 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_SEL_DIGITAL BIT(6) > > +#define EXYNOS9_FLTCON_SEL_DELAY 0 > > +#define EXYNOS9_FLTCON_MASK 0xff > > +#define EXYNOS9_FLTCON_LEN 8 > > +#define EXYNOS9_FLTCON_NR_PIN 4 > > > > I guess we discussed using EXYNOS9 prefix during the review of > Exynos850 initial submission, and decided against it. But in case of > this SoC (which is obviously Exynos, but is called Google), I'm not > even sure which name would be appropriate. I mean, if it's ok to use > EXYNOS9 prefix, then maybe I should go ahead and rename existing > EXYNOS850 definitions to EXYNOS9 too, as it belongs to the same > platform family, to avoid any confusion. If Exynos850 also has this filter selection functionality, and we want to share the same macro names between gs101 and exynos850 then I think EXYNOS9_ prefix makes sense. That is what they are called in the downstream kernel IIRC > > Krzysztof, what's your take on this? > > > #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 e54847040b4a..449f8109d8b5 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c > > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c > > @@ -1104,6 +1104,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 9af93e3d8d9f..de2ca8e8b378 100644 > > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h > > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h > > @@ -82,6 +82,21 @@ 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 some SoCs (gs101 and possibly others) have a selectable > > + * filter on alive banks of 'delay/analog' or 'digital'. If the filter > > + * selection is not available then the default filter is used (digital). > > + */ > > + > > Maybe remove this empty line? Will fix regards, Peter > > > +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 +137,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 +150,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 +166,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. > > * @pin_base: starting pin number of the bank. > > * @soc_priv: per-bank private data for SoC-specific code. > > @@ -169,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 pin_base; > > -- > > 2.42.0.655.g421f12c284-goog > >
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c index a8212fc126bf..800831aa8357 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -269,6 +269,68 @@ struct exynos_eint_gpio_save { u32 eint_mask; }; +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; + + /* + * This function sets the desired filter (digital or delay) to + * every pin in the bank. Note the filter selection bitfield is + * only found on alive banks. 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_con |= EXYNOS9_FLTCON_EN; + + if (filter) + flt_con |= EXYNOS9_FLTCON_SEL_DIGITAL; + + flt_reg = EXYNOS_GPIO_EFLTCON_OFFSET + bank->fltcon_offset; + + if (bank->nr_pins > EXYNOS9_FLTCON_NR_PIN) + /* + * if nr_pins > 4, we should set FLTCON0 register fully. + * (pin0 ~ 3). So loop 4 times in case of FLTCON0. + */ + 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) { + val = readl(d->virt_base + flt_reg + 0x4); + loop_cnt = (bank->nr_pins - EXYNOS9_FLTCON_NR_PIN); + + 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 +383,9 @@ __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_SEL_DELAY); } return 0; @@ -555,6 +620,10 @@ __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_SEL_DIGITAL); + bank->irq_chip = devm_kmemdup(dev, irq_chip, sizeof(*irq_chip), GFP_KERNEL); if (!bank->irq_chip) { @@ -658,6 +727,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 +735,9 @@ 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_SEL_DELAY); if (!irq_chip) { irq_chip = bank->irq_chip; irq_chip->set_eint_wakeup_mask(drvdata, @@ -707,11 +780,18 @@ 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_SEL_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 7bd6d82c9f36..63b2426ad5d6 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_SEL_DIGITAL BIT(6) +#define EXYNOS9_FLTCON_SEL_DELAY 0 +#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 e54847040b4a..449f8109d8b5 100644 --- a/drivers/pinctrl/samsung/pinctrl-samsung.c +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c @@ -1104,6 +1104,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 9af93e3d8d9f..de2ca8e8b378 100644 --- a/drivers/pinctrl/samsung/pinctrl-samsung.h +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h @@ -82,6 +82,21 @@ 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 some SoCs (gs101 and possibly others) have a selectable + * filter on alive banks of 'delay/analog' or 'digital'. If the filter + * selection 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 +137,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 +150,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 +166,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. * @pin_base: starting pin number of the bank. * @soc_priv: per-bank private data for SoC-specific code. @@ -169,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 pin_base;
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, and on resume the digital filter is reapplied to all pins in the bank. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- drivers/pinctrl/samsung/pinctrl-exynos.c | 82 ++++++++++++++++++++++- drivers/pinctrl/samsung/pinctrl-exynos.h | 7 ++ drivers/pinctrl/samsung/pinctrl-samsung.c | 2 + drivers/pinctrl/samsung/pinctrl-samsung.h | 23 +++++++ 4 files changed, 113 insertions(+), 1 deletion(-)