Re: [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes

[Claudiu.Beznea]

On 04.08.2020 14:42, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello,
> 
> On 04/08/2020 14:07:37+0300, Claudiu Beznea wrote:
>>  void __init at91rm9200_pm_init(void)
>>  {
>> +     static const int modes[] __initconst = {
> 
> You don't need that to be static as it is now local to the function.
> 
>> +             AT91_PM_STANDBY, AT91_PM_ULP0
>> +     };
>> +
>>       if (!IS_ENABLED(CONFIG_SOC_AT91RM9200))
>>               return;
>>
>> +     at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
> 
> For rm9200 and at91sam9, I would not allow changing the pm_modes and
> simply enforce standby_mode = AT91_PM_STANDBY and suspend_mode =
> AT91_PM_ULP0.I don't think you have any user that ever changed that
> behaviour also that avoids increasing the boot time for those slow SoCs.

OK, but bootargs is parsed at a moment when there is no information about
the machine that is running the code. And enforcing this in *_pm_init()
functions for rm9200 and at91sam9 may change suspend and standby mode that
user selected. If there is no user up to this moment there is still the
possibility of being one in the future.

> 
>>       at91_dt_ramc();
>>
>>       /*
>> @@ -838,9 +888,14 @@ void __init at91rm9200_pm_init(void)
>>
>>  void __init sam9x60_pm_init(void)
>>  {
>> +     static const int modes[] __initconst = {
>> +             AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP0_FAST, AT91_PM_ULP1,
>> +     };
>> +
>>       if (!IS_ENABLED(CONFIG_SOC_SAM9X60))
>>               return;
>>
>> +     at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
>>       at91_pm_modes_init();
>>       at91_dt_ramc();
>>       at91_pm_init(at91sam9x60_idle);
>> @@ -851,14 +906,19 @@ void __init sam9x60_pm_init(void)
>>
>>  void __init at91sam9_pm_init(void)
>>  {
>> +     static const int modes[] __initconst = {
>> +             AT91_PM_STANDBY, AT91_PM_ULP0,
>> +     };
>> +
>>       if (!IS_ENABLED(CONFIG_SOC_AT91SAM9))
>>               return;
>>
>> +     at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
>>       at91_dt_ramc();
>>       at91_pm_init(at91sam9_idle);
>>  }
>>
>> -void __init sama5_pm_init(void)
>> +static void __init sama5_pm(void)
>>  {
>>       if (!IS_ENABLED(CONFIG_SOC_SAMA5))
>>               return;
>> @@ -867,13 +927,32 @@ void __init sama5_pm_init(void)
>>       at91_pm_init(NULL);
>>  }
>>
>> +void __init sama5_pm_init(void)
>> +{
>> +     static const int modes[] __initconst = {
>> +             AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP0_FAST,
>> +     };
>> +
>> +     if (!IS_ENABLED(CONFIG_SOC_SAMA5))
>> +             return;
>> +
>> +     at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
>> +     sama5_pm();
>> +}
>> +
>>  void __init sama5d2_pm_init(void)
>>  {
>> +     static const int modes[] __initconst = {
>> +             AT91_PM_STANDBY, AT91_PM_ULP0, AT91_PM_ULP0_FAST, AT91_PM_ULP1,
>> +             AT91_PM_BACKUP,
>> +     };
>> +
>>       if (!IS_ENABLED(CONFIG_SOC_SAMA5D2))
>>               return;
>>
>> +     at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
>>       at91_pm_modes_init();
>> -     sama5_pm_init();
>> +     sama5_pm();
> 
> I would call those two directly:
>         at91_dt_ramc();
>         at91_pm_init(NULL);
> 
> instead of having a function that doesn't do much.
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>