Re: [PATCH v2 1/1] ARM : omap3 : fix wrong container_of in clock36xx.c

[Mike Turquette <mturquette@linaro.org>]

Quoting Jean-Philippe Francois (2013-05-30 01:50:27)
> omap36xx_pwrdn_clk_enable_with_hsdiv_restore expects the parent hw of the clock
> to be a clk_hw_omap. However, looking at cclock3xxx_data.c, all concerned clock
> have parent defined as clk_divider. Instead of using container_of to eventually get
> to the register and directly mess with the divider, change freq via clk_set_rate, 
> and let the clock framework toggle the divider value.
> Tested with  3.9 on dm3730.
> 
> Signed-off-by: Jean-Philippe Fran��ois <jp.francois@cynove.com>

Did you use git-format-patch to create this patch?  Its a bit nicer to
use that or if you just use diff then use "diff -up" or "diff -uprN"
(taken from Documentation/SubmittingPatches.txt).

Also did you test this to make sure it works?  I don't mean a boot test,
but actually disabling/re-enabling an HSDIVIDER on 3630?  The previous
code just programmed the clksel field to 1, and this code divides the
rate by 2, then restores it.  I just used that as an example in my
previous email and it needs to be verified that it works (though it
should if I remember this errata correctly).

If that testing is done and everything looks good then please add:

Acked-by: Mike Turquette <mturquette@linaro.org>

> 
> Index: b/arch/arm/mach-omap2/clock36xx.c
> ===================================================================
> --- a/arch/arm/mach-omap2/clock36xx.c
> +++ b/arch/arm/mach-omap2/clock36xx.c
> @@ -39,30 +39,25 @@
>   */
>  int omap36xx_pwrdn_clk_enable_with_hsdiv_restore(struct clk_hw *clk)
>  {
> -       struct clk_hw_omap *parent;
> -       struct clk_hw *parent_hw;
> -       u32 dummy_v, orig_v, clksel_shift;
>         int ret;
>  
>         /* Clear PWRDN bit of HSDIVIDER */
>         ret = omap2_dflt_clk_enable(clk);
>  
> -       parent_hw = __clk_get_hw(__clk_get_parent(clk->clk));
> -       parent = to_clk_hw_omap(parent_hw);
> -
> -       /* Restore the dividers */
> +       /* kick parent's clksel register after toggling PWRDN bit */
>         if (!ret) {
> -               clksel_shift = __ffs(parent->clksel_mask);
> -               orig_v = __raw_readl(parent->clksel_reg);
> -               dummy_v = orig_v;
> -
> -               /* Write any other value different from the Read value */
> -               dummy_v ^= (1 << clksel_shift);
> -               __raw_writel(dummy_v, parent->clksel_reg);
> -
> -               /* Write the original divider */
> -               __raw_writel(orig_v, parent->clksel_reg);
> +               struct clk *parent = clk_get_parent(clk->clk);
> +               unsigned long parent_rate = clk_get_rate(parent);
> +               ret = clk_set_rate(parent, parent_rate/2);
> +               if(ret)
> +                       goto badfreq;
> +               ret = clk_set_rate(parent, parent_rate);
> +               if(ret)
> +                       goto badfreq;
>         }
> +       return ret;
>  
> + badfreq :
> +       omap2_dflt_clk_disable(clk);
>         return ret;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html