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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9b9f5daa-2456-4ad0-9e56-a6978c35b197@tuxon.dev>
Date: Fri, 6 Dec 2024 16:24:19 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Cristian Birsan <cristian.birsan@...rochip.com>
Subject: Re: [PATCH] ARM: at91: pm: change BU Power Switch to automatic mode

Hi, Nicolas,

On 06.12.2024 16:14, Nicolas Ferre wrote:
> Claudiu,
> 
> On 02/12/2024 at 17:44, Nicolas Ferre wrote:
>> On 02/12/2024 at 09:05, Claudiu Beznea wrote:
>>> Hi, Nicolas,
>>>
>>> On 25.11.2024 18:56, nicolas.ferre@...rochip.com wrote:
>>>> From: Nicolas Ferre <nicolas.ferre@...rochip.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@...rochip.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.
> 
> Ok, I remember now: I was wondering if I needed to remove the whole
> sfrbu_regs.xxx mechanism and define more generically the content of
> include/soc/at91/sama7-sfrbu.h for sama5d2, but if we need one day to use
> the STATE bit or even the SMCTRL bit of sama5d2, then it should be kept.
> 
> So, now that the mechanism is in place, I would prefer that we keep it:
> okay for you?

OK

> 
> Do you want me to re-spin a v2 for the rest?

No, I can handle the insignificant typo while applying.

Thank you,
Claudiu

> 
> Best regards,
>   Nicolas
> 
>>> 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);
>>>>
>>
>>
>