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

[Andreas Kemnade <andreas@kemnade.info>]

[-- 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 --]