Re: [PATCH v3 3/3] clk: ti: dpll: apply the spread spectrum configuration right away

[Dario Binacchi]

Hi Brian,

Sorry for the noise, resending as plain text since my previous reply
was rejected by the mailing lists.

On Tue, Nov 3, 2026 at 9:04 PM Brian Masney <masneyb@xxxxxxxxxx> wrote:
>
> Hi Dario,
>
> > The spread spectrum clocking (SSC) registers are only programmed by
> > set_rate(), which the clock core skips when the requested rate is
> > already the current one, e.g. when the bootloader has set it up.
> > As a result, the SSC configuration is not applied, as it was with the
> > vendor specific properties.
> >
> > Apply it from set_spread_spectrum() instead, reprogramming the DPLL
> > at its current rate.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Skip reprogramming only if the configuration is already applied
> >   to the hardware
> >
> > Changes in v2:
> > - Check the DPLL lock state instead of its parent, as the reference
> >   and bypass clocks can be the same clock
> >
> >  drivers/clk/ti/dpll3xxx.c | 49 ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 43 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
> > index 634fe0a1f9fd..d32d81a27bfe 100644
> > --- a/drivers/clk/ti/dpll3xxx.c
> > +++ b/drivers/clk/ti/dpll3xxx.c
> > @@ -690,17 +690,22 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
> >   * @hw: pointer to the clock to set the spread spectrum configuration for
> >   * @ss_conf: spread spectrum configuration
> >   *
> > - * Stores the spread spectrum clocking (SSC) configuration of the DPLL. The
> > - * configuration is programmed along with the DPLL multiplier and divider
> > - * at the next rate change. The hardware supports center-spread and
> > - * down-spread only. Returns 0 on success, or a negative error value on
> > - * failure.
> > + * Stores the spread spectrum clocking (SSC) configuration of the DPLL and,
> > + * if the DPLL is locked, reprograms it at its current rate to apply it.
> > + * Otherwise the configuration is applied at the next rate change. The
> > + * hardware supports center-spread and down-spread only. Returns 0 on
> > + * success, negative error value otherwise.
> >   */
> >  int omap3_noncore_dpll_set_spread_spectrum(struct clk_hw *hw,
> >                                          const struct clk_spread_spectrum *ss_conf)
> >  {
> >       struct clk_hw_omap *clk = to_clk_hw_omap(hw);
> >       struct dpll_data *dd = clk->dpll_data;
> > +     struct clk_spread_spectrum old_conf;
> > +     struct clk_rate_request req;
> > +     unsigned long rate;
> > +     int ret;
> > +     u32 ctrl;
> >
> >       if (!dd || !dd->ssc_enable_mask)
> >               return -EOPNOTSUPP;
> > @@ -718,9 +723,41 @@ int omap3_noncore_dpll_set_spread_spectrum(struct clk_hw *hw,
> >               return -EINVAL;
> >       }
> >
> > +     ctrl = ti_clk_ll_ops->clk_readl(&dd->control_reg);
> > +
> > +     /* Already applied */
> > +     if (!memcmp(ss_conf, &dd->ssc_conf, sizeof(*ss_conf)) &&
> > +         !!(ctrl & dd->ssc_enable_mask) == (ss_conf->method != CLK_SPREAD_NO))
>
> The == on this last line has me a little confused right now. Should this
> be an && instead?

The == is intended. I agree it is not easy to read, so in v4 I will
split the check into the disabled and enabled cases.

Thanks and regards,
Dario

>
> Brian
>
> --
> Brian Masney <masneyb@xxxxxxxxxx>



-- 
Dario Binacchi
Senior Embedded Software Engineer
M. +39 328 0625246
dario.binacchi@xxxxxxxxxxxxxxxxxxxx
―――――――――――――――
Amarula Solutions SRL
 Via Felice Cavallotti 25D, 41012 Carpi, MO, IT
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com