| 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: <c399c68b-ee60-4d54-b901-a40e744f287c@topic.nl>
Date: Wed, 14 May 2025 07:42:04 +0200
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Michal Simek <michal.simek@....com>, linux-phy@...ts.infradead.org,
Radhey Shyam Pandey <radhey.shyam.pandey@....com>,
"Katakam, Harini" <harini.katakam@....com>
CC: Kishon Vijay Abraham I <kishon@...nel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Vinod Koul <vkoul@...nel.org>, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] phy-zynqmp: Postpone getting clock rate until actually
needed
On 29-04-2025 10:36, Michal Simek wrote:
> + Radhey, Harini
>
> On 4/28/25 08:35, Mike Looijmans wrote:
>> At probe time the driver would display the following error and abort:
>> xilinx-psgtr fd400000.phy: Invalid rate 0 for reference clock 0
>>
>> At probe time, the associated GTR driver (e.g. SATA or PCIe) hasn't
>> initialized the clock yet, so clk_get_rate() likely returns 0 if the clock
>> is programmable. So this driver only works if the clock is fixed.
>>
>> The PHY driver doesn't need to know the clock frequency at probe yet, so
>> wait until the associated driver initializes the lane before requesting the
>> clock rate setting.
>>
>> In addition to allowing the driver to be used with programmable clocks,
>> this also reduces the driver's runtime memory footprint by removing an
>> array of pointers from struct xpsgtr_phy.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
>> ---
>>
>> Changes in v2:
>> Explain the issue and the fix better in the commit text
>> Propagate errors (as reported by Laurent Pinchart)
>>
>> drivers/phy/xilinx/phy-zynqmp.c | 70 +++++++++++++++++----------------
>> 1 file changed, 37 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
>> index 05a4a59f7c40..fe6b4925d166 100644
>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> @@ -222,7 +222,6 @@ struct xpsgtr_phy {
>> * @siou: siou base address
>> * @gtr_mutex: mutex for locking
>> * @phys: PHY lanes
>> - * @refclk_sscs: spread spectrum settings for the reference clocks
>> * @clk: reference clocks
>> * @tx_term_fix: fix for GT issue
>> * @saved_icm_cfg0: stored value of ICM CFG0 register
>> @@ -235,7 +234,6 @@ struct xpsgtr_dev {
>> void __iomem *siou;
>> struct mutex gtr_mutex; /* mutex for locking */
>> struct xpsgtr_phy phys[NUM_LANES];
>> - const struct xpsgtr_ssc *refclk_sscs[NUM_LANES];
>> struct clk *clk[NUM_LANES];
>> bool tx_term_fix;
>> unsigned int saved_icm_cfg0;
>> @@ -398,13 +396,40 @@ static int xpsgtr_wait_pll_lock(struct phy *phy)
>> return ret;
>> }
>> +/* Get the spread spectrum (SSC) settings for the reference clock rate */
>> +static const struct xpsgtr_ssc *xpsgtr_find_sscs(struct xpsgtr_phy *gtr_phy)
>> +{
>> + unsigned long rate;
>> + struct clk *clk;
>> + unsigned int i;
>> +
>> + clk = gtr_phy->dev->clk[gtr_phy->refclk];
>> + rate = clk_get_rate(clk);
>> +
>> + for (i = 0 ; i < ARRAY_SIZE(ssc_lookup); i++) {
>> + /* Allow an error of 100 ppm */
>> + unsigned long error = ssc_lookup[i].refclk_rate / 10000;
>> +
>> + if (abs(rate - ssc_lookup[i].refclk_rate) < error)
>> + return &ssc_lookup[i];
>> + }
>> +
>> + dev_err(gtr_phy->dev->dev, "Invalid rate %lu for reference clock %u\n",
>> + rate, gtr_phy->refclk);
>> +
>> + return NULL;
>> +}
>> +
>> /* Configure PLL and spread-sprectrum clock. */
>> -static void xpsgtr_configure_pll(struct xpsgtr_phy *gtr_phy)
>> +static int xpsgtr_configure_pll(struct xpsgtr_phy *gtr_phy)
>> {
>> const struct xpsgtr_ssc *ssc;
>> u32 step_size;
>> - ssc = gtr_phy->dev->refclk_sscs[gtr_phy->refclk];
>> + ssc = xpsgtr_find_sscs(gtr_phy);
>> + if (!ssc)
>> + return -EINVAL;
>> +
>> step_size = ssc->step_size;
>> xpsgtr_clr_set(gtr_phy->dev, PLL_REF_SEL(gtr_phy->lane),
>> @@ -446,6 +471,8 @@ static void xpsgtr_configure_pll(struct xpsgtr_phy
>> *gtr_phy)
>> xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_3_MSB,
>> STEP_SIZE_3_MASK, (step_size & STEP_SIZE_3_MASK) |
>> FORCE_STEP_SIZE | FORCE_STEPS);
>> +
>> + return 0;
>> }
>> /* Configure the lane protocol. */
>> @@ -658,7 +685,10 @@ static int xpsgtr_phy_init(struct phy *phy)
>> * Configure the PLL, the lane protocol, and perform protocol-specific
>> * initialization.
>> */
>> - xpsgtr_configure_pll(gtr_phy);
>> + ret = xpsgtr_configure_pll(gtr_phy);
>> + if (ret)
>> + goto out;
>> +
>> xpsgtr_lane_set_protocol(gtr_phy);
>> switch (gtr_phy->protocol) {
>> @@ -823,8 +853,7 @@ static struct phy *xpsgtr_xlate(struct device *dev,
>> }
>> refclk = args->args[3];
>> - if (refclk >= ARRAY_SIZE(gtr_dev->refclk_sscs) ||
>> - !gtr_dev->refclk_sscs[refclk]) {
>> + if (refclk >= ARRAY_SIZE(gtr_dev->clk)) {
>> dev_err(dev, "Invalid reference clock number %u\n", refclk);
>> return ERR_PTR(-EINVAL);
>> }
>> @@ -928,9 +957,7 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev
>> *gtr_dev)
>> {
>> unsigned int refclk;
>> - for (refclk = 0; refclk < ARRAY_SIZE(gtr_dev->refclk_sscs); ++refclk) {
>> - unsigned long rate;
>> - unsigned int i;
>> + for (refclk = 0; refclk < ARRAY_SIZE(gtr_dev->clk); ++refclk) {
>> struct clk *clk;
>> char name[8];
>> @@ -946,29 +973,6 @@ static int xpsgtr_get_ref_clocks(struct xpsgtr_dev
>> *gtr_dev)
>> continue;
>> gtr_dev->clk[refclk] = clk;
>> -
>> - /*
>> - * Get the spread spectrum (SSC) settings for the reference
>> - * clock rate.
>> - */
>> - rate = clk_get_rate(clk);
>> -
>> - for (i = 0 ; i < ARRAY_SIZE(ssc_lookup); i++) {
>> - /* Allow an error of 100 ppm */
>> - unsigned long error = ssc_lookup[i].refclk_rate / 10000;
>> -
>> - if (abs(rate - ssc_lookup[i].refclk_rate) < error) {
>> - gtr_dev->refclk_sscs[refclk] = &ssc_lookup[i];
>> - break;
>> - }
>> - }
>> -
>> - if (i == ARRAY_SIZE(ssc_lookup)) {
>> - dev_err(gtr_dev->dev,
>> - "Invalid rate %lu for reference clock %u\n",
>> - rate, refclk);
>> - return -EINVAL;
>> - }
>> }
>> return 0;
>
> Sorry for delay.
> Radhey/Harini: Please test this but I have read that code and changes looks
> good to me.
>
> Acked-by: Michal Simek <michal.simek@....com>
>
> Thanks,
> Michal
>
Any further comments? Anything I need to do?
M.
Powered by blists - more mailing lists