[PATCH v3 0/2] Fix PLL lock timeout and calibration wait time

[PATCH v3 0/2] Fix PLL lock timeout and calibration wait time

[Devarsh Thakkar]

This fixes PLL lockup and O_CMN_READY timeout by moving the polling
function after common state machine gets enabled. Also fix the
calibration wait time to optimize the polling time.

Changelog:
V3: 
-   Use read-modify-write for using calibrated value for PLL
    lock time
-   Move out PLL clock configuration part to power_on function

V2: Separate patch for calibration logic and
    return error code on polling 

Previous versions:
V2: https://lore.kernel.org/all/20250326152320.3835249-1-devarsht@ti.com/
V1: https://lore.kernel.org/all/20241230125319.941372-1-devarsht@ti.com/

Test logs:
Link: https://gist.github.com/devarsht/89e4830e886774fcd50aa6e29cce3a79

Rangediff:
V2->V3:
https://gist.github.com/devarsht/c4d2c4f6715ec7aa4df4cb2c7991b7aa

Devarsh Thakkar (2):
  phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling
  phy: cadence: cdns-dphy: Update calibration wait time for startup
    state machine

 drivers/phy/cadence/cdns-dphy.c | 111 +++++++++++++++++++++++++-------
 1 file changed, 87 insertions(+), 24 deletions(-)

-- 
2.39.1

[PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Devarsh Thakkar]

PLL lockup and O_CMN_READY assertion can only happen after common state
machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
polling them before the common state machine was enabled. To fix this :

- Add new function callbacks for polling on PLL lock and O_CMN_READY
  assertion.
- As state machine and clocks get enabled in power_on callback only, move
  the clock related programming part from configure callback to power_on
  callback and poll for the PLL lockup and O_CMN_READY assertion after
  state machine gets enabled.
- The configure callback only saves the PLL configuration received from the
  client driver which will be applied later on in power_on callback.
- Add checks to ensure configure is called before power_on and state
  machine is in disabled state before power_on callback is called.
- Disable state machine in power_off so that client driver can
  re-configure the PLL by following up a power_off, configure, power_on
  sequence.

Cc: stable@vger.kernel.org
Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V3:
- Move out clock programming logic to power_on as PLL polling and enable
  can happen only after SSM enable
- Disable state machine on power off

V2: 
- Return error code on polling timeout
- Moved out calibration logic to separate patch

 drivers/phy/cadence/cdns-dphy.c | 109 +++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 24 deletions(-)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index ed87a3970f83..a94109a63788 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
 	u8 pll_ipdiv;
 	u8 pll_opdiv;
 	u16 pll_fbdiv;
+	u64 hs_clk_rate;
 	unsigned int nlanes;
 };
 
@@ -99,6 +100,8 @@ struct cdns_dphy_ops {
 	void (*set_pll_cfg)(struct cdns_dphy *dphy,
 			    const struct cdns_dphy_cfg *cfg);
 	unsigned long (*get_wakeup_time_ns)(struct cdns_dphy *dphy);
+	int (*wait_for_pll_lock)(struct cdns_dphy *dphy);
+	int (*wait_for_cmn_ready)(struct cdns_dphy *dphy);
 };
 
 struct cdns_dphy {
@@ -108,6 +111,8 @@ struct cdns_dphy {
 	struct clk *pll_ref_clk;
 	const struct cdns_dphy_ops *ops;
 	struct phy *phy;
+	bool is_configured;
+	bool is_powered;
 };
 
 /* Order of bands is important since the index is the band number. */
@@ -191,6 +196,26 @@ static unsigned long cdns_dphy_get_wakeup_time_ns(struct cdns_dphy *dphy)
 	return dphy->ops->get_wakeup_time_ns(dphy);
 }
 
+static int cdns_dphy_wait_for_pll_lock(struct cdns_dphy *dphy)
+{
+	int ret = 0;
+
+	if (dphy->ops->wait_for_pll_lock)
+		ret = dphy->ops->wait_for_pll_lock(dphy);
+
+	return ret;
+}
+
+static int cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
+{
+	int ret = 0;
+
+	if (dphy->ops->wait_for_cmn_ready)
+		ret = dphy->ops->wait_for_cmn_ready(dphy);
+
+	return ret;
+}
+
 static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
 {
 	/* Default wakeup time is 800 ns (in a simulated environment). */
@@ -232,7 +257,6 @@ static unsigned long cdns_dphy_j721e_get_wakeup_time_ns(struct cdns_dphy *dphy)
 static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
 					const struct cdns_dphy_cfg *cfg)
 {
-	u32 status;
 
 	/*
 	 * set the PWM and PLL Byteclk divider settings to recommended values
@@ -249,13 +273,6 @@ static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
 
 	writel(DPHY_TX_J721E_WIZ_LANE_RSTB,
 	       dphy->regs + DPHY_TX_J721E_WIZ_RST_CTRL);
-
-	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
-			   (status & DPHY_TX_WIZ_PLL_LOCK), 0, POLL_TIMEOUT_US);
-
-	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
-			   (status & DPHY_TX_WIZ_O_CMN_READY), 0,
-			   POLL_TIMEOUT_US);
 }
 
 static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
@@ -263,6 +280,23 @@ static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
 	writel(div, dphy->regs + DPHY_TX_J721E_WIZ_PSM_FREQ);
 }
 
+static int cdns_dphy_j721e_wait_for_pll_lock(struct cdns_dphy *dphy)
+{
+	u32 status;
+
+	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
+			       status & DPHY_TX_WIZ_PLL_LOCK, 0, POLL_TIMEOUT_US);
+}
+
+static int cdns_dphy_j721e_wait_for_cmn_ready(struct cdns_dphy *dphy)
+{
+	u32 status;
+
+	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
+			       status & DPHY_TX_WIZ_O_CMN_READY, 0,
+			       POLL_TIMEOUT_US);
+}
+
 /*
  * This is the reference implementation of DPHY hooks. Specific integration of
  * this IP may have to re-implement some of them depending on how they decided
@@ -278,6 +312,8 @@ static const struct cdns_dphy_ops j721e_dphy_ops = {
 	.get_wakeup_time_ns = cdns_dphy_j721e_get_wakeup_time_ns,
 	.set_pll_cfg = cdns_dphy_j721e_set_pll_cfg,
 	.set_psm_div = cdns_dphy_j721e_set_psm_div,
+	.wait_for_pll_lock = cdns_dphy_j721e_wait_for_pll_lock,
+	.wait_for_cmn_ready = cdns_dphy_j721e_wait_for_cmn_ready,
 };
 
 static int cdns_dphy_config_from_opts(struct phy *phy,
@@ -298,6 +334,7 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
 		return ret;
 
 	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy) / 1000;
+	cfg->hs_clk_rate = opts->hs_clk_rate;
 
 	return 0;
 }
@@ -334,13 +371,23 @@ static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
 static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
 {
 	struct cdns_dphy *dphy = phy_get_drvdata(phy);
-	struct cdns_dphy_cfg cfg = { 0 };
-	int ret, band_ctrl;
-	unsigned int reg;
+	int ret = 0;
 
-	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
-	if (ret)
-		return ret;
+	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &dphy->cfg);
+	if (!ret)
+		dphy->is_configured = true;
+
+	return ret;
+}
+
+static int cdns_dphy_power_on(struct phy *phy)
+{
+	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+	int ret = 0, band_ctrl;
+	u32 reg;
+
+	if (!dphy->is_configured || dphy->is_powered)
+		return -EINVAL;
 
 	/*
 	 * Configure the internal PSM clk divider so that the DPHY has a
@@ -363,9 +410,9 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
 	 * Configure the DPHY PLL that will be used to generate the TX byte
 	 * clk.
 	 */
-	cdns_dphy_set_pll_cfg(dphy, &cfg);
+	cdns_dphy_set_pll_cfg(dphy, &dphy->cfg);
 
-	band_ctrl = cdns_dphy_tx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate);
+	band_ctrl = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
 	if (band_ctrl < 0)
 		return band_ctrl;
 
@@ -373,19 +420,26 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
 	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
 	writel(reg, dphy->regs + DPHY_BAND_CFG);
 
-	return 0;
-}
+	/* Start TX state machine. */
+	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
+	       dphy->regs + DPHY_CMN_SSM);
 
-static int cdns_dphy_power_on(struct phy *phy)
-{
-	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+	ret = cdns_dphy_wait_for_pll_lock(dphy);
+	if (ret) {
+		dev_err(&dphy->phy->dev, "Failed to lock PLL with err %d\n", ret);
+		return ret;
+	}
+
+	ret = cdns_dphy_wait_for_cmn_ready(dphy);
+	if (ret) {
+		dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with err %d\n", ret);
+		return ret;
+	}
 
 	clk_prepare_enable(dphy->psm_clk);
 	clk_prepare_enable(dphy->pll_ref_clk);
 
-	/* Start TX state machine. */
-	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
-	       dphy->regs + DPHY_CMN_SSM);
+	dphy->is_powered = true;
 
 	return 0;
 }
@@ -393,10 +447,17 @@ static int cdns_dphy_power_on(struct phy *phy)
 static int cdns_dphy_power_off(struct phy *phy)
 {
 	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+	u32 reg;
 
 	clk_disable_unprepare(dphy->pll_ref_clk);
 	clk_disable_unprepare(dphy->psm_clk);
 
+	/* Stop TX state machine. */
+	reg = readl(dphy->regs + DPHY_CMN_SSM);
+	writel(reg & ~DPHY_CMN_SSM_EN, dphy->regs + DPHY_CMN_SSM);
+
+	dphy->is_powered = false;
+
 	return 0;
 }
 
-- 
2.39.1

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Vinod Koul]

On 02-05-25, 09:04, Devarsh Thakkar wrote:
> PLL lockup and O_CMN_READY assertion can only happen after common state
> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
> polling them before the common state machine was enabled. To fix this :
> 
> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>   assertion.
> - As state machine and clocks get enabled in power_on callback only, move
>   the clock related programming part from configure callback to power_on
>   callback and poll for the PLL lockup and O_CMN_READY assertion after
>   state machine gets enabled.
> - The configure callback only saves the PLL configuration received from the
>   client driver which will be applied later on in power_on callback.
> - Add checks to ensure configure is called before power_on and state
>   machine is in disabled state before power_on callback is called.
> - Disable state machine in power_off so that client driver can
>   re-configure the PLL by following up a power_off, configure, power_on
>   sequence.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V3:
> - Move out clock programming logic to power_on as PLL polling and enable
>   can happen only after SSM enable
> - Disable state machine on power off
> 
> V2: 
> - Return error code on polling timeout
> - Moved out calibration logic to separate patch
> 
>  drivers/phy/cadence/cdns-dphy.c | 109 +++++++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index ed87a3970f83..a94109a63788 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
>  	u8 pll_ipdiv;
>  	u8 pll_opdiv;
>  	u16 pll_fbdiv;
> +	u64 hs_clk_rate;
>  	unsigned int nlanes;
>  };
>  
> @@ -99,6 +100,8 @@ struct cdns_dphy_ops {
>  	void (*set_pll_cfg)(struct cdns_dphy *dphy,
>  			    const struct cdns_dphy_cfg *cfg);
>  	unsigned long (*get_wakeup_time_ns)(struct cdns_dphy *dphy);
> +	int (*wait_for_pll_lock)(struct cdns_dphy *dphy);
> +	int (*wait_for_cmn_ready)(struct cdns_dphy *dphy);
>  };
>  
>  struct cdns_dphy {
> @@ -108,6 +111,8 @@ struct cdns_dphy {
>  	struct clk *pll_ref_clk;
>  	const struct cdns_dphy_ops *ops;
>  	struct phy *phy;
> +	bool is_configured;
> +	bool is_powered;
>  };
>  
>  /* Order of bands is important since the index is the band number. */
> @@ -191,6 +196,26 @@ static unsigned long cdns_dphy_get_wakeup_time_ns(struct cdns_dphy *dphy)
>  	return dphy->ops->get_wakeup_time_ns(dphy);
>  }
>  
> +static int cdns_dphy_wait_for_pll_lock(struct cdns_dphy *dphy)
> +{
> +	int ret = 0;
> +
> +	if (dphy->ops->wait_for_pll_lock)
> +		ret = dphy->ops->wait_for_pll_lock(dphy);
> +
> +	return ret;
> +}
> +
> +static int cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> +	int ret = 0;
> +
> +	if (dphy->ops->wait_for_cmn_ready)
> +		ret = dphy->ops->wait_for_cmn_ready(dphy);
> +
> +	return ret;

We can drop ret from both, also it is called only once, so maybe open
code this one line?

> +}
> +
>  static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
>  {
>  	/* Default wakeup time is 800 ns (in a simulated environment). */
> @@ -232,7 +257,6 @@ static unsigned long cdns_dphy_j721e_get_wakeup_time_ns(struct cdns_dphy *dphy)
>  static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
>  					const struct cdns_dphy_cfg *cfg)
>  {
> -	u32 status;
>  
>  	/*
>  	 * set the PWM and PLL Byteclk divider settings to recommended values
> @@ -249,13 +273,6 @@ static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
>  
>  	writel(DPHY_TX_J721E_WIZ_LANE_RSTB,
>  	       dphy->regs + DPHY_TX_J721E_WIZ_RST_CTRL);
> -
> -	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> -			   (status & DPHY_TX_WIZ_PLL_LOCK), 0, POLL_TIMEOUT_US);
> -
> -	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> -			   (status & DPHY_TX_WIZ_O_CMN_READY), 0,
> -			   POLL_TIMEOUT_US);
>  }
>  
>  static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
> @@ -263,6 +280,23 @@ static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
>  	writel(div, dphy->regs + DPHY_TX_J721E_WIZ_PSM_FREQ);
>  }
>  
> +static int cdns_dphy_j721e_wait_for_pll_lock(struct cdns_dphy *dphy)
> +{
> +	u32 status;
> +
> +	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> +			       status & DPHY_TX_WIZ_PLL_LOCK, 0, POLL_TIMEOUT_US);
> +}
> +
> +static int cdns_dphy_j721e_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> +	u32 status;
> +
> +	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> +			       status & DPHY_TX_WIZ_O_CMN_READY, 0,
> +			       POLL_TIMEOUT_US);
> +}
> +
>  /*
>   * This is the reference implementation of DPHY hooks. Specific integration of
>   * this IP may have to re-implement some of them depending on how they decided
> @@ -278,6 +312,8 @@ static const struct cdns_dphy_ops j721e_dphy_ops = {
>  	.get_wakeup_time_ns = cdns_dphy_j721e_get_wakeup_time_ns,
>  	.set_pll_cfg = cdns_dphy_j721e_set_pll_cfg,
>  	.set_psm_div = cdns_dphy_j721e_set_psm_div,
> +	.wait_for_pll_lock = cdns_dphy_j721e_wait_for_pll_lock,
> +	.wait_for_cmn_ready = cdns_dphy_j721e_wait_for_cmn_ready,
>  };
>  
>  static int cdns_dphy_config_from_opts(struct phy *phy,
> @@ -298,6 +334,7 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
>  		return ret;
>  
>  	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy) / 1000;
> +	cfg->hs_clk_rate = opts->hs_clk_rate;
>  
>  	return 0;
>  }
> @@ -334,13 +371,23 @@ static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
>  static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  {
>  	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> -	struct cdns_dphy_cfg cfg = { 0 };
> -	int ret, band_ctrl;
> -	unsigned int reg;
> +	int ret = 0;

Superfluous init

>  
> -	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> -	if (ret)
> -		return ret;
> +	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &dphy->cfg);
> +	if (!ret)
> +		dphy->is_configured = true;
> +
> +	return ret;
> +}
> +
> +static int cdns_dphy_power_on(struct phy *phy)
> +{
> +	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> +	int ret = 0, band_ctrl;
> +	u32 reg;
> +
> +	if (!dphy->is_configured || dphy->is_powered)
> +		return -EINVAL;
>  
>  	/*
>  	 * Configure the internal PSM clk divider so that the DPHY has a
> @@ -363,9 +410,9 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	 * Configure the DPHY PLL that will be used to generate the TX byte
>  	 * clk.
>  	 */
> -	cdns_dphy_set_pll_cfg(dphy, &cfg);
> +	cdns_dphy_set_pll_cfg(dphy, &dphy->cfg);
>  
> -	band_ctrl = cdns_dphy_tx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate);
> +	band_ctrl = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
>  	if (band_ctrl < 0)
>  		return band_ctrl;
>  
> @@ -373,19 +420,26 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
>  	writel(reg, dphy->regs + DPHY_BAND_CFG);
>  
> -	return 0;
> -}
> +	/* Start TX state machine. */
> +	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
> +	       dphy->regs + DPHY_CMN_SSM);
>  
> -static int cdns_dphy_power_on(struct phy *phy)
> -{
> -	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> +	ret = cdns_dphy_wait_for_pll_lock(dphy);
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "Failed to lock PLL with err %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = cdns_dphy_wait_for_cmn_ready(dphy);
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with err %d\n", ret);
> +		return ret;
> +	}
>  
>  	clk_prepare_enable(dphy->psm_clk);
>  	clk_prepare_enable(dphy->pll_ref_clk);
>  
> -	/* Start TX state machine. */
> -	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
> -	       dphy->regs + DPHY_CMN_SSM);
> +	dphy->is_powered = true;
>  
>  	return 0;
>  }
> @@ -393,10 +447,17 @@ static int cdns_dphy_power_on(struct phy *phy)
>  static int cdns_dphy_power_off(struct phy *phy)
>  {
>  	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> +	u32 reg;
>  
>  	clk_disable_unprepare(dphy->pll_ref_clk);
>  	clk_disable_unprepare(dphy->psm_clk);
>  
> +	/* Stop TX state machine. */
> +	reg = readl(dphy->regs + DPHY_CMN_SSM);
> +	writel(reg & ~DPHY_CMN_SSM_EN, dphy->regs + DPHY_CMN_SSM);
> +
> +	dphy->is_powered = false;
> +
>  	return 0;
>  }
>  
> -- 
> 2.39.1

-- 
~Vinod

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Devarsh Thakkar]

Hi Vinod,

Thanks for the review.

On 14/05/25 15:59, Vinod Koul wrote:
> On 02-05-25, 09:04, Devarsh Thakkar wrote:
>> PLL lockup and O_CMN_READY assertion can only happen after common state
>> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
>> polling them before the common state machine was enabled. To fix this :
>>
>> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>>    assertion.
>> - As state machine and clocks get enabled in power_on callback only, move
>>    the clock related programming part from configure callback to power_on
>>    callback and poll for the PLL lockup and O_CMN_READY assertion after
>>    state machine gets enabled.
>> - The configure callback only saves the PLL configuration received from the
>>    client driver which will be applied later on in power_on callback.
>> - Add checks to ensure configure is called before power_on and state
>>    machine is in disabled state before power_on callback is called.
>> - Disable state machine in power_off so that client driver can
>>    re-configure the PLL by following up a power_off, configure, power_on
>>    sequence.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V3:
>> - Move out clock programming logic to power_on as PLL polling and enable
>>    can happen only after SSM enable
>> - Disable state machine on power off
>>
>> V2:
>> - Return error code on polling timeout
>> - Moved out calibration logic to separate patch
>>
>>   drivers/phy/cadence/cdns-dphy.c | 109 +++++++++++++++++++++++++-------
>>   1 file changed, 85 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
>> index ed87a3970f83..a94109a63788 100644
>> --- a/drivers/phy/cadence/cdns-dphy.c
>> +++ b/drivers/phy/cadence/cdns-dphy.c
>> @@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
>>   	u8 pll_ipdiv;
>>   	u8 pll_opdiv;
>>   	u16 pll_fbdiv;
>> +	u64 hs_clk_rate;
>>   	unsigned int nlanes;
>>   };
>>   
>> @@ -99,6 +100,8 @@ struct cdns_dphy_ops {
>>   	void (*set_pll_cfg)(struct cdns_dphy *dphy,
>>   			    const struct cdns_dphy_cfg *cfg);
>>   	unsigned long (*get_wakeup_time_ns)(struct cdns_dphy *dphy);
>> +	int (*wait_for_pll_lock)(struct cdns_dphy *dphy);
>> +	int (*wait_for_cmn_ready)(struct cdns_dphy *dphy);
>>   };
>>   
>>   struct cdns_dphy {
>> @@ -108,6 +111,8 @@ struct cdns_dphy {
>>   	struct clk *pll_ref_clk;
>>   	const struct cdns_dphy_ops *ops;
>>   	struct phy *phy;
>> +	bool is_configured;
>> +	bool is_powered;
>>   };
>>   
>>   /* Order of bands is important since the index is the band number. */
>> @@ -191,6 +196,26 @@ static unsigned long cdns_dphy_get_wakeup_time_ns(struct cdns_dphy *dphy)
>>   	return dphy->ops->get_wakeup_time_ns(dphy);
>>   }
>>   
>> +static int cdns_dphy_wait_for_pll_lock(struct cdns_dphy *dphy)
>> +{
>> +	int ret = 0;
>> +
>> +	if (dphy->ops->wait_for_pll_lock)
>> +		ret = dphy->ops->wait_for_pll_lock(dphy);
>> +
>> +	return ret;
>> +}
>> +
>> +static int cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
>> +{
>> +	int ret = 0;
>> +
>> +	if (dphy->ops->wait_for_cmn_ready)
>> +		ret = dphy->ops->wait_for_cmn_ready(dphy);
>> +
>> +	return ret;
> 
> We can drop ret from both, also it is called only once, so maybe open
> code this one line?
> 

Agreed.

>> +}
>> +
>>   static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
>>   {
>>   	/* Default wakeup time is 800 ns (in a simulated environment). */
>> @@ -232,7 +257,6 @@ static unsigned long cdns_dphy_j721e_get_wakeup_time_ns(struct cdns_dphy *dphy)
>>   static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
>>   					const struct cdns_dphy_cfg *cfg)
>>   {
>> -	u32 status;
>>   
>>   	/*
>>   	 * set the PWM and PLL Byteclk divider settings to recommended values
>> @@ -249,13 +273,6 @@ static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
>>   
>>   	writel(DPHY_TX_J721E_WIZ_LANE_RSTB,
>>   	       dphy->regs + DPHY_TX_J721E_WIZ_RST_CTRL);
>> -
>> -	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
>> -			   (status & DPHY_TX_WIZ_PLL_LOCK), 0, POLL_TIMEOUT_US);
>> -
>> -	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
>> -			   (status & DPHY_TX_WIZ_O_CMN_READY), 0,
>> -			   POLL_TIMEOUT_US);
>>   }
>>   
>>   static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
>> @@ -263,6 +280,23 @@ static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
>>   	writel(div, dphy->regs + DPHY_TX_J721E_WIZ_PSM_FREQ);
>>   }
>>   
>> +static int cdns_dphy_j721e_wait_for_pll_lock(struct cdns_dphy *dphy)
>> +{
>> +	u32 status;
>> +
>> +	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
>> +			       status & DPHY_TX_WIZ_PLL_LOCK, 0, POLL_TIMEOUT_US);
>> +}
>> +
>> +static int cdns_dphy_j721e_wait_for_cmn_ready(struct cdns_dphy *dphy)
>> +{
>> +	u32 status;
>> +
>> +	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
>> +			       status & DPHY_TX_WIZ_O_CMN_READY, 0,
>> +			       POLL_TIMEOUT_US);
>> +}
>> +
>>   /*
>>    * This is the reference implementation of DPHY hooks. Specific integration of
>>    * this IP may have to re-implement some of them depending on how they decided
>> @@ -278,6 +312,8 @@ static const struct cdns_dphy_ops j721e_dphy_ops = {
>>   	.get_wakeup_time_ns = cdns_dphy_j721e_get_wakeup_time_ns,
>>   	.set_pll_cfg = cdns_dphy_j721e_set_pll_cfg,
>>   	.set_psm_div = cdns_dphy_j721e_set_psm_div,
>> +	.wait_for_pll_lock = cdns_dphy_j721e_wait_for_pll_lock,
>> +	.wait_for_cmn_ready = cdns_dphy_j721e_wait_for_cmn_ready,
>>   };
>>   
>>   static int cdns_dphy_config_from_opts(struct phy *phy,
>> @@ -298,6 +334,7 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
>>   		return ret;
>>   
>>   	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy) / 1000;
>> +	cfg->hs_clk_rate = opts->hs_clk_rate;
>>   
>>   	return 0;
>>   }
>> @@ -334,13 +371,23 @@ static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
>>   static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>>   {
>>   	struct cdns_dphy *dphy = phy_get_drvdata(phy);
>> -	struct cdns_dphy_cfg cfg = { 0 };
>> -	int ret, band_ctrl;
>> -	unsigned int reg;
>> +	int ret = 0;
> 
> Superfluous init
> 

Agreed. I will do the fixups in next revision.

Thanks,
Devarsh

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Tomi Valkeinen]

Hi,

On 02/05/2025 06:34, Devarsh Thakkar wrote:
> PLL lockup and O_CMN_READY assertion can only happen after common state
> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
> polling them before the common state machine was enabled. To fix this :
> 
> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>   assertion.
> - As state machine and clocks get enabled in power_on callback only, move
>   the clock related programming part from configure callback to power_on
>   callback and poll for the PLL lockup and O_CMN_READY assertion after
>   state machine gets enabled.
> - The configure callback only saves the PLL configuration received from the
>   client driver which will be applied later on in power_on callback.
> - Add checks to ensure configure is called before power_on and state
>   machine is in disabled state before power_on callback is called.
> - Disable state machine in power_off so that client driver can
>   re-configure the PLL by following up a power_off, configure, power_on
>   sequence.

Is the DPHY & PLL documented in the TRM somewhere?

I just find the sequence a bit odd. For example, you wait for the PLL to
lock, and after that, you enable the PLL ref clock. Maybe I'm missing
something here, but... that should not work.

 Tomi

> Cc: stable@vger.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V3:
> - Move out clock programming logic to power_on as PLL polling and enable
>   can happen only after SSM enable
> - Disable state machine on power off
> 
> V2: 
> - Return error code on polling timeout
> - Moved out calibration logic to separate patch
> 
>  drivers/phy/cadence/cdns-dphy.c | 109 +++++++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index ed87a3970f83..a94109a63788 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
>  	u8 pll_ipdiv;
>  	u8 pll_opdiv;
>  	u16 pll_fbdiv;
> +	u64 hs_clk_rate;
>  	unsigned int nlanes;
>  };
>  
> @@ -99,6 +100,8 @@ struct cdns_dphy_ops {
>  	void (*set_pll_cfg)(struct cdns_dphy *dphy,
>  			    const struct cdns_dphy_cfg *cfg);
>  	unsigned long (*get_wakeup_time_ns)(struct cdns_dphy *dphy);
> +	int (*wait_for_pll_lock)(struct cdns_dphy *dphy);
> +	int (*wait_for_cmn_ready)(struct cdns_dphy *dphy);
>  };
>  
>  struct cdns_dphy {
> @@ -108,6 +111,8 @@ struct cdns_dphy {
>  	struct clk *pll_ref_clk;
>  	const struct cdns_dphy_ops *ops;
>  	struct phy *phy;
> +	bool is_configured;
> +	bool is_powered;
>  };
>  
>  /* Order of bands is important since the index is the band number. */
> @@ -191,6 +196,26 @@ static unsigned long cdns_dphy_get_wakeup_time_ns(struct cdns_dphy *dphy)
>  	return dphy->ops->get_wakeup_time_ns(dphy);
>  }
>  
> +static int cdns_dphy_wait_for_pll_lock(struct cdns_dphy *dphy)
> +{
> +	int ret = 0;
> +
> +	if (dphy->ops->wait_for_pll_lock)
> +		ret = dphy->ops->wait_for_pll_lock(dphy);
> +
> +	return ret;
> +}
> +
> +static int cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> +	int ret = 0;
> +
> +	if (dphy->ops->wait_for_cmn_ready)
> +		ret = dphy->ops->wait_for_cmn_ready(dphy);
> +
> +	return ret;
> +}
> +
>  static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
>  {
>  	/* Default wakeup time is 800 ns (in a simulated environment). */
> @@ -232,7 +257,6 @@ static unsigned long cdns_dphy_j721e_get_wakeup_time_ns(struct cdns_dphy *dphy)
>  static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
>  					const struct cdns_dphy_cfg *cfg)
>  {
> -	u32 status;
>  
>  	/*
>  	 * set the PWM and PLL Byteclk divider settings to recommended values
> @@ -249,13 +273,6 @@ static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
>  
>  	writel(DPHY_TX_J721E_WIZ_LANE_RSTB,
>  	       dphy->regs + DPHY_TX_J721E_WIZ_RST_CTRL);
> -
> -	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> -			   (status & DPHY_TX_WIZ_PLL_LOCK), 0, POLL_TIMEOUT_US);
> -
> -	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> -			   (status & DPHY_TX_WIZ_O_CMN_READY), 0,
> -			   POLL_TIMEOUT_US);
>  }
>  
>  static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
> @@ -263,6 +280,23 @@ static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
>  	writel(div, dphy->regs + DPHY_TX_J721E_WIZ_PSM_FREQ);
>  }
>  
> +static int cdns_dphy_j721e_wait_for_pll_lock(struct cdns_dphy *dphy)
> +{
> +	u32 status;
> +
> +	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> +			       status & DPHY_TX_WIZ_PLL_LOCK, 0, POLL_TIMEOUT_US);
> +}
> +
> +static int cdns_dphy_j721e_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> +	u32 status;
> +
> +	return readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> +			       status & DPHY_TX_WIZ_O_CMN_READY, 0,
> +			       POLL_TIMEOUT_US);
> +}
> +
>  /*
>   * This is the reference implementation of DPHY hooks. Specific integration of
>   * this IP may have to re-implement some of them depending on how they decided
> @@ -278,6 +312,8 @@ static const struct cdns_dphy_ops j721e_dphy_ops = {
>  	.get_wakeup_time_ns = cdns_dphy_j721e_get_wakeup_time_ns,
>  	.set_pll_cfg = cdns_dphy_j721e_set_pll_cfg,
>  	.set_psm_div = cdns_dphy_j721e_set_psm_div,
> +	.wait_for_pll_lock = cdns_dphy_j721e_wait_for_pll_lock,
> +	.wait_for_cmn_ready = cdns_dphy_j721e_wait_for_cmn_ready,
>  };
>  
>  static int cdns_dphy_config_from_opts(struct phy *phy,
> @@ -298,6 +334,7 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
>  		return ret;
>  
>  	opts->wakeup = cdns_dphy_get_wakeup_time_ns(dphy) / 1000;
> +	cfg->hs_clk_rate = opts->hs_clk_rate;
>  
>  	return 0;
>  }
> @@ -334,13 +371,23 @@ static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
>  static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  {
>  	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> -	struct cdns_dphy_cfg cfg = { 0 };
> -	int ret, band_ctrl;
> -	unsigned int reg;
> +	int ret = 0;
>  
> -	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> -	if (ret)
> -		return ret;
> +	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &dphy->cfg);
> +	if (!ret)
> +		dphy->is_configured = true;
> +
> +	return ret;
> +}
> +
> +static int cdns_dphy_power_on(struct phy *phy)
> +{
> +	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> +	int ret = 0, band_ctrl;
> +	u32 reg;
> +
> +	if (!dphy->is_configured || dphy->is_powered)
> +		return -EINVAL;
>  
>  	/*
>  	 * Configure the internal PSM clk divider so that the DPHY has a
> @@ -363,9 +410,9 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	 * Configure the DPHY PLL that will be used to generate the TX byte
>  	 * clk.
>  	 */
> -	cdns_dphy_set_pll_cfg(dphy, &cfg);
> +	cdns_dphy_set_pll_cfg(dphy, &dphy->cfg);
>  
> -	band_ctrl = cdns_dphy_tx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate);
> +	band_ctrl = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
>  	if (band_ctrl < 0)
>  		return band_ctrl;
>  
> @@ -373,19 +420,26 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
>  	writel(reg, dphy->regs + DPHY_BAND_CFG);
>  
> -	return 0;
> -}
> +	/* Start TX state machine. */
> +	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
> +	       dphy->regs + DPHY_CMN_SSM);
>  
> -static int cdns_dphy_power_on(struct phy *phy)
> -{
> -	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> +	ret = cdns_dphy_wait_for_pll_lock(dphy);
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "Failed to lock PLL with err %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = cdns_dphy_wait_for_cmn_ready(dphy);
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with err %d\n", ret);
> +		return ret;
> +	}
>  
>  	clk_prepare_enable(dphy->psm_clk);
>  	clk_prepare_enable(dphy->pll_ref_clk);
>  
> -	/* Start TX state machine. */
> -	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
> -	       dphy->regs + DPHY_CMN_SSM);
> +	dphy->is_powered = true;
>  
>  	return 0;
>  }
> @@ -393,10 +447,17 @@ static int cdns_dphy_power_on(struct phy *phy)
>  static int cdns_dphy_power_off(struct phy *phy)
>  {
>  	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> +	u32 reg;
>  
>  	clk_disable_unprepare(dphy->pll_ref_clk);
>  	clk_disable_unprepare(dphy->psm_clk);
>  
> +	/* Stop TX state machine. */
> +	reg = readl(dphy->regs + DPHY_CMN_SSM);
> +	writel(reg & ~DPHY_CMN_SSM_EN, dphy->regs + DPHY_CMN_SSM);
> +
> +	dphy->is_powered = false;
> +
>  	return 0;
>  }
>  

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Devarsh Thakkar]

Hi Tomi,

Thanks for the review.

On 26/05/25 16:29, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/05/2025 06:34, Devarsh Thakkar wrote:
>> PLL lockup and O_CMN_READY assertion can only happen after common state
>> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
>> polling them before the common state machine was enabled. To fix this :
>>
>> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>>    assertion.
>> - As state machine and clocks get enabled in power_on callback only, move
>>    the clock related programming part from configure callback to power_on
>>    callback and poll for the PLL lockup and O_CMN_READY assertion after
>>    state machine gets enabled.
>> - The configure callback only saves the PLL configuration received from the
>>    client driver which will be applied later on in power_on callback.
>> - Add checks to ensure configure is called before power_on and state
>>    machine is in disabled state before power_on callback is called.
>> - Disable state machine in power_off so that client driver can
>>    re-configure the PLL by following up a power_off, configure, power_on
>>    sequence.
> 
> Is the DPHY & PLL documented in the TRM somewhere?
> 

I had got this information from cadence support. But I think it is also 
documented in J721E TRM [1]. DPHY Tx startup sequence is same as DPHY Tx 
and there is a initialization diagram for the same in J721E TRM [1] 
referenced at 12.7.2.4.1.2.1 Start-up Sequence Timing Diagram. It shows 
O_CMN_READY polling at the end after common configuration pin setup and 
Table 12-1533. Common Configuration-Related Setup mentions state machine 
enable part under the common configuration setup which happens before 
the polling. And the observations with this patch do sync with 
understanding as we see PLL locking up faster without any timeout which 
was the case before this patch.

> I just find the sequence a bit odd. For example, you wait for the PLL to
> lock, and after that, you enable the PLL ref clock. Maybe I'm missing
> something here, but... that should not work.

I think it's my bad, but it works somehow. But it makes sense to enable 
the clock before we start polling for PLL lock, so will update it in 
next revision.

> 
>   Tomi
> 
Regards
Devarsh

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Devarsh Thakkar]

Hello,

On 20/06/25 20:05, Devarsh Thakkar wrote:
> Hi Tomi,
> 
> Thanks for the review.
> 
> On 26/05/25 16:29, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 02/05/2025 06:34, Devarsh Thakkar wrote:
>>> PLL lockup and O_CMN_READY assertion can only happen after common state
>>> machine gets enabled (by programming DPHY_CMN_SSM register), but 
>>> driver was
>>> polling them before the common state machine was enabled. To fix this :
>>>
>>> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>>>    assertion.
>>> - As state machine and clocks get enabled in power_on callback only, 
>>> move
>>>    the clock related programming part from configure callback to 
>>> power_on
>>>    callback and poll for the PLL lockup and O_CMN_READY assertion after
>>>    state machine gets enabled.
>>> - The configure callback only saves the PLL configuration received 
>>> from the
>>>    client driver which will be applied later on in power_on callback.
>>> - Add checks to ensure configure is called before power_on and state
>>>    machine is in disabled state before power_on callback is called.
>>> - Disable state machine in power_off so that client driver can
>>>    re-configure the PLL by following up a power_off, configure, power_on
>>>    sequence.
>>
>> Is the DPHY & PLL documented in the TRM somewhere?
>>
> 
> I had got this information from cadence support. But I think it is also 
> documented in J721E TRM [1]. DPHY Tx startup sequence is same as DPHY Tx 
> and there is a initialization diagram for the same in J721E TRM [1] 
> referenced at 12.7.2.4.1.2.1 Start-up Sequence Timing Diagram. It shows 
> O_CMN_READY polling at the end after common configuration pin setup and 
> Table 12-1533. Common Configuration-Related Setup mentions state machine 
> enable part under the common configuration setup which happens before 
> the polling. And the observations with this patch do sync with 
> understanding as we see PLL locking up faster without any timeout which 
> was the case before this patch.
> 
>> I just find the sequence a bit odd. For example, you wait for the PLL to
>> lock, and after that, you enable the PLL ref clock. Maybe I'm missing
>> something here, but... that should not work.
> 
> I think it's my bad, but it works somehow. But it makes sense to enable 
> the clock before we start polling for PLL lock, so will update it in 
> next revision.
> 

Sorry, had missed to add the reference :

[1]: https://www.ti.com/lit/zip/spruil1

Regards
Devarsh

>>
>>   Tomi
>>
> Regards
> Devarsh

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Tomi Valkeinen]

Hi,

On 02/05/2025 06:34, Devarsh Thakkar wrote:
> PLL lockup and O_CMN_READY assertion can only happen after common state
> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
> polling them before the common state machine was enabled. To fix this :
> 
> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>   assertion.
> - As state machine and clocks get enabled in power_on callback only, move
>   the clock related programming part from configure callback to power_on
>   callback and poll for the PLL lockup and O_CMN_READY assertion after
>   state machine gets enabled.
> - The configure callback only saves the PLL configuration received from the
>   client driver which will be applied later on in power_on callback.
> - Add checks to ensure configure is called before power_on and state
>   machine is in disabled state before power_on callback is called.
> - Disable state machine in power_off so that client driver can
>   re-configure the PLL by following up a power_off, configure, power_on
>   sequence.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V3:
> - Move out clock programming logic to power_on as PLL polling and enable
>   can happen only after SSM enable
> - Disable state machine on power off
> 
> V2: 
> - Return error code on polling timeout
> - Moved out calibration logic to separate patch
> 
>  drivers/phy/cadence/cdns-dphy.c | 109 +++++++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index ed87a3970f83..a94109a63788 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
>  	u8 pll_ipdiv;
>  	u8 pll_opdiv;
>  	u16 pll_fbdiv;
> +	u64 hs_clk_rate;

This has a minor conflict with my cdns-dsi series, as I also add
hs_clk_rate but as u32. Also, applying both serieses,
cdns_dphy_config_from_opts() becomes odd as the hs_clk_rate will be
assigned back and forth between opts and cfg. Can you check this?

 Tomi

Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Devarsh Thakkar]

Hi,

On 18/06/25 15:31, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/05/2025 06:34, Devarsh Thakkar wrote:
>> PLL lockup and O_CMN_READY assertion can only happen after common state
>> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
>> polling them before the common state machine was enabled. To fix this :
>>
>> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>>    assertion.
>> - As state machine and clocks get enabled in power_on callback only, move
>>    the clock related programming part from configure callback to power_on
>>    callback and poll for the PLL lockup and O_CMN_READY assertion after
>>    state machine gets enabled.
>> - The configure callback only saves the PLL configuration received from the
>>    client driver which will be applied later on in power_on callback.
>> - Add checks to ensure configure is called before power_on and state
>>    machine is in disabled state before power_on callback is called.
>> - Disable state machine in power_off so that client driver can
>>    re-configure the PLL by following up a power_off, configure, power_on
>>    sequence.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V3:
>> - Move out clock programming logic to power_on as PLL polling and enable
>>    can happen only after SSM enable
>> - Disable state machine on power off
>>
>> V2:
>> - Return error code on polling timeout
>> - Moved out calibration logic to separate patch
>>
>>   drivers/phy/cadence/cdns-dphy.c | 109 +++++++++++++++++++++++++-------
>>   1 file changed, 85 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
>> index ed87a3970f83..a94109a63788 100644
>> --- a/drivers/phy/cadence/cdns-dphy.c
>> +++ b/drivers/phy/cadence/cdns-dphy.c
>> @@ -79,6 +79,7 @@ struct cdns_dphy_cfg {
>>   	u8 pll_ipdiv;
>>   	u8 pll_opdiv;
>>   	u16 pll_fbdiv;
>> +	u64 hs_clk_rate;
> 
> This has a minor conflict with my cdns-dsi series, as I also add
> hs_clk_rate but as u32. Also, applying both serieses,
> cdns_dphy_config_from_opts() becomes odd as the hs_clk_rate will be
> assigned back and forth between opts and cfg. Can you check this?

Understood. I think after re-basing on top of your patch, I don't need 
to assign cfg->hs_clk_rate in cdns_dphy_config_from_opts since you 
already assigned it to realized clock value in 
cdns_dsi_get_dphy_pll_cfg. I can rebase on top of your series.


Regards
Devarsh

[PATCH v3 2/2] phy: cadence: cdns-dphy: Update calibration wait time for startup state machine

[Devarsh Thakkar]

Do read-modify-write so that we re-use the characterized reset value as
specified in TRM [1] to program calibration wait time which defines number
of cycles to wait for after startup state machine is in bandgap enable
state.

This fixes PLL lock timeout error faced while using RPi DSI Panel on TI's
AM62L and J721E SoC [2].

[1] AM62P TRM (Section ): https://www.ti.com/lit/pdf/spruj83

[2]: Link:
https://gist.github.com/devarsht/89e4830e886774fcd50aa6e29cce3a79

Cc: stable@vger.kernel.org
Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V3: Do read-modify-write to preserve reset value for calibration wait time
V2: Introduced this as as separate patch

 drivers/phy/cadence/cdns-dphy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index a94109a63788..2e28c56c4bdd 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -30,6 +30,7 @@
 
 #define DPHY_CMN_SSM			DPHY_PMA_CMN(0x20)
 #define DPHY_CMN_SSM_EN			BIT(0)
+#define DPHY_CMN_SSM_CAL_WAIT_TIME	GENMASK(8, 1)
 #define DPHY_CMN_TX_MODE_EN		BIT(9)
 
 #define DPHY_CMN_PWM			DPHY_PMA_CMN(0x40)
@@ -421,7 +422,8 @@ static int cdns_dphy_power_on(struct phy *phy)
 	writel(reg, dphy->regs + DPHY_BAND_CFG);
 
 	/* Start TX state machine. */
-	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
+	reg = readl(dphy->regs + DPHY_CMN_SSM);
+	writel((reg & DPHY_CMN_SSM_CAL_WAIT_TIME) | DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
 	       dphy->regs + DPHY_CMN_SSM);
 
 	ret = cdns_dphy_wait_for_pll_lock(dphy);
-- 
2.39.1