[PATCH] stm32mp: psci: Implement PSCI system suspend and DRAM SSR

[Marek Vasut]

On 2/10/22 20:20, Patrick DELAUNAY wrote:

[...]

>> +    writel(RCC_MP_CIFR_WKUPF, STM32_RCC_BASE + RCC_MP_CIFR);
>> +    setbits_le32(STM32_RCC_BASE + RCC_MP_CIER, RCC_MP_CIFR_WKUPF);
>> +
>> +    setbits_le32(STM32_PWR_BASE + PWR_MPUCR,
>> +             PWR_MPUCR_CSSF | PWR_MPUCR_CSTDBYDIS | PWR_MPUCR_PDDS);
>> +
>> +    psci_v7_flush_dcache_all();
>> +    ddr_sr_mode_ssr();
> 
> => here SSR is forced in DDR self refresh entry,
> 
>        it was normal in TF-A which manage ASR and HSR mode are managed
> 
>        but that seens strange for SPL as only SSR mode is correclty 
> managed (DDR_PWRCTL = 0x0)
> 
>        previous mode is not restored after in this code

The ASR should be restored if it was enabled before, indeed.

>        => I think the  ddr_sr_mode_ssr() can be done in DDR driver
> 
>      as it is done in TF-A driver = ./drivers/st/ddr/stm32mp1_ram.c

There is no trace of STM32MP1 ASR/SSR/HSR implementation in ATF :
$ git grep -i [ash]sr drivers/st/ddr/stm32mp1_ram.c
gives no results in
a1f02f4f3 ("Merge "docs(changelog): generate changelog" into integration")
or
8d9c1b3ca ("Merge changes from topic "st-format-signedness" into 
integration").

So back to U-Boot ...

The secure part has no access to DM structures, so no, it is better to 
keep all the bits and pieces of the PSCI in one place. It also makes the 
PSCI code self-contained and easy to understand, as it should be.

As for the various modes:
- ASR is Automatic SR, that's handled by the controller automatically
   even at runtime, and it is enabled in separate patch
   (ram: stm32mp1: Unconditionally enable ASR)
- SSR is Software SR, that's what's implemented here and that's where
   the system software puts the DRAM into SR
- HSR is Hardware SR, what exactly is this useful for ?

It seems ASR is recommended for runtime power saving and SSR for suspend 
power saving. So what is HSR for ?

> static int stm32mp1_ddr_setup(void)
> {
> 
> .....
> 
> 
>      /*
>       * Initialization sequence has configured DDR registers with settings.
>       * The Self Refresh (SR) mode corresponding to these settings has now
>       * to be set.
>       */
>      ddr_set_sr_mode(ddr_read_sr_mode());
> 
>   .....
> 
> }
> 
> => same can be done at the end of  DDr driver in SPL:
> 
>       drivers/ram/stm32mp1/stm32mp1_ddr.c::stm32mp1_ddr_init()
> 
> 
> void stm32mp1_ddr_init(struct ddr_info *priv,
>                 const struct stm32mp1_ddr_config *config)
> {
> 
> ....
> /* 12. set back registers in step 8 to the orginal values if desidered */
>      stm32mp1_refresh_restore(priv->ctl, config->c_reg.rfshctl3,
>                   config->c_reg.pwrctl);
> 
> +   /* Force the Self Refresh (SR) mode = SSR for SPL, HSR or ASR are 
> not supported */
> 
> +   ddr_sr_mode_ssr();
> 
>      /* enable uMCTL2 AXI port 0 and 1 */
> .....
> 
> }
> 
> 
> Then the call of ddr_sr_mode_ssr() can be removed here.
> 
> 
> see also ./drivers/clk/clk_stm32mp1.c::stm32mp1_clktree()
> 
> => Software Self-Refresh mode (SSR) during DDR initilialization

SSR does cut off DRAM access, ASR does not, I think you have those two 
confused, no ? You can enable ASR in the DDR driver (see ram: stm32mp1: 
Unconditionally enable ASR), but you must only enable/disable SSR on 
entry/exit from CStop, i.e. here in this PSCI callback.

>> +    ddr_sw_self_refresh_in();
>> +    setbits_le32(STM32_PWR_BASE + PWR_CR3, PWR_CR3_DDRSREN);
>> +    writel(0x3, STM32_RCC_BASE + RCC_MP_SREQSETR);
>> +
>> +    /* Zzz, enter stop mode */
>> +    asm volatile(
>> +        "wfi\n"
>> +        "isb\n"
>> +        "dsb\n");
> 
> are you sure of the instruction order ?
> 
>      in TF-A, stm32_pwr_down_wfi_load() => the order is dsb / isb / wfi
> 
> and you can be replaced by ARM function = arch/arm/include/asm/system.h

This function also does not exist in either source, sorry.

>      /*
>       * Synchronize on memory accesses and instruction flow before the WFI
>       * instruction.
>       */
>      dsb();
>      isb();
>      wfi();

I think it does make more sense to do ISB/DSB before the WFI, yes.

>> +
>> +    writel(0x3, STM32_RCC_BASE + RCC_MP_SREQCLRR);
>> +    ddr_sw_self_refresh_exit();
>> +
>> +    writel(SYSCFG_CMPCR_MPUEN, STM32_SYSCFG_BASE + SYSCFG_CMPENSETR);
>> +    clrbits_le32(STM32_SYSCFG_BASE + SYSCFG_CMPCR, 
>> SYSCFG_CMPCR_SW_CTRL);
>> +}
> 
> 
> I also need to test this patch on ST boards...

I will send V2 soon.