On Thu, Aug 9, 2018 at 10:26 AM Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx> wrote: > > Hi Evan, > > > On 8/9/2018 3:25 AM, Evan Green wrote: > > On Tue, Jul 31, 2018 at 3:09 AM Can Guo <cang@xxxxxxxxxxxxxx> wrote: > >> Add UFS PHY support to make SDM845 UFS work with common PHY framework. > >> > >> Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx> > >> --- > >> 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 > > ... > >> 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. > > > > > 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 <evgreen@xxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
