Re: [PATCH v1 7/8] phy: qcom-qmp: Move UFS phy to phy_poweron/off

Stephen Boyd
2019-01-18T22:39:06Z

Quoting Evan Green (2019-01-11 15:01:28)
> For UFS, move the actual firing up of the PHY to phy_poweron and
> phy_poweroff callbacks, rather than init/exit. UFS calls
> phy_poweroff during suspend, so now all clocks and regulators for
> the phy can be powered down during suspend.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
>
> ---
>
> drivers/phy/qualcomm/phy-qcom-qmp.c | 82 ++++++++---------------------
> 1 file changed, 23 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index eb1cac8f0fd4e..7766c6384d0a8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1224,7 +1223,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
> int ret;
>
> dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> -
> if (cfg->has_ufsphy_reset) {
> /*
> * Get UFS reset, which is delayed until now to avoid a

Nitpick: Drop this hunk.

> @@ -1360,7 +1353,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
>
> /* Put PHY into POWER DOWN state: active low */
> qphy_clrbits(qphy->pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
> -
> if (cfg->has_lane_rst)
> reset_control_assert(qphy->lane_rst);
>

And this hunk.

> @@ -1371,44 +1363,6 @@ 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);

So we never need to poll this bit anymore?

> - 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, int submode)
> {
> @@ -1658,9 +1612,15 @@ 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,
> + .init = qcom_qmp_phy_enable,
> + .exit = qcom_qmp_phy_disable,
> + .set_mode = qcom_qmp_phy_set_mode,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct phy_ops qcom_qmp_ufs_ops = {
> + .power_on = qcom_qmp_phy_enable,
> + .power_off = qcom_qmp_phy_disable,
> .set_mode = qcom_qmp_phy_set_mode,
> .owner = THIS_MODULE,
> };

So the UFS and the non-UFS phys will use the same single function, but
the callers of the phys will see that phy_power_on() powers on the phy
for UFS but does nothing for non-UFS devices? Do the users of this
common phy call the API differently between drivers? Kishon, is there
guidance on how phys are supposed to be used by drivers?