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

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

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.

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

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy