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

[Mike Looijmans <mike.looijmans@topic.nl>]

On 29-04-2025 10:36, Michal Simek wrote:
> + 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@topic.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@amd.com>
>
> Thanks,
> Michal
>
Any further comments? Anything I need to do?

M.