ARM: mx28: Clear CLKGATE bit prior to changing DIV field

On Wed, Jan 18, 2012 at 5:44 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:

> Now you are doing clk_enable()'s business in clk_set_rate().
> The same result could be achieved by calling clk_enable() prior to
> clk_set_rate(). clk_set_rate() could simply return an error code, if
> the clock is not enabled.

Ok, understood.

Your suggestion below works fine:


I saw your comments about the need for clk_set_rate, so it looks like
it is not safe to remove it.

Can Lothar's suggestion be accepted?

In your previous reply you seemed to prefer a solution at mxs-saif.c,
but if we need to keep the clk_set_rate for saif, then we
need the clk_prepare_enable for saif clk. I am a bit unsure of what
your proposal is.

Regards,

Fabio Estevam

Add Aisheng into the thread ...

On Thu, Jan 19, 2012 at 12:18:53AM -0200, Fabio Estevam wrote:
> On Wed, Jan 18, 2012 at 5:44 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> 
> > Now you are doing clk_enable()'s business in clk_set_rate().
> > The same result could be achieved by calling clk_enable() prior to
> > clk_set_rate(). clk_set_rate() could simply return an error code, if
> > the clock is not enabled.
> 
> Ok, understood.
> 
> Your suggestion below works fine:
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index 5d68e41..c697b4a 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -809,6 +809,8 @@ int __init mx28_clocks_init(void)
>  	clk_prepare_enable(&xbus_clk);
>  	clk_prepare_enable(&emi_clk);
>  	clk_prepare_enable(&uart_clk);
> +	clk_prepare_enable(&saif0_clk);
> +	clk_prepare_enable(&saif1_clk);
> 
>From power saving point of view, I'm not sure you want to keep saif
clock on all the time.  So if the clock is off when you try to call
clk_set_rate(), you may want to turn it back to off after clk_set_rate()
is done.

>  	clk_set_parent(&lcdif_clk, &ref_pix_clk);
>  	clk_set_parent(&saif0_clk, &pll0_clk);
> 
> Or also the patch below instead:
> 
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -814,15 +814,6 @@ int __init mx28_clocks_init(void)
>         clk_set_parent(&saif0_clk, &pll0_clk);
>         clk_set_parent(&saif1_clk, &pll0_clk);
> 
> -       /*
> -        * Set an initial clock rate for the saif internal logic to work
> -        * properly. This is important when working in EXTMASTER mode that
> -        * uses the other saif's BITCLK&LRCLK but it still needs a basic
> -        * clock which should be fast enough for the internal logic.
> -        */
> -       clk_set_rate(&saif0_clk, 24000000);
> -       clk_set_rate(&saif1_clk, 24000000);
> -
>         clkdev_add_table(lookups, ARRAY_SIZE(lookups));
> ---
> 
> Shawn,
> 
> I saw your comments about the need for clk_set_rate, so it looks like
> it is not safe to remove it.
> 
That piece of code was added by Aisheng for saif recording support.

> Can Lothar's suggestion be accepted?
> 
I agree with Lothar's suggestion.  The first thing for me to do may be
picking up Wolfram's patch below, which was posted some time ago, so
that we can have clk_set_rate() returns error when the clock is gated.

[PATCH] arm: mx28: check for gated clocks when setting saif divider
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-September/064712.html

> In your previous reply you seemed to prefer a solution at mxs-saif.c,
> but if we need to keep the clk_set_rate for saif, then we
> need the clk_prepare_enable for saif clk. I am a bit unsure of what
> your proposal is.
> 
My proposal is we need to clk_prepare_enable before clk_set_rate a clock
if the clock is gated, and then clk_disable_unprepare the clock after
clk_set_rate is done.  This applies whatever codes that want to
clk_set_rate a mxs clock.

On Thu, Jan 19, 2012 at 11:17:34AM +0800, Shawn Guo wrote:
> >From power saving point of view, I'm not sure you want to keep saif
> clock on all the time.  So if the clock is off when you try to call
> clk_set_rate(), you may want to turn it back to off after clk_set_rate()
> is done.

You really shouldn't expose these kinds of SoC specific oddities outside
of the API - it makes a mockery of having an API in the first place.

Can you not do:

clk_set_rate(clk, rate)
{
	clk_prepare(clk);

	reprogram_clock(clk);

	clk_unprepare(clk);
}

A clk_prepare() call on an already prepared clock should have no impact
other than incrementing the counter, which will be balanced on the other
side by the clk_unprepare().

On Thu, Jan 19, 2012 at 02:53:13PM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 19, 2012 at 11:17:34AM +0800, Shawn Guo wrote:
> > >From power saving point of view, I'm not sure you want to keep saif
> > clock on all the time.  So if the clock is off when you try to call
> > clk_set_rate(), you may want to turn it back to off after clk_set_rate()
> > is done.
> 
> You really shouldn't expose these kinds of SoC specific oddities outside
> of the API - it makes a mockery of having an API in the first place.
> 
> Can you not do:
> 
Yes, we can do this for now, but I'm not sure how it will live when we
migrate to common clock framework.  I actually had a brief talk with
Mike Turquette on that.  He does not think this case is common enough
and deserve being handled in clock framework.  He is trying to avoid
cross calling of clock API.  I kinda agree with him that for some cases
the client drivers may need to know the details of clock hardware at
some level.

Regards,
Shawn

> clk_set_rate(clk, rate)
> {
> 	clk_prepare(clk);
> 
> 	reprogram_clock(clk);
> 
> 	clk_unprepare(clk);
> }
> 
> A clk_prepare() call on an already prepared clock should have no impact
> other than incrementing the counter, which will be balanced on the other
> side by the clk_unprepare().
>