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

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

[Mike Looijmans]

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). 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;
+
 	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;
-- 
2.43.0


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

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

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

[Mike Looijmans]

Gentle ping, any feedback on this?


On 14-03-2025 16:04, 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). 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;
> +
>   	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;


-- 
Mike Looijmans





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

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

[Laurent Pinchart]

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

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

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail
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.


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



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

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

[Laurent Pinchart]

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

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

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

[Mike Looijmans]

On 23-04-2025 11:30, Laurent Pinchart wrote:
> 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 ?

Come to think of it, the DEFER already happens at probe, after 
devm_clk_get_optional().

The issue is that the PHY doesn't know yet which function it will have 
(SATA/PCIe/...) so it must also wait for its client.

It's also a nice code cleanup, as it removes an array of pointers 
(refclk_sscs) from the device data struct (xpsgtr_dev).


Now that I'm looking at this again, a good next step would be that if 
the PHY doesn't particularly like the frequency the clock is set at, it 
should try a clk_set_rate() instead of just giving up. When the 
connection with the client is made, the PHY knows what frequencies are 
acceptable, so it can program the clock itself (if the clock is 
programmable, which is the case on all Topic modules). That would get 
rid of the assigned-clock-frequencies in the devicetree. A much more 
logical use of the clock framework.


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


-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl




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