| Message ID | 20250307-pinctrl-fltcon-suspend-v4-3-2d775e486036@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | samsung: pinctrl: Add support for eint_fltcon_offset and filter selection on gs101 | expand |
On 07/03/2025 11:29, Peter Griffin wrote: > gs101 differs to other SoCs in that fltcon1 register doesn't > always exist. Additionally the offset of fltcon0 is not fixed > and needs to use the newly added eint_fltcon_offset variable. > > Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration") > Cc: stable@vger.kernel.org It looks this depends on previous commit, right? That's really not optimal, although I understand that if you re-order patches this code would be soon changed, just like you changed other suspend/resume callbacks in patch #2? > Reviewed-by: André Draszik <andre.draszik@linaro.org> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > --- > Changes since v2: > * make it clear exynos_set_wakeup(bank) is conditional on bank type (Andre) > * align style where the '+' is placed (Andre) > * remove unnecessary braces (Andre) > --- ... > +void gs101_pinctrl_suspend(struct samsung_pin_bank *bank) > +{ > + struct exynos_eint_gpio_save *save = bank->soc_priv; > + const void __iomem *regs = bank->eint_base; > + > + if (bank->eint_type == EINT_TYPE_GPIO) { > + save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET > + + bank->eint_offset); > + > + save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET > + + bank->eint_fltcon_offset); > + > + /* fltcon1 register only exists for pins 4-7 */ > + if (bank->nr_pins > 4) > + save->eint_fltcon1 = readl(regs + > + EXYNOS_GPIO_EFLTCON_OFFSET > + + bank->eint_fltcon_offset + 4); > + > + save->eint_mask = readl(regs + bank->irq_chip->eint_mask > + + bank->eint_offset); > + > + pr_debug("%s: save con %#010x\n", > + bank->name, save->eint_con); > + pr_debug("%s: save fltcon0 %#010x\n", > + bank->name, save->eint_fltcon0); > + if (bank->nr_pins > 4) > + pr_debug("%s: save fltcon1 %#010x\n", > + 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_set_wakeup(bank); Missing {}. Run checkpatch --strict. Best regards, Krzysztof
Hi Krzysztof, Thanks for the review feedback. On Tue, 11 Mar 2025 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 07/03/2025 11:29, Peter Griffin wrote: > > gs101 differs to other SoCs in that fltcon1 register doesn't > > always exist. Additionally the offset of fltcon0 is not fixed > > and needs to use the newly added eint_fltcon_offset variable. > > > > Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration") > > Cc: stable@vger.kernel.org > > It looks this depends on previous commit, right? Yes that's right, it depends on the refactoring in the previous patch. To fix the bug (which is an Serror on suspend for gs101), we need the dedicated gs101 callback so it can have the knowledge that fltcon1 doesn't always exist and it's varying offset. > That's really not > optimal, although I understand that if you re-order patches this code > would be soon changed, just like you changed other suspend/resume > callbacks in patch #2? Originally it was just one patch, but the previous review feedback was to split the refactor into a dedicated patch, and then add gs101 specific parts separately. The refactoring was done primarily so we can fix this bug without affecting the other platforms, so I don't re-ordering the patches will help. Thanks, Peter > > > > Reviewed-by: André Draszik <andre.draszik@linaro.org> > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > --- > > Changes since v2: > > * make it clear exynos_set_wakeup(bank) is conditional on bank type (Andre) > > * align style where the '+' is placed (Andre) > > * remove unnecessary braces (Andre) > > --- > > ... > > > +void gs101_pinctrl_suspend(struct samsung_pin_bank *bank) > > +{ > > + struct exynos_eint_gpio_save *save = bank->soc_priv; > > + const void __iomem *regs = bank->eint_base; > > + > > + if (bank->eint_type == EINT_TYPE_GPIO) { > > + save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET > > + + bank->eint_offset); > > + > > + save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET > > + + bank->eint_fltcon_offset); > > + > > + /* fltcon1 register only exists for pins 4-7 */ > > + if (bank->nr_pins > 4) > > + save->eint_fltcon1 = readl(regs + > > + EXYNOS_GPIO_EFLTCON_OFFSET > > + + bank->eint_fltcon_offset + 4); > > + > > + save->eint_mask = readl(regs + bank->irq_chip->eint_mask > > + + bank->eint_offset); > > + > > + pr_debug("%s: save con %#010x\n", > > + bank->name, save->eint_con); > > + pr_debug("%s: save fltcon0 %#010x\n", > > + bank->name, save->eint_fltcon0); > > + if (bank->nr_pins > 4) > > + pr_debug("%s: save fltcon1 %#010x\n", > > + 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_set_wakeup(bank); > > Missing {}. Run checkpatch --strict. > > > Best regards, > Krzysztof
Hi Krzysztof, On Wed, 12 Mar 2025 at 11:31, Peter Griffin <peter.griffin@linaro.org> wrote: > > Hi Krzysztof, > > Thanks for the review feedback. > > On Tue, 11 Mar 2025 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On 07/03/2025 11:29, Peter Griffin wrote: > > > gs101 differs to other SoCs in that fltcon1 register doesn't > > > always exist. Additionally the offset of fltcon0 is not fixed > > > and needs to use the newly added eint_fltcon_offset variable. > > > > > > Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration") > > > Cc: stable@vger.kernel.org > > > > It looks this depends on previous commit, right? > > Yes that's right, it depends on the refactoring in the previous patch. > To fix the bug (which is an Serror on suspend for gs101), we need the > dedicated gs101 callback so it can have the knowledge that fltcon1 > doesn't always exist and it's varying offset. and also dependent on the first patch that adds the eint_fltcon_offset :) Peter
On 12/03/2025 12:39, Peter Griffin wrote: > Hi Krzysztof, > > On Wed, 12 Mar 2025 at 11:31, Peter Griffin <peter.griffin@linaro.org> wrote: >> >> Hi Krzysztof, >> >> Thanks for the review feedback. >> >> On Tue, 11 Mar 2025 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> >>> On 07/03/2025 11:29, Peter Griffin wrote: >>>> gs101 differs to other SoCs in that fltcon1 register doesn't >>>> always exist. Additionally the offset of fltcon0 is not fixed >>>> and needs to use the newly added eint_fltcon_offset variable. >>>> >>>> Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration") >>>> Cc: stable@vger.kernel.org >>> >>> It looks this depends on previous commit, right? >> >> Yes that's right, it depends on the refactoring in the previous patch. >> To fix the bug (which is an Serror on suspend for gs101), we need the >> dedicated gs101 callback so it can have the knowledge that fltcon1 >> doesn't always exist and it's varying offset. > > and also dependent on the first patch that adds the eint_fltcon_offset :) That would be fine because it's a fix as well. Ah, well, let's keep the dependency, but then I think syntax would be: Cc: <stable@vger.kernel.org> # depends on the previous three patches Best regards, Krzysztof
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c index 57c98d2451b54b00d50e0e948e272ed53d386c34..fca447ebc5f5956b7e8d2f2d08f23622095b1ee6 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c @@ -1455,15 +1455,15 @@ static const struct samsung_pin_ctrl gs101_pin_ctrl[] __initconst = { .pin_banks = gs101_pin_alive, .nr_banks = ARRAY_SIZE(gs101_pin_alive), .eint_wkup_init = exynos_eint_wkup_init, - .suspend = exynos_pinctrl_suspend, - .resume = exynos_pinctrl_resume, + .suspend = gs101_pinctrl_suspend, + .resume = gs101_pinctrl_resume, }, { /* pin banks of gs101 pin-controller (FAR_ALIVE) */ .pin_banks = gs101_pin_far_alive, .nr_banks = ARRAY_SIZE(gs101_pin_far_alive), .eint_wkup_init = exynos_eint_wkup_init, - .suspend = exynos_pinctrl_suspend, - .resume = exynos_pinctrl_resume, + .suspend = gs101_pinctrl_suspend, + .resume = gs101_pinctrl_resume, }, { /* pin banks of gs101 pin-controller (GSACORE) */ .pin_banks = gs101_pin_gsacore, @@ -1477,29 +1477,29 @@ static const struct samsung_pin_ctrl gs101_pin_ctrl[] __initconst = { .pin_banks = gs101_pin_peric0, .nr_banks = ARRAY_SIZE(gs101_pin_peric0), .eint_gpio_init = exynos_eint_gpio_init, - .suspend = exynos_pinctrl_suspend, - .resume = exynos_pinctrl_resume, + .suspend = gs101_pinctrl_suspend, + .resume = gs101_pinctrl_resume, }, { /* pin banks of gs101 pin-controller (PERIC1) */ .pin_banks = gs101_pin_peric1, .nr_banks = ARRAY_SIZE(gs101_pin_peric1), .eint_gpio_init = exynos_eint_gpio_init, - .suspend = exynos_pinctrl_suspend, - .resume = exynos_pinctrl_resume, + .suspend = gs101_pinctrl_suspend, + .resume = gs101_pinctrl_resume, }, { /* pin banks of gs101 pin-controller (HSI1) */ .pin_banks = gs101_pin_hsi1, .nr_banks = ARRAY_SIZE(gs101_pin_hsi1), .eint_gpio_init = exynos_eint_gpio_init, - .suspend = exynos_pinctrl_suspend, - .resume = exynos_pinctrl_resume, + .suspend = gs101_pinctrl_suspend, + .resume = gs101_pinctrl_resume, }, { /* pin banks of gs101 pin-controller (HSI2) */ .pin_banks = gs101_pin_hsi2, .nr_banks = ARRAY_SIZE(gs101_pin_hsi2), .eint_gpio_init = exynos_eint_gpio_init, - .suspend = exynos_pinctrl_suspend, - .resume = exynos_pinctrl_resume, + .suspend = gs101_pinctrl_suspend, + .resume = gs101_pinctrl_resume, }, }; diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c index 75b9ab19e4e8f81bf85cd75573485b7f2e717e7b..5f0045d03346600557fa6735bad709897c71935c 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -798,6 +798,40 @@ void exynos_pinctrl_suspend(struct samsung_pin_bank *bank) exynos_set_wakeup(bank); } +void gs101_pinctrl_suspend(struct samsung_pin_bank *bank) +{ + struct exynos_eint_gpio_save *save = bank->soc_priv; + const void __iomem *regs = bank->eint_base; + + if (bank->eint_type == EINT_TYPE_GPIO) { + save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET + + bank->eint_offset); + + save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + + bank->eint_fltcon_offset); + + /* fltcon1 register only exists for pins 4-7 */ + if (bank->nr_pins > 4) + save->eint_fltcon1 = readl(regs + + EXYNOS_GPIO_EFLTCON_OFFSET + + bank->eint_fltcon_offset + 4); + + save->eint_mask = readl(regs + bank->irq_chip->eint_mask + + bank->eint_offset); + + pr_debug("%s: save con %#010x\n", + bank->name, save->eint_con); + pr_debug("%s: save fltcon0 %#010x\n", + bank->name, save->eint_fltcon0); + if (bank->nr_pins > 4) + pr_debug("%s: save fltcon1 %#010x\n", + 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_set_wakeup(bank); +} + void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank) { struct exynos_eint_gpio_save *save = bank->soc_priv; @@ -816,6 +850,42 @@ void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank) exynos_set_wakeup(bank); } +void gs101_pinctrl_resume(struct samsung_pin_bank *bank) +{ + struct exynos_eint_gpio_save *save = bank->soc_priv; + + void __iomem *regs = bank->eint_base; + void __iomem *eint_fltcfg0 = regs + EXYNOS_GPIO_EFLTCON_OFFSET + + bank->eint_fltcon_offset; + + if (bank->eint_type == EINT_TYPE_GPIO) { + pr_debug("%s: con %#010x => %#010x\n", bank->name, + readl(regs + EXYNOS_GPIO_ECON_OFFSET + + bank->eint_offset), save->eint_con); + + pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name, + readl(eint_fltcfg0), save->eint_fltcon0); + + /* fltcon1 register only exists for pins 4-7 */ + if (bank->nr_pins > 4) + pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name, + readl(eint_fltcfg0 + 4), save->eint_fltcon1); + + pr_debug("%s: mask %#010x => %#010x\n", bank->name, + readl(regs + bank->irq_chip->eint_mask + + bank->eint_offset), save->eint_mask); + + writel(save->eint_con, regs + EXYNOS_GPIO_ECON_OFFSET + + bank->eint_offset); + writel(save->eint_fltcon0, eint_fltcfg0); + + if (bank->nr_pins > 4) + writel(save->eint_fltcon1, eint_fltcfg0 + 4); + writel(save->eint_mask, regs + bank->irq_chip->eint_mask + + bank->eint_offset); + } +} + void exynos_pinctrl_resume(struct samsung_pin_bank *bank) { struct exynos_eint_gpio_save *save = bank->soc_priv; diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h index 35c2bc4ea488bda600ebfbda1492f5f49dbd9849..773f161a82a38cbaad05fcbc09a936300f5c7595 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.h +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h @@ -225,6 +225,8 @@ void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank); void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank); void exynos_pinctrl_suspend(struct samsung_pin_bank *bank); void exynos_pinctrl_resume(struct samsung_pin_bank *bank); +void gs101_pinctrl_suspend(struct samsung_pin_bank *bank); +void gs101_pinctrl_resume(struct samsung_pin_bank *bank); struct samsung_retention_ctrl * exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata, const struct samsung_retention_data *data);
gs101 differs to other SoCs in that fltcon1 register doesn't always exist. Additionally the offset of fltcon0 is not fixed and needs to use the newly added eint_fltcon_offset variable. Fixes: 4a8be01a1a7a ("pinctrl: samsung: Add gs101 SoC pinctrl configuration") Cc: stable@vger.kernel.org Reviewed-by: André Draszik <andre.draszik@linaro.org> Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- Changes since v2: * make it clear exynos_set_wakeup(bank) is conditional on bank type (Andre) * align style where the '+' is placed (Andre) * remove unnecessary braces (Andre) --- drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 24 ++++----- drivers/pinctrl/samsung/pinctrl-exynos.c | 70 ++++++++++++++++++++++++++ drivers/pinctrl/samsung/pinctrl-exynos.h | 2 + 3 files changed, 84 insertions(+), 12 deletions(-)