linux-kernel - Re: [PATCH v2 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cddb8c30-c614-4fae-9cbd-f5776f930165@ideasonboard.com>
Date: Wed, 2 Apr 2025 14:55:42 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Devarsh Thakkar <devarsht@...com>, vkoul@...nel.org, kishon@...nel.org,
 mripard@...nel.org, linux-phy@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Cc: sakari.ailus@...ux.intel.com, u.kleine-koenig@...libre.com,
 vigneshr@...com, aradhya.bhatia@...ux.dev, s-jain1@...com, r-donadkar@...com
Subject: Re: [PATCH v2 1/2] phy: cadence: cdns-dphy: Fix PLL lock and
 O_CMN_READY polling

Hi,

On 26/03/2025 17:23, Devarsh Thakkar wrote:
> PLL lockup and O_CMN_READY assertion can only happen after common state
> machine gets enabled, but driver was polling them before the common state
> machine was enabled. To fix this, add new function callbacks polling on PLL
> lock and O_CMN_READY assertion and call them only after common state
> machine gets enabled.
> 
> Cc: stable@...r.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@...com>
> ---
> V2:
> - Return error code on polling timeout
> - Moved out calibration logic to separate patch
> 
>   drivers/phy/cadence/cdns-dphy.c | 57 ++++++++++++++++++++++++++++-----
>   1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index ed87a3970f83..c4de9e4d3e93 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -99,6 +99,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 {
> @@ -191,6 +193,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 +254,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 +270,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 +277,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 +309,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,
> @@ -373,6 +406,14 @@ 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);
>   
> +	ret = cdns_dphy_wait_for_pll_lock(dphy);
> +	if (ret)
> +		dev_err(&dphy->phy->dev, "Failed to lock PLL with err %d\n", 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);
> +

Shouldn't these return an error? Or what's the reason these are ok (and 
if so, should the prints be dev_dbg?)

  Tomi

>   	return 0;
>   }
>