On platforms such as Google gs101, direct mmio register access to the
PMU registers doesn't necessarily work and access must happen via a
regmap created by the PMU driver instead.
In preparation for supporting such SoCs convert the existing mmio
accesses to using a regmap wrapper.
With this change in place, a follow-up patch can update the driver to
optionally acquire the PMU-created regmap without having to change the
rest of the code.
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
There is one checkpatch warning, relating to the non-const
regmap_config. It can not easily be made const at this stage, but a
follow-up patch allows us to make it const and the warning will go away
anyway.
v8:
- add missing \n in some of the new log messages
v4:
- rework the loop in exynos_pd_power() slightly, to not return 0 early
to allow more code to be run after pd on/off register write without
changing the loop again, required for gs101.
- add error message in case first regmap write in exynos_pd_power() fails
---
drivers/pmdomain/samsung/exynos-pm-domains.c | 83 +++++++++++++++++++++-------
1 file changed, 62 insertions(+), 21 deletions(-)
diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c b/drivers/pmdomain/samsung/exynos-pm-domains.c
index 5c3aa8983087..68b1e7ba8729 100644
--- a/drivers/pmdomain/samsung/exynos-pm-domains.c
+++ b/drivers/pmdomain/samsung/exynos-pm-domains.c
@@ -9,15 +9,14 @@
// conjunction with runtime-pm. Support for both device-tree and non-device-tree
// based power domain support is included.
-#include <linux/io.h>
#include <linux/err.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/pm_domain.h>
#include <linux/delay.h>
#include <linux/of.h>
-#include <linux/of_address.h>
#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
struct exynos_pm_domain_config {
/* Value for LOCAL_PWR_CFG and STATUS fields for each domain */
@@ -28,7 +27,7 @@ struct exynos_pm_domain_config {
* Exynos specific wrapper around the generic power domain
*/
struct exynos_pm_domain {
- void __iomem *base;
+ struct regmap *regmap;
struct generic_pm_domain pd;
u32 local_pwr_cfg;
};
@@ -36,31 +35,42 @@ struct exynos_pm_domain {
static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
{
struct exynos_pm_domain *pd;
- void __iomem *base;
u32 timeout, pwr;
- char *op;
+ int err;
pd = container_of(domain, struct exynos_pm_domain, pd);
- base = pd->base;
pwr = power_on ? pd->local_pwr_cfg : 0;
- writel_relaxed(pwr, base);
+ err = regmap_write(pd->regmap, 0, pwr);
+ if (err) {
+ pr_err("Regmap write for power domain %s %sable failed: %d\n",
+ domain->name, power_on ? "en" : "dis", err);
+ return err;
+ }
/* Wait max 1ms */
timeout = 10;
-
- while ((readl_relaxed(base + 0x4) & pd->local_pwr_cfg) != pwr) {
- if (!timeout) {
- op = (power_on) ? "enable" : "disable";
- pr_err("Power domain %s %s failed\n", domain->name, op);
- return -ETIMEDOUT;
+ while (timeout-- > 0) {
+ unsigned int val;
+
+ err = regmap_read(pd->regmap, 0x4, &val);
+ if (err || ((val & pd->local_pwr_cfg) != pwr)) {
+ cpu_relax();
+ usleep_range(80, 100);
+ continue;
}
- timeout--;
- cpu_relax();
- usleep_range(80, 100);
+
+ break;
}
- return 0;
+ if (!timeout && !err)
+ /* Only return timeout if no other error also occurred. */
+ err = -ETIMEDOUT;
+ if (err)
+ pr_err("Power domain %s %sable failed: %d\n", domain->name,
+ power_on ? "en" : "dis", err);
+
+ return err;
}
static int exynos_pd_power_on(struct generic_pm_domain *domain)
@@ -109,8 +119,18 @@ static int exynos_pd_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
struct of_phandle_args child, parent;
struct exynos_pm_domain *pd;
+ struct resource *res;
+ void __iomem *base;
+ unsigned int val;
int on, ret;
+ struct regmap_config reg_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .use_relaxed_mmio = true,
+ };
+
pm_domain_cfg = of_device_get_match_data(dev);
pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
if (!pd)
@@ -120,9 +140,26 @@ static int exynos_pd_probe(struct platform_device *pdev)
if (!pd->pd.name)
return -ENOMEM;
- pd->base = of_iomap(np, 0);
- if (!pd->base)
- return -ENODEV;
+ /*
+ * The resource typically points into the address space of the PMU.
+ * Therefore, avoid using devm_platform_get_and_ioremap_resource() and
+ * instead use platform_get_resource() and devm_ioremap() to avoid
+ * conflicts due to address space overlap.
+ */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return dev_err_probe(dev, -ENXIO, "missing IO resources\n");
+
+ base = devm_ioremap(dev, res->start, resource_size(res));
+ if (!base)
+ return dev_err_probe(dev, -ENOMEM,
+ "failed to ioremap PMU regs\n");
+
+ reg_config.max_register = resource_size(res) - reg_config.reg_stride;
+ pd->regmap = devm_regmap_init_mmio(dev, base, ®_config);
+ if (IS_ERR(pd->regmap))
+ return dev_err_probe(dev, PTR_ERR(base),
+ "failed to init regmap\n");
pd->pd.power_off = exynos_pd_power_off;
pd->pd.power_on = exynos_pd_power_on;
@@ -137,7 +174,11 @@ static int exynos_pd_probe(struct platform_device *pdev)
of_device_is_compatible(np, "samsung,exynos4210-pd"))
exynos_pd_power_off(&pd->pd);
- on = readl_relaxed(pd->base + 0x4) & pd->local_pwr_cfg;
+ ret = regmap_read(pd->regmap, 0x4, &val);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to read status\n");
+
+ on = val & pd->local_pwr_cfg;
pm_genpd_init(&pd->pd, NULL, !on);
ret = of_genpd_add_provider_simple(np, &pd->pd);
--
2.53.0.851.ga537e3e6e9-goog
On Wed, 18 Mar 2026 15:27:50 +0000, André Draszik <andre.draszik@linaro.org> wrote: > diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c b/drivers/pmdomain/samsung/exynos-pm-domains.c > index 5c3aa8983087..68b1e7ba8729 100644 > --- a/drivers/pmdomain/samsung/exynos-pm-domains.c > +++ b/drivers/pmdomain/samsung/exynos-pm-domains.c > @@ -120,9 +140,26 @@ static int exynos_pd_probe(struct platform_device *pdev) > [ ... skip 20 lines ... ] > + > + reg_config.max_register = resource_size(res) - reg_config.reg_stride; > + pd->regmap = devm_regmap_init_mmio(dev, base, ®_config); > + if (IS_ERR(pd->regmap)) > + return dev_err_probe(dev, PTR_ERR(base), > + "failed to init regmap\n"); PTR_ERR(pd->regmap) -- Krzysztof Kozlowski <krzk@kernel.org>
On Sat, 2026-03-21 at 14:16 +0100, Krzysztof Kozlowski wrote: > On Wed, 18 Mar 2026 15:27:50 +0000, André Draszik <andre.draszik@linaro.org> wrote: > > diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c b/drivers/pmdomain/samsung/exynos-pm-domains.c > > index 5c3aa8983087..68b1e7ba8729 100644 > > --- a/drivers/pmdomain/samsung/exynos-pm-domains.c > > +++ b/drivers/pmdomain/samsung/exynos-pm-domains.c > > @@ -120,9 +140,26 @@ static int exynos_pd_probe(struct platform_device *pdev) > > [ ... skip 20 lines ... ] > > + > > + reg_config.max_register = resource_size(res) - reg_config.reg_stride; > > + pd->regmap = devm_regmap_init_mmio(dev, base, ®_config); > > + if (IS_ERR(pd->regmap)) > > + return dev_err_probe(dev, PTR_ERR(base), > > + "failed to init regmap\n"); > > PTR_ERR(pd->regmap) Thanks! A.
On Wed, 18 Mar 2026 at 16:28, André Draszik <andre.draszik@linaro.org> wrote:
>
> On platforms such as Google gs101, direct mmio register access to the
> PMU registers doesn't necessarily work and access must happen via a
> regmap created by the PMU driver instead.
>
> In preparation for supporting such SoCs convert the existing mmio
> accesses to using a regmap wrapper.
>
> With this change in place, a follow-up patch can update the driver to
> optionally acquire the PMU-created regmap without having to change the
> rest of the code.
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
[...]
> @@ -36,31 +35,42 @@ struct exynos_pm_domain {
> static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
> {
> struct exynos_pm_domain *pd;
> - void __iomem *base;
> u32 timeout, pwr;
> - char *op;
> + int err;
>
> pd = container_of(domain, struct exynos_pm_domain, pd);
> - base = pd->base;
>
> pwr = power_on ? pd->local_pwr_cfg : 0;
> - writel_relaxed(pwr, base);
> + err = regmap_write(pd->regmap, 0, pwr);
> + if (err) {
> + pr_err("Regmap write for power domain %s %sable failed: %d\n",
> + domain->name, power_on ? "en" : "dis", err);
> + return err;
> + }
>
> /* Wait max 1ms */
> timeout = 10;
> -
> - while ((readl_relaxed(base + 0x4) & pd->local_pwr_cfg) != pwr) {
> - if (!timeout) {
> - op = (power_on) ? "enable" : "disable";
> - pr_err("Power domain %s %s failed\n", domain->name, op);
> - return -ETIMEDOUT;
> + while (timeout-- > 0) {
> + unsigned int val;
> +
> + err = regmap_read(pd->regmap, 0x4, &val);
> + if (err || ((val & pd->local_pwr_cfg) != pwr)) {
> + cpu_relax();
> + usleep_range(80, 100);
> + continue;
> }
> - timeout--;
> - cpu_relax();
> - usleep_range(80, 100);
> +
> + break;
> }
>
[...]
As a follow-up patch on top, please consider converting the open-coded
polling loop above into a readx_poll_timeout_atomic().
That said, the series looks ready to me, but I am awaiting an ack from
a DT maintainer on patch4 before applying.
Kind regards
Uffe
On 19.03.2026 11:13, Ulf Hansson wrote:
> On Wed, 18 Mar 2026 at 16:28, André Draszik <andre.draszik@linaro.org> wrote:
>> On platforms such as Google gs101, direct mmio register access to the
>> PMU registers doesn't necessarily work and access must happen via a
>> regmap created by the PMU driver instead.
>>
>> In preparation for supporting such SoCs convert the existing mmio
>> accesses to using a regmap wrapper.
>>
>> With this change in place, a follow-up patch can update the driver to
>> optionally acquire the PMU-created regmap without having to change the
>> rest of the code.
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> [...]
>
>> @@ -36,31 +35,42 @@ struct exynos_pm_domain {
>> static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>> {
>> struct exynos_pm_domain *pd;
>> - void __iomem *base;
>> u32 timeout, pwr;
>> - char *op;
>> + int err;
>>
>> pd = container_of(domain, struct exynos_pm_domain, pd);
>> - base = pd->base;
>>
>> pwr = power_on ? pd->local_pwr_cfg : 0;
>> - writel_relaxed(pwr, base);
>> + err = regmap_write(pd->regmap, 0, pwr);
>> + if (err) {
>> + pr_err("Regmap write for power domain %s %sable failed: %d\n",
>> + domain->name, power_on ? "en" : "dis", err);
>> + return err;
>> + }
>>
>> /* Wait max 1ms */
>> timeout = 10;
>> -
>> - while ((readl_relaxed(base + 0x4) & pd->local_pwr_cfg) != pwr) {
>> - if (!timeout) {
>> - op = (power_on) ? "enable" : "disable";
>> - pr_err("Power domain %s %s failed\n", domain->name, op);
>> - return -ETIMEDOUT;
>> + while (timeout-- > 0) {
>> + unsigned int val;
>> +
>> + err = regmap_read(pd->regmap, 0x4, &val);
>> + if (err || ((val & pd->local_pwr_cfg) != pwr)) {
>> + cpu_relax();
>> + usleep_range(80, 100);
>> + continue;
>> }
>> - timeout--;
>> - cpu_relax();
>> - usleep_range(80, 100);
>> +
>> + break;
>> }
>>
> [...]
>
> As a follow-up patch on top, please consider converting the open-coded
> polling loop above into a readx_poll_timeout_atomic().
This has been tried and it doesn't work in all cases required for power
domain driver:
https://lore.kernel.org/all/5c19e4ef-c4fd-4bf5-88b3-46c86751b14e@samsung.com/
Probably a comment about that could be added directly to this code to
avoid such conversion and breakage in the future.
> That said, the series looks ready to me, but I am awaiting an ack from
> a DT maintainer on patch4 before applying.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Marek, On Thu, 2026-03-19 at 11:29 +0100, Marek Szyprowski wrote: > On 19.03.2026 11:13, Ulf Hansson wrote: > > As a follow-up patch on top, please consider converting the open-coded > > polling loop above into a readx_poll_timeout_atomic(). > > This has been tried and it doesn't work in all cases required for power > domain driver: > > https://lore.kernel.org/all/5c19e4ef-c4fd-4bf5-88b3-46c86751b14e@samsung.com/ > > Probably a comment about that could be added directly to this code to > avoid such conversion and breakage in the future. I am planning to revisit this in the future and am hoping that we can figure out what goes wrong when using regmap_read_poll_timeout(). Hopefully such a comment would only be short-lived, so maybe not really worth it? I can add it, though, if you prefer. Cheers, Andre'
On 19.03.2026 12:58, André Draszik wrote: > On Thu, 2026-03-19 at 11:29 +0100, Marek Szyprowski wrote: >> On 19.03.2026 11:13, Ulf Hansson wrote: >>> As a follow-up patch on top, please consider converting the open-coded >>> polling loop above into a readx_poll_timeout_atomic(). >> This has been tried and it doesn't work in all cases required for power >> domain driver: >> >> https://lore.kernel.org/all/5c19e4ef-c4fd-4bf5-88b3-46c86751b14e@samsung.com/ >> >> Probably a comment about that could be added directly to this code to >> avoid such conversion and breakage in the future. > I am planning to revisit this in the future and am hoping that we can > figure out what goes wrong when using regmap_read_poll_timeout(). > > Hopefully such a comment would only be short-lived, so maybe not really > worth it? I can add it, though, if you prefer. Well, I think I've already pointed what goes wrong with regmap_read_poll_timeout() in the above mentioned thread. You would need to use regmap_read_poll_timeout_atomic() and modify it the same way as commit 7349a69cf312 did for read_poll_timeout_atomic(). Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
On Thu, 19 Mar 2026 at 16:57, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 19.03.2026 12:58, André Draszik wrote: > > On Thu, 2026-03-19 at 11:29 +0100, Marek Szyprowski wrote: > >> On 19.03.2026 11:13, Ulf Hansson wrote: > >>> As a follow-up patch on top, please consider converting the open-coded > >>> polling loop above into a readx_poll_timeout_atomic(). > >> This has been tried and it doesn't work in all cases required for power > >> domain driver: > >> > >> https://lore.kernel.org/all/5c19e4ef-c4fd-4bf5-88b3-46c86751b14e@samsung.com/ > >> > >> Probably a comment about that could be added directly to this code to > >> avoid such conversion and breakage in the future. > > I am planning to revisit this in the future and am hoping that we can > > figure out what goes wrong when using regmap_read_poll_timeout(). > > > > Hopefully such a comment would only be short-lived, so maybe not really > > worth it? I can add it, though, if you prefer. > > Well, I think I've already pointed what goes wrong with > regmap_read_poll_timeout() in the above mentioned thread. You would need > to use regmap_read_poll_timeout_atomic() and modify it the same way as > commit 7349a69cf312 did for read_poll_timeout_atomic(). Thanks a lot for bringing this to our attention! To me, it looks like the regmap helpers should really use readx_poll_timeout_atomic, rather than open-coding the loop from the regular iopoll helpers. Kind regards Uffe
© 2016 - 2026 Red Hat, Inc.