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

[Claudiu.Beznea]

On 04.08.2020 18:08, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 04/08/2020 15:00:38+0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
>>
>>
>> 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.
>>
> 
> So let's prevent users from doing that. Unused arguments are silently
> ignored which is exactly what we want to do.

Can you share what are you thinking about? You want to not parse
atmel.pm_modes for this machines?

> You won't make me believe
> there is actually a use case for swapping the standby and suspend
> meanings.
What i want to say is this:
bootargs contains atmel.pm_modes=ulp0,standby

this leads to
standby_mode=ulp0
suspend_mode=standby

But you want in code to force
standby_mode=standby
suspend_mode=ulp0

The question is: is this what you are thinking this should be done?

> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>