| 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
| ||
|
Message-ID: <gvqa3g7cv2di3je2q3xiojkzrfedlh2ugqrtnh5vagbtvo5w3a@a42vb32ds74s>
Date: Mon, 30 Dec 2024 19:12:31 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Devarsh Thakkar <devarsht@...com>
Cc: vkoul@...nel.org, kishon@...nel.org, mripard@...nel.org,
linux-phy@...ts.infradead.org, linux-kernel@...r.kernel.org, sakari.ailus@...ux.intel.com,
vigneshr@...com, aradhya.bhatia@...ux.dev, s-jain1@...com, r-donadkar@...com,
tomi.valkeinen@...asonboard.com, j-choudhary@...com, h-shenoy@...com, praneeth@...com,
u-kumar1@...com
Subject: Re: [PATCH] phy: cadence: cdns-dphy: Fix PLL lock and common ready
poll timeout
Hi Devarsh,
Thanks for the patch, must have been a fun debug :)
On Dec 30, 2024 at 18:23:19 +0530, Devarsh Thakkar wrote:
> After adding the initial checks for register poll timeout and traces for
> measuring total initialization time [1], it was observed that the driver
> was timing out while polling for PLL lock up to happen and O_CMN_READY bit
> to be set:
>
> "[ 19.967949] phy phy-301c0000.phy.2: Timed out waiting for DPHY PLL lock
> assertion [ 19.968976] phy phy-301c0000.phy.2: Timed out waiting for
> o_cmn_ready assertion"
>
> this also increased the overall DSIT Tx + DPHY Tx initialization time which
> was observed as 271ms (20.238306 - 19.966884) on TI's AM62L SoC:
>
> "kworker/u8:1-29 [001] ..... 19.966884:
> cdns_dsi_bridge_atomic_pre_enable: Initializing DSITx and DPHY Tx
> kworker/u8:1-29 [001] ..... 20.238306:
> cdns_dsi_bridge_atomic_pre_enable: Initialization complete"
>
> This was caused due to multiple issues as discussed below along with the
> updates done to fix those issues :
>
> 1) 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 for
> polling on PLL lock and O_CMN_READY assertion and call them only after
> common state machine gets enabled.
>
> 2) The cadence DPHY IP registers (as described in J721E TRM [2]) has
> default reset values for register fields in some of the registers
> which were getting reset to 0 as driver was not preserving them and
> overwriting those bits to 0 while updating the registers thus impacting
> the overall PLL lockup time. For e.g. DPHY_TX_CMN0_CMN_DIG_TBIT2 has
> bits 1-8 used for PLL wait time calibrations with default value as 0x14h
> and DPHY_TX_CMN0_CMN_DIG_TBIT10 has bits 27-20 used for PWM Byteclock
> divider which is default set to 0x8. To avoid resetting these register
> bit-fields, perform read-modify-write while updating above registers.
IMHO these are separate changes, and thus should be separate patches,
even if they are both done to reduce the time spent in the wait loop.
Ideally first patch to fix (2) across the driver, checking if there are
any other registers with default values that are being overwritten.
And the second patch to fix (1)
>
> With above updates, the PLL lockup and O_CMN_READ timeout assertion is not
> observed anymore and DSI Tx + DPHY Tx initialization time is reduced to
> 394 us (19.435259 - 19.434861) as shared below:
>
> "kworker/u8:6-74 [000] ..... 19.434861:
> cdns_dsi_bridge_atomic_pre_enable: Initializing DSITx and DPHY Tx
> kworker/u8:6-74 [000] ..... 19.435259:
> cdns_dsi_bridge_atomic_pre_enable: Initialization complete"
>
> NOTE: This is tested on top of existing series on cadence DSI under review
> [4].
>
> Full profiling logs before and after changes done also attached here [3].
>
> [1]: Profiling patch:
> Link: https://gist.github.com/devarsht/f3bcb6a2e7e97a0667c817cfa208ed4c
>
> [2]: J721E TRM Section 12.8.4 DPHY_TX Registers:
> Link: https://www.ti.com/lit/zip/spruil1
>
> [3]: Profiling logs:
> Link: https://gist.github.com/devarsht/bc84be106209c373a0be4e33b5f019bb
>
> [4]: Tested on top of existing cadence DSI series:
> Link: https://lore.kernel.org/all/20241019195411.266860-1-aradhya.bhatia@linux.dev
>
> Cc: stable@...r.kernel.org
> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> Signed-off-by: Devarsh Thakkar <devarsht@...com>
> ---
> drivers/phy/cadence/cdns-dphy.c | 49 ++++++++++++++++++++++++++-------
> 1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index dddb66de6dba..c248a506ddc2 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -98,6 +98,8 @@ struct cdns_dphy_ops {
> enum cdns_dphy_clk_lane_cfg cfg);
> void (*set_pll_cfg)(struct cdns_dphy *dphy,
> const struct cdns_dphy_cfg *cfg);
> + void (*wait_for_pll_lock)(struct cdns_dphy *dphy);
> + void (*wait_for_cmn_ready)(struct cdns_dphy *dphy);
> unsigned long (*get_wakeup_time_ns)(struct cdns_dphy *dphy);
> };
>
> @@ -191,6 +193,18 @@ static unsigned long cdns_dphy_get_wakeup_time_ns(struct cdns_dphy *dphy)
> return dphy->ops->get_wakeup_time_ns(dphy);
> }
>
> +static void cdns_dphy_wait_for_pll_lock(struct cdns_dphy *dphy)
> +{
> + if (dphy->ops->wait_for_pll_lock)
> + dphy->ops->wait_for_pll_lock(dphy);
> +}
> +
> +static void cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> + if (dphy->ops->wait_for_cmn_ready)
> + dphy->ops->wait_for_cmn_ready(dphy);
> +}
> +
> static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
> {
> /* Default wakeup time is 800 ns (in a simulated environment). */
> @@ -212,7 +226,7 @@ static void cdns_dphy_ref_set_pll_cfg(struct cdns_dphy *dphy,
> writel(DPHY_CMN_FBDIV_FROM_REG |
> DPHY_CMN_FBDIV_VAL(fbdiv_low, fbdiv_high),
> dphy->regs + DPHY_CMN_FBDIV);
> - writel(DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
> + writel(readl(dphy->regs + DPHY_CMN_PWM) | DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
> DPHY_CMN_PWM_DIV(0x8),
> dphy->regs + DPHY_CMN_PWM);
> }
> @@ -232,13 +246,11 @@ 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
> * which is same as that of in ref ops
> */
> - writel(DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
> + writel(readl(dphy->regs + DPHY_CMN_PWM) | DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
> DPHY_CMN_PWM_DIV(0x8),
> dphy->regs + DPHY_CMN_PWM);
>
> @@ -249,13 +261,25 @@ 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);
> +static void cdns_dphy_j721e_wait_for_pll_lock(struct cdns_dphy *dphy)
> +{
> + u32 status;
> +
> + if (readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> + status & DPHY_TX_WIZ_PLL_LOCK, 0, POLL_TIMEOUT_US))
> + dev_err(&dphy->phy->dev, "Timed out waiting for DPHY PLL lock assertion\n");
> +}
>
> - 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_wait_for_cmn_ready(struct cdns_dphy *dphy)
> +{
> + u32 status;
> +
> + if (readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> + status & DPHY_TX_WIZ_O_CMN_READY, 0,
> + POLL_TIMEOUT_US))
> + dev_err(&dphy->phy->dev, "Timed out waiting for o_cmn_ready assertion\n");
> }
>
> static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
> @@ -278,6 +302,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,
> @@ -384,9 +410,12 @@ static int cdns_dphy_power_on(struct phy *phy)
> clk_prepare_enable(dphy->pll_ref_clk);
>
> /* Start TX state machine. */
> - writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
> + writel(readl(dphy->regs + DPHY_CMN_SSM) | DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
> dphy->regs + DPHY_CMN_SSM);
>
> + cdns_dphy_wait_for_pll_lock(dphy);
> + cdns_dphy_wait_for_cmn_ready(dphy);
> +
> return 0;
> }
>
> --
> 2.39.1
>
--
Thanks,
Jai
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists