| Message ID | 1534915951-8783-1-git-send-email-j-keerthy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: ti: pm33xx: Enable DS0 for the platforms on which it is functional | expand |
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
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
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 >
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
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 >
* 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
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
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 {
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(+)