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

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

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?

> 
> 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 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);
>