Re: [PATCH] ARM: at91: pm: change BU Power Switch to automatic mode

[Nicolas Ferre <nicolas.ferre@microchip.com>]

On 02/12/2024 at 09:05, Claudiu Beznea wrote:
> Hi, Nicolas,
> 
> On 25.11.2024 18:56, nicolas.ferre@microchip.com wrote:
>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>
>> Change how the Backup Unit Power is configured and force the
>> automatic/hardware mode.
>> This change eliminates the need for software management of the power
>> switch, ensuring it transitions to the backup power source before
>> entering low power modes.
>>
>> This is done in the only locaton where this swich was configured. It's
> 
> s/locaton/location
> 
>> usually done in the bootloader.
>>
>> Previously, the loss of the VDDANA (or VDDIN33) power source was not
>> automatically compensated by an alternative power source. This resulted
>> in the loss of Backup Unit content, including Backup Self-refresh low
>> power mode information, OTP emulation configuration, and boot
>> configuration, for instance.
> 
> Should we add a fixes for this?

Not so easy to tell as there's a loose dependency with the bootloader. 
But it's true that switching to automatic never harm. So probably yes.

>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> ---
>>   arch/arm/mach-at91/pm.c | 31 ++++++++++++++++++++-----------
>>   1 file changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
>> index b9b995f8a36e..05a1547642b6 100644
>> --- a/arch/arm/mach-at91/pm.c
>> +++ b/arch/arm/mach-at91/pm.c
>> @@ -598,7 +598,21 @@ static int at91_suspend_finish(unsigned long val)
>>        return 0;
>>   }
>>
>> -static void at91_pm_switch_ba_to_vbat(void)
>> +/**
>> + * at91_pm_switch_ba_to_auto() - Configure Backup Unit Power Switch
>> + * to automatic/hardware mode.
>> + *
>> + * The Backup Unit Power Switch can be managed either by software or hardware.
>> + * Enabling hardware mode allows the automatic transition of power between
>> + * VDDANA (or VDDIN33) and VDDBU (or VBAT, respectively), based on the
>> + * availability of these power sources.
>> + *
>> + * If the Backup Unit Power Switch is already in automatic mode, no action is
>> + * required. If it is in software-controlled mode, it is switched to automatic
>> + * mode to enhance safety and eliminate the need for toggling between power
>> + * sources.
>> + */
>> +static void at91_pm_switch_ba_to_auto(void)
>>   {
>>        unsigned int offset = offsetof(struct at91_pm_sfrbu_regs, pswbu);
>>        unsigned int val;
>> @@ -609,24 +623,19 @@ static void at91_pm_switch_ba_to_vbat(void)
>>
>>        val = readl(soc_pm.data.sfrbu + offset);
>>
>> -     /* Already on VBAT. */
>> -     if (!(val & soc_pm.sfrbu_regs.pswbu.state))
>> +     /* Already on auto/hardware. */
>> +     if (!(val & soc_pm.sfrbu_regs.pswbu.ctrl))
>>                return;
>>
>> -     val &= ~soc_pm.sfrbu_regs.pswbu.softsw;
> 
> It seems that softsw and state members of at91_pm_sfrbu_regs.pswbu along
> with their initialization could be dropped. What do you think?

I think that I tried when writing the patch but I think that there's a 
little difference with sama5d2 register layout. Give me a couple more 
days to come back to this and verify.

> I can do it while applying, if any.
> 
> Thank you,
> Claudiu
> 
> 
>> -     val |= soc_pm.sfrbu_regs.pswbu.key | soc_pm.sfrbu_regs.pswbu.ctrl;
>> +     val &= ~soc_pm.sfrbu_regs.pswbu.ctrl;
>> +     val |= soc_pm.sfrbu_regs.pswbu.key;
>>        writel(val, soc_pm.data.sfrbu + offset);
>> -
>> -     /* Wait for update. */
>> -     val = readl(soc_pm.data.sfrbu + offset);
>> -     while (val & soc_pm.sfrbu_regs.pswbu.state)
>> -             val = readl(soc_pm.data.sfrbu + offset);
>>   }
>>
>>   static void at91_pm_suspend(suspend_state_t state)
>>   {
>>        if (soc_pm.data.mode == AT91_PM_BACKUP) {
>> -             at91_pm_switch_ba_to_vbat();
>> +             at91_pm_switch_ba_to_auto();
>>
>>                cpu_suspend(0, at91_suspend_finish);
>>