On Tue, Mar 26, 2019 at 12:49 AM Kishon Vijay Abraham I <kishon@xxxxxx> wrote: > > Hi, > > On 21/03/19 10:47 PM, Evan Green wrote: > > The goal with this series is to enable shutting off regulators that power > > UFS during system suspend. > > > > In "the good life" version of this, we'd just disable the regulators > > in phy_poweroff() and be done with it. Unfortunately, that's not symmetric, > > as regulators are not enabled during phy_poweron(). Ok, so you might think > > we could just move the regulator enable and anything else that needs to > > come along into phy_poweron(), so that we can then undo it all in > > phy_poweroff(). That's where things get tricky. > > > > The qcom-qmp-phy overloaded the phy_init() and phy_poweron() callbacks, > > basically to mean "init phase 1" and "init phase 2". There are two phases > > because they have this phy_reset bit outside of the phy (in the UFS > > controller registers), and they need to make sure this bit is toggled at > > specific points in the phy init sequence. So there's this implicit > > sequence in the init dance between ufs-qcom.c and phy-qcom-qmp.c: > > 1) ufs-qcom asserts the PHY reset bit. > > 2) phy-qcom-qmp phy_init() does most of its initialization, but exits early. > > 3) ufs-qcom deasserts the PHY reset bit. > > 4) phy-qcom-qmp phy_poweron() finishes its initialization. > > > > This init dance is very difficult to follow in the code (since it's split > > between two drivers and not spelled out well), and arguably represents a > > deficiency in the hardware description of these devices. > > > > In this series I'm proposing tweaking the bindings for the Qualcomm > > UFS controller and PHY. In it we expose a reset controller from the > > UFS controller, that is then picked up and used from the PHY code. > > With this, the phy code can be reorganized to complete its initialization > > in a single function, removing the implicit two-phase overloading. > > > > Then I can move most of the phy initialization, including enabling > > the regulators, into phy_poweron(). Now, when phy_poweroff() is called, > > the phy actually powers off. This finally disables the regulators > > and allows me to save power in system suspend. > > > > Because the UFS PHY reset bit is now toggled in the PHY, rather > > than in ufs-qcom, this also percolated to all other PHYs using > > ufs-qcom, which from what I can see is just 8996. > > > > I removed the calls to phy_poweroff() during clock gating. This > > was originally dialing down a clock or two, while leaving the phy powered. > > I've now changed the semantics of phy_poweroff() to, well, actually power off. > > This works great for userlands that have set UFS's spm_lvl to 5 (off) like > > I have, but maybe changes power consumption for devices that have spm_lvl > > set to 3. I could try to use phy_init() and phy_poweron() as the two > > different possible transitions (fully off, and clocks off respectively), > > but I'm not sure if it actually matters, and I like the idea that > > phy_poweroff() really does power the thing off. > > > > Also, I don't have an 8996 device to test. If someone is able to test this > > out and perhaps point out any (hopefully obvious) bugs in the 8996 portion, > > I'd be grateful. > > > > This patch is based atop phy-next. > > Merged the series except the dts patches. Yay, thanks Kishon. Andy, Bjorn, are you able to take the DT patches? -Evan
