| Message ID | 20250124-clk-ssc-v1-1-2d39f6baf2af@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: Support spread spectrum and use it in clk-scmi | expand |
On Fri, Jan 24, 2025 at 10:25:17PM +0800, Peng Fan (OSS) wrote: > 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; > + > + if (!clk || !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; > + > + if (core->prepare_count) { > + ret = -EBUSY; > + goto fail; fail is too vague to be useful as a name. Also this should be goto unlock; > + } > + > + ret = clk_pm_runtime_get(core); > + if (ret) > + goto fail; > + > + if (core->ops->set_spread_spectrum) > + ret = core->ops->set_spread_spectrum(core->hw, &clk_ss); > + > + clk_pm_runtime_put(core); unlock: > + clk_prepare_unlock(); > +fail: > + return ret; > +} regards, dan carpenter
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
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; + + if (!clk || !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; + + if (core->prepare_count) { + ret = -EBUSY; + goto fail; + } + + ret = clk_pm_runtime_get(core); + if (ret) + goto fail; + + 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); + /** * 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 2e6e603b749342931c0d0693c3e72b62c000791b..478005f4d53ed0698ea17331730c755e08ea7984 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -84,6 +84,21 @@ struct clk_duty { unsigned int den; }; +/** + * struct clk_spread_spectrum - Structure encoding spread spectrum of a clock + * + * @modfreq: Modulation frequency + * @spreadpercent: Modulation percent + * @method: Modulation method + * @enable: enable or disable modulation + */ +struct clk_spread_spectrum { + unsigned int modfreq; + unsigned int spreadpercent; + unsigned int method; + bool enable; +}; + /** * 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 +193,11 @@ struct clk_duty { * separately via calls to .set_parent and .set_rate. * Returns 0 on success, -EERROR otherwise. * + * @set_spread_spectrum: Configure the modulation frequency, modulation percentage + * and method. This callback is optional for clocks that does not + * support spread spectrum feature or no need to enable this feature. + * 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 +275,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); unsigned long (*recalc_accuracy)(struct clk_hw *hw, unsigned long parent_accuracy); int (*get_phase)(struct clk_hw *hw); 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 + * @enable: enable or disable + * + * 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, + unsigned int spreadpercent, unsigned int method, + bool enable); /** * clk_has_parent - check if a clock is a possible parent for another * @clk: clock source @@ -1088,6 +1103,13 @@ static inline int clk_set_rate_exclusive(struct clk *clk, unsigned long rate) return 0; } +static inline int clk_set_spread_spectrum(struct clk *clk, unsigned int modfreq, + unsigned int spreadpercent, + unsigned int method, bool enable) +{ + return 0; +} + static inline long clk_round_rate(struct clk *clk, unsigned long rate) { return 0;