From: "André Draszik" <andre.draszik@linaro.org>
To: Peter Griffin <peter.griffin@linaro.org>,
Krzysztof Kozlowski <krzk@kernel.org>,
Sylwester Nawrocki <s.nawrocki@samsung.com>,
Alim Akhtar <alim.akhtar@samsung.com>,
Linus Walleij <linus.walleij@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org, linux-gpio@vger.kernel.org,
linux-kernel@vger.kernel.org, tudor.ambarus@linaro.org,
willmcvicker@google.com, semen.protsenko@linaro.org,
kernel-team@android.com, jaewon02.kim@samsung.com
Subject: Re: [PATCH v3 2/4] pinctrl: samsung: add dedicated SoC eint suspend/resume callbacks
Date: Thu, 06 Mar 2025 21:29:39 +0000 [thread overview]
Message-ID: <c1b8a01c0ef3761a33c9f2530152cb2eab1ff039.camel@linaro.org> (raw)
In-Reply-To: <20250306-pinctrl-fltcon-suspend-v3-2-f9ab4ff6a24e@linaro.org>
On Thu, 2025-03-06 at 20:42 +0000, Peter Griffin wrote:
> gs101 needs it's own suspend/resume callbacks to use the newly
> added eint_fltcon_offset for saving & restoring fltcon0 & fltcon1
> registers. It also differs to previous SoCs in that fltcon1
> register doesn't always exist for each bank.
>
> exynosautov920 also has dedicated logic for using eint_con_offset
> and eint_mask_offset for saving & restoring it's registers.
>
> Refactor the existing platform specific suspend/resume callback
> so that each SoC variant has their own callback containing the
> SoC specific logic.
>
> Additionally we now call drvdata->suspend() & drvdata->resume()
> from within the loop that iterates the banks in
> samsung_pinctrl_suspend() and samsung_pinctrl_resume().
>
> This simplifies the logic, and allows us to remove the
> clk_enable() and clk_disable() from the callbacks.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> Changes since v2:
> * Remove useless init (Andre)
> * make it clear set_wakeup is conditional on bank->eint_type (Andre)
I think you missed
https://lore.kernel.org/all/4f91fe1c5eed00e58a3587bceaef3e5e2a1124cf.camel@linaro.org/
See below
> Changes since v1:
> * Split code refactor & gs101 parts into separate patches (Andre)
> ---
> drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 28 ++--
> drivers/pinctrl/samsung/pinctrl-exynos.c | 201 ++++++++++---------------
> drivers/pinctrl/samsung/pinctrl-exynos.h | 6 +-
> drivers/pinctrl/samsung/pinctrl-samsung.c | 11 +-
> drivers/pinctrl/samsung/pinctrl-samsung.h | 8 +-
> 5 files changed, 109 insertions(+), 145 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index e28fe81776466b693417c66bb15752d609b79eb1..57c98d2451b54b00d50e0e948e272ed53d386c34 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> @@ -1112,8 +1112,8 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
> .pin_banks = exynosautov920_pin_banks0,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks0),
> .eint_wkup_init = exynos_eint_wkup_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> .retention_data = &exynosautov920_retention_data,
> }, {
> /* pin-controller instance 1 AUD data */
> @@ -1124,43 +1124,43 @@ static const struct samsung_pin_ctrl exynosautov920_pin_ctrl[] = {
> .pin_banks = exynosautov920_pin_banks2,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks2),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 3 HSI1 data */
> .pin_banks = exynosautov920_pin_banks3,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks3),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 4 HSI2 data */
> .pin_banks = exynosautov920_pin_banks4,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks4),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 5 HSI2UFS data */
> .pin_banks = exynosautov920_pin_banks5,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks5),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 6 PERIC0 data */
> .pin_banks = exynosautov920_pin_banks6,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks6),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> }, {
> /* pin-controller instance 7 PERIC1 data */
> .pin_banks = exynosautov920_pin_banks7,
> .nr_banks = ARRAY_SIZE(exynosautov920_pin_banks7),
> .eint_gpio_init = exynos_eint_gpio_init,
> - .suspend = exynos_pinctrl_suspend,
> - .resume = exynos_pinctrl_resume,
> + .suspend = exynosautov920_pinctrl_suspend,
> + .resume = exynosautov920_pinctrl_resume,
> },
> };
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index ac6dc22b37c98ed5b7fca3335764f19abb2f71cc..f10ff09c1af01c11ff9229aaef77df32eb057b7b 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -761,153 +761,114 @@ __init int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
> return 0;
> }
>
> -static void exynos_pinctrl_suspend_bank(
> - struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +static void exynos_set_wakeup(struct samsung_pin_bank *bank)
> {
> - struct exynos_eint_gpio_save *save = bank->soc_priv;
> - const void __iomem *regs = bank->eint_base;
> + struct exynos_irq_chip *irq_chip;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for saving state\n");
> - return;
> + if (bank->irq_chip) {
> + irq_chip = bank->irq_chip;
> + irq_chip->set_eint_wakeup_mask(bank->drvdata, irq_chip);
> }
> -
> - save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> - + bank->eint_offset);
> - save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset);
> - save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset + 4);
> - save->eint_mask = readl(regs + bank->irq_chip->eint_mask
> - + bank->eint_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> -
> - pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
> - pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
> - pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
> - pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
> }
>
> -static void exynosauto_pinctrl_suspend_bank(struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +void exynos_pinctrl_suspend(struct samsung_pin_bank *bank)
> {
> struct exynos_eint_gpio_save *save = bank->soc_priv;
> const void __iomem *regs = bank->eint_base;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for saving state\n");
> - return;
> - }
> -
> - save->eint_con = readl(regs + bank->pctl_offset + bank->eint_con_offset);
> - save->eint_mask = readl(regs + bank->pctl_offset + bank->eint_mask_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> -
> - pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
> - pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
> + 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
> + + 2 * bank->eint_offset);
> + save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_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);
> + 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 exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
> +void exynosautov920_pinctrl_suspend(struct samsung_pin_bank *bank)
> {
> - struct samsung_pin_bank *bank = drvdata->pin_banks;
> - struct exynos_irq_chip *irq_chip = NULL;
> - int i;
> + struct exynos_eint_gpio_save *save = bank->soc_priv;
> + const void __iomem *regs = bank->eint_base;
>
> - for (i = 0; i < drvdata->nr_banks; ++i, ++bank) {
> - if (bank->eint_type == EINT_TYPE_GPIO) {
> - if (bank->eint_con_offset)
> - exynosauto_pinctrl_suspend_bank(drvdata, bank);
> - else
> - exynos_pinctrl_suspend_bank(drvdata, bank);
> - }
> - else if (bank->eint_type == EINT_TYPE_WKUP) {
> - if (!irq_chip) {
> - irq_chip = bank->irq_chip;
> - irq_chip->set_eint_wakeup_mask(drvdata,
> - irq_chip);
> - }
> - }
> - }
> + if (bank->eint_type == EINT_TYPE_GPIO) {
> + save->eint_con = readl(regs + bank->pctl_offset +
> + bank->eint_con_offset);
> + save->eint_mask = readl(regs + bank->pctl_offset +
> + bank->eint_mask_offset);
> + pr_debug("%s: save con %#010x\n",
> + bank->name, save->eint_con);
> + pr_debug("%s: save mask %#010x\n",
> + bank->name, save->eint_mask);
> + } else if (bank->eint_type == EINT_TYPE_WKUP)
> + exynos_set_wakeup(bank);
> }
>
> -static void exynos_pinctrl_resume_bank(
> - struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +void exynos_pinctrl_resume(struct samsung_pin_bank *bank)
> {
> struct exynos_eint_gpio_save *save = bank->soc_priv;
> void __iomem *regs = bank->eint_base;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for restoring state\n");
> - return;
> + 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset), save->eint_fltcon0);
> + pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> + readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset + 4),
> + save->eint_fltcon1);
here
> + 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset);
> + writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> + + 2 * bank->eint_offset + 4);
> + writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> + + bank->eint_offset);
> }
> -
> - 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(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset), save->eint_fltcon0);
> - pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
> - readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset + 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, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset);
> - writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
> - + 2 * bank->eint_offset + 4);
> - writel(save->eint_mask, regs + bank->irq_chip->eint_mask
> - + bank->eint_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> }
>
> -static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvdata,
> - struct samsung_pin_bank *bank)
> +void exynosautov920_pinctrl_resume(struct samsung_pin_bank *bank)
> {
> struct exynos_eint_gpio_save *save = bank->soc_priv;
> void __iomem *regs = bank->eint_base;
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for restoring state\n");
> - return;
> + if (bank->eint_type == EINT_TYPE_GPIO) {
> + /* exynosautov920 has eint_con_offset for all but one bank */
> + if (!bank->eint_con_offset)
> + exynos_pinctrl_resume(bank);
> +
> + pr_debug("%s: con %#010x => %#010x\n", bank->name,
> + readl(regs + bank->pctl_offset + bank->eint_con_offset),
> + save->eint_con);
and here.
Cheers,
A.
next prev parent reply other threads:[~2025-03-06 21:29 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-06 20:42 [PATCH v3 0/4] samsung: pinctrl: Add support for eint_fltcon_offset and filter selection on gs101 Peter Griffin
2025-03-06 20:42 ` [PATCH v3 1/4] pinctrl: samsung: add support for eint_fltcon_offset Peter Griffin
2025-03-06 20:42 ` [PATCH v3 2/4] pinctrl: samsung: add dedicated SoC eint suspend/resume callbacks Peter Griffin
2025-03-06 21:15 ` André Draszik
2025-03-06 21:29 ` André Draszik [this message]
2025-03-06 20:42 ` [PATCH v3 3/4] pinctrl: samsung: add gs101 specific " Peter Griffin
2025-03-06 21:25 ` André Draszik
2025-03-06 20:42 ` [PATCH v3 4/4] pinctrl: samsung: Add filter selection support for alive bank on gs101 Peter Griffin
2025-03-06 21:24 ` André Draszik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c1b8a01c0ef3761a33c9f2530152cb2eab1ff039.camel@linaro.org \
--to=andre.draszik@linaro.org \
--cc=alim.akhtar@samsung.com \
--cc=jaewon02.kim@samsung.com \
--cc=kernel-team@android.com \
--cc=krzk@kernel.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=peter.griffin@linaro.org \
--cc=s.nawrocki@samsung.com \
--cc=semen.protsenko@linaro.org \
--cc=tudor.ambarus@linaro.org \
--cc=willmcvicker@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).