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 wrote:
Add UFS PHY support to make SDM845 UFS work with common PHY framework.
Signed-off-by: Can Guo
---
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 wo
Re: [PATCH v7 3/4] phy: Add QMP phy based UFS phy support for sdm845
Hi,
On 6/19/2018 2:06 PM, Can Guo wrote:
> +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.
> + */
Instead of relying on UFS glue driver to assert/de-assert PHY
which requires UFS PHY initialization to be split in init() and
poweron(), we can rather register reset_controller from ufs-qcom
driver.
PHY driver can then assert/de-assert as per UFS PHY requirement
in init() function itself and there won't be any need to have
poweron() routine for UFS as init can perform complete PHY
initialization without any dependency on ufs-qcom glue driver.
-Manu
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v7 3/4] phy: Add QMP phy based UFS phy support for sdm845
On Tue, Jun 19, 2018 at 1:38 AM Can Guo wrote:
>
> Add UFS PHY support to make SDM845 UFS work with common PHY framework.
>
> Signed-off-by: Can Guo
> ---
> 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"
> +* 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
