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

[Brian Masney]

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?

Brian

-- 
Brian Masney <masneyb@xxxxxxxxxx>