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

[Evan Green]

On Fri, Jan 18, 2019 at 2:39 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx>
> >
> > ---
> >
> >  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.

Ok.

>
> > @@ -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.

Ok.

>
> > @@ -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?

We do poll that bit, we just do it in qcom_qmp_phy_enable now, since I
removed the conditional that exited early for UFS PHYs. It's almost
like someone copied and pasted the lower half of the function into
qcom_qmp_phy_poweron :)

>
> > -               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?
>

I'll leave that one for Kishon.
-Evan