Date:   Tue, 26 Mar 2019 13:18:35 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Evan Green <evgreen@...omium.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>,
        <devicetree@...r.kernel.org>,
        Pedro Sousa <pedrom.sousa@...opsys.com>,
        liwei <liwei213@...wei.com>, <linux-scsi@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>,
        Bart Van Assche <Bart.VanAssche@....com>,
        <linux-kernel@...r.kernel.org>, Andy Gross <andy.gross@...aro.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

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.

Thanks
Kishon
> 
> Changes in v5:
> - Updated tags
> 
> Changes in v4:
> - Do reset_control_* unconditionally since null is handled (Stephen).
> - Keep doing reset_control* unconditionally through refactor (Stephen).
> 
> Changes in v3:
> - Refactor to only expose the reset controller in one change (Stephen).
> - Add period to comment (Stephen).
> - Reset err to 0 in ignored error case (Stephen).
> - Add include of reset-controller.h (Stephen)
> - Refactored to move reset control in a single commit (Stephen)
> - Use no_pcs_sw_reset as an indicator of UFS reset in qmp-phy (Stephen).
> - Assign ret = PTR_ERR() earlier, for better reuse (Stephen).
> - Refactor init => poweron for all PHYs and UFS in one step (Stephen)
> 
> Changes in v2:
> - Added resets to example (Stephen).
> - Remove include of reset.h (Stephen)
> - Fix error print of phy_power_on (Stephen)
> - Comment for reset controller warnings on id != 0 (Stephen)
> - Add static to ufs_qcom_reset_ops (Stephen).
> - Use devm_* to get the reset (Stephen)
> - Clear ufs_reset on error getting it
> - Remove needless error print (Stephen)
> - Use devm_ to get the reset (Stephen)
> - Removed whitespace changes (Stephen)
> 
> Evan Green (8):
>   dt-bindings: ufs: Add #reset-cells for Qualcomm controllers
>   dt-bindings: phy-qcom-qmp: Add UFS PHY reset
>   dt-bindings: phy: qcom-ufs: Add resets property
>   arm64: dts: sdm845: Add UFS PHY reset
>   arm64: dts: msm8996: Add UFS PHY reset controller
>   scsi: ufs: qcom: Expose the reset controller for PHY
>   phy: qcom: Utilize UFS reset controller
>   phy: ufs-qcom: Refactor all init steps into phy_poweron
> 
>  .../devicetree/bindings/phy/qcom-qmp-phy.txt  |   6 +-
>  .../devicetree/bindings/ufs/ufs-qcom.txt      |   5 +-
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |   3 +
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          |   3 +
>  drivers/phy/qualcomm/phy-qcom-qmp.c           | 112 +++++++++--------
>  drivers/phy/qualcomm/phy-qcom-ufs-i.h         |   5 +-
>  drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c  |  25 +---
>  drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c  |  25 +---
>  drivers/phy/qualcomm/phy-qcom-ufs.c           |  57 ++++++---
>  drivers/scsi/ufs/Kconfig                      |   1 +
>  drivers/scsi/ufs/ufs-qcom.c                   | 114 +++++++++++-------
>  drivers/scsi/ufs/ufs-qcom.h                   |   4 +
>  13 files changed, 193 insertions(+), 171 deletions(-)
> 

Powered by blists - more mailing lists