On Fri, Jan 12, 2024 at 09:03:48PM +0530, Nitin Rawat wrote:
> Commit 052553af6a31 ("ufs/phy: qcom: Refactor to use phy_init call")
> puts enabling regulators & clks, calibrating UFS PHY, starting serdes
> and polling PCS ready status into phy_power_on.
>
> In Current code regulators enable, clks enable, calibrating UFS PHY,
> start_serdes and polling PCS_ready_status are part of phy_power_on.
>
> UFS PHY registers are retained after power collapse, meaning calibrating
> UFS PHY, start_serdes and polling PCS_ready_status can be done only when
> hba is powered_on, and not needed every time when phy_power_on is called
> during resume. Hence keep the code which enables PHY's regulators & clks
> in phy_power_on and move the rest steps into phy_calibrate function.
>
> Refactor the code to enable PHY regulators & clks in phy_power_on and
> move rest of the code to phy_calibrate function.
>
This patch should come before UFS patch since you are introducing the
calibrate() callback here only.
> Co-developed-by: Can Guo <quic_cang@xxxxxxxxxxx>
> Signed-off-by: Can Guo <quic_cang@xxxxxxxxxxx>
> Co-developed-by: Naveen Kumar Goud Arepalli <quic_narepall@xxxxxxxxxxx>
> Signed-off-by: Naveen Kumar Goud Arepalli <quic_narepall@xxxxxxxxxxx>
> Signed-off-by: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 183 +++++++++---------------
> 1 file changed, 67 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 3c2e6255e26f..ae0218738b0b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -32,14 +32,15 @@
> /* QPHY_SW_RESET bit */
> #define SW_RESET BIT(0)
> /* QPHY_POWER_DOWN_CONTROL */
> -#define SW_PWRDN BIT(0)
> +#define SW_PWRUP BIT(0)
> +#define SW_PWRDN 0
Why 0?
> /* QPHY_START_CONTROL bits */
> #define SERDES_START BIT(0)
> #define PCS_START BIT(1)
> /* QPHY_PCS_READY_STATUS bit */
> #define PCS_READY BIT(0)
>
> -#define PHY_INIT_COMPLETE_TIMEOUT 10000
> +#define PHY_INIT_COMPLETE_TIMEOUT 1000000
Why? This is not mentioned in the commit message. If it is not related to this
refactoring, then it should be a separate patch with justification.
>
> struct qmp_phy_init_tbl {
> unsigned int offset;
> @@ -1464,8 +1465,25 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
> qmp_ufs_pcs_init(qmp, &cfg->tbls_hs_g4);
> }
>
> -static int qmp_ufs_com_init(struct qmp_ufs *qmp)
> +static int qmp_ufs_power_off(struct phy *phy)
> +{
> + struct qmp_ufs *qmp = phy_get_drvdata(phy);
> + const struct qmp_phy_cfg *cfg = qmp->cfg;
> +
> + /* Put PHY into POWER DOWN state: active low */
> + qphy_clrbits(qmp->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> + SW_PWRDN);
> +
> + clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
> +
> + regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> +
> + return 0;
> +}
> +
> +static int qmp_ufs_power_on(struct phy *phy)
> {
> + struct qmp_ufs *qmp = phy_get_drvdata(phy);
> const struct qmp_phy_cfg *cfg = qmp->cfg;
> void __iomem *pcs = qmp->pcs;
> int ret;
> @@ -1480,8 +1498,7 @@ static int qmp_ufs_com_init(struct qmp_ufs *qmp)
> if (ret)
> goto err_disable_regulators;
>
> - qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
> -
> + qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRUP);
Newline please. As mentioned above, why can't you use existing SW_PWRDN macro.
> return 0;
>
> err_disable_regulators:
> @@ -1490,61 +1507,7 @@ static int qmp_ufs_com_init(struct qmp_ufs *qmp)
> return ret;
> }
>
[...]
> +static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
> +{
> + const struct qmp_phy_cfg *cfg = qmp->cfg;
> + int ret;
> +
> + if (!cfg->no_pcs_sw_reset)
> + return 0;
> +
> + /*
> + * Get UFS reset, which is delayed until now to avoid a
> + * circular dependency where UFS needs its PHY, but the PHY
> + * needs this UFS reset.
> + */
> +
> + qmp->ufs_reset = devm_reset_control_get_exclusive(qmp->dev,
> + "ufsphy");
You have moved this to probe from power_on() without any justification. What
about the circular dependency mentioned in the comment.
- Mani
--
மணிவண்ணன் சதாசிவம்
