linux-kernel - Re: [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 26 Mar 2019 09:17:48 -0700
From:   Evan Green <evgreen@...omium.org>
To:     Kishon Vijay Abraham I <kishon@...com>,
        Andy Gross <andy.gross@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Stephen Boyd <swboyd@...omium.org>,
        Marc Gonzalez <marc.w.gonzalez@...e.fr>,
        Can Guo <cang@...eaurora.org>,
        Vivek Gautam <vivek.gautam@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        Asutosh Das <asutoshd@...eaurora.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Pedro Sousa <pedrom.sousa@...opsys.com>,
        liwei <liwei213@...wei.com>, SCSI <linux-scsi@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Bart Van Assche <Bart.VanAssche@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        Subhash Jadavani <subhashj@...eaurora.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Avri Altman <avri.altman@....com>,
        Mark Rutland <mark.rutland@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Janek Kotas <jank@...ence.com>,
        David Brown <david.brown@...aro.org>
Subject: Re: [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend

On Tue, Mar 26, 2019 at 12:49 AM Kishon Vijay Abraham I <kishon@...com> 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