| Message ID | 20250124-clk-ssc-v1-1-2d39f6baf2af@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| 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
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
On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote: > 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. > To clarify, from the SCMI point of view, I expressed the idea that it would make sense to have a common SSC interface on the SCMI backend too instead of a custom NXP since you are adding a common CLK framework feature, BUT only if it turns out, from this discussion, that a common general way of configuring SSC can be found...and I dont know that, so I am waiting to see what this discussion with CLK framework and iMX maintainers goes before excluding the SCMI CLK vendor OEM types scenario...it would be ideal and easier NOT to use SCMI vendor extensions BUT ONLY if this NXP SSC/config generic solution is deemed to be really generic and usable by any other vendor. Thanks, Cristian
On Mon, Feb 03, 2025 at 07:47:22PM +0800, Peng Fan wrote: > On Mon, Feb 03, 2025 at 09:43:39AM +0000, Cristian Marussi wrote: > >On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote: > >> 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 Hi Peng, > >> >> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) > >> >> } > >> >> EXPORT_SYMBOL_GPL(clk_set_max_rate); [snip] > >> > > >> >> + * > >> >> + * 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. > >> > > > >To clarify, from the SCMI point of view, I expressed the idea that it > >would make sense to have a common SSC interface on the SCMI backend too > >instead of a custom NXP since you are adding a common CLK framework feature, > >BUT only if it turns out, from this discussion, that a common general way of > >configuring SSC can be found...and I dont know that, so I am waiting to see > >what this discussion with CLK framework and iMX maintainers goes before > >excluding the SCMI CLK vendor OEM types scenario...it would be ideal and > >easier NOT to use SCMI vendor extensions BUT ONLY if this NXP SSC/config generic > >solution is deemed to be really generic and usable by any other vendor. > > You may misunderstand. Using DT properties for clk-scmi.c to configure SSC > of a single clock means to add properties under clk scmi node, such > as: > "arm,modfreq = <x>, <y>, <z>; > arm,moddepth = <a>, <b>, <c>; > arm,modmethod = <j>, <l>, <m>;" > > And during probe in clk-scmi.c, the properties needs to be parsed and when > clk is configured, the ssc settings need to be passed to scmi platform. > > But per the i.MX8M SSC patchset that DT maintainers raised, > the properties(fsl,modfreq and etc) needs to in consumer side, > not provider side. > > So it is not feasible to add properties here. > Thanks for the clarification. > "assigned-clock-sscs" could be put under consumer node and clocks > could be configured with SSC when needed. This is a generic property. > Yes I understood this, the property that describes SSC that you are adding is generic BUT are also the related params needed to describe effectively the SSC IOW, if we add, as desirable, a generic new SSC type in extended configs instead of using a vendor oem, will these info down below passed to the SCMI: + /* + * extConfigValue[7:0] - spread percentage (%) + * extConfigValue[23:8] - Modulation Frequency (KHz) + * extConfigValue[24] - Enable/Disable + * extConfigValue[31:25] - Reserved + */ ... be enough to describe the required SSC config to any generic SCMI server from any vendor using any HW ? ... or is it plausible and maybe frequent/usual that other vendors could need additional specific params to be fed to the server, so that we will end up using the new standard SSC only for NXP ? IOW the property is generic, agreed, but are the described params above generic enough too ? ... that was my concern from an UN-educated point of view...of course, I am talking about the most common scenarios, if some other vendor ends up needing some arcane/magic specific param that cannot fit the above, they will be relegated to the OEM custom spaces even if this common SSC is available. > Back to NXP SCMI vendor extension, if SCMI spec could include SSC, NXP > SCMI could move to align with spec and not use vendor extension. > Agreed, conditional to the above doubts. Apologies if I have asked dumb/obvious questions, but I am not familiar with the subject matter enough... Thanks, Cristian
On Mon, Feb 03, 2025 at 09:43:39AM +0000, Cristian Marussi wrote: >On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote: >> 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. >> > >To clarify, from the SCMI point of view, I expressed the idea that it >would make sense to have a common SSC interface on the SCMI backend too >instead of a custom NXP since you are adding a common CLK framework feature, >BUT only if it turns out, from this discussion, that a common general way of >configuring SSC can be found...and I dont know that, so I am waiting to see >what this discussion with CLK framework and iMX maintainers goes before >excluding the SCMI CLK vendor OEM types scenario...it would be ideal and >easier NOT to use SCMI vendor extensions BUT ONLY if this NXP SSC/config generic >solution is deemed to be really generic and usable by any other vendor. You may misunderstand. Using DT properties for clk-scmi.c to configure SSC of a single clock means to add properties under clk scmi node, such as: "arm,modfreq = <x>, <y>, <z>; arm,moddepth = <a>, <b>, <c>; arm,modmethod = <j>, <l>, <m>;" And during probe in clk-scmi.c, the properties needs to be parsed and when clk is configured, the ssc settings need to be passed to scmi platform. But per the i.MX8M SSC patchset that DT maintainers raised, the properties(fsl,modfreq and etc) needs to in consumer side, not provider side. So it is not feasible to add properties here. "assigned-clock-sscs" could be put under consumer node and clocks could be configured with SSC when needed. This is a generic property. Back to NXP SCMI vendor extension, if SCMI spec could include SSC, NXP SCMI could move to align with spec and not use vendor extension. Thanks, Peng. > >Thanks, >Cristian
> Subject: Re: [PATCH 1/3] clk: Introduce clk_set_spread_spectrum > > On Mon, Feb 03, 2025 at 07:47:22PM +0800, Peng Fan wrote: > > On Mon, Feb 03, 2025 at 09:43:39AM +0000, Cristian Marussi wrote: > > >On Sun, Feb 02, 2025 at 06:42:56PM +0800, Peng Fan wrote: > > >> 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..a4fe4a60f839244b7 > 36e3c > > >> >> 2751eeb38dc4577b1f 100644 > > >> >> --- a/drivers/clk/clk.c > > >> >> +++ b/drivers/clk/clk.c > > Hi Peng, > > > >> >> @@ -2790,6 +2790,45 @@ int clk_set_max_rate(struct clk *clk, > > >> >> unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate); > > [snip] > > > >> > > > >> >> + * > > >> >> + * 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. > > >> > > > > > >To clarify, from the SCMI point of view, I expressed the idea that it > > >would make sense to have a common SSC interface on the SCMI > backend too > > >instead of a custom NXP since you are adding a common CLK > framework feature, > > >BUT only if it turns out, from this discussion, that a common general > way of > > >configuring SSC can be found...and I dont know that, so I am waiting > to see > > >what this discussion with CLK framework and iMX maintainers goes > before > > >excluding the SCMI CLK vendor OEM types scenario...it would be > ideal and > > >easier NOT to use SCMI vendor extensions BUT ONLY if this NXP > SSC/config generic > > >solution is deemed to be really generic and usable by any other > vendor. > > > > You may misunderstand. Using DT properties for clk-scmi.c to > configure SSC > > of a single clock means to add properties under clk scmi node, such > > as: > > "arm,modfreq = <x>, <y>, <z>; > > arm,moddepth = <a>, <b>, <c>; > > arm,modmethod = <j>, <l>, <m>;" > > > > And during probe in clk-scmi.c, the properties needs to be parsed and > when > > clk is configured, the ssc settings need to be passed to scmi platform. > > > > But per the i.MX8M SSC patchset that DT maintainers raised, > > the properties(fsl,modfreq and etc) needs to in consumer side, > > not provider side. > > > > So it is not feasible to add properties here. > > > > Thanks for the clarification. > > > "assigned-clock-sscs" could be put under consumer node and clocks > > could be configured with SSC when needed. This is a generic property. > > > > Yes I understood this, the property that describes SSC that you are > adding is generic BUT are also the related params needed to describe > effectively the SSC > > IOW, if we add, as desirable, a generic new SSC type in extended > configs > instead of using a vendor oem, will these info down below passed to > the SCMI: > > + /* > + * extConfigValue[7:0] - spread percentage (%) > + * extConfigValue[23:8] - Modulation Frequency (KHz) > + * extConfigValue[24] - Enable/Disable > + * extConfigValue[31:25] - Reserved > + */ From SSC view, spread percent(depth), modulation freq, modulation method is required to be passed to SCMI server. Enable maybe optional(depth set to 0 should be same as disable). The upper encodings using extConfig is NXP defined, we could update following spec. > > > ... be enough to describe the required SSC config to any generic SCMI > server > from any vendor using any HW ? > > ... or is it plausible and maybe frequent/usual that other vendors could > need additional specific params to be fed to the server, so that we > will end up using the new standard SSC only for NXP ? I checked TI/STM32/Renesas spec. https://www.ti.com/lit/an/spraby0a/spraby0a.pdf?ts=1738492934158&ref_url=https%253A%252F%252Fwww.google.com.hk%252F https://www.st.com/resource/en/application_note/an4850-stm32-mcus-spreadspectrum-clock-generation-principles-properties-and-implementation-stmicroelectronics.pdf https://www.renesas.com/en/products/clocks-timing/application-specific-clocks/spread-spectrum-clocks?srsltid=AfmBOoqSceW72dYO41RZVhT1YxhyKeXNhWUTfSrSCpfZt2A2cy7JgaGv same parameters are required. From ADI: https://www.analog.com/en/resources/technical-articles/clock-generation-with-spread-spectrum.html " Creating a spread-spectrum CLK by dithering the CLK frequency is not as straightforward as it might appear. We begin by defining parameters that comprise a spread-spectrum CLK: spreading rate, spreading style, modulation rate, and modulation waveform. Spreading rate is the ratio of the range of dithering (or spreading) frequency over the original CLK frequency, fC. Spreading style is down-spreading, center-spreading, or up-spreading. If we assume that the spreading frequency range is Δf, the spreading rate, δ, is defined as: Down spreading: δ = -Δf /fC x 100% Center spreading: δ = ±1/2Δf/fC x 100% Up spreading: δ = Δf/fC x 100% " fC is same as spread percentage/depth. > > IOW the property is generic, agreed, but are the described params > above > generic enough too ? ... I think so. Thanks, Peng. that was my concern from an UN-educated > point > of view...of course, I am talking about the most common scenarios, if > some other vendor ends up needing some arcane/magic specific param > that > cannot fit the above, they will be relegated to the OEM custom spaces > even if this common SSC is available. > > > Back to NXP SCMI vendor extension, if SCMI spec could include SSC, > NXP > > SCMI could move to align with spec and not use vendor extension. > > > > Agreed, conditional to the above doubts. > > Apologies if I have asked dumb/obvious questions, but I am not > familiar > with the subject matter enough... > > Thanks, > Cristian
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;