| Message ID | 1596539258-20719-3-git-send-email-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AT91 PM improvements | expand |
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.
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 >
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.
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 >
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.
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;
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(-)