| 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: <21e81030-189f-4f21-8800-f62cb351a3d6@ti.com>
Date: Fri, 20 Jun 2025 20:11:59 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Vinod Koul <vkoul@...nel.org>
CC: <kishon@...nel.org>, <mripard@...nel.org>, <linux-phy@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <sakari.ailus@...ux.intel.com>,
<u.kleine-koenig@...libre.com>, <vigneshr@...com>,
<aradhya.bhatia@...ux.dev>, <s-jain1@...com>, <r-donadkar@...com>,
<tomi.valkeinen@...asonboard.com>
Subject: Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and
O_CMN_READY polling
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@...r.kernel.org
>> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
>> Signed-off-by: Devarsh Thakkar <devarsht@...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
Powered by blists - more mailing lists