[PATCH v2 0/3] AT91 PM improvements

[PATCH v2 0/3] AT91 PM improvements

[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

[PATCH v2 1/3] ARM: at91: pm: add support for ULP0 fast wakeup

[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

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

[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

[PATCH v2 3/3] ARM: at91: pm: of_node_put() after its usage

[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

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

[Alexandre Belloni]

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

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
> 

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

[Alexandre Belloni]

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

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

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

[Alexandre Belloni]

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