Re: [PATCH E 12/14] OMAP clock: unnecessary clock flag removal fiesta

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Wed, Jan 28, 2009 at 12:28:01PM -0700, Paul Walmsley wrote:
> 4. The CONFIG_PARTICIPANT flags indicates to the clock rate and parent
> changing functions that they should not be used on this clock.  Better
> just to remove the clock function pointers that operate on those
> clocks.  The name of the flag is just terrible: its meaning has almost
> nothing to do with its name, and the use of the CONFIG_ prefix makes
> it appear to be a Kconfig option.  Get rid of it.

Hmm.  Completely agree over the naming issue, however I think this
opens a hole.

> @@ -780,12 +772,6 @@ int omap2_clk_set_rate(struct clk *clk, unsigned long rate)
>  
>  	pr_debug("clock: set_rate for clock %s to rate %ld\n", clk->name, rate);
>  
> -	/* CONFIG_PARTICIPANT clocks are changed only in sets via the
> -	   rate table mechanism, driven by mpu_speed  */
> -	if (clk->flags & CONFIG_PARTICIPANT)
> -		return -EINVAL;
> -
> -	/* dpll_ck, core_ck, virt_prcm_set; plus all clksel clocks */
>  	if (clk->set_rate)
>  		ret = clk->set_rate(clk, rate);
>  

The removal of this can be mitigated as indicated; not having a set_rate
method does indeed cause this to return -EINVAL.

> @@ -830,9 +816,6 @@ int omap2_clk_set_parent(struct clk *clk, struct clk *new_parent)
>  {
>  	u32 field_val, v, parent_div;
>  
> -	if (clk->flags & CONFIG_PARTICIPANT)
> -		return -EINVAL;
> -
>  	if (!clk->clksel)
>  		return -EINVAL;
>  

This one, however, is not...

> @@ -1020,16 +1018,13 @@ static struct clk mpu_ck = {	/* Control cpu */
>  	.parent		= &core_ck,
>  	.prcm_mod	= MPU_MOD,
>  	.flags		= CLOCK_IN_OMAP242X | CLOCK_IN_OMAP243X |
> -				ALWAYS_ENABLED | DELAYED_APP |
> -				CONFIG_PARTICIPANT | RATE_PROPAGATES,
> +				ALWAYS_ENABLED | DELAYED_APP,
>  	.clkdm		= { .name = "mpu_clkdm" },
>  	.init		= &omap2_init_clksel_parent,
>  	.clksel_reg	= CM_CLKSEL,
>  	.clksel_mask	= OMAP24XX_CLKSEL_MPU_MASK,
>  	.clksel		= mpu_clksel,
>  	.recalc		= &omap2_clksel_recalc,
> -	.round_rate	= &omap2_clksel_round_rate,
> -	.set_rate	= &omap2_clksel_set_rate
>  };
>  
>  /*

For example, here we have a clock which was previously marked with
CONFIG_PARTICIPANT, and has a non-NULL clksel member.  This means
that clk_set_parent() would return -EINVAL before, but would have
an effect after the removal of this flag.

The same is true of dsp_fck, dsp_irate_ick, iva1_ifck, core_l3_ck,
usb_l4_ick and mdm_ick.

So, is this change safe for these clocks?

(Note: all the other flags have already gone in my tree.)