Replace the open-coded PD status polling with
regmap_read_poll_timeout(). This change simplifies the code without
altering functionality.
Signed-off-by: André Draszik <andre.draszik@linaro.org>
---
drivers/pmdomain/samsung/exynos-pm-domains.c | 29 ++++++++--------------------
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c b/drivers/pmdomain/samsung/exynos-pm-domains.c
index 383126245811cb8e4dbae3b99ced3f06d3093f35..431548ad9a7e40c0a77ac6672081b600c90ddd4e 100644
--- a/drivers/pmdomain/samsung/exynos-pm-domains.c
+++ b/drivers/pmdomain/samsung/exynos-pm-domains.c
@@ -13,7 +13,6 @@
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/pm_domain.h>
-#include <linux/delay.h>
#include <linux/of.h>
#include <linux/pm_runtime.h>
#include <linux/regmap.h>
@@ -35,7 +34,8 @@ struct exynos_pm_domain {
static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
{
struct exynos_pm_domain *pd;
- u32 timeout, pwr;
+ unsigned int val;
+ u32 pwr;
int err;
pd = container_of(domain, struct exynos_pm_domain, pd);
@@ -45,25 +45,12 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
if (err)
return err;
- /* Wait max 1ms */
- timeout = 10;
- 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;
- }
-
- return 0;
- }
-
- if (!err)
- err = -ETIMEDOUT;
- pr_err("Power domain %s %sable failed: %d\n", domain->name,
- power_on ? "en" : "dis", err);
+ err = regmap_read_poll_timeout(pd->regmap, 0x4, val,
+ (val & pd->local_pwr_cfg) == pwr,
+ 100, 1 * USEC_PER_MSEC);
+ if (err)
+ pr_err("Power domain %s %sable failed: %d (%#.2x)\n",
+ domain->name, power_on ? "en" : "dis", err, val);
return err;
}
--
2.51.0.788.g6d19910ace-goog
On 16.10.2025 17:58, André Draszik wrote:
> Replace the open-coded PD status polling with
> regmap_read_poll_timeout(). This change simplifies the code without
> altering functionality.
>
> Signed-off-by: André Draszik <andre.draszik@linaro.org>
> ---
> drivers/pmdomain/samsung/exynos-pm-domains.c | 29 ++++++++--------------------
> 1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c b/drivers/pmdomain/samsung/exynos-pm-domains.c
> index 383126245811cb8e4dbae3b99ced3f06d3093f35..431548ad9a7e40c0a77ac6672081b600c90ddd4e 100644
> --- a/drivers/pmdomain/samsung/exynos-pm-domains.c
> +++ b/drivers/pmdomain/samsung/exynos-pm-domains.c
> @@ -13,7 +13,6 @@
> #include <linux/platform_device.h>
> #include <linux/slab.h>
> #include <linux/pm_domain.h>
> -#include <linux/delay.h>
> #include <linux/of.h>
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> @@ -35,7 +34,8 @@ struct exynos_pm_domain {
> static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
> {
> struct exynos_pm_domain *pd;
> - u32 timeout, pwr;
> + unsigned int val;
> + u32 pwr;
> int err;
>
> pd = container_of(domain, struct exynos_pm_domain, pd);
> @@ -45,25 +45,12 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
> if (err)
> return err;
>
> - /* Wait max 1ms */
> - timeout = 10;
> - 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;
> - }
> -
> - return 0;
> - }
> -
> - if (!err)
> - err = -ETIMEDOUT;
> - pr_err("Power domain %s %sable failed: %d\n", domain->name,
> - power_on ? "en" : "dis", err);
> + err = regmap_read_poll_timeout(pd->regmap, 0x4, val,
> + (val & pd->local_pwr_cfg) == pwr,
> + 100, 1 * USEC_PER_MSEC);
> + if (err)
> + pr_err("Power domain %s %sable failed: %d (%#.2x)\n",
> + domain->name, power_on ? "en" : "dis", err, val);
I've posted my 'tested-by' tag for this patchset, but in meantime I
found that this patch causes regression from time to time on old Exynos
SoCs (especially when all debugs are disabled). It looks that there are
some subtle differences between reading the status register up to 10
times with cpu_relax()+usleep_range() and the
regmap_read_poll_timeout(). I will try to analyze this a bit more and
provide details, but I suspect that the old loop might take a bit longer
than the 1ms from the comment above this code.
> return err;
> }
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On 21.10.2025 22:38, Marek Szyprowski wrote:
> On 16.10.2025 17:58, André Draszik wrote:
>> Replace the open-coded PD status polling with
>> regmap_read_poll_timeout(). This change simplifies the code without
>> altering functionality.
>>
>> Signed-off-by: André Draszik <andre.draszik@linaro.org>
>> ---
>> drivers/pmdomain/samsung/exynos-pm-domains.c | 29
>> ++++++++--------------------
>> 1 file changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c
>> b/drivers/pmdomain/samsung/exynos-pm-domains.c
>> index
>> 383126245811cb8e4dbae3b99ced3f06d3093f35..431548ad9a7e40c0a77ac6672081b600c90ddd4e
>> 100644
>> --- a/drivers/pmdomain/samsung/exynos-pm-domains.c
>> +++ b/drivers/pmdomain/samsung/exynos-pm-domains.c
>> @@ -13,7 +13,6 @@
>> #include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/pm_domain.h>
>> -#include <linux/delay.h>
>> #include <linux/of.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/regmap.h>
>> @@ -35,7 +34,8 @@ struct exynos_pm_domain {
>> static int exynos_pd_power(struct generic_pm_domain *domain, bool
>> power_on)
>> {
>> struct exynos_pm_domain *pd;
>> - u32 timeout, pwr;
>> + unsigned int val;
>> + u32 pwr;
>> int err;
>> pd = container_of(domain, struct exynos_pm_domain, pd);
>> @@ -45,25 +45,12 @@ static int exynos_pd_power(struct
>> generic_pm_domain *domain, bool power_on)
>> if (err)
>> return err;
>> - /* Wait max 1ms */
>> - timeout = 10;
>> - 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;
>> - }
>> -
>> - return 0;
>> - }
>> -
>> - if (!err)
>> - err = -ETIMEDOUT;
>> - pr_err("Power domain %s %sable failed: %d\n", domain->name,
>> - power_on ? "en" : "dis", err);
>> + err = regmap_read_poll_timeout(pd->regmap, 0x4, val,
>> + (val & pd->local_pwr_cfg) == pwr,
>> + 100, 1 * USEC_PER_MSEC);
>> + if (err)
>> + pr_err("Power domain %s %sable failed: %d (%#.2x)\n",
>> + domain->name, power_on ? "en" : "dis", err, val);
>
> I've posted my 'tested-by' tag for this patchset, but in meantime I
> found that this patch causes regression from time to time on old
> Exynos SoCs (especially when all debugs are disabled). It looks that
> there are some subtle differences between reading the status register
> up to 10 times with cpu_relax()+usleep_range() and the
> regmap_read_poll_timeout(). I will try to analyze this a bit more and
> provide details, but I suspect that the old loop might take a bit
> longer than the 1ms from the comment above this code.
It looks that during the early boot all calls to
regmap_read_poll_timeout() lasts exactly 10ms (measured with ktime_get()
and ktime_to_ms()), what means that timekeeping source doesn't provide
resolution high enough for the 1ms timeout. This in turn results in
premature end of regmap_read_poll_timeout() loop after only one cycle of
read+wait+read, what is not always enough for power domain to turn on/off.
According to the commit 7349a69cf312 ("iopoll: Do not use timekeeping in
read_poll_timeout_atomic()"), ktime_get(), which is used also by
regmap_read_poll_timeout(), is not reliable in all contexts, so I think
that this patch should be dropped as there is no easy way to fix this.
The alternative would be to use regmap_read_poll_timeout_atomic(), which
need to be fixed the same way as regmap_read_poll_timeout_atomic() by
the mentioned commit, but in such case we would effectively switch from
usleep to udelay.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
© 2016 - 2025 Red Hat, Inc.