lkml.org 
[lkml]   [2019]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 8/8] phy: ufs-qcom: Refactor all init steps into phy_poweron
    From
    Date
    Quoting Evan Green (2019-02-05 10:59:02)
    > The phy code was using implicit sequencing between the PHY driver
    > and the UFS driver to implement certain hardware requirements.
    > Specifically, the PHY reset register in the UFS controller needs
    > to be deasserted before serdes start occurs in the PHY.
    >
    > Before this change, the code was doing this by utilizing the two
    > phy callbacks, phy_init() and phy_poweron(), as "init step 1" and
    > "init step 2", where the UFS driver would deassert reset between
    > these two steps.
    >
    > This makes it challenging to power off the regulators in suspend,
    > as regulators are initialized in init, not in poweron(), but only
    > poweroff() is called during suspend, not exit().
    >
    > 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.
    >
    > QMP is a little tricky because the PHY is also shared with PCIe and
    > USB3, which have their own definitions for init() and poweron(). Rename
    > the meaty functions to _enable() and _disable() to disentangle from the
    > PHY core names, and then create two different ops structures: one for
    > UFS and one for the other PHY types.
    >
    > In phy-qcom-ufs, remove the 'is_powered_on' and 'is_started' guards,
    > as the generic PHY code does the reference counting. The
    > 14/20nm-specific init functions get collapsed into the generic power_on()
    > function, with the addition of a calibrate() callback specific to 14/20nm.
    >
    > Signed-off-by: Evan Green <evgreen@chromium.org>

    Reviewed-by: Stephen Boyd <swboyd@chromium.org>

    although it may change if the patch before this is changed
    significantly.

    > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
    > index 2861c64b9bcc..3597c4c0e50f 100644
    > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
    > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
    > @@ -1341,8 +1341,7 @@ static int qcom_qmp_phy_com_exit(struct qcom_qmp *qmp)
    > return 0;
    > }
    >
    > -/* PHY Initialization */
    > -static int qcom_qmp_phy_init(struct phy *phy)
    > +static int qcom_qmp_phy_enable(struct phy *phy)
    > {
    > struct qmp_phy *qphy = phy_get_drvdata(phy);
    > struct qcom_qmp *qmp = qphy->qmp;
    > @@ -1423,14 +1422,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
    > goto err_lane_rst;
    > }
    >
    > - /*
    > - * 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.
    > - */
    > - 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.
    > @@ -1442,7 +1433,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
    > usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
    >
    > /* Pull PHY out of reset state */
    > - qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
    > + if (!cfg->no_pcs_sw_reset)
    > + qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
    > +
    > if (cfg->has_phy_dp_com_ctrl)
    > qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
    >

    This is non-obvious, but OK I can see now that it's fine that everything
    is combined into one function. I didn't realize that phy_init() was
    polling the ready bit in addition to power on doing that under certain
    circumstances.

    Quite honestly, I find the whole implementation to be obtuse; combining
    all the phy code together into one function and then changing the
    execution paths via booleans and the 'type' member. It makes following
    and reviewing this code really painful. We should make phy ops for each
    type of phy: USB, USB+DP, PCIE, UFS, etc. and then have those ops call
    simpler building block functions that do the common things. Then
    reviewers don't have to untangle the execution paths in their minds to
    make sure that nothing goes wrong when a stray 'if (my_phy_flag)' is
    thrown into the init function and have to wonder if that causes problems
    for something else.

    \
     
     \ /
      Last update: 2019-02-06 23:05    [W:4.307 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site