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

[PATCH v4 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.

NOTE: This needs to be applied on top of  
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/           
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/           
  from the series:                                                                                  
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.com/    

Changelog:
V4:                                                                                                 
- Optimize wait_for_pll_lock, wait_for_cmn_ready calls to oneline                                   
  using conditional operator                                                                        
- Remove superflous init for ret variable in cdns_dphy_configure                                    
- Enable pll and psm ref clocks before configuring PLL                                              
- Update commit message to refer to TRM                                                             
- Rebased on top of:                                                                                
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/           
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/           
  from the series:                                                                                  
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.com/    

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:
V3: https://lore.kernel.org/all/20250502033451.2291330-1-devarsht@ti.com/
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/d08d851399ca327e5594266a8d66d478

Rangediff:
V3->V4:
https://gist.github.com/devarsht/e4db52e1f4aec2d45596b3ed019e92ef

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 | 126 ++++++++++++++++++++++++--------
 1 file changed, 94 insertions(+), 32 deletions(-)

-- 
2.39.1

[PATCH v4 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 which is
incorrect.  This is as per the DPHY initialization sequence as mentioned in
J721E TRM [1] at section "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 where the common configuration pin setup step enables state machine
as referenced in "Table 12-1533. Common Configuration-Related Setup
mentions state machine"

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.

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

Cc: stable@vger.kernel.org
Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V4:
- Optimize wait_for_pll_lock, wait_for_cmn_ready calls to oneline
  using conditional operator
- Remove superflous init for ret variable in cdns_dphy_configure
- Enable pll and psm ref clocks before configuring PLL
- Update commit message to refer to TRM
- Rebased on top of:
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/
  from the series:
  https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.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 | 124 +++++++++++++++++++++++---------
 1 file changed, 92 insertions(+), 32 deletions(-)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index 33a42e72362e..da8de0a9d086 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -92,6 +92,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 {
@@ -101,6 +103,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. */
@@ -186,6 +190,16 @@ 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)
+{
+	return dphy->ops->wait_for_pll_lock ? dphy->ops->wait_for_pll_lock(dphy) : 0;
+}
+
+static int cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
+{
+	return  dphy->ops->wait_for_cmn_ready ? dphy->ops->wait_for_cmn_ready(dphy) : 0;
+}
+
 static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
 {
 	/* Default wakeup time is 800 ns (in a simulated environment). */
@@ -227,7 +241,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
@@ -244,13 +257,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)
@@ -258,6 +264,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
@@ -273,6 +296,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,
@@ -328,21 +353,36 @@ 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;
 
-	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;
+	u32 reg;
+
+	if (!dphy->is_configured || dphy->is_powered)
+		return -EINVAL;
+
+	clk_prepare_enable(dphy->psm_clk);
+	clk_prepare_enable(dphy->pll_ref_clk);
 
 	/*
 	 * Configure the internal PSM clk divider so that the DPHY has a
 	 * 1MHz clk (or something close).
 	 */
 	ret = cdns_dphy_setup_psm(dphy);
-	if (ret)
-		return ret;
+	if (ret) {
+		dev_err(&dphy->phy->dev, "Failed to setup PSM with error %d\n", ret);
+		goto err_power_on;
+	}
 
 	/*
 	 * Configure attach clk lanes to data lanes: the DPHY has 2 clk lanes
@@ -357,40 +397,60 @@ 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);
-	if (band_ctrl < 0)
-		return band_ctrl;
+	ret = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
+	if (ret < 0) {
+		dev_err(&dphy->phy->dev, "Failed to get band control value with error %d\n", ret);
+		goto err_power_on;
+	}
 
-	reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, band_ctrl) |
-	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
+	reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, ret) |
+	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, ret);
 	writel(reg, dphy->regs + DPHY_BAND_CFG);
 
-	return 0;
-}
-
-static int cdns_dphy_power_on(struct phy *phy)
-{
-	struct cdns_dphy *dphy = phy_get_drvdata(phy);
-
-	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);
 
+	ret = cdns_dphy_wait_for_pll_lock(dphy);
+	if (ret) {
+		dev_err(&dphy->phy->dev, "Failed to lock PLL with error %d\n", ret);
+		goto err_power_on;
+	}
+
+	ret = cdns_dphy_wait_for_cmn_ready(dphy);
+	if (ret) {
+		dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with error %d\n",
+			ret);
+		goto err_power_on;
+	}
+
+	dphy->is_powered = true;
+
 	return 0;
+
+err_power_on:
+	clk_disable_unprepare(dphy->pll_ref_clk);
+	clk_disable_unprepare(dphy->psm_clk);
+
+	return ret;
 }
 
 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

[PATCH v4 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 since earlier calibration wait time was getting
overwritten to zero value thus failing the PLL to lockup and causing
timeout.

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

Cc: stable@vger.kernel.org
Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V4: No change
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 da8de0a9d086..24a25606996c 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)
@@ -410,7 +411,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

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

[kernel test robot]

Hi Devarsh,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.16-rc4 next-20250704]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Devarsh-Thakkar/phy-cadence-cdns-dphy-Fix-PLL-lock-and-O_CMN_READY-polling/20250704-210349
base:   linus/master
patch link:    https://lore.kernel.org/r/20250704125915.1224738-2-devarsht%40ti.com
patch subject: [PATCH v4 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling
config: x86_64-buildonly-randconfig-003-20250705 (https://download.01.org/0day-ci/archive/20250705/202507051038.XCl5miJ7-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250705/202507051038.XCl5miJ7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507051038.XCl5miJ7-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/phy/cadence/cdns-dphy.c:408:45: error: no member named 'hs_clk_rate' in 'struct cdns_dphy_cfg'
     408 |         ret = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
         |                                          ~~~~~~~~~ ^
   1 error generated.


vim +408 drivers/phy/cadence/cdns-dphy.c

   370	
   371	static int cdns_dphy_power_on(struct phy *phy)
   372	{
   373		struct cdns_dphy *dphy = phy_get_drvdata(phy);
   374		int ret;
   375		u32 reg;
   376	
   377		if (!dphy->is_configured || dphy->is_powered)
   378			return -EINVAL;
   379	
   380		clk_prepare_enable(dphy->psm_clk);
   381		clk_prepare_enable(dphy->pll_ref_clk);
   382	
   383		/*
   384		 * Configure the internal PSM clk divider so that the DPHY has a
   385		 * 1MHz clk (or something close).
   386		 */
   387		ret = cdns_dphy_setup_psm(dphy);
   388		if (ret) {
   389			dev_err(&dphy->phy->dev, "Failed to setup PSM with error %d\n", ret);
   390			goto err_power_on;
   391		}
   392	
   393		/*
   394		 * Configure attach clk lanes to data lanes: the DPHY has 2 clk lanes
   395		 * and 8 data lanes, each clk lane can be attache different set of
   396		 * data lanes. The 2 groups are named 'left' and 'right', so here we
   397		 * just say that we want the 'left' clk lane to drive the 'left' data
   398		 * lanes.
   399		 */
   400		cdns_dphy_set_clk_lane_cfg(dphy, DPHY_CLK_CFG_LEFT_DRIVES_LEFT);
   401	
   402		/*
   403		 * Configure the DPHY PLL that will be used to generate the TX byte
   404		 * clk.
   405		 */
   406		cdns_dphy_set_pll_cfg(dphy, &dphy->cfg);
   407	
 > 408		ret = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
   409		if (ret < 0) {
   410			dev_err(&dphy->phy->dev, "Failed to get band control value with error %d\n", ret);
   411			goto err_power_on;
   412		}
   413	
   414		reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, ret) |
   415		      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, ret);
   416		writel(reg, dphy->regs + DPHY_BAND_CFG);
   417	
   418		/* Start TX state machine. */
   419		writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
   420		       dphy->regs + DPHY_CMN_SSM);
   421	
   422		ret = cdns_dphy_wait_for_pll_lock(dphy);
   423		if (ret) {
   424			dev_err(&dphy->phy->dev, "Failed to lock PLL with error %d\n", ret);
   425			goto err_power_on;
   426		}
   427	
   428		ret = cdns_dphy_wait_for_cmn_ready(dphy);
   429		if (ret) {
   430			dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with error %d\n",
   431				ret);
   432			goto err_power_on;
   433		}
   434	
   435		dphy->is_powered = true;
   436	
   437		return 0;
   438	
   439	err_power_on:
   440		clk_disable_unprepare(dphy->pll_ref_clk);
   441		clk_disable_unprepare(dphy->psm_clk);
   442	
   443		return ret;
   444	}
   445	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Devarsh Thakkar]

Hello,

On 05/07/25 08:32, kernel test robot wrote:
> Hi Devarsh,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.16-rc4 next-20250704]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Devarsh-Thakkar/phy-cadence-cdns-dphy-Fix-PLL-lock-and-O_CMN_READY-polling/20250704-210349
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20250704125915.1224738-2-devarsht%40ti.com
> patch subject: [PATCH v4 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling
> config: x86_64-buildonly-randconfig-003-20250705 (https://download.01.org/0day-ci/archive/20250705/202507051038.XCl5miJ7-lkp@intel.com/config)
> compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250705/202507051038.XCl5miJ7-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202507051038.XCl5miJ7-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/phy/cadence/cdns-dphy.c:408:45: error: no member named 'hs_clk_rate' in 'struct cdns_dphy_cfg'
>       408 |         ret = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
>           |                                          ~~~~~~~~~ ^
>     1 error generated.
> 

This is expected, since as mentioned in cover-letter this patch needs to 
be applied on top of [1] and [2] from the series [3] as suggested in the 
review [4] for the previous revision.

[1]: 
https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/ 

[2]: 
https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/ 

[3]: 
https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.com/
[4]: 
https://lore.kernel.org/all/218ba1a0-068d-4bb2-bba2-2739afa7f470@ideasonboard.com/

Regards
Devarsh

> 
> vim +408 drivers/phy/cadence/cdns-dphy.c
> 
>     370	
>     371	static int cdns_dphy_power_on(struct phy *phy)
>     372	{
>     373		struct cdns_dphy *dphy = phy_get_drvdata(phy);
>     374		int ret;
>     375		u32 reg;
>     376	
>     377		if (!dphy->is_configured || dphy->is_powered)
>     378			return -EINVAL;
>     379	
>     380		clk_prepare_enable(dphy->psm_clk);
>     381		clk_prepare_enable(dphy->pll_ref_clk);
>     382	
>     383		/*
>     384		 * Configure the internal PSM clk divider so that the DPHY has a
>     385		 * 1MHz clk (or something close).
>     386		 */
>     387		ret = cdns_dphy_setup_psm(dphy);
>     388		if (ret) {
>     389			dev_err(&dphy->phy->dev, "Failed to setup PSM with error %d\n", ret);
>     390			goto err_power_on;
>     391		}
>     392	
>     393		/*
>     394		 * Configure attach clk lanes to data lanes: the DPHY has 2 clk lanes
>     395		 * and 8 data lanes, each clk lane can be attache different set of
>     396		 * data lanes. The 2 groups are named 'left' and 'right', so here we
>     397		 * just say that we want the 'left' clk lane to drive the 'left' data
>     398		 * lanes.
>     399		 */
>     400		cdns_dphy_set_clk_lane_cfg(dphy, DPHY_CLK_CFG_LEFT_DRIVES_LEFT);
>     401	
>     402		/*
>     403		 * Configure the DPHY PLL that will be used to generate the TX byte
>     404		 * clk.
>     405		 */
>     406		cdns_dphy_set_pll_cfg(dphy, &dphy->cfg);
>     407	
>   > 408		ret = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
>     409		if (ret < 0) {
>     410			dev_err(&dphy->phy->dev, "Failed to get band control value with error %d\n", ret);
>     411			goto err_power_on;
>     412		}
>     413	
>     414		reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, ret) |
>     415		      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, ret);
>     416		writel(reg, dphy->regs + DPHY_BAND_CFG);
>     417	
>     418		/* Start TX state machine. */
>     419		writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
>     420		       dphy->regs + DPHY_CMN_SSM);
>     421	
>     422		ret = cdns_dphy_wait_for_pll_lock(dphy);
>     423		if (ret) {
>     424			dev_err(&dphy->phy->dev, "Failed to lock PLL with error %d\n", ret);
>     425			goto err_power_on;
>     426		}
>     427	
>     428		ret = cdns_dphy_wait_for_cmn_ready(dphy);
>     429		if (ret) {
>     430			dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with error %d\n",
>     431				ret);
>     432			goto err_power_on;
>     433		}
>     434	
>     435		dphy->is_powered = true;
>     436	
>     437		return 0;
>     438	
>     439	err_power_on:
>     440		clk_disable_unprepare(dphy->pll_ref_clk);
>     441		clk_disable_unprepare(dphy->psm_clk);
>     442	
>     443		return ret;
>     444	}
>     445	
> 

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

[Harikrishna Shenoy]

Hi Devarsh,

Thank you for the patch.

On 04/07/25 18:29, Devarsh Thakkar wrote:
> 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 since earlier calibration wait time was getting
> overwritten to zero value thus failing the PLL to lockup and causing
> timeout.
>
> [1] AM62P TRM (Section 14.8.6.3.2.1.1 DPHY_TX_DPHYTX_CMN0_CMN_DIG_TBIT2):
> Link: https://www.ti.com/lit/pdf/spruj83
>
> Cc: stable@vger.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>


Tested-by: Harikrishna Shenoy <h-shenoy@ti.com>


> ---
> V4: No change
> 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 da8de0a9d086..24a25606996c 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)
> @@ -410,7 +411,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);
>

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

[Harikrishna Shenoy]

Hi Devarsh,

Thanks for the patch.

On 04/07/25 18:29, 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 which is
> incorrect.  This is as per the DPHY initialization sequence as mentioned in
> J721E TRM [1] at section "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 where the common configuration pin setup step enables state machine
> as referenced in "Table 12-1533. Common Configuration-Related Setup
> mentions state machine"
>
> 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.
>
> [1]: https://www.ti.com/lit/zip/spruil1
>
> Cc: stable@vger.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>

Tested-by: Harikrishna Shenoy <h-shenoy@ti.com>


> ---
> V4:
> - Optimize wait_for_pll_lock, wait_for_cmn_ready calls to oneline
>    using conditional operator
> - Remove superflous init for ret variable in cdns_dphy_configure
> - Enable pll and psm ref clocks before configuring PLL
> - Update commit message to refer to TRM
> - Rebased on top of:
>    https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/
>    https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/
>    from the series:
>    https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.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 | 124 +++++++++++++++++++++++---------
>   1 file changed, 92 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index 33a42e72362e..da8de0a9d086 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -92,6 +92,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 {
> @@ -101,6 +103,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. */
> @@ -186,6 +190,16 @@ 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)
> +{
> +	return dphy->ops->wait_for_pll_lock ? dphy->ops->wait_for_pll_lock(dphy) : 0;
> +}
> +
> +static int cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> +	return  dphy->ops->wait_for_cmn_ready ? dphy->ops->wait_for_cmn_ready(dphy) : 0;
> +}
> +
>   static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
>   {
>   	/* Default wakeup time is 800 ns (in a simulated environment). */
> @@ -227,7 +241,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
> @@ -244,13 +257,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)
> @@ -258,6 +264,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
> @@ -273,6 +296,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,
> @@ -328,21 +353,36 @@ 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;
>   
> -	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;
> +	u32 reg;
> +
> +	if (!dphy->is_configured || dphy->is_powered)
> +		return -EINVAL;
> +
> +	clk_prepare_enable(dphy->psm_clk);
> +	clk_prepare_enable(dphy->pll_ref_clk);
>   
>   	/*
>   	 * Configure the internal PSM clk divider so that the DPHY has a
>   	 * 1MHz clk (or something close).
>   	 */
>   	ret = cdns_dphy_setup_psm(dphy);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "Failed to setup PSM with error %d\n", ret);
> +		goto err_power_on;
> +	}
>   
>   	/*
>   	 * Configure attach clk lanes to data lanes: the DPHY has 2 clk lanes
> @@ -357,40 +397,60 @@ 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);
> -	if (band_ctrl < 0)
> -		return band_ctrl;
> +	ret = cdns_dphy_tx_get_band_ctrl(dphy->cfg.hs_clk_rate);
> +	if (ret < 0) {
> +		dev_err(&dphy->phy->dev, "Failed to get band control value with error %d\n", ret);
> +		goto err_power_on;
> +	}
>   
> -	reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, band_ctrl) |
> -	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
> +	reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, ret) |
> +	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, ret);
>   	writel(reg, dphy->regs + DPHY_BAND_CFG);
>   
> -	return 0;
> -}
> -
> -static int cdns_dphy_power_on(struct phy *phy)
> -{
> -	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> -
> -	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);
>   
> +	ret = cdns_dphy_wait_for_pll_lock(dphy);
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "Failed to lock PLL with error %d\n", ret);
> +		goto err_power_on;
> +	}
> +
> +	ret = cdns_dphy_wait_for_cmn_ready(dphy);
> +	if (ret) {
> +		dev_err(&dphy->phy->dev, "O_CMN_READY signal failed to assert with error %d\n",
> +			ret);
> +		goto err_power_on;
> +	}
> +
> +	dphy->is_powered = true;
> +
>   	return 0;
> +
> +err_power_on:
> +	clk_disable_unprepare(dphy->pll_ref_clk);
> +	clk_disable_unprepare(dphy->psm_clk);
> +
> +	return ret;
>   }
>   
>   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 v4 2/2] phy: cadence: cdns-dphy: Update calibration wait time for startup state machine

[Tomi Valkeinen]

Hi,

On 04/07/2025 15:59, Devarsh Thakkar wrote:
> 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 since earlier calibration wait time was getting
> overwritten to zero value thus failing the PLL to lockup and causing
> timeout.
> 
> [1] AM62P TRM (Section 14.8.6.3.2.1.1 DPHY_TX_DPHYTX_CMN0_CMN_DIG_TBIT2):
> Link: https://www.ti.com/lit/pdf/spruj83
> 
> Cc: stable@vger.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V4: No change
> 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 da8de0a9d086..24a25606996c 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)
> @@ -410,7 +411,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);

That's not how you do read-modify-write. You should first read the
register, then clear the fields you want to set/clear, then set the
fields, then write.

reg = readl(dphy->regs + DPHY_CMN_SSM, dphy->regs + DPHY_CMN_SSM);
reg &= ~(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN)
reg |= DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN;
writel(reg, dphy->regs + DPHY_CMN_SSM);

That's the general form, in this particular case the and-line is not
necessary, of course.

There's also FIELD_MODIFY() (and related), but I'm not sure if I like
them... Up to you.

 Tomi

Re: [PATCH v4 0/2] Fix PLL lock timeout and calibration wait time

[Tomi Valkeinen]

Hi,

On 04/07/2025 15:59, Devarsh Thakkar wrote:
> 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.
> 
> NOTE: This needs to be applied on top of  
>   https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/           
>   https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/           
>   from the series:                                                                                  
>   https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.com/    
> 
> Changelog:
> V4:                                                                                                 
> - Optimize wait_for_pll_lock, wait_for_cmn_ready calls to oneline                                   
>   using conditional operator                                                                        
> - Remove superflous init for ret variable in cdns_dphy_configure                                    
> - Enable pll and psm ref clocks before configuring PLL                                              
> - Update commit message to refer to TRM                                                             
> - Rebased on top of:                                                                                
>   https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-4-862c841dbe02@ideasonboard.com/           
>   https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-5-862c841dbe02@ideasonboard.com/           
>   from the series:                                                                                  
>   https://lore.kernel.org/all/20250618-cdns-dsi-impro-v4-0-862c841dbe02@ideasonboard.com/    
> 
> 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:
> V3: https://lore.kernel.org/all/20250502033451.2291330-1-devarsht@ti.com/
> 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/d08d851399ca327e5594266a8d66d478
> 
> Rangediff:
> V3->V4:
> https://gist.github.com/devarsht/e4db52e1f4aec2d45596b3ed019e92ef
> 
> 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 | 126 ++++++++++++++++++++++++--------
>  1 file changed, 94 insertions(+), 32 deletions(-)
> 

For the series:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

 Tomi

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

[Devarsh Thakkar]

Hi Tomi,

Thanks for the review.
On 23/07/25 13:36, Tomi Valkeinen wrote:
> Hi,
> 
> On 04/07/2025 15:59, Devarsh Thakkar wrote:
>> 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 since earlier calibration wait time was getting
>> overwritten to zero value thus failing the PLL to lockup and causing
>> timeout.
>>
>> [1] AM62P TRM (Section 14.8.6.3.2.1.1 DPHY_TX_DPHYTX_CMN0_CMN_DIG_TBIT2):
>> Link: https://www.ti.com/lit/pdf/spruj83
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V4: No change
>> 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 da8de0a9d086..24a25606996c 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)
>> @@ -410,7 +411,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);
> 
> That's not how you do read-modify-write. You should first read the
> register, then clear the fields you want to set/clear, then set the
> fields, then write.
> 
> reg = readl(dphy->regs + DPHY_CMN_SSM, dphy->regs + DPHY_CMN_SSM);
> reg &= ~(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN)
> reg |= DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN;
> writel(reg, dphy->regs + DPHY_CMN_SSM);
> 
> That's the general form, in this particular case the and-line is not
> necessary, of course.
> 
> There's also FIELD_MODIFY() (and related), but I'm not sure if I like
> them... Up to you.

As discussed offline, here we just wanted to preserve the value for 
DPHY_CMN_SSM_CAL_WAIT_TIME bits as these are holding the calibration 
wait time, reset rest and then enable the SSM by setting DPHY_CMN_SSM_EN 
| DPHY_CMN_TX_MODE_EN and the patch rightly achieves this. I assume we 
are aligned on this as I see your review here [1].

Kindly let me know if otherwise.

[1]: 
https://lore.kernel.org/all/2bec3583-6078-4650-a8d0-6cfe8fcec3f3@ideasonboard.com/

Regards
Devarsh

> 
>   Tomi
>