| 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: <c71408b5-2ead-4408-83b3-dad30f153d2d@amd.com>
Date: Tue, 29 Apr 2025 10:36:49 +0200
From: Michal Simek <michal.simek@....com>
To: Mike Looijmans <mike.looijmans@...ic.nl>, 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
+ 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
Powered by blists - more mailing lists