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

[Stephen Boyd <sboyd@kernel.org>]

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.

> +
> +       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?

> +
> +       if (core->prepare_count) {

Why does prepare count matter?

> +               ret = -EBUSY;
> +               goto fail;

We just left without releasing the lock.

> +       }
> +
> +       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?

> + * @enable: enable or disable

Isn't 'disable' equal to spread_percent of zero?

> + *
> + * 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?

> +                           unsigned int spreadpercent, unsigned int method,

I'd assume 'method' would be some sort of enum?

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