From mboxrd@z Thu Jan 1 00:00:00 1970 From: alexandre.belloni@free-electrons.com (Alexandre Belloni) Date: Wed, 25 Jan 2017 16:02:56 +0100 Subject: [RFC PATCH 1/5] regulator: Extend the power-management APIs In-Reply-To: <20170110140556.172b3ec0@bbrezillon> References: <1480687036-5037-1-git-send-email-boris.brezillon@free-electrons.com> <1480687036-5037-2-git-send-email-boris.brezillon@free-electrons.com> <20170109191758.7zzxeh45pvwiw3jo@sirena.org.uk> <20170110093355.700f8225@bbrezillon> <20170110121026.zstf7a6cflzk366l@sirena.org.uk> <20170110140556.172b3ec0@bbrezillon> Message-ID: <20170125150256.umqkq44blmv24y5l@piout.net> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 wrote: > > > On Tue, Jan 10, 2017 at 09:33:55AM +0100, Boris Brezillon wrote: > > > Mark Brown 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