| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f7c0101ff26ac0d4ef70a374ae31ee5d23b3c8fc.camel@linaro.org>
Date: Thu, 06 Mar 2025 11:57:35 +0000
From: André Draszik <andre.draszik@...aro.org>
To: Peter Griffin <peter.griffin@...aro.org>, Krzysztof Kozlowski
<krzk@...nel.org>, Sylwester Nawrocki <s.nawrocki@...sung.com>, Alim Akhtar
<alim.akhtar@...sung.com>, Linus Walleij <linus.walleij@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
tudor.ambarus@...aro.org, willmcvicker@...gle.com,
semen.protsenko@...aro.org, kernel-team@...roid.com,
jaewon02.kim@...sung.com
Subject: Re: [PATCH v2 2/4] pinctrl: samsung: add dedicated SoC eint
suspend/resume callbacks
On Sat, 2025-03-01 at 11:43 +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@...aro.org>
> ---
> 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, 111 insertions(+), 143 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index e28fe8177646..57c98d2451b5 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 ac6dc22b37c9..d65a9fba0781 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -761,153 +761,118 @@ __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 = NULL;
This is a useless init and the value is overwritten right away.
>
> - if (clk_enable(bank->drvdata->pclk)) {
> - dev_err(bank->gpio_chip.parent,
> - "unable to enable clock for saving state\n");
> - return;
> + if (bank->eint_type == EINT_TYPE_WKUP) {
> + 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;
> + exynos_set_wakeup(bank);
> +
> + if (bank->eint_type == EINT_TYPE_GPIO) {
Can you swap the order, and make the call to exynos_set_wakeup()
an else, to avoid some needless tests and make it clearer that
set_wakeup() is conditional as well, i.e.:
if (bank->eint_type == EINT_TYPE_GPIO) {
...
} else {
exynos_set_wakeup(bank);
}
What makes it hard to read is that as-is, it looks like the
set_wakeup() is unconditional, while it's also based on
bank->eint_type (which is also more obvious before your patch).
Or maybe even have exynos_set_wakeup() return a bool, as in
the follow-up gs101 patch, there are three tests of this type.
Other than that, I think I looks good.
Cheers,
Andre'
> + 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);
> }
> -
> - 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);
> }
>
> -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);
> - }
> - }
> + exynos_set_wakeup(bank);
> +
> + 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);
> }
> }
>
> -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);
> + 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);
> + pr_debug("%s: mask %#010x => %#010x\n", bank->name,
> + readl(regs + bank->pctl_offset +
> + bank->eint_mask_offset), save->eint_mask);
> +
> + writel(save->eint_con,
> + regs + bank->pctl_offset + bank->eint_con_offset);
> + writel(save->eint_mask,
> + regs + bank->pctl_offset + bank->eint_mask_offset);
> }
> -
> - pr_debug("%s: con %#010x => %#010x\n", bank->name,
> - readl(regs + bank->pctl_offset + bank->eint_con_offset), save->eint_con);
> - pr_debug("%s: mask %#010x => %#010x\n", bank->name,
> - readl(regs + bank->pctl_offset + bank->eint_mask_offset), save->eint_mask);
> -
> - writel(save->eint_con, regs + bank->pctl_offset + bank->eint_con_offset);
> - writel(save->eint_mask, regs + bank->pctl_offset + bank->eint_mask_offset);
> -
> - clk_disable(bank->drvdata->pclk);
> -}
> -
> -void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
> -{
> - struct samsung_pin_bank *bank = drvdata->pin_banks;
> - int i;
> -
> - for (i = 0; i < drvdata->nr_banks; ++i, ++bank)
> - if (bank->eint_type == EINT_TYPE_GPIO) {
> - if (bank->eint_con_offset)
> - exynosauto_pinctrl_resume_bank(drvdata, bank);
> - else
> - exynos_pinctrl_resume_bank(drvdata, bank);
> - }
> }
>
> 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 33df21d5c9d6..35c2bc4ea488 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> @@ -221,8 +221,10 @@ struct exynos_muxed_weint_data {
>
> int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d);
> int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d);
> -void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata);
> -void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata);
> +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);
> struct samsung_retention_ctrl *
> exynos_retention_init(struct samsung_pinctrl_drv_data *drvdata,
> const struct samsung_retention_data *data);
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index 963060920301..375634d8cc79 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -1349,6 +1349,9 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
> const u8 *widths = bank->type->fld_width;
> enum pincfg_type type;
>
> + if (drvdata->suspend)
> + drvdata->suspend(bank);
> +
> /* Registers without a powerdown config aren't lost */
> if (!widths[PINCFG_TYPE_CON_PDN])
> continue;
> @@ -1373,8 +1376,6 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
>
> clk_disable(drvdata->pclk);
>
> - if (drvdata->suspend)
> - drvdata->suspend(drvdata);
> if (drvdata->retention_ctrl && drvdata->retention_ctrl->enable)
> drvdata->retention_ctrl->enable(drvdata);
>
> @@ -1406,9 +1407,6 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> return ret;
> }
>
> - if (drvdata->resume)
> - drvdata->resume(drvdata);
> -
> for (i = 0; i < drvdata->nr_banks; i++) {
> struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
> void __iomem *reg = bank->pctl_base + bank->pctl_offset;
> @@ -1416,6 +1414,9 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
> const u8 *widths = bank->type->fld_width;
> enum pincfg_type type;
>
> + if (drvdata->resume)
> + drvdata->resume(bank);
> +
> /* Registers without a powerdown config aren't lost */
> if (!widths[PINCFG_TYPE_CON_PDN])
> continue;
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> index 371e4f02bbfb..e939e5bb0347 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> @@ -285,8 +285,8 @@ struct samsung_pin_ctrl {
> int (*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
> int (*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
> void (*pud_value_init)(struct samsung_pinctrl_drv_data *drvdata);
> - void (*suspend)(struct samsung_pinctrl_drv_data *);
> - void (*resume)(struct samsung_pinctrl_drv_data *);
> + void (*suspend)(struct samsung_pin_bank *bank);
> + void (*resume)(struct samsung_pin_bank *bank);
> };
>
> /**
> @@ -335,8 +335,8 @@ struct samsung_pinctrl_drv_data {
>
> struct samsung_retention_ctrl *retention_ctrl;
>
> - void (*suspend)(struct samsung_pinctrl_drv_data *);
> - void (*resume)(struct samsung_pinctrl_drv_data *);
> + void (*suspend)(struct samsung_pin_bank *bank);
> + void (*resume)(struct samsung_pin_bank *bank);
> };
>
> /**
>
Powered by blists - more mailing lists