Re: [PATCH v3 2/4] pinctrl: samsung: add dedicated SoC eint suspend/resume callbacks

From: André Draszik
Date: Thu Mar 06 2025 - 16:30:00 EST

Next message: Fenghua Yu: "Re: [PATCH v7 17/49] x86/resctrl: Rewrite and move the for_each_*_rdt_resource() walkers"
Previous message: Fenghua Yu: "Re: [PATCH v7 16/49] x86/resctrl: Move monitor init work to a resctrl init call"
In reply to: André Draszik: "Re: [PATCH v3 2/4] pinctrl: samsung: add dedicated SoC eint suspend/resume callbacks"
Next in thread: Peter Griffin: "[PATCH v3 3/4] pinctrl: samsung: add gs101 specific eint suspend/resume callbacks"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx/

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 message: Fenghua Yu: "Re: [PATCH v7 17/49] x86/resctrl: Rewrite and move the for_each_*_rdt_resource() walkers"
Previous message: Fenghua Yu: "Re: [PATCH v7 16/49] x86/resctrl: Move monitor init work to a resctrl init call"
In reply to: André Draszik: "Re: [PATCH v3 2/4] pinctrl: samsung: add dedicated SoC eint suspend/resume callbacks"
Next in thread: Peter Griffin: "[PATCH v3 3/4] pinctrl: samsung: add gs101 specific eint suspend/resume callbacks"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]