Re: [PATCH v4 2/5] clk: Introduce clk_hw_set_spread_spectrum

[Stephen Boyd <sboyd@kernel.org>]

Quoting Peng Fan (2025-09-15 01:29:36)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index b821b2cdb155331c85fafbd2fac8ab3703a08e4d..06db8918a1b35e3280e565272bc4603a88295a92 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2802,6 +2802,32 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_max_rate);
>  
> +int clk_hw_set_spread_spectrum(struct clk_hw *hw, struct clk_spread_spectrum *conf)
> +{
> +       struct clk_core *core;
> +       int ret;
> +
> +       if (!hw)
> +               return 0;
> +
> +       core = hw->core;
> +
> +       clk_prepare_lock();
> +
> +       ret = clk_pm_runtime_get(core);
> +       if (ret)
> +               goto fail;
> +
> +       if (core->ops->set_spread_spectrum)
> +               ret = core->ops->set_spread_spectrum(hw, conf);
> +
> +       clk_pm_runtime_put(core);
> +
> +fail:
> +       clk_prepare_unlock();
> +       return ret;
> +}

Does it need to be exported? 

> +
>  /**
>   * clk_get_parent - return the parent of a clk
>   * @clk: the clk whose parent gets returned
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 630705a47129453c241f1b1755f2c2f2a7ed8f77..4f48a4df95a1c54638a0e91e0a449fcc8aa40b80 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -84,6 +84,19 @@ struct clk_duty {
>         unsigned int den;
>  };
>  
> +/**
> + * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock
> + *
> + * @modfreq_hz:                Modulation frequency
> + * @spread_bp:         Modulation percent in permyriad
> + * @method:            Modulation method
> + */
> +struct clk_spread_spectrum {
> +       u32 modfreq_hz;
> +       u32 spread_bp;
> +       u32 method;

What are the possible values of 'method'? I'm guessing it's the defines
in the dt-bindings header? Please connect these two somehow, maybe
through an enum. Also, why are these u32? Shouldn't these be more common
types like unsigned long or unsigned long long instead being exactly 32
bits?

> +};
> +
>  /**
>   * struct clk_ops -  Callback operations for hardware clocks; these are to
>   * be provided by the clock implementation, and will be called by drivers
> @@ -178,6 +191,12 @@ struct clk_duty {
>   *             separately via calls to .set_parent and .set_rate.
>   *             Returns 0 on success, -EERROR otherwise.
>   *
> + * @set_spread_spectrum: Optional callback used to configure the spread
> + *             spectrum modulation frequency, percentage, and method
> + *             to reduce EMI by spreading the clock frequency over a
> + *             wider range.
> + *             Returns 0 on success, -EERROR otherwise.
> + *
>   * @recalc_accuracy: Recalculate the accuracy of this clock. The clock accuracy
>   *             is expressed in ppb (parts per billion). The parent accuracy is
>   *             an input parameter.
> @@ -255,6 +274,8 @@ struct clk_ops {
>         int             (*set_rate_and_parent)(struct clk_hw *hw,
>                                     unsigned long rate,
>                                     unsigned long parent_rate, u8 index);
> +       int             (*set_spread_spectrum)(struct clk_hw *hw,
> +                                              struct clk_spread_spectrum *clk_ss);

const clk_ss pointer? And is it actually 'conf' or 'ss_conf'?

>         unsigned long   (*recalc_accuracy)(struct clk_hw *hw,
>                                            unsigned long parent_accuracy);
>         int             (*get_phase)(struct clk_hw *hw);
> @@ -1430,6 +1451,7 @@ void clk_hw_get_rate_range(struct clk_hw *hw, unsigned long *min_rate,
>                            unsigned long *max_rate);
>  void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate,
>                            unsigned long max_rate);
> +int clk_hw_set_spread_spectrum(struct clk_hw *hw, struct clk_spread_spectrum *conf);

const conf? And 'ss_conf' again?