linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
@ 2018-08-22  5:32 Keerthy
  2018-08-22  7:34 ` Johan Hovold
  0 siblings, 1 reply; 8+ messages in thread
From: Keerthy @ 2018-08-22  5:32 UTC (permalink / raw)
  To: tony
  Cc: ssantosh, linux-kernel, linux-omap, linux-arm-kernel, d-gerlach,
	j-keerthy, t-kristo

Enable DS0 for only those platforms on which it is functional

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 arch/arm/mach-omap2/pm33xx-core.c    | 5 +++++
 drivers/soc/ti/pm33xx.c              | 9 +++++++++
 include/linux/platform_data/pm33xx.h | 2 ++
 3 files changed, 16 insertions(+)

diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
index f4971e4..f0f6e8e 100644
--- a/arch/arm/mach-omap2/pm33xx-core.c
+++ b/arch/arm/mach-omap2/pm33xx-core.c
@@ -135,6 +135,11 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
 {
 	int ret = 0;
 
+	if (!(args & WFI_FLAG_DEEP_SLEEP0)) {
+		pr_err("DS0 mode not supported\n");
+		return -ENOTSUPP;
+	}
+
 	amx3_pre_suspend_common();
 	scu_power_mode(scu_base, SCU_PM_POWEROFF);
 	ret = cpu_suspend(args, fn);
diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
index d0dab32..53238d7 100644
--- a/drivers/soc/ti/pm33xx.c
+++ b/drivers/soc/ti/pm33xx.c
@@ -324,6 +324,15 @@ static int am33xx_pm_probe(struct platform_device *pdev)
 	suspend_wfi_flags |= WFI_FLAG_SAVE_EMIF;
 	suspend_wfi_flags |= WFI_FLAG_WAKE_M3;
 
+	/*
+	 * Deep Sleep0 mode is currently functional only on am437x-gp-evm,
+	 * am33xx-evm and boneblack family. Hence set the DS0 flag
+	 */
+	if (of_machine_is_compatible("ti,am437x-gp-evm") ||
+	    of_machine_is_compatible("ti,am335x-bone-black") ||
+	    of_machine_is_compatible("ti,am335x-evm"))
+		suspend_wfi_flags |= WFI_FLAG_DEEP_SLEEP0;
+
 	ret = pm_ops->init();
 	if (ret) {
 		dev_err(dev, "Unable to call core pm init!\n");
diff --git a/include/linux/platform_data/pm33xx.h b/include/linux/platform_data/pm33xx.h
index fbf5ed7..5c54b64 100644
--- a/include/linux/platform_data/pm33xx.h
+++ b/include/linux/platform_data/pm33xx.h
@@ -28,12 +28,14 @@
  * WFI_FLAG_WAKE_M3: Disable MPU clock or clockdomain to cause wkup_m3 to
  *		     execute when WFI instruction executes.
  * WFI_FLAG_RTC_ONLY: Configure the RTC to enter RTC+DDR mode.
+ * WFI_FLAG_DEEP_SLEEP0: Configure to enter Depp Sleep 0 mode.
  */
 #define WFI_FLAG_FLUSH_CACHE		BIT(0)
 #define WFI_FLAG_SELF_REFRESH		BIT(1)
 #define WFI_FLAG_SAVE_EMIF		BIT(2)
 #define WFI_FLAG_WAKE_M3		BIT(3)
 #define WFI_FLAG_RTC_ONLY		BIT(4)
+#define WFI_FLAG_DEEP_SLEEP0		BIT(5)
 
 #ifndef __ASSEMBLER__
 struct am33xx_pm_sram_addr {
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22  5:32 [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional Keerthy
@ 2018-08-22  7:34 ` Johan Hovold
  2018-08-22  7:37   ` Johan Hovold
  0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2018-08-22  7:34 UTC (permalink / raw)
  To: Keerthy
  Cc: tony, d-gerlach, linux-kernel, t-kristo, ssantosh, linux-omap,
	linux-arm-kernel

On Wed, Aug 22, 2018 at 11:02:31AM +0530, Keerthy wrote:
> Enable DS0 for only those platforms on which it is functional
> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  arch/arm/mach-omap2/pm33xx-core.c    | 5 +++++
>  drivers/soc/ti/pm33xx.c              | 9 +++++++++
>  include/linux/platform_data/pm33xx.h | 2 ++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
> index f4971e4..f0f6e8e 100644
> --- a/arch/arm/mach-omap2/pm33xx-core.c
> +++ b/arch/arm/mach-omap2/pm33xx-core.c
> @@ -135,6 +135,11 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>  {
>  	int ret = 0;
>  
> +	if (!(args & WFI_FLAG_DEEP_SLEEP0)) {
> +		pr_err("DS0 mode not supported\n");
> +		return -ENOTSUPP;
> +	}
> +
>  	amx3_pre_suspend_common();
>  	scu_power_mode(scu_base, SCU_PM_POWEROFF);
>  	ret = cpu_suspend(args, fn);
> diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
> index d0dab32..53238d7 100644
> --- a/drivers/soc/ti/pm33xx.c
> +++ b/drivers/soc/ti/pm33xx.c
> @@ -324,6 +324,15 @@ static int am33xx_pm_probe(struct platform_device *pdev)
>  	suspend_wfi_flags |= WFI_FLAG_SAVE_EMIF;
>  	suspend_wfi_flags |= WFI_FLAG_WAKE_M3;
>  
> +	/*
> +	 * Deep Sleep0 mode is currently functional only on am437x-gp-evm,
> +	 * am33xx-evm and boneblack family. Hence set the DS0 flag
> +	 */
> +	if (of_machine_is_compatible("ti,am437x-gp-evm") ||
> +	    of_machine_is_compatible("ti,am335x-bone-black") ||
> +	    of_machine_is_compatible("ti,am335x-evm"))
> +		suspend_wfi_flags |= WFI_FLAG_DEEP_SLEEP0;

What about other (out-of-tree) machines which supports DS0 and which
this change would break?

I think this needs to be a blacklist if anything.

Please also expand in the commit message why you think this is needed.

Last, what tree is this against? There's no am43xx_suspend() in
linux-next (and you add compatibles above for am33xx too).

Thanks,
Johan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22  7:34 ` Johan Hovold
@ 2018-08-22  7:37   ` Johan Hovold
  2018-08-22  8:20     ` J, KEERTHY
  0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2018-08-22  7:37 UTC (permalink / raw)
  To: Keerthy
  Cc: tony, d-gerlach, linux-kernel, t-kristo, ssantosh, linux-omap,
	linux-arm-kernel

On Wed, Aug 22, 2018 at 09:34:09AM +0200, Johan Hovold wrote:
> On Wed, Aug 22, 2018 at 11:02:31AM +0530, Keerthy wrote:
> > Enable DS0 for only those platforms on which it is functional
> > 
> > Signed-off-by: Keerthy <j-keerthy@ti.com>
> > ---
> >  arch/arm/mach-omap2/pm33xx-core.c    | 5 +++++
> >  drivers/soc/ti/pm33xx.c              | 9 +++++++++
> >  include/linux/platform_data/pm33xx.h | 2 ++
> >  3 files changed, 16 insertions(+)
> > 
> > diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
> > index f4971e4..f0f6e8e 100644
> > --- a/arch/arm/mach-omap2/pm33xx-core.c
> > +++ b/arch/arm/mach-omap2/pm33xx-core.c
> > @@ -135,6 +135,11 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
> >  {
> >  	int ret = 0;
> >  
> > +	if (!(args & WFI_FLAG_DEEP_SLEEP0)) {
> > +		pr_err("DS0 mode not supported\n");
> > +		return -ENOTSUPP;
> > +	}
> > +
> >  	amx3_pre_suspend_common();
> >  	scu_power_mode(scu_base, SCU_PM_POWEROFF);
> >  	ret = cpu_suspend(args, fn);
> > diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
> > index d0dab32..53238d7 100644
> > --- a/drivers/soc/ti/pm33xx.c
> > +++ b/drivers/soc/ti/pm33xx.c
> > @@ -324,6 +324,15 @@ static int am33xx_pm_probe(struct platform_device *pdev)
> >  	suspend_wfi_flags |= WFI_FLAG_SAVE_EMIF;
> >  	suspend_wfi_flags |= WFI_FLAG_WAKE_M3;
> >  
> > +	/*
> > +	 * Deep Sleep0 mode is currently functional only on am437x-gp-evm,
> > +	 * am33xx-evm and boneblack family. Hence set the DS0 flag
> > +	 */
> > +	if (of_machine_is_compatible("ti,am437x-gp-evm") ||
> > +	    of_machine_is_compatible("ti,am335x-bone-black") ||
> > +	    of_machine_is_compatible("ti,am335x-evm"))
> > +		suspend_wfi_flags |= WFI_FLAG_DEEP_SLEEP0;
> 
> What about other (out-of-tree) machines which supports DS0 and which
> this change would break?
> 
> I think this needs to be a blacklist if anything.
> 
> Please also expand in the commit message why you think this is needed.
> 
> Last, what tree is this against? There's no am43xx_suspend() in
> linux-next (and you add compatibles above for am33xx too).

Sorry, there is indeed an am43xx_suspend(), but you are adding
compatibles for am33xx which use am33xx_suspend().

Johan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22  7:37   ` Johan Hovold
@ 2018-08-22  8:20     ` J, KEERTHY
  2018-08-22  8:43       ` Johan Hovold
  0 siblings, 1 reply; 8+ messages in thread
From: J, KEERTHY @ 2018-08-22  8:20 UTC (permalink / raw)
  To: Johan Hovold
  Cc: tony, d-gerlach, linux-kernel, t-kristo, ssantosh, linux-omap,
	linux-arm-kernel



On 8/22/2018 1:07 PM, Johan Hovold wrote:
> On Wed, Aug 22, 2018 at 09:34:09AM +0200, Johan Hovold wrote:
>> On Wed, Aug 22, 2018 at 11:02:31AM +0530, Keerthy wrote:
>>> Enable DS0 for only those platforms on which it is functional
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> ---
>>>   arch/arm/mach-omap2/pm33xx-core.c    | 5 +++++
>>>   drivers/soc/ti/pm33xx.c              | 9 +++++++++
>>>   include/linux/platform_data/pm33xx.h | 2 ++
>>>   3 files changed, 16 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
>>> index f4971e4..f0f6e8e 100644
>>> --- a/arch/arm/mach-omap2/pm33xx-core.c
>>> +++ b/arch/arm/mach-omap2/pm33xx-core.c
>>> @@ -135,6 +135,11 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>>>   {
>>>   	int ret = 0;
>>>   
>>> +	if (!(args & WFI_FLAG_DEEP_SLEEP0)) {
>>> +		pr_err("DS0 mode not supported\n");
>>> +		return -ENOTSUPP;
>>> +	}
>>> +
>>>   	amx3_pre_suspend_common();
>>>   	scu_power_mode(scu_base, SCU_PM_POWEROFF);
>>>   	ret = cpu_suspend(args, fn);
>>> diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
>>> index d0dab32..53238d7 100644
>>> --- a/drivers/soc/ti/pm33xx.c
>>> +++ b/drivers/soc/ti/pm33xx.c
>>> @@ -324,6 +324,15 @@ static int am33xx_pm_probe(struct platform_device *pdev)
>>>   	suspend_wfi_flags |= WFI_FLAG_SAVE_EMIF;
>>>   	suspend_wfi_flags |= WFI_FLAG_WAKE_M3;
>>>   
>>> +	/*
>>> +	 * Deep Sleep0 mode is currently functional only on am437x-gp-evm,
>>> +	 * am33xx-evm and boneblack family. Hence set the DS0 flag
>>> +	 */
>>> +	if (of_machine_is_compatible("ti,am437x-gp-evm") ||
>>> +	    of_machine_is_compatible("ti,am335x-bone-black") ||
>>> +	    of_machine_is_compatible("ti,am335x-evm"))
>>> +		suspend_wfi_flags |= WFI_FLAG_DEEP_SLEEP0;
>>
>> What about other (out-of-tree) machines which supports DS0 and which
>> this change would break?
>>
>> I think this needs to be a blacklist if anything.
>>
>> Please also expand in the commit message why you think this is needed.

Currently when one does echo mem > /sys/power/state on unsuppored 
machines there can be a crash or a hang. So bail out with a message.

>>
>> Last, what tree is this against? There's no am43xx_suspend() in
>> linux-next (and you add compatibles above for am33xx too).
> 
> Sorry, there is indeed an am43xx_suspend(), but you are adding
> compatibles for am33xx which use am33xx_suspend().

am33xx_pm_probe is a common probe function for both am33 and am43.
AFAIK for am33 family am335x-evm and am335x-bone-black support Deep 
Sleep mode. For am43 family am43tx-gp-evm alone supports at the moment.

Can you let me know of other am33 machines that support DS0 mode?
I could have simply used ti,am33xx compatible which covers entire am33 
family but then am33xx-bone (bone white) does not support this mode.

> 
> Johan
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22  8:20     ` J, KEERTHY
@ 2018-08-22  8:43       ` Johan Hovold
  2018-08-22 11:07         ` J, KEERTHY
  0 siblings, 1 reply; 8+ messages in thread
From: Johan Hovold @ 2018-08-22  8:43 UTC (permalink / raw)
  To: J, KEERTHY
  Cc: Johan Hovold, tony, d-gerlach, linux-kernel, t-kristo, ssantosh,
	linux-omap, linux-arm-kernel

On Wed, Aug 22, 2018 at 01:50:29PM +0530, J, KEERTHY wrote:
> 
> 
> On 8/22/2018 1:07 PM, Johan Hovold wrote:
> > On Wed, Aug 22, 2018 at 09:34:09AM +0200, Johan Hovold wrote:
> >> On Wed, Aug 22, 2018 at 11:02:31AM +0530, Keerthy wrote:
> >>> Enable DS0 for only those platforms on which it is functional
> >>>
> >>> Signed-off-by: Keerthy <j-keerthy@ti.com>
> >>> ---
> >>>   arch/arm/mach-omap2/pm33xx-core.c    | 5 +++++
> >>>   drivers/soc/ti/pm33xx.c              | 9 +++++++++
> >>>   include/linux/platform_data/pm33xx.h | 2 ++
> >>>   3 files changed, 16 insertions(+)
> >>>
> >>> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
> >>> index f4971e4..f0f6e8e 100644
> >>> --- a/arch/arm/mach-omap2/pm33xx-core.c
> >>> +++ b/arch/arm/mach-omap2/pm33xx-core.c
> >>> @@ -135,6 +135,11 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
> >>>   {
> >>>   	int ret = 0;
> >>>   
> >>> +	if (!(args & WFI_FLAG_DEEP_SLEEP0)) {
> >>> +		pr_err("DS0 mode not supported\n");
> >>> +		return -ENOTSUPP;
> >>> +	}
> >>> +
> >>>   	amx3_pre_suspend_common();
> >>>   	scu_power_mode(scu_base, SCU_PM_POWEROFF);
> >>>   	ret = cpu_suspend(args, fn);
> >>> diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
> >>> index d0dab32..53238d7 100644
> >>> --- a/drivers/soc/ti/pm33xx.c
> >>> +++ b/drivers/soc/ti/pm33xx.c
> >>> @@ -324,6 +324,15 @@ static int am33xx_pm_probe(struct platform_device *pdev)
> >>>   	suspend_wfi_flags |= WFI_FLAG_SAVE_EMIF;
> >>>   	suspend_wfi_flags |= WFI_FLAG_WAKE_M3;
> >>>   
> >>> +	/*
> >>> +	 * Deep Sleep0 mode is currently functional only on am437x-gp-evm,
> >>> +	 * am33xx-evm and boneblack family. Hence set the DS0 flag
> >>> +	 */
> >>> +	if (of_machine_is_compatible("ti,am437x-gp-evm") ||
> >>> +	    of_machine_is_compatible("ti,am335x-bone-black") ||
> >>> +	    of_machine_is_compatible("ti,am335x-evm"))
> >>> +		suspend_wfi_flags |= WFI_FLAG_DEEP_SLEEP0;
> >>
> >> What about other (out-of-tree) machines which supports DS0 and which
> >> this change would break?
> >>
> >> I think this needs to be a blacklist if anything.
> >>
> >> Please also expand in the commit message why you think this is needed.
> 
> Currently when one does echo mem > /sys/power/state on unsuppored 
> machines there can be a crash or a hang. So bail out with a message.

Yes, but why is this unsupported on some machines? Which machines, and
why? Your commit messages should be self-contained and hold the
information needed to determine whether your patch makes sense in the
first place.

> >> Last, what tree is this against? There's no am43xx_suspend() in
> >> linux-next (and you add compatibles above for am33xx too).
> > 
> > Sorry, there is indeed an am43xx_suspend(), but you are adding
> > compatibles for am33xx which use am33xx_suspend().
> 
> am33xx_pm_probe is a common probe function for both am33 and am43.

Yes, but you add a check for your new flag only to am43xx_suspend(), not
to am33xx_suspend() which is used by the am33xx compatibles you add.

> AFAIK for am33 family am335x-evm and am335x-bone-black support Deep 
> Sleep mode. For am43 family am43tx-gp-evm alone supports at the moment.

But these are development boards (EVKs), not SOC families (or
chip revisions). What about all the products that customers to TI who
have bought these SoCs have built?

> Can you let me know of other am33 machines that support DS0 mode?

I have a customer who use DS0, whose DTS is not yet in mainline, and
whose setup this patch would break for example.

> I could have simply used ti,am33xx compatible which covers entire am33 
> family but then am33xx-bone (bone white) does not support this mode.

Yes, and a blacklist would make much more sense for something like this
if where talking about specific boards.

Also note that your patch doesn't even handle bone-white as you didn't
add a check to am33xx_suspend() as I pointed out above.

Johan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22  8:43       ` Johan Hovold
@ 2018-08-22 11:07         ` J, KEERTHY
  2018-08-22 14:22           ` Tony Lindgren
  0 siblings, 1 reply; 8+ messages in thread
From: J, KEERTHY @ 2018-08-22 11:07 UTC (permalink / raw)
  To: Johan Hovold, tony
  Cc: d-gerlach, linux-kernel, t-kristo, ssantosh, linux-omap,
	linux-arm-kernel



On 8/22/2018 2:13 PM, Johan Hovold wrote:
> On Wed, Aug 22, 2018 at 01:50:29PM +0530, J, KEERTHY wrote:
>>
>>
>> On 8/22/2018 1:07 PM, Johan Hovold wrote:
>>> On Wed, Aug 22, 2018 at 09:34:09AM +0200, Johan Hovold wrote:
>>>> On Wed, Aug 22, 2018 at 11:02:31AM +0530, Keerthy wrote:
>>>>> Enable DS0 for only those platforms on which it is functional
>>>>>
>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>> ---
>>>>>    arch/arm/mach-omap2/pm33xx-core.c    | 5 +++++
>>>>>    drivers/soc/ti/pm33xx.c              | 9 +++++++++
>>>>>    include/linux/platform_data/pm33xx.h | 2 ++
>>>>>    3 files changed, 16 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
>>>>> index f4971e4..f0f6e8e 100644
>>>>> --- a/arch/arm/mach-omap2/pm33xx-core.c
>>>>> +++ b/arch/arm/mach-omap2/pm33xx-core.c
>>>>> @@ -135,6 +135,11 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>>>>>    {
>>>>>    	int ret = 0;
>>>>>    
>>>>> +	if (!(args & WFI_FLAG_DEEP_SLEEP0)) {
>>>>> +		pr_err("DS0 mode not supported\n");
>>>>> +		return -ENOTSUPP;
>>>>> +	}
>>>>> +
>>>>>    	amx3_pre_suspend_common();
>>>>>    	scu_power_mode(scu_base, SCU_PM_POWEROFF);
>>>>>    	ret = cpu_suspend(args, fn);
>>>>> diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
>>>>> index d0dab32..53238d7 100644
>>>>> --- a/drivers/soc/ti/pm33xx.c
>>>>> +++ b/drivers/soc/ti/pm33xx.c
>>>>> @@ -324,6 +324,15 @@ static int am33xx_pm_probe(struct platform_device *pdev)
>>>>>    	suspend_wfi_flags |= WFI_FLAG_SAVE_EMIF;
>>>>>    	suspend_wfi_flags |= WFI_FLAG_WAKE_M3;
>>>>>    
>>>>> +	/*
>>>>> +	 * Deep Sleep0 mode is currently functional only on am437x-gp-evm,
>>>>> +	 * am33xx-evm and boneblack family. Hence set the DS0 flag
>>>>> +	 */
>>>>> +	if (of_machine_is_compatible("ti,am437x-gp-evm") ||
>>>>> +	    of_machine_is_compatible("ti,am335x-bone-black") ||
>>>>> +	    of_machine_is_compatible("ti,am335x-evm"))
>>>>> +		suspend_wfi_flags |= WFI_FLAG_DEEP_SLEEP0;
>>>>
>>>> What about other (out-of-tree) machines which supports DS0 and which
>>>> this change would break?
>>>>
>>>> I think this needs to be a blacklist if anything.
>>>>
>>>> Please also expand in the commit message why you think this is needed.
>>
>> Currently when one does echo mem > /sys/power/state on unsuppored
>> machines there can be a crash or a hang. So bail out with a message.
> 
> Yes, but why is this unsupported on some machines? Which machines, and
> why? Your commit messages should be self-contained and hold the
> information needed to determine whether your patch makes sense in the
> first place.

Okay

> 
>>>> Last, what tree is this against? There's no am43xx_suspend() in
>>>> linux-next (and you add compatibles above for am33xx too).
>>>
>>> Sorry, there is indeed an am43xx_suspend(), but you are adding
>>> compatibles for am33xx which use am33xx_suspend().
>>
>> am33xx_pm_probe is a common probe function for both am33 and am43.
> 
> Yes, but you add a check for your new flag only to am43xx_suspend(), not
> to am33xx_suspend() which is used by the am33xx compatibles you add.

Got it. I will add a check there as well.

> 
>> AFAIK for am33 family am335x-evm and am335x-bone-black support Deep
>> Sleep mode. For am43 family am43tx-gp-evm alone supports at the moment.
> 
> But these are development boards (EVKs), not SOC families (or
> chip revisions). What about all the products that customers to TI who
> have bought these SoCs have built?
> 
>> Can you let me know of other am33 machines that support DS0 mode?
> 
> I have a customer who use DS0, whose DTS is not yet in mainline, and
> whose setup this patch would break for example.

okay

> 
>> I could have simply used ti,am33xx compatible which covers entire am33
>> family but then am33xx-bone (bone white) does not support this mode.
> 
> Yes, and a blacklist would make much more sense for something like this
> if where talking about specific boards.
> 
> Also note that your patch doesn't even handle bone-white as you didn't
> add a check to am33xx_suspend() as I pointed out above.

Tony,

Black list is easier here?

Regards,
Keerthy
> 
> Johan
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22 11:07         ` J, KEERTHY
@ 2018-08-22 14:22           ` Tony Lindgren
  2018-08-27  7:05             ` Johan Hovold
  0 siblings, 1 reply; 8+ messages in thread
From: Tony Lindgren @ 2018-08-22 14:22 UTC (permalink / raw)
  To: J, KEERTHY
  Cc: Johan Hovold, d-gerlach, linux-kernel, t-kristo, ssantosh,
	linux-omap, linux-arm-kernel

* J, KEERTHY <j-keerthy@ti.com> [180822 11:11]:
> On 8/22/2018 2:13 PM, Johan Hovold wrote:
> > Yes, and a blacklist would make much more sense for something like this
> > if where talking about specific boards.
>
> Black list is easier here?

After thinking about this a bit more I think the boards supporting
deep sleep should add a PM related dts property to enable deep sleep.

The board maintainers need to test and verify deep sleep for each
board, it's not something that just works for the SoC in general.
Some boards may use different powering for things like DDR where
it's power might be controlled by a GPIO regulator. And in some
cases deeper idle states may depend also on the PMIC being used.

Maybe we already have some dts property we can use to describe
the idle states the board hardware supports?

Regards,

Tony


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional
  2018-08-22 14:22           ` Tony Lindgren
@ 2018-08-27  7:05             ` Johan Hovold
  0 siblings, 0 replies; 8+ messages in thread
From: Johan Hovold @ 2018-08-27  7:05 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: J, KEERTHY, Johan Hovold, d-gerlach, linux-kernel, t-kristo,
	ssantosh, linux-omap, linux-arm-kernel

On Wed, Aug 22, 2018 at 07:22:20AM -0700, Tony Lindgren wrote:
> * J, KEERTHY <j-keerthy@ti.com> [180822 11:11]:
> > On 8/22/2018 2:13 PM, Johan Hovold wrote:
> > > Yes, and a blacklist would make much more sense for something like this
> > > if where talking about specific boards.
> >
> > Black list is easier here?
> 
> After thinking about this a bit more I think the boards supporting
> deep sleep should add a PM related dts property to enable deep sleep.
> 
> The board maintainers need to test and verify deep sleep for each
> board, it's not something that just works for the SoC in general.
> Some boards may use different powering for things like DDR where
> it's power might be controlled by a GPIO regulator. And in some
> cases deeper idle states may depend also on the PMIC being used.
> 
> Maybe we already have some dts property we can use to describe
> the idle states the board hardware supports?

Yeah, unless you can infer this from an existing tree I guess you need
to add a new property. And indeed, a driver blacklist would suffer from the
same fundamental problem (with an ever expanding list of machines) as a
whitelist even if it would avoid regressing currently working systems.

Johan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-08-27  7:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-22  5:32 [PATCH] soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional Keerthy
2018-08-22  7:34 ` Johan Hovold
2018-08-22  7:37   ` Johan Hovold
2018-08-22  8:20     ` J, KEERTHY
2018-08-22  8:43       ` Johan Hovold
2018-08-22 11:07         ` J, KEERTHY
2018-08-22 14:22           ` Tony Lindgren
2018-08-27  7:05             ` Johan Hovold

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).