[PATCH] omap3: give off mode enable a more prominent place

[PATCH] omap3: give off mode enable a more prominent place

[Andreas Kemnade]

Enabling off mode was only reachable deeply hidden
in the debugfs. As powersaving is an important feature,
move the option out of its shady place.
The debugfs file can still be used to override the default.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 arch/arm/mach-omap2/Kconfig    | 8 ++++++++
 arch/arm/mach-omap2/pm-debug.c | 4 ++++
 arch/arm/mach-omap2/pm.h       | 4 ++++
 arch/arm/mach-omap2/pm34xx.c   | 7 ++++++-
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 5e33d1a90664..8305b94c0e57 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -231,6 +231,14 @@ config OMAP3_SDRC_AC_TIMING
 	  wish to say no.  Selecting yes without understanding what is
 	  going on could result in system crashes;
 
+config OMAP3_PM_OFFMODE
+	bool "Enable Off-Mode"
+	depends on ARCH_OMAP3
+	help
+	  Enables deeper power-saving states of the SOC, so that hwmods
+	  do not only go to RET state but to OFF state. This option
+	  is overridable via debugfs.
+
 endmenu
 
 endif
diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 5a8839203958..3474027112c2 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -37,7 +37,11 @@
 #include "prm2xxx_3xxx.h"
 #include "pm.h"
 
+#ifdef CONFIG_OMAP3_PM_OFFMODE
+u32 enable_off_mode = 1;
+#else
 u32 enable_off_mode;
+#endif
 
 #ifdef CONFIG_DEBUG_FS
 #include <linux/debugfs.h>
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index c73776b82348..cd5f98f4d28b 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -55,8 +55,12 @@ extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
 #ifdef CONFIG_PM_DEBUG
 extern u32 enable_off_mode;
 #else
+#ifdef CONFIG_OMAP3_PM_OFFMODE
+#define enable_off_mode 1
+#else
 #define enable_off_mode 0
 #endif
+#endif
 
 #if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
 extern void pm_dbg_update_time(struct powerdomain *pwrdm, int prev);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 1a90050361f1..d7d83a09013f 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -416,7 +416,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
 	if (!pwrst)
 		return -ENOMEM;
 	pwrst->pwrdm = pwrdm;
-	pwrst->next_state = PWRDM_POWER_RET;
+
+	if (IS_ENABLED(CONFIG_OMAP3_PM_OFFMODE))
+		pwrst->next_state = PWRDM_POWER_OFF;
+	else
+		pwrst->next_state = PWRDM_POWER_RET;
+
 	list_add(&pwrst->node, &pwrst_list);
 
 	if (pwrdm_has_hdwr_sar(pwrdm))
-- 
2.11.0

Re: [PATCH] omap3: give off mode enable a more prominent place

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Sat,  2 Feb 2019 06:58:27 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> Enabling off mode was only reachable deeply hidden
> in the debugfs. As powersaving is an important feature,
> move the option out of its shady place.
> The debugfs file can still be used to override the default.

oops, this should be an rfc. So take it a bit with care.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] omap3: give off mode enable a more prominent place

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190202 06:01]:
> Enabling off mode was only reachable deeply hidden
> in the debugfs. As powersaving is an important feature,
> move the option out of its shady place.

How about let's enable always if we have the twl4030
configured to allow it? You can just check if the dts has
"ti,twl4030-power-idle" or "ti,twl4030-power-idle-osc-off"
properties set.

In order to enable deeper idle states, the user space still
needs to idle the UARTs and possibly other hardware blocking
idle. So we should be safe there.

> The debugfs file can still be used to override the default.

Yes let's keep the debugfs switch around. But it should
be optional for CONFIG_DEBUGFS like I think it now is.

> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -416,7 +416,12 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
>  	if (!pwrst)
>  		return -ENOMEM;
>  	pwrst->pwrdm = pwrdm;
> -	pwrst->next_state = PWRDM_POWER_RET;
> +
> +	if (IS_ENABLED(CONFIG_OMAP3_PM_OFFMODE))
> +		pwrst->next_state = PWRDM_POWER_OFF;
> +	else
> +		pwrst->next_state = PWRDM_POWER_RET;
> +
>  	list_add(&pwrst->node, &pwrst_list);
>  
>  	if (pwrdm_has_hdwr_sar(pwrdm))

You can check for the PMIC properties by adding a function
for omap3_pm_check_pmic() or similar and call it
from omap3_pm_init(). And then you can set the needed flags
in omap3_pm_check_pmic() such as omap3_has_twl and off mode.
You might want to make it somewhat PMIC independent as in
theory somebody may still want to add support for other
PMICs such as cpacp for omap3 although that's unlikely.

Regards,

Tony

Re: [PATCH] omap3: give off mode enable a more prominent place

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Mon, 4 Feb 2019 07:56:04 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190202 06:01]:
> > Enabling off mode was only reachable deeply hidden
> > in the debugfs. As powersaving is an important feature,
> > move the option out of its shady place.  
> 
> How about let's enable always if we have the twl4030
> configured to allow it? You can just check if the dts has
> "ti,twl4030-power-idle" or "ti,twl4030-power-idle-osc-off"
> properties set.
> 
> In order to enable deeper idle states, the user space still
> needs to idle the UARTs and possibly other hardware blocking
> idle. So we should be safe there.
> 
Let us not mix up runtime pm and system pm. The uarts need
to be idled for runtime suspend, but they are off/ret for
system suspend without userspace intervention, so allowing off mode
will have an influence even without uart runtime suspend,
and also probably for other powerdomains (non-core/per).
So we still need to be sure to handle at least some erratas and
context save/restore correctly.

Your Idea seems to be in pseudocode
if (powersaving_wanted)
	enable_off_mode()

I had something in mind like
if (system_is_trusted_to_handle_offmode()
	enable_off_mode()

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] omap3: give off mode enable a more prominent place

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190204 18:33]:
> On Mon, 4 Feb 2019 07:56:04 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Andreas Kemnade <andreas@kemnade.info> [190202 06:01]:
> > > Enabling off mode was only reachable deeply hidden
> > > in the debugfs. As powersaving is an important feature,
> > > move the option out of its shady place.  
> > 
> > How about let's enable always if we have the twl4030
> > configured to allow it? You can just check if the dts has
> > "ti,twl4030-power-idle" or "ti,twl4030-power-idle-osc-off"
> > properties set.
> > 
> > In order to enable deeper idle states, the user space still
> > needs to idle the UARTs and possibly other hardware blocking
> > idle. So we should be safe there.
> > 
> Let us not mix up runtime pm and system pm. The uarts need
> to be idled for runtime suspend, but they are off/ret for
> system suspend without userspace intervention, so allowing off mode
> will have an influence even without uart runtime suspend,
> and also probably for other powerdomains (non-core/per).
> So we still need to be sure to handle at least some erratas and
> context save/restore correctly.

True that's a good point.

> Your Idea seems to be in pseudocode
> if (powersaving_wanted)
> 	enable_off_mode()
> 
> I had something in mind like
> if (system_is_trusted_to_handle_offmode()
> 	enable_off_mode()

For omap3, the properties for "ti,twl4030-power-idle" or
"ti,twl4030-power-idle-osc-off" mean just that.

The PMIC is wired and configured for off mode, and those
properties should not be set unless the system is truly capable
of entering off mode. If not set, we should not enable off
idle by default.

Otherwise the boards should be already using just
"ti,twl4030-power" or "ti,twl4030-power-reset".

So it should be safe to set a flag for off_mode based
on that flag during boot, or set a flag for off
mode allowed if the flag is needed later on.

Regards,

Tony

Re: [PATCH] omap3: give off mode enable a more prominent place

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]

On Mon, 4 Feb 2019 10:43:17 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190204 18:33]:
> > On Mon, 4 Feb 2019 07:56:04 -0800
> > Tony Lindgren <tony@atomide.com> wrote:
> >   
> > > * Andreas Kemnade <andreas@kemnade.info> [190202 06:01]:  
> > > > Enabling off mode was only reachable deeply hidden
> > > > in the debugfs. As powersaving is an important feature,
> > > > move the option out of its shady place.    
> > > 
> > > How about let's enable always if we have the twl4030
> > > configured to allow it? You can just check if the dts has
> > > "ti,twl4030-power-idle" or "ti,twl4030-power-idle-osc-off"
> > > properties set.
> > > 
> > > In order to enable deeper idle states, the user space still
> > > needs to idle the UARTs and possibly other hardware blocking
> > > idle. So we should be safe there.
> > >   
> > Let us not mix up runtime pm and system pm. The uarts need
> > to be idled for runtime suspend, but they are off/ret for
> > system suspend without userspace intervention, so allowing off mode
> > will have an influence even without uart runtime suspend,
> > and also probably for other powerdomains (non-core/per).
> > So we still need to be sure to handle at least some erratas and
> > context save/restore correctly.  
> 
> True that's a good point.
> 
> > Your Idea seems to be in pseudocode
> > if (powersaving_wanted)
> > 	enable_off_mode()
> > 
> > I had something in mind like
> > if (system_is_trusted_to_handle_offmode()
> > 	enable_off_mode()  
> 
> For omap3, the properties for "ti,twl4030-power-idle" or
> "ti,twl4030-power-idle-osc-off" mean just that.
> 
Hmm, system = software + hardware. At least that I had in mind
when writing this text.
So for the software part: I guess off mode is on our all test
schedule, so it should be reliable.
dt describes the hardware, so there should be any MODE7 quirks
defined if they are required and the proper pmic setting.

But wait... twl4030-power.c is to a big part about switching regulators
on and off. But how does that connect to hwmods going to ret or off
mode? That is imho slightly another topic.
Ok, looking a bit closer, there is the sys_off_mode line
which needs to be used.
So we have to derive that from the devicetree.

> The PMIC is wired and configured for off mode, and those
> properties should not be set unless the system is truly capable
> of entering off mode. If not set, we should not enable off
> idle by default.
> 
> Otherwise the boards should be already using just
> "ti,twl4030-power" or "ti,twl4030-power-reset".
> 
We have also
ti,twl4030-power-omap3-sdp,
ti,twl4030-power-omap3-ldp,
ti,twl4030-power-omap3-evm

so we have to maintain a list, especially if we want to allow another
pmic.

But since it is about connecting the soc to the pmic, we could also add a
flag in the dtb on the soc side, so we are prepared if someone uses another
pmic. It feels a bit strange to query something from devicetree for another
device.

Regards,
Andreas
PS: I hope my omap-gta04.dtsi-related patches have made their way into
your review queue and are not starving in a generic lkml fallback folder

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] omap3: give off mode enable a more prominent place

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190206 06:38]:
> But wait... twl4030-power.c is to a big part about switching regulators
> on and off. But how does that connect to hwmods going to ret or off
> mode? That is imho slightly another topic.

The pmic wiring is what must be configured for the regulators
to actually get cut off. And the wiring may or may not be done
properly.

The SoC powerdomain bit to attempt off mode is a policy type
decision and should not be configured in the device tree.

> Ok, looking a bit closer, there is the sys_off_mode line
> which needs to be used.
> So we have to derive that from the devicetree.

Yes but that still does not mean it's wired right to the
PMIC :) We do have devices where sys_off_mode and sys_clkreq
lines are swapped for example. So enabling off mode really
should be only allowed if the PMIC is configured right.

And we also support devices that do muxing in the bootloader
like all the Nokia devices do except for dynamic pins.

I guess reading how the sys_clkreq is configured might
work, but not necessarily until the PMIC driver has probed
depending which device handles the muxing. And it won't
work for devices with lines swapped.

> > The PMIC is wired and configured for off mode, and those
> > properties should not be set unless the system is truly capable
> > of entering off mode. If not set, we should not enable off
> > idle by default.
> > 
> > Otherwise the boards should be already using just
> > "ti,twl4030-power" or "ti,twl4030-power-reset".
> > 
> We have also
> ti,twl4030-power-omap3-sdp,
> ti,twl4030-power-omap3-ldp,
> ti,twl4030-power-omap3-evm
> 
> so we have to maintain a list, especially if we want to allow another
> pmic.

Oops so it seems. Those are all early devices with their
own quirks. Anyways, that list is unlikely to change
any longer for twl4030.

> But since it is about connecting the soc to the pmic, we could also add a
> flag in the dtb on the soc side, so we are prepared if someone uses another
> pmic. It feels a bit strange to query something from devicetree for another
> device.

Well if you have something that actually describes the hardware
in mind for the SoC, sure. But let's not start adding any policy
flags to the dts.

Any other pmics would need their own checks to determine if
off mode is configured. It might depend on the firmware on the
pmic for example. But I don't think we need to worry about that
right now since we don't have any such users in the mainline
kernel.

> PS: I hope my omap-gta04.dtsi-related patches have made their way into
> your review queue and are not starving in a generic lkml fallback folder

Yup I should have the thread tagged in my inbox.

Regards,

Tony