Re: [PATCH v3 2/3] clk: ti: dpll: use the generic spread spectrum configuration

[Brian Masney]

Hi Dario,

> The clock framework provides a generic API and DT property for spread
> spectrum clocking (SSC) since commit c86814e70390 ("clk: Introduce
> clk_hw_set_spread_spectrum"). Implement the set_spread_spectrum callback
> and convert the deprecated vendor specific properties to the generic
> configuration, so that existing DTs keep working.
> 
> The SSC registers are programmed at the next rate change, as with the
> vendor specific properties. The generic framework expresses the spread
> in permyriad rather than permille, hence the change in the deltaM step
> computation, and also supports up-spread, which is rejected as the
> hardware does not support it.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v3:
> - Keep parsing the deprecated vendor specific properties
> 
>  drivers/clk/ti/clock.h    |  2 ++
>  drivers/clk/ti/dpll.c     | 20 +++++++++----
>  drivers/clk/ti/dpll3xxx.c | 61 ++++++++++++++++++++++++++++++++-------
>  include/linux/clk/ti.h    |  9 ++----
>  4 files changed, 70 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h
> index d5e24fe4ae3a..83225e506314 100644
> --- a/drivers/clk/ti/clock.h
> +++ b/drivers/clk/ti/clock.h
> @@ -271,6 +271,8 @@ int omap3_noncore_dpll_set_rate_and_parent(struct clk_hw *hw,
>  					   unsigned long rate,
>  					   unsigned long parent_rate,
>  					   u8 index);
> +int omap3_noncore_dpll_set_spread_spectrum(struct clk_hw *hw,
> +					   const struct clk_spread_spectrum *ss_conf);
>  int omap3_noncore_dpll_determine_rate(struct clk_hw *hw,
>  				      struct clk_rate_request *req);
>  int omap2_dpll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req);
> diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
> index e01272cd628d..1329b4bc02f0 100644
> --- a/drivers/clk/ti/dpll.c
> +++ b/drivers/clk/ti/dpll.c
> @@ -51,6 +51,7 @@ static const struct clk_ops dpll_ck_ops = {
>  	.set_parent	= &omap3_noncore_dpll_set_parent,
>  	.set_rate_and_parent	= &omap3_noncore_dpll_set_rate_and_parent,
>  	.determine_rate	= &omap3_noncore_dpll_determine_rate,
> +	.set_spread_spectrum	= &omap3_noncore_dpll_set_spread_spectrum,
>  	.get_parent	= &omap2_init_dpll_parent,
>  	.save_context	= &omap3_noncore_dpll_save_context,
>  	.restore_context = &omap3_noncore_dpll_restore_context,
> @@ -63,6 +64,7 @@ static const struct clk_ops dpll_no_gate_ck_ops = {
>  	.set_parent	= &omap3_noncore_dpll_set_parent,
>  	.set_rate_and_parent	= &omap3_noncore_dpll_set_rate_and_parent,
>  	.determine_rate	= &omap3_noncore_dpll_determine_rate,
> +	.set_spread_spectrum	= &omap3_noncore_dpll_set_spread_spectrum,
>  	.save_context	= &omap3_noncore_dpll_save_context,
>  	.restore_context = &omap3_noncore_dpll_restore_context
>  };
> @@ -278,6 +280,7 @@ static void __init of_ti_dpll_setup(struct device_node *node,
>  	struct dpll_data *dd = NULL;
>  	int ssc_clk_index;
>  	u8 dpll_mode = 0;
> +	u32 ssc_modfreq = 0, ssc_deltam = 0;

Please keep the reverse Christmas tree order here.

>  	u32 min_div;
>  
>  	dd = kmemdup(ddt, sizeof(*dd), GFP_KERNEL);
> @@ -349,11 +352,18 @@ static void __init of_ti_dpll_setup(struct device_node *node,
>  					&dd->ssc_modfreq_reg))
>  			goto cleanup;
>  
> -		of_property_read_u32(node, "ti,ssc-modfreq-hz",
> -				     &dd->ssc_modfreq);
> -		of_property_read_u32(node, "ti,ssc-deltam", &dd->ssc_deltam);
> -		dd->ssc_downspread =
> -			of_property_read_bool(node, "ti,ssc-downspread");
> +		/* Deprecated, superseded by assigned-clock-sscs */
> +		of_property_read_u32(node, "ti,ssc-modfreq-hz", &ssc_modfreq);
> +		of_property_read_u32(node, "ti,ssc-deltam", &ssc_deltam);
> +		if (ssc_modfreq && ssc_deltam) {
> +			dd->ssc_conf.modfreq_hz = ssc_modfreq;
> +			/* permille to permyriad */
> +			dd->ssc_conf.spread_bp = ssc_deltam * 10;

Would it be worthwhile to add a maximum range check here so the u32 does
not overflow?

> +			if (of_property_read_bool(node, "ti,ssc-downspread"))
> +				dd->ssc_conf.method = CLK_SPREAD_DOWN;
> +			else
> +				dd->ssc_conf.method = CLK_SPREAD_CENTER;
> +		}
>  	}
>  
>  	if (of_property_read_bool(node, "ti,low-power-stop"))
> diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
> index 8c51b988a04f..634fe0a1f9fd 100644
> --- a/drivers/clk/ti/dpll3xxx.c
> +++ b/drivers/clk/ti/dpll3xxx.c
> @@ -295,30 +295,32 @@ static void _lookup_sddiv(struct clk_hw_omap *clk, u8 *sd_div, u16 m, u8 n)
>   * omap3_noncore_dpll_ssc_program - set spread-spectrum clocking registers
>   * @clk:	struct clk * of DPLL to set
>   *
> - * Enable the DPLL spread spectrum clocking if frequency modulation and
> - * frequency spreading have been set, otherwise disable it.
> + * Enable the DPLL spread spectrum clocking according to the configuration
> + * set by omap3_noncore_dpll_set_spread_spectrum(), otherwise disable it.
>   */
>  static void omap3_noncore_dpll_ssc_program(struct clk_hw_omap *clk)
>  {
>  	struct dpll_data *dd = clk->dpll_data;
> +	const struct clk_spread_spectrum *conf = &dd->ssc_conf;
> +	bool downspread = conf->method == CLK_SPREAD_DOWN;
>  	unsigned long ref_rate;
>  	u32 v, ctrl, mod_freq_divider, exponent, mantissa;
>  	u32 deltam_step, deltam_ceil;
>  
>  	ctrl = ti_clk_ll_ops->clk_readl(&dd->control_reg);
>  
> -	if (dd->ssc_modfreq && dd->ssc_deltam) {
> +	if (conf->method != CLK_SPREAD_NO) {
>  		ctrl |= dd->ssc_enable_mask;
>  
> -		if (dd->ssc_downspread)
> +		if (downspread)
>  			ctrl |= dd->ssc_downspread_mask;
>  		else
>  			ctrl &= ~dd->ssc_downspread_mask;
>  
>  		ref_rate = clk_hw_get_rate(dd->clk_ref);
>  		mod_freq_divider =
> -		    (ref_rate / dd->last_rounded_n) / (4 * dd->ssc_modfreq);
> -		if (dd->ssc_modfreq > (ref_rate / 70))
> +		    (ref_rate / dd->last_rounded_n) / (4 * conf->modfreq_hz);
> +		if (conf->modfreq_hz > (ref_rate / 70))
>  			pr_warn("clock: SSC modulation frequency of DPLL %s greater than %ld\n",
>  				__clk_get_name(clk->hw.clk), ref_rate / 70);
>  
> @@ -337,9 +339,10 @@ static void omap3_noncore_dpll_ssc_program(struct clk_hw_omap *clk)
>  		v |= exponent << __ffs(dd->ssc_modfreq_exp_mask);
>  		ti_clk_ll_ops->clk_writel(v, &dd->ssc_modfreq_reg);
>  
> -		deltam_step = dd->last_rounded_m * dd->ssc_deltam;
> -		deltam_step /= 10;
> -		if (dd->ssc_downspread)
> +		/* spread_bp is in permyriad, convert to percent first */
> +		deltam_step = dd->last_rounded_m * conf->spread_bp;
> +		deltam_step /= 100;
> +		if (downspread)
>  			deltam_step /= 2;
>  
>  		deltam_step <<= __ffs(dd->ssc_deltam_int_mask);
> @@ -353,7 +356,7 @@ static void omap3_noncore_dpll_ssc_program(struct clk_hw_omap *clk)
>  		if (deltam_step & dd->ssc_deltam_frac_mask)
>  			deltam_ceil++;
>  
> -		if ((dd->ssc_downspread &&
> +		if ((downspread &&
>  		     ((dd->last_rounded_m - (2 * deltam_ceil)) < 20 ||
>  		      dd->last_rounded_m > 2045)) ||
>  		    ((dd->last_rounded_m - deltam_ceil) < 20 ||
> @@ -682,6 +685,44 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned long rate,
>  	return ret;
>  }
>  
> +/**
> + * omap3_noncore_dpll_set_spread_spectrum - set the DPLL SSC configuration
> + * @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.
> + */
> +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;
> +
> +	if (!dd || !dd->ssc_enable_mask)
> +		return -EOPNOTSUPP;
> +
> +	switch (ss_conf->method) {
> +	case CLK_SPREAD_NO:
> +		break;
> +	case CLK_SPREAD_CENTER:
> +	case CLK_SPREAD_DOWN:
> +		if (!ss_conf->modfreq_hz || !ss_conf->spread_bp)

clk_hw_set_spread_spectrum() already contains this check, and can be
dropped here.

Brian

-- 
Brian Masney <masneyb@xxxxxxxxxx>