Re: [PATCH] phy-zynqmp: Postpone getting clock rate until actually needed

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@topic.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