[PATCH] ram: stm32mp1: Conditionally enable ASR

[Patrick DELAUNAY]

On 4/26/22 12:49, Patrick DELAUNAY wrote:
> Hi Marek,
>
> On 4/14/22 19:34, Marek Vasut wrote:
>> On 4/14/22 18:48, Marek Vasut wrote:
>>> On 4/14/22 18:37, Patrick DELAUNAY wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>>> on ST platform the ASR/SSR/HSR request are already provided by the 
>>>> DDR settings with pwrctl register value
>>>>
>>>> it is managed in TF-A by
>>>>
>>>> arm-trusted-firmware/drivers/st/ddr/stm32mp1_ddr_helpers.c
>>>
>>> Sure, I don't use ATF and I have no intention of ever using ATF on 
>>> this platform.
>
>
> Yes i know.
>
>
>>>
>>>> enumstm32mp1_ddr_sr_mode ddr_read_sr_mode(void)
>>>> {
>>>> uint32_tpwrctl = mmio_read_32(stm32mp_ddrctrl_base() + 
>>>> DDRCTRL_PWRCTL);
>>>> switch(pwrctl & (DDRCTRL_PWRCTL_EN_DFI_DRAM_CLK_DISABLE |
>>>> DDRCTRL_PWRCTL_SELFREF_EN)) {
>>>> case0U:
>>>> returnDDR_SSR_MODE;
>>>> caseDDRCTRL_PWRCTL_EN_DFI_DRAM_CLK_DISABLE:
>>>> returnDDR_HSR_MODE;
>>>> caseDDRCTRL_PWRCTL_EN_DFI_DRAM_CLK_DISABLE | 
>>>> DDRCTRL_PWRCTL_SELFREF_EN:
>>>> returnDDR_ASR_MODE;
>>>> default:
>>>> returnDDR_SR_MODE_INVALID;
>>>> }
>>>> }
>>>>
>>>> no need to add an other property
>>>
>>> This is for U-Boot, plain, stock, without any other software 
>>> partaking in it.
>>
>> Note that this patch just reinstates the old behavior before v2022.04 
>> release, except it adds a DT property to enable the new behavior with 
>> ASR and makes it non-default.
>
>
> I agree that ASR can't be activated by default to avoid issue, so the 
> previous shortcut
>
> (always activate ASR in SPL) must be changed.
>
>
> Today my concerns is only the binding.
>
> The same binding for DDR sub system (control and PHY) must be shared 
> by all the bootloader; It is my request.
>
> in stm32mp1_ddr_setup() = ddr_set_sr_mode(ddr_read_sr_mode());
>
> The ASR activation is already managed by existing binding with the 
> pwrctl register value => DDR_PWRCTL in ./arch/arm/dts/stm32mp15-ddr.dtsi
>
> as it is explained in ST Microelectronics documentation (AN5168: DDR 
> configuration on STM32MP1 Series MPUs) and managed by the tool CubeMX 
> and TF-A driver.
>
>
> I propose to do the same, by using the same/existing binding, so the 
> same DDR configuration file
>
> generated by CubeMX tools can be used of TF-A and SPL
>
> => ASR deactivated by default when the
>
> in stm32mp1_ddr_setup() = ddr_set_sr_mode(ddr_read_sr_mode());
>
> requested pwrctl value is 0x0 (default today)
>
> => ASR is activated when it is request with pwrctl value
>
> So in futur the HSR mode could be also added, as it is done today in 
> TF-A stm32mp1_ddr_setup() =
>     ddr_set_sr_mode(ddr_read_sr_mode());
>
>
>> diff --git a/drivers/ram/stm32mp1/stm32mp1_ddr.c 
>> b/drivers/ram/stm32mp1/stm32mp1_ddr.c
>> index 528a171b454..fd11e02aff4 100644
>> --- a/drivers/ram/stm32mp1/stm32mp1_ddr.c
>> +++ b/drivers/ram/stm32mp1/stm32mp1_ddr.c
>> @@ -845,7 +845,8 @@ start:
>>                   config->c_reg.pwrctl);
>>
>>  /* Enable auto-self-refresh, which saves a bit of power at runtime. */
>> -    stm32mp1_asr_enable(priv);
>> +    if (config->info.enable_asr)
>> +        stm32mp1_asr_enable(priv);
>
> I propose (based on TF-A code = ddr_read_sr_mode())
>
>
> /* Enable auto-self-refresh, which saves a bit of power at runtime. */
> -    stm32mp1_asr_enable(priv);
>
> + if (config->c_reg.pwrctl & (DDRCTRL_PWRCTL_EN_DFI_DRAM_CLK_DISABLE | 
> DDRCTRL_PWRCTL_SELFREF_EN) == DDRCTRL_PWRCTL_EN_DFI_DRAM_CLK_DISABLE | 
> DDRCTRL_PWRCTL_SELFREF_EN)
>
> + stm32mp1_asr_enable(priv);
>
>
> With
>
> #define DDRCTRL_PWRCTL_EN_DFI_DRAM_CLK_DISABLE    BIT(3)
>
> If one board need to activate the ASR mode, the associated field in 
> the DDR node need to be change, with:
>
> #define DDR_PWRCTL 0x00000009
>
>
> Regards,
>
>
> Patrick
>