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 [off-list ref] wrote:

quoted

On Tue, Jan 10, 2017 at 09:33:55AM +0100, Boris Brezillon wrote:

quoted

Mark Brown [off-list ref] wrote:  

quoted

quoted

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.  

quoted

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.

quoted

quoted

quoted

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  

quoted

Just to be sure, you mean regulator devices that do not support the  
->set_suspend_xx() hooks, right?  

Yes.

quoted

quoted

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

quoted

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.

quoted

quoted

quoted

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.  

quoted

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.

quoted

quoted

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.  

quoted

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.

quoted

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.

quoted

quoted

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