Messages in this thread | | | Date | Fri, 06 Jul 2018 16:01:56 +0800 | From | cang@codeauro ... | Subject | Re: [PATCH v7 3/4] phy: Add QMP phy based UFS phy support for sdm845 |
| |
On 2018-06-28 04:17, Evan Green wrote: > On Tue, Jun 19, 2018 at 1:38 AM Can Guo <cang@codeaurora.org> wrote: >> >> Add UFS PHY support to make SDM845 UFS work with common PHY framework. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp.c | 173 >> +++++++++++++++++++++++++++++++++++- >> drivers/phy/qualcomm/phy-qcom-qmp.h | 15 ++++ >> 2 files changed, 187 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c >> b/drivers/phy/qualcomm/phy-qcom-qmp.c >> index 9be9754..852792d 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,15 @@ 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 PHY that has not software reset control bits in its >> address > > This line of the comment is unclear. Maybe: > "For UFS PHYs that do not have software reset control bits, defer > starting serdes until the power on callback" >
Sure, thank you.
>> + * space, it should skip starting serdes here. UFS PHY Serdes >> shall >> + * start when UFS explicitly calls PHY power on. >> + */ >> + if ((cfg->type == PHY_TYPE_UFS) && cfg->no_pcs_sw_reset) >> + goto out; >> + >> + /* >> * Pull out PHY from POWER DOWN state. >> * This is active low enable signal to power-down PHY. >> */ >> @@ -1159,6 +1286,7 @@ static int qcom_qmp_phy_init(struct phy *phy) >> } >> qmp->phy_initialized = true; >> >> +out: >> return ret; >> >> err_pcs_ready: >> @@ -1181,7 +1309,8 @@ static int qcom_qmp_phy_exit(struct phy *phy) >> clk_disable_unprepare(qphy->pipe_clk); >> >> /* PHY reset */ >> - qphy_setbits(qphy->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); >> + if (!cfg->no_pcs_sw_reset) >> + qphy_setbits(qphy->pcs, cfg->regs[QPHY_SW_RESET], >> SW_RESET); >> >> /* stop SerDes and Phy-Coding-Sublayer */ >> qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], >> cfg->start_ctrl); >> @@ -1199,6 +1328,44 @@ static int qcom_qmp_phy_exit(struct phy *phy) >> return 0; >> } >> >> +static int qcom_qmp_phy_poweron(struct phy *phy) >> +{ >> + struct qmp_phy *qphy = phy_get_drvdata(phy); >> + struct qcom_qmp *qmp = qphy->qmp; >> + const struct qmp_phy_cfg *cfg = qmp->cfg; >> + void __iomem *pcs = qphy->pcs; >> + void __iomem *status; >> + unsigned int mask, val; >> + int ret = 0; >> + >> + if (cfg->type != PHY_TYPE_UFS) >> + return 0; >> + >> + /* >> + * For UFS PHY that has not software reset control, serdes >> start >> + * should only happen when UFS driver explicitly calls >> phy_power_on >> + * after it deasserts software reset. >> + */ >> + if (cfg->no_pcs_sw_reset && !qmp->phy_initialized && >> + (qmp->init_count != 0)) { >> + /* start SerDes and Phy-Coding-Sublayer */ >> + qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], >> cfg->start_ctrl); >> + >> + status = pcs + cfg->regs[QPHY_PCS_READY_STATUS]; >> + mask = cfg->mask_pcs_ready; >> + >> + ret = readl_poll_timeout(status, val, !(val & mask), >> 1, >> + PHY_INIT_COMPLETE_TIMEOUT); >> + if (ret) { >> + dev_err(qmp->dev, "phy initialization >> timed-out\n"); >> + return ret; >> + } >> + qmp->phy_initialized = true; >> + } >> + >> + return ret; >> +} >> + >> static int qcom_qmp_phy_set_mode(struct phy *phy, enum phy_mode mode) >> { >> struct qmp_phy *qphy = phy_get_drvdata(phy); >> @@ -1428,6 +1595,7 @@ static int phy_pipe_clk_register(struct qcom_qmp >> *qmp, struct device_node *np) >> static const struct phy_ops qcom_qmp_phy_gen_ops = { >> .init = qcom_qmp_phy_init, >> .exit = qcom_qmp_phy_exit, >> + .power_on = qcom_qmp_phy_poweron, > > The USB pipe clk discussion earlier this year got me on the lookout > for asymmetric phy init functions. If we're flipping on START_CTRL in > phy_poweron, shouldn't we be flipping it back off in phy_poweroff? > From the comments, it seems like this was done so that some sort of > UFS reset step could happen. Would that sequencing still happen > correctly if the PHY did: > > phy_init > phy_power_on > (phy_power_off) > phy_power_on > > I'm unsure. Does suspend/resume work? > > -Evan
Hi Evan,
Thank you for your comments. As there is no phy_power_off implemented,phy_power_off actually does nothing.Back to your question, above sequence does not have issue here with current patch, as the PHY is initialized already (qmp->phy_initialized is TRUE), the START_CTRL shall not be set again.
+ if (cfg->no_pcs_sw_reset && !qmp->phy_initialized && + (qmp->init_count != 0)) {
I am working with Vivek and Manu internally to re-fine the patch. As Manu commented, we want to find a way to remove the poweron method as it is only used by SDM845 UFS PHY.
- Can
| |