[PATCH] pmdomain: bcm: bcm2835-power: check if the ASB register is equal to enable

[Maira Canal]

Hi Stefan,

On 10/23/23 15:14, Stefan Wahren wrote:
> Hi Maíra,
> 
> Am 23.10.23 um 16:49 schrieb Maíra Canal:
>> The commit c494a447c14e ("soc: bcm: bcm2835-power: Refactor ASB control")
>> refactored the ASB control by using a general function to handle both
>> the enable and disable. But this patch introduced a subtle regression:
>> we need to check if (readl(base + reg) & ASB_ACK) == enable, not just
>> check if (readl(base + reg) & ASB_ACK) == true.
> 
> thanks for finding this regression
> 
>>
>> Currently, this is causing an invalid register state in V3D when
>> unloading and loading the driver, because `bcm2835_asb_disable()` will
>> return -ETIMEDOUT and `bcm2835_asb_power_off()` will fail to disable the
>> ASB slave for V3D.
>>
>> Fixes: c494a447c14e ("soc: bcm: bcm2835-power: Refactor ASB control")
>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>> ---
>>   drivers/pmdomain/bcm/bcm2835-power.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pmdomain/bcm/bcm2835-power.c 
>> b/drivers/pmdomain/bcm/bcm2835-power.c
>> index 1a179d4e011c..2d3636bc277e 100644
>> --- a/drivers/pmdomain/bcm/bcm2835-power.c
>> +++ b/drivers/pmdomain/bcm/bcm2835-power.c
>> @@ -175,7 +175,7 @@ static int bcm2835_asb_control(struct 
>> bcm2835_power *power, u32 reg, bool enable
>>       }
>>       writel(PM_PASSWORD | val, base + reg);
>>
>> -    while (readl(base + reg) & ASB_ACK) {
>> +    while ((readl(base + reg) & ASB_ACK) == enable) {
> 
> but shouldn't it be:
> 
> while (!!(readl(base + reg) & ASB_ACK) == enable) ?

You're correct. Thanks for the correction! I'll send a v2.

Best Regards,
- Maíra

> 
>>           cpu_relax();
>>           if (ktime_get_ns() - start >= 1000)
>>               return -ETIMEDOUT;
>> -- 
>> 2.41.0
>>