| 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: <20250423093008.GC17813@pendragon.ideasonboard.com>
Date: Wed, 23 Apr 2025 12:30:08 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mike Looijmans <mike.looijmans@...ic.nl>
Cc: linux-phy@...ts.infradead.org,
Kishon Vijay Abraham I <kishon@...nel.org>,
Michal Simek <michal.simek@....com>, Vinod Koul <vkoul@...nel.org>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] phy-zynqmp: Postpone getting clock rate until actually
needed
Hi Mike,
On Wed, Apr 23, 2025 at 08:59:43AM +0200, Mike Looijmans wrote:
> On 23-04-2025 00:18, Laurent Pinchart wrote:
> > Hi Mike,
> >
> > Thank you for the patch.
> >
> > On Fri, Mar 14, 2025 at 04:04:18PM +0100, 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
> >>
> >> This issue was that at probe time, the system has not decided yet whether
> >> the GTR is to be used for SATA (150MHz) or PCIe (100 MHz).
> > At what point does the system decide that ? I've only used (and tested)
> > this driver for DisplayPort.
>
> In retrospect, yeah, the commit text could be a bit more elaborate on that.
>
> This PHY driver probes before the I2C bus which contains the programmable
> clock chip that provides the GT clocks for the PS. This isn't a problem, as
> the PHY lane won't be needed yet.
>
> When the SATA (or PCIe or network or displayport or ...) driver probes, the
> clock gets programmed. The PHY lane won't be enabled until after that.
Would it be possible to defer probing (as in returning -EPROBE_DEFER) of
the PHY until the clock is available ?
> >> The driver
> >> doesn't need to know the clock frequency at that point yet, so wait until
> >> the lane is actually being initialized before requesting the clock rate
> >> setting.
> >>
> >> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
> >> ---
> >>
> >> drivers/phy/xilinx/phy-zynqmp.c | 61 ++++++++++++++++-----------------
> >> 1 file changed, 30 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> >> index 05a4a59f7c40..e29e3e51d380 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)
> >> {
> >> 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;
> >
> > Isn't it an issue that we now fail here without propagating an error
> > back to the caller ? The function is called by xpsgtr_phy_init(), which
> > return an int, so I think returning an error code here would make sense.
>
> Agree, I'll make a v2
>
> > The rest of the patch looks good to me. I would however appreciate
> > feedback from someone at AMD more knowledgeable than I am with the
> > hardware. Michal, could you propose an appropriate second maintainer for
> > this driver ?
> >
> >> +
> >> step_size = ssc->step_size;
> >>
> >> xpsgtr_clr_set(gtr_phy->dev, PLL_REF_SEL(gtr_phy->lane),
> >> @@ -823,8 +848,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 +952,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 +968,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;
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists