[v2,2/3] ARM: at91: pm: add per soc validation of pm modes
diff mbox series

Message ID 1596539258-20719-3-git-send-email-claudiu.beznea@microchip.com
State New
Headers show
Series
  • AT91 PM improvements
Related show

Commit Message

Claudiu Beznea Aug. 4, 2020, 11:07 a.m. UTC
Not all SoCs supports all the PM mode. User may end up settings,
e.g. backup mode, on a non SAMA5D2 device, but the mode to not be valid.
If backup mode is used on a devices not supporting it there will be no
way of resuming other than rebooting.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 arch/arm/mach-at91/pm.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 81 insertions(+), 2 deletions(-)

Comments

Alexandre Belloni Aug. 4, 2020, 11:42 a.m. UTC | #1
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.

>  	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.
Claudiu Beznea Aug. 4, 2020, 3 p.m. UTC | #2
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
>
Alexandre Belloni Aug. 4, 2020, 3:08 p.m. UTC | #3
On 04/08/2020 15:00:38+0000, Claudiu.Beznea@microchip.com 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. You won't make me believe
there is actually a use case for swapping the standby and suspend
meanings.
Claudiu Beznea Aug. 4, 2020, 3:45 p.m. UTC | #4
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@microchip.com 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
>
Alexandre Belloni Aug. 4, 2020, 3:56 p.m. UTC | #5
On 04/08/2020 15:45:40+0000, Claudiu.Beznea@microchip.com wrote:
> 
> 
> 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@microchip.com 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?
> 

Well, as you said, when parsing we don't know on which machine we are
running so let's keep parsing it anyway.

> > 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?
> 

Yes, I think we need to enforce standby_mode=standby and
suspend_mode=ulp0 for rm9200 and at91sam9. This is how it always have
been.

You have two ways of doing that:

Etiher you enforce the values after parsing, in at91rm9200_pm_init and
at91sam9_pm_init. Or, when parsing your store the values in a different
location than soc_pm.data and update soc_pm.data only in sama5_pm_init,
sama5d2_pm_init and sam9x60_pm_init. I feel like the first solution is
easier.

Patch
diff mbox series

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 04fdcbd57100..51902c4c9bb4 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -785,6 +785,51 @@  static const struct of_device_id atmel_pmc_ids[] __initconst = {
 	{ /* sentinel */ },
 };
 
+static void __init at91_pm_modes_validate(const int *modes, int len)
+{
+	u8 i, standby = 0, suspend = 0;
+	int mode;
+
+	for (i = 0; i < len; i++) {
+		if (standby && suspend)
+			break;
+
+		if (modes[i] == soc_pm.data.standby_mode && !standby) {
+			standby = 1;
+			continue;
+		}
+
+		if (modes[i] == soc_pm.data.suspend_mode && !suspend) {
+			suspend = 1;
+			continue;
+		}
+	}
+
+	if (!standby) {
+		if (soc_pm.data.suspend_mode == AT91_PM_STANDBY)
+			mode = AT91_PM_ULP0;
+		else
+			mode = AT91_PM_STANDBY;
+
+		pr_warn("AT91: PM: %s mode not supported! Using %s.\n",
+			pm_modes[soc_pm.data.standby_mode].pattern,
+			pm_modes[mode].pattern);
+		soc_pm.data.standby_mode = mode;
+	}
+
+	if (!suspend) {
+		if (soc_pm.data.standby_mode == AT91_PM_ULP0)
+			mode = AT91_PM_STANDBY;
+		else
+			mode = AT91_PM_ULP0;
+
+		pr_warn("AT91: PM: %s mode not supported! Using %s.\n",
+			pm_modes[soc_pm.data.suspend_mode].pattern,
+			pm_modes[mode].pattern);
+		soc_pm.data.suspend_mode = mode;
+	}
+}
+
 static void __init at91_pm_init(void (*pm_idle)(void))
 {
 	struct device_node *pmc_np;
@@ -823,9 +868,14 @@  static void __init at91_pm_init(void (*pm_idle)(void))
 
 void __init at91rm9200_pm_init(void)
 {
+	static const int modes[] __initconst = {
+		AT91_PM_STANDBY, AT91_PM_ULP0
+	};
+
 	if (!IS_ENABLED(CONFIG_SOC_AT91RM9200))
 		return;
 
+	at91_pm_modes_validate(modes, ARRAY_SIZE(modes));
 	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();
 
 	soc_pm.ws_ids = sama5d2_ws_ids;
 	soc_pm.config_shdwc_ws = at91_sama5d2_config_shdwc_ws;