On Thu, Aug 9, 2018 at 10:26 AM Vivek Gautam
[off-list ref] wrote:

Hi Evan,


On 8/9/2018 3:25 AM, Evan Green wrote:

quoted

On Tue, Jul 31, 2018 at 3:09 AM Can Guo [off-list ref] wrote:

quoted

Add UFS PHY support to make SDM845 UFS work with common PHY framework.

Signed-off-by: Can Guo <redacted>
---
  drivers/phy/qualcomm/phy-qcom-qmp.c | 172 +++++++++++++++++++++++++++++++++++-
  drivers/phy/qualcomm/phy-qcom-qmp.h |  15 ++++
  2 files changed, 186 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 9be9754..de7ff18 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c

...

quoted

  static void qcom_qmp_phy_configure(void __iomem *base,
                                    const unsigned int *regs,
                                    const struct qmp_phy_init_tbl tbl[],

@@ -1131,6 +1249,14 @@ static int qcom_qmp_phy_init(struct phy *phy)
         qcom_qmp_phy_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);

         /*
+        * UFS PHY requires the deassert of software reset before serdes start.
+        * For UFS PHYs that do not have software reset control bits, defer
+        * starting serdes until the power on callback.
+        */

I'm relatively thick when it comes to PHYs, but I'm a little confused.
The sequence of code right below this (not shown) looks like it is
deasserting reset before starting serdes, seemingly doing what the
comment wants. I guess the problem is the lack of SW reset? So then
you defer serdes start until UFS does... something. Can you explain
how deferring to after UFS HC init actually helps? Is it the UFS HC
that releases reset on the PHY?

As you can see in [1], the ufs first asserts the sw_reset, then phy
initialization is done.
This phy_init() is just programming the phy registers. Now as per the H/W
programming doc, we can't start the phy until we de-assert the sw_reset.
So the sequence as per the programming doc should be:

assert SW_reset --> program phy serdes/tx/rx/pcs registers --> deassert
SW_reset --> start serdes --> test PCS status

That's the reason that serdes_start has been moved to phy_power_on(), as
that seemed
a more cleaner way of handling the above sequence.
UFS HC init doesn't help more than this in terms of phy initialization.

Ok that makes sense. Thank you for the explanation.

quoted

I was hoping the next patch would help, but I'm still confused. It
looks like you've added a call to phy_power_on in
ufs_qcom_setup_clocks, but there's also one still in
ufs_qcom_power_up_sequence. What does the original phy_power_on in
ufs_qcom_power_up_sequence do now? It seems like that one would do the
power on too early, and then your new added call in
ufs_qcom_setup_clocks would do nothing.

I think [patch 4/5] of this series handles this. We skip the
phy_power_on until
we do phy_init.
phy_power_on/off() in setup_clocks() is also used for suspend/resume case
and that's the reason you see couple of phy_power_on(). Patch 4/5 should
handle
this now.

Ok got it. I was confused about the ordering. setup_clocks is actually
called first (via __ufshcd_setup_clocks > ufshcd_hba_init >
ufshcd_init), which is why you need the boolean to defer this power on
until later.

Reviewed-by: Evan Green <redacted>