[RFC PATCH 1/5] regulator: Extend the power-management APIs

[Alexandre Belloni]

Hi Mark,

Any comment on that one so we can move forward?

On 10/01/2017 at 14:05:56 +0100, Boris Brezillon wrote :
> On Tue, 10 Jan 2017 12:10:26 +0000
> Mark Brown <broonie at kernel.org> wrote:
> 
> > On Tue, Jan 10, 2017 at 09:33:55AM +0100, Boris Brezillon wrote:
> > > Mark Brown <broonie at kernel.org> wrote:  
> > 
> > > > However it
> > > > *is* a bit more than just making sure that the device suspend ordering
> > > > is good (though that's definitely part of it), there will be things
> > > > kicked off by hardware signalling without software knowing about it.  
> > 
> > > Do you have an example, so that I can understand the use case?  
> > 
> > Think about how a CPU suspends and signals the PMIC to go into suspend
> > mode - things signalled by hardware state changes that the hardware does
> > autonomously.
> 
> Got it.
> 
> > 
> > > > Anything that doesn't affect a hardware supported runtime state probably
> > > > needs to be split off and handled separately as that's the much more
> > > > risky bit  
> > 
> > > Just to be sure, you mean regulator devices that do not support the  
> > > ->set_suspend_xx() hooks, right?  
> > 
> > Yes.
> > 
> > > >, moving changing of suspend mode earlier isn't going to cause
> > > > too much grief, that patch should just be split out and can probably
> > > > just go straight in.  
> > 
> > > Yes, I just thought it would be clearer to have everything implemented
> > > in the same function. Since calling ->set_suspend_xx() does not have
> > > any impact on the runtime state, it can be called whenever we want
> > > (assuming we can still communicate with the regulator device to
> > > configure this suspend state).
> > > But if you prefer to have it split out in 2 different function, with the
> > > 'set suspend mode' bits called from the regulator_suspend_begin(), I'm
> > > fine with that.  
> > 
> > No, I'm mainly saying that these things should be done in two separate
> > patches rather than talking about how the end code looks.  To a large
> > extent it does't matter when we apply the hardware supported suspend
> > modes, they won't take effect while software is running anyway.
> 
> Okay, that shouldn't be a problem then.
> 
> > 
> > > > Requring these functions to be called from every single driver seems
> > > > like we're doing something wrong - if we're going to do this we should
> > > > find some way to loop over all regulators and apply any unapplied
> > > > changes.  
> > 
> > > I agree. Actually, I forgot that we had PM ops at the device class
> > > level. Maybe we could just move these generic ->suspend()/->resume()
> > > implementation here.  
> > 
> > Yeah, I need to check if those class level operations always get run.
> > 
> > > > Batching things up at the end of suspend would also mean that
> > > > we'd be able to minimise the chances that we get the ordering wrong.  
> > 
> > > Unfortunately that's not possible, for the exact same reason calling
> > > regulator_suspend_prepare() from the platform ->prepare() hook did not
> > > work for me: at that point, all devices have been suspended, and this
> > > includes the i2c controller which we're using to communicate with the
> > > PMIC exposing those regulators.  
> > 
> > Do those devices actually get meaningfully suspended on your system,
> 
> I think so.
> 
> > and
> > even on systems where we do if we are going to use the dependency graphs
> > we should be able to arrange to do something with that to reorder both
> > them and the regulators to as near the end of the queue as we can get
> > tehm - that way we minimise the chances of being bitten by any
> > unexpressed depdencies (which I expect we have a lot of given how
> > resistant people are to writing proper DTs).
> 
> Yes, maybe we'll need a mechanism to mark some devices for
> late-suspend. I currently don't need it because the runtime changes I'm
> applying are not preventing the system from running correctly:
> increasing the voltage output and moving into low-power mode (the
> reason behind voltage change is probably related to the poor precision
> of the low-power mode, which forces us to take an higher margin to
> prevent voltage from crossing the min constraint of the DDR memory).
> 
> Of course, we should not only take my specific case into account, but
> I'd except people to properly define the dependencies if they really
> need to.
> 
> > 
> > > 2/ Rely on the device model dependency graph, and enter the suspend
> > >    state when the regulator device is being suspended (this is the
> > >    solution I'm proposing in this patch).  
> > 
> > That's future work though (which is happening but still), right now we
> > know the graph doesn't work properly.  It also leaves us more open to
> > unexpressed dependencies which are
> 
> I miss the end of the story :-).
> 
> One last comment: between solution #1 and solution #2, #2 will always
> suspend the regulator later than #1. Now, if we add
> 
> 3/ Make sure the i2c controller driving the bus the PMIC is connected
> to is kept enabled, and enter regulators suspend state after all
> devices have been suspended.
> 
> of course #3 will suspend things later than #2, but at the expense of
> driver/DT specialization, and we're not guaranteed to not face another
> weird dependency constraint like "suspend dev X" -> "suspend reg Y" ->
> "suspend dev Z" in the future.
> 
> Also note that the 'suspend dev connected on a bus before the bus
> controller' dependencies are already well supported thanks to the
> device model dev->parent relationship. Actually, this is what I'm
> relying on for my "suspend PMIC's regulators before the i2c controller"
> constraint.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com