Re: [PATCH 1/3] clk: Introduce clk_set_spread_spectrum

[Peng Fan]

On Tue, Jan 28, 2025 at 12:25:28PM -0800, Stephen Boyd wrote:
>Quoting Peng Fan (OSS) (2025-01-24 06:25:17)
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index cf7720b9172ff223d86227aad144e15375ddfd86..a4fe4a60f839244b736e3c2751eeb38dc4577b1f 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>>  
>> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
>> +                           unsigned int spreadpercent, unsigned int method,
>> +                           bool enable)
>> +{
>> +       struct clk_spread_spectrum clk_ss;
>> +       struct clk_core *core;
>> +       int ret = 0;
>
>The assignment looks unnecessary.

To avoid uninitialized variable warning.

>
>> +
>> +       if (!clk || !clk->core)
>
>How do you not have clk->core?
>
>> +               return 0;
>> +
>> +       clk_ss.modfreq = modfreq;
>> +       clk_ss.spreadpercent = spreadpercent;
>> +       clk_ss.method = method;
>> +       clk_ss.enable = enable;
>> +
>> +       clk_prepare_lock();
>> +
>> +       core = clk->core;
>
>Why do we need to get the core under the lock?

Drop in v2.

>
>> +
>> +       if (core->prepare_count) {
>
>Why does prepare count matter?

I was thinking to configure Spread Spectrum(SS) before
prepare/enable a clock. But it should be fine to not
check prepare count.

>
>> +               ret = -EBUSY;
>> +               goto fail;
>
>We just left without releasing the lock.

True. Dan also reported this. Fix in V2.

>
>> +       }
>> +
>> +       ret = clk_pm_runtime_get(core);
>> +       if (ret)
>> +               goto fail;
>
>We just left without releasing the lock.
>
>> +
>> +       if (core->ops->set_spread_spectrum)
>> +               ret = core->ops->set_spread_spectrum(core->hw, &clk_ss);
>> +
>> +       clk_pm_runtime_put(core);
>> +       clk_prepare_unlock();
>> +fail:
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(clk_set_spread_spectrum);
>> +
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index b607482ca77e987b9344c38f25ebb5c8d35c1d39..49a7f7eb8b03233e11cd3b92768896c4e45c4e7c 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -858,6 +858,21 @@ int clk_set_rate(struct clk *clk, unsigned long rate);
>>   */
>>  int clk_set_rate_exclusive(struct clk *clk, unsigned long rate);
>>  
>> +/**
>> + * clk_set_spread_spectrum - set the spread spectrum for a clock
>> + * @clk: clock source
>> + * @modfreq: modulation freq
>> + * @spreadpercent: modulation percentage
>> + * @method: down spread, up spread, center spread or else
>
>Did we get cut off?

Sorry I not get this point.

>
>> + * @enable: enable or disable
>
>Isn't 'disable' equal to spread_percent of zero?

yeah. Drop the last parameter.

>
>> + *
>> + * Configure the spread spectrum parameters for a clock.
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq,
>
>Does this need to be a consumer API at all? Usually SSC is figured out
>when making a board and you have to pass some certification testing
>because some harmonics are interfering. Is the DT property sufficient
>for now and then we can do it when the driver probes in the framework?

I suppose 'DT property' you are refering the stm32 and i.MX8M SSC patchsets.
I am proposing a generic interface for drivers to enable SSC.
Otherwise we need to introduce vendor properties for each vendor.
And looking at clk-scmi.c, we need a generic way to enable SSC, I think SCMI
maintainers not agree to add vendor properties for it.

>
>> +                           unsigned int spreadpercent, unsigned int method,
>
>I'd assume 'method' would be some sort of enum?

sure. Fix in V2.

Thanks,
Peng

>
>> +                           bool enable);
>>  /**
>>   * clk_has_parent - check if a clock is a possible parent for another
>>   * @clk: clock source