From: Vinod Koul <vkoul@kernel.org>
To: Devarsh Thakkar <devarsht@ti.com>
Cc: kishon@kernel.org, mripard@kernel.org,
linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org,
sakari.ailus@linux.intel.com, u.kleine-koenig@baylibre.com,
vigneshr@ti.com, aradhya.bhatia@linux.dev, s-jain1@ti.com,
r-donadkar@ti.com, tomi.valkeinen@ideasonboard.com
Subject: Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling
Date: Wed, 14 May 2025 11:29:46 +0100 [thread overview]
Message-ID: <aCRwmohPq5E2RsGC@vaman> (raw)
In-Reply-To: <20250502033451.2291330-2-devarsht@ti.com>
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
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
next prev parent reply other threads:[~2025-05-14 12:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-02 3:34 [PATCH v3 0/2] Fix PLL lock timeout and calibration wait time Devarsh Thakkar
2025-05-02 3:34 ` [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling Devarsh Thakkar
2025-05-14 10:29 ` Vinod Koul [this message]
2025-06-20 14:41 ` Devarsh Thakkar
2025-05-26 10:59 ` Tomi Valkeinen
2025-06-20 14:35 ` Devarsh Thakkar
2025-06-20 15:08 ` Devarsh Thakkar
2025-06-18 10:01 ` Tomi Valkeinen
2025-06-20 15:07 ` Devarsh Thakkar
2025-05-02 3:34 ` [PATCH v3 2/2] phy: cadence: cdns-dphy: Update calibration wait time for startup state machine Devarsh Thakkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aCRwmohPq5E2RsGC@vaman \
--to=vkoul@kernel.org \
--cc=aradhya.bhatia@linux.dev \
--cc=devarsht@ti.com \
--cc=kishon@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=mripard@kernel.org \
--cc=r-donadkar@ti.com \
--cc=s-jain1@ti.com \
--cc=sakari.ailus@linux.intel.com \
--cc=tomi.valkeinen@ideasonboard.com \
--cc=u.kleine-koenig@baylibre.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).