Message-ID: <154949065550.115909.10216592012769136925@swboyd.mtv.corp.google.com>
Date:   Wed, 06 Feb 2019 14:04:15 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Gross <andy.gross@...aro.org>,
        Evan Green <evgreen@...omium.org>,
        Kishon Vijay Abraham I <kishon@...com>
Cc:     Marc Gonzalez <marc.w.gonzalez@...e.fr>,
        Can Guo <cang@...eaurora.org>,
        Vivek Gautam <vivek.gautam@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        Asutosh Das <asutoshd@...eaurora.org>,
        Evan Green <evgreen@...omium.org>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        Arnd Bergmann <arnd@...db.de>, linux-scsi@...r.kernel.org,
        Grygorii Strashko <grygorii.strashko@...com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-kernel@...r.kernel.org, Manu Gautam <mgautam@...eaurora.org>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Vinayak Holikatti <vinholikatti@...il.com>
Subject: Re: [PATCH v3 8/8] phy: ufs-qcom: Refactor all init steps into phy_poweron

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

Reviewed-by: Stephen Boyd <swboyd@...omium.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.

Powered by blists - more mailing lists