* [PATCH v2 0/3] AT91 PM improvements @ 2020-08-04 11:07 Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 1/3] ARM: at91: pm: add support for ULP0 fast wakeup Claudiu Beznea ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Claudiu Beznea @ 2020-08-04 11:07 UTC (permalink / raw) To: nicolas.ferre, alexandre.belloni, ludovic.desroches, wenyou.yang Cc: linux-arm-kernel, linux-kernel, Claudiu Beznea Hi, This series adds ULP0 fast mode intended to reduce the suspend/resume time in the detriment of power consumption (patch 1/3). Along with this patch 2/3 adds code to avoid requesting a PM mode not available on platforms not supporting it. Patch 3/3 decrements a device_node refcount after its usage. Thank you, Claudiu Beznea Changes in v2: - in patch 2/3: - move per SoC supported mode arrays in *_pm_init() functions. - use 2 booleans in at91_pm_modes_validate() for modes validation - continue to use array of supported modes in at91rm9200_pm_init() and at91sam9_pm_init() instead of forcing soc_pm.data.standby_mode = AT91_PM_STANDBY and soc_pm.data.suspend_mode = AT91_PM_ULP0 to breaking the user configuration (it might request ulp0 as standby mode or vice versa) Claudiu Beznea (3): ARM: at91: pm: add support for ULP0 fast wakeup ARM: at91: pm: add per soc validation of pm modes ARM: at91: pm: of_node_put() after its usage arch/arm/mach-at91/pm.c | 93 ++++++++++++++++++++++++++++++++++++++--- arch/arm/mach-at91/pm.h | 5 ++- arch/arm/mach-at91/pm_suspend.S | 41 ++++++++++++++++-- 3 files changed, 127 insertions(+), 12 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] ARM: at91: pm: add support for ULP0 fast wakeup 2020-08-04 11:07 [PATCH v2 0/3] AT91 PM improvements Claudiu Beznea @ 2020-08-04 11:07 ` Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 3/3] ARM: at91: pm: of_node_put() after its usage Claudiu Beznea 2 siblings, 0 replies; 9+ messages in thread From: Claudiu Beznea @ 2020-08-04 11:07 UTC (permalink / raw) To: nicolas.ferre, alexandre.belloni, ludovic.desroches, wenyou.yang Cc: linux-arm-kernel, linux-kernel, Claudiu Beznea ULP0 fast improves suspend/resume time with few milliseconds the drawback being the power consumption. The mean values measured for suspend/resume time are as follows (measured on SAMA5D2 Xplained board), ULP0 compared with fast ULP0: - ulp0 fast: suspend time: 169 ms, resume time: 216 ms - ulp0 : suspend time: 197 ms, resume time: 258 ms Current consumption while suspended (measured on SAMA5D2 Xplained board): - ulp0 fast: 730uA - ulp0 : 270uA Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- arch/arm/mach-at91/pm.c | 9 +++++---- arch/arm/mach-at91/pm.h | 5 +++-- arch/arm/mach-at91/pm_suspend.S | 41 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index 074bde64064e..04fdcbd57100 100644 --- a/arch/arm/mach-at91/pm.c +++ b/arch/arm/mach-at91/pm.c @@ -51,10 +51,11 @@ static struct at91_soc_pm soc_pm = { }; static const match_table_t pm_modes __initconst = { - { AT91_PM_STANDBY, "standby" }, - { AT91_PM_ULP0, "ulp0" }, - { AT91_PM_ULP1, "ulp1" }, - { AT91_PM_BACKUP, "backup" }, + { AT91_PM_STANDBY, "standby" }, + { AT91_PM_ULP0, "ulp0" }, + { AT91_PM_ULP0_FAST, "ulp0-fast" }, + { AT91_PM_ULP1, "ulp1" }, + { AT91_PM_BACKUP, "backup" }, { -1, NULL }, }; diff --git a/arch/arm/mach-at91/pm.h b/arch/arm/mach-at91/pm.h index 218e8d1a30fb..bfb260be371e 100644 --- a/arch/arm/mach-at91/pm.h +++ b/arch/arm/mach-at91/pm.h @@ -19,8 +19,9 @@ #define AT91_PM_STANDBY 0x00 #define AT91_PM_ULP0 0x01 -#define AT91_PM_ULP1 0x02 -#define AT91_PM_BACKUP 0x03 +#define AT91_PM_ULP0_FAST 0x02 +#define AT91_PM_ULP1 0x03 +#define AT91_PM_BACKUP 0x04 #ifndef __ASSEMBLY__ struct at91_pm_data { diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S index be9764e8d3fa..0184de05c1be 100644 --- a/arch/arm/mach-at91/pm_suspend.S +++ b/arch/arm/mach-at91/pm_suspend.S @@ -164,7 +164,22 @@ ENDPROC(at91_backup_mode) .macro at91_pm_ulp0_mode ldr pmc, .pmc_base + ldr tmp2, .pm_mode + ldr tmp3, .mckr_offset + + /* Check if ULP0 fast variant has been requested. */ + cmp tmp2, #AT91_PM_ULP0_FAST + bne 0f + + /* Set highest prescaler for power saving */ + ldr tmp1, [pmc, tmp3] + bic tmp1, tmp1, #AT91_PMC_PRES + orr tmp1, tmp1, #AT91_PMC_PRES_64 + str tmp1, [pmc, tmp3] + wait_mckrdy + b 1f +0: /* Turn off the crystal oscillator */ ldr tmp1, [pmc, #AT91_CKGR_MOR] bic tmp1, tmp1, #AT91_PMC_MOSCEN @@ -192,7 +207,18 @@ ENDPROC(at91_backup_mode) /* Wait for interrupt */ 1: at91_cpu_idle - /* Restore RC oscillator state */ + /* Check if ULP0 fast variant has been requested. */ + cmp tmp2, #AT91_PM_ULP0_FAST + bne 5f + + /* Set lowest prescaler for fast resume. */ + ldr tmp1, [pmc, tmp3] + bic tmp1, tmp1, #AT91_PMC_PRES + str tmp1, [pmc, tmp3] + wait_mckrdy + b 6f + +5: /* Restore RC oscillator state */ ldr tmp1, .saved_osc_status tst tmp1, #AT91_PMC_MOSCRCS beq 4f @@ -216,6 +242,7 @@ ENDPROC(at91_backup_mode) str tmp1, [pmc, #AT91_CKGR_MOR] wait_moscrdy +6: .endm /** @@ -473,23 +500,29 @@ ENDPROC(at91_backup_mode) ENTRY(at91_ulp_mode) ldr pmc, .pmc_base ldr tmp2, .mckr_offset + ldr tmp3, .pm_mode /* Save Master clock setting */ ldr tmp1, [pmc, tmp2] str tmp1, .saved_mckr /* - * Set the Master clock source to slow clock + * Set master clock source to: + * - MAINCK if using ULP0 fast variant + * - slow clock, otherwise */ bic tmp1, tmp1, #AT91_PMC_CSS + cmp tmp3, #AT91_PM_ULP0_FAST + bne save_mck + orr tmp1, tmp1, #AT91_PMC_CSS_MAIN +save_mck: str tmp1, [pmc, tmp2] wait_mckrdy at91_plla_disable - ldr r0, .pm_mode - cmp r0, #AT91_PM_ULP1 + cmp tmp3, #AT91_PM_ULP1 beq ulp1_mode at91_pm_ulp0_mode -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes 2020-08-04 11:07 [PATCH v2 0/3] AT91 PM improvements Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 1/3] ARM: at91: pm: add support for ULP0 fast wakeup Claudiu Beznea @ 2020-08-04 11:07 ` Claudiu Beznea 2020-08-04 11:42 ` Alexandre Belloni 2020-08-04 11:07 ` [PATCH v2 3/3] ARM: at91: pm: of_node_put() after its usage Claudiu Beznea 2 siblings, 1 reply; 9+ messages in thread From: Claudiu Beznea @ 2020-08-04 11:07 UTC (permalink / raw) To: nicolas.ferre, alexandre.belloni, ludovic.desroches, wenyou.yang Cc: linux-arm-kernel, linux-kernel, Claudiu Beznea 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(-) 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes 2020-08-04 11:07 ` [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes Claudiu Beznea @ 2020-08-04 11:42 ` Alexandre Belloni 2020-08-04 15:00 ` Claudiu.Beznea 0 siblings, 1 reply; 9+ messages in thread From: Alexandre Belloni @ 2020-08-04 11:42 UTC (permalink / raw) To: Claudiu Beznea Cc: nicolas.ferre, ludovic.desroches, wenyou.yang, linux-arm-kernel, linux-kernel 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. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes 2020-08-04 11:42 ` Alexandre Belloni @ 2020-08-04 15:00 ` Claudiu.Beznea 2020-08-04 15:08 ` Alexandre Belloni 0 siblings, 1 reply; 9+ messages in thread From: Claudiu.Beznea @ 2020-08-04 15:00 UTC (permalink / raw) To: alexandre.belloni Cc: Nicolas.Ferre, Ludovic.Desroches, wenyou.yang, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes 2020-08-04 15:00 ` Claudiu.Beznea @ 2020-08-04 15:08 ` Alexandre Belloni 2020-08-04 15:45 ` Claudiu.Beznea 0 siblings, 1 reply; 9+ messages in thread From: Alexandre Belloni @ 2020-08-04 15:08 UTC (permalink / raw) To: Claudiu.Beznea Cc: Nicolas.Ferre, Ludovic.Desroches, wenyou.yang, linux-arm-kernel, linux-kernel 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. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes 2020-08-04 15:08 ` Alexandre Belloni @ 2020-08-04 15:45 ` Claudiu.Beznea 2020-08-04 15:56 ` Alexandre Belloni 0 siblings, 1 reply; 9+ messages in thread From: Claudiu.Beznea @ 2020-08-04 15:45 UTC (permalink / raw) To: alexandre.belloni Cc: Nicolas.Ferre, Ludovic.Desroches, wenyou.yang, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes 2020-08-04 15:45 ` Claudiu.Beznea @ 2020-08-04 15:56 ` Alexandre Belloni 0 siblings, 0 replies; 9+ messages in thread From: Alexandre Belloni @ 2020-08-04 15:56 UTC (permalink / raw) To: Claudiu.Beznea Cc: Nicolas.Ferre, Ludovic.Desroches, wenyou.yang, linux-arm-kernel, linux-kernel 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. -- Alexandre Belloni, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] ARM: at91: pm: of_node_put() after its usage 2020-08-04 11:07 [PATCH v2 0/3] AT91 PM improvements Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 1/3] ARM: at91: pm: add support for ULP0 fast wakeup Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes Claudiu Beznea @ 2020-08-04 11:07 ` Claudiu Beznea 2 siblings, 0 replies; 9+ messages in thread From: Claudiu Beznea @ 2020-08-04 11:07 UTC (permalink / raw) To: nicolas.ferre, alexandre.belloni, ludovic.desroches, wenyou.yang Cc: linux-arm-kernel, linux-kernel, Claudiu Beznea Put node after it has been used. Fixes: 13f16017d3e3f ("ARM: at91: pm: Tie the USB clock mask to the pmc") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- arch/arm/mach-at91/pm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c index 51902c4c9bb4..e332c6b1548c 100644 --- a/arch/arm/mach-at91/pm.c +++ b/arch/arm/mach-at91/pm.c @@ -841,6 +841,7 @@ static void __init at91_pm_init(void (*pm_idle)(void)) pmc_np = of_find_matching_node_and_match(NULL, atmel_pmc_ids, &of_id); soc_pm.data.pmc = of_iomap(pmc_np, 0); + of_node_put(pmc_np); if (!soc_pm.data.pmc) { pr_err("AT91: PM not supported, PMC not found\n"); return; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-04 15:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-04 11:07 [PATCH v2 0/3] AT91 PM improvements Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 1/3] ARM: at91: pm: add support for ULP0 fast wakeup Claudiu Beznea 2020-08-04 11:07 ` [PATCH v2 2/3] ARM: at91: pm: add per soc validation of pm modes Claudiu Beznea 2020-08-04 11:42 ` Alexandre Belloni 2020-08-04 15:00 ` Claudiu.Beznea 2020-08-04 15:08 ` Alexandre Belloni 2020-08-04 15:45 ` Claudiu.Beznea 2020-08-04 15:56 ` Alexandre Belloni 2020-08-04 11:07 ` [PATCH v2 3/3] ARM: at91: pm: of_node_put() after its usage Claudiu Beznea
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).