Re: [PATCH v3 06/10] pmdomain: samsung: convert to regmap_read_poll_timeout()

[Marek Szyprowski <m.szyprowski@samsung.com>]

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