[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

MX28 Reference Manual states the following about the CLKGATE bit of register HW_CLKCTRL_SAIF0:

"The DIV field can change ONLY when this clock gate bit field is low."

So clear this bit prior to writing to the DIV field as required.

This also fixes the following error during mxs-sgtl5000 probe.

[    0.660000] saif0_clk_set_rate: divider writing timeout                     
[    0.670000] mxs-sgtl5000: probe of mxs-sgtl5000.0 failed with error -110    
[    0.670000] ALSA device list:                                               
[    0.680000]   No soundcards found.  

Audio is functional after this change.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-mxs/clock-mx28.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index 5d68e41..f85c09d 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -475,6 +475,8 @@ static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 		return -EINVAL;						\
 									\
 	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
+									\
+	reg &= ~BM_CLKCTRL_##rs##_CLKGATE;				\
 	reg &= ~BM_CLKCTRL_##rs##_DIV;					\
 	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
-- 
1.7.1

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Marek Vasut]

> MX28 Reference Manual states the following about the CLKGATE bit of
> register HW_CLKCTRL_SAIF0:
> 
> "The DIV field can change ONLY when this clock gate bit field is low."
> 
> So clear this bit prior to writing to the DIV field as required.
> 
> This also fixes the following error during mxs-sgtl5000 probe.
> 
> [    0.660000] saif0_clk_set_rate: divider writing timeout
> [    0.670000] mxs-sgtl5000: probe of mxs-sgtl5000.0 failed with error -110
> [    0.670000] ALSA device list:
> [    0.680000]   No soundcards found.
> 
> Audio is functional after this change.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-mxs/clock-mx28.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c
> b/arch/arm/mach-mxs/clock-mx28.c index 5d68e41..f85c09d 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -475,6 +475,8 @@ static int name##_set_rate(struct clk *clk, unsigned
> long rate)		\ return -EINVAL;						
\
>  									\
>  	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
> +									\
> +	reg &= ~BM_CLKCTRL_##rs##_CLKGATE;				\
>  	reg &= ~BM_CLKCTRL_##rs##_DIV;					\
>  	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
>  	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\

Can you really do this in one swipe ? Or do you need to actually do one register 
write to ungate and another to do the configuration?

M

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

On 1/17/12, Marek Vasut <marek.vasut@gmail.com> wrote:

> Can you really do this in one swipe ? Or do you need to actually do one
> register
> write to ungate and another to do the configuration?

Yes, I can combine the CLKGATE and DIV field clearing operations.

Just sent v2 based on your suggestion.

Thanks,

Fabio Estevam

[PATCH v2] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

MX28 Reference Manual states the following about the CLKGATE bit of register HW_CLKCTRL_SAIF0:

"The DIV field can change ONLY when this clock gate bit field is low."

So clear this bit prior to writing to the DIV field as required.

This also fixes the following error during mxs-sgtl5000 probe.

[    0.660000] saif0_clk_set_rate: divider writing timeout                     
[    0.670000] mxs-sgtl5000: probe of mxs-sgtl5000.0 failed with error -110    
[    0.670000] ALSA device list:                                               
[    0.680000]   No soundcards found.  

Audio is functional after this change.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Clear CLKGATE  and DIV fields at the same time

 arch/arm/mach-mxs/clock-mx28.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index 5d68e41..0bcfd97 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -475,7 +475,7 @@ static int name##_set_rate(struct clk *clk, unsigned long rate)		\
 		return -EINVAL;						\
 									\
 	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
-	reg &= ~BM_CLKCTRL_##rs##_DIV;					\
+	reg &= ~(BM_CLKCTRL_##rs##_CLKGATE | BM_CLKCTRL_##rs##_DIV);	\
 	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
 	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
 									\
-- 
1.7.1

[PATCH v2] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Marek Vasut]

> MX28 Reference Manual states the following about the CLKGATE bit of
> register HW_CLKCTRL_SAIF0:
> 
> "The DIV field can change ONLY when this clock gate bit field is low."
> 
> So clear this bit prior to writing to the DIV field as required.
> 
> This also fixes the following error during mxs-sgtl5000 probe.
> 
> [    0.660000] saif0_clk_set_rate: divider writing timeout
> [    0.670000] mxs-sgtl5000: probe of mxs-sgtl5000.0 failed with error -110
> [    0.670000] ALSA device list:
> [    0.680000]   No soundcards found.
> 
> Audio is functional after this change.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Clear CLKGATE  and DIV fields at the same time
> 
>  arch/arm/mach-mxs/clock-mx28.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c
> b/arch/arm/mach-mxs/clock-mx28.c index 5d68e41..0bcfd97 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -475,7 +475,7 @@ static int name##_set_rate(struct clk *clk, unsigned
> long rate)		\ return -EINVAL;						
\
>  									\
>  	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
> -	reg &= ~BM_CLKCTRL_##rs##_DIV;					\
> +	reg &= ~(BM_CLKCTRL_##rs##_CLKGATE | BM_CLKCTRL_##rs##_DIV);	\
>  	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
>  	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
>  									\

Acked-by: Marek Vasut <marek.vasut@gmail.com>

[PATCH v2] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Shawn Guo]

On Tue, Jan 17, 2012 at 03:58:51PM -0200, Fabio Estevam wrote:
> MX28 Reference Manual states the following about the CLKGATE bit of register HW_CLKCTRL_SAIF0:
> 
This is stated for not only SAIF but also other clocks with a divider.

> "The DIV field can change ONLY when this clock gate bit field is low."
> 
I'm afraid that the change of DIV field does not necessarily mean there
is a desired frequency output on the clock.  I remember I have done a
testing to see that I can update the DIV field of one clock with
clearing its gate bit but keeping its parent clock gated.  But obviously
you won't get a frequency output on the clock in this case.

So translating the above statement to what I have seen, I would say
the rate of one clock can only be changed when the clock is enabled.
To clk api client drivers, that means clk_enable() has been called
on the clock to have the clock itself and its parent clock enabled.

> So clear this bit prior to writing to the DIV field as required.
> 
I would say clk_enable() prior to clk_set_rate() is required.

> This also fixes the following error during mxs-sgtl5000 probe.
> 
> [    0.660000] saif0_clk_set_rate: divider writing timeout                     
> [    0.670000] mxs-sgtl5000: probe of mxs-sgtl5000.0 failed with error -110    
> [    0.670000] ALSA device list:                                               
> [    0.680000]   No soundcards found.  
> 
> Audio is functional after this change.
> 
This is an known issue.  Some people prefer to fix it in the mxs clock
code while I incline to say the client driver should be fixed for this.

> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> Changes since v1:
> - Clear CLKGATE  and DIV fields at the same time
> 
>  arch/arm/mach-mxs/clock-mx28.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index 5d68e41..0bcfd97 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -475,7 +475,7 @@ static int name##_set_rate(struct clk *clk, unsigned long rate)		\
>  		return -EINVAL;						\
>  									\
>  	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
> -	reg &= ~BM_CLKCTRL_##rs##_DIV;					\
> +	reg &= ~(BM_CLKCTRL_##rs##_CLKGATE | BM_CLKCTRL_##rs##_DIV);	\
>  	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
>  	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
>  									\
So you end up with leaving the gate bit of the clock changed by
clk_set_rate() call.  I'm not sure that is desirable.

Regards,
Shawn

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Lothar Waßmann]

Hi,

Fabio Estevam writes:
> MX28 Reference Manual states the following about the CLKGATE bit of register HW_CLKCTRL_SAIF0:
> 
> "The DIV field can change ONLY when this clock gate bit field is low."
> 
> So clear this bit prior to writing to the DIV field as required.
> 
> This also fixes the following error during mxs-sgtl5000 probe.
> 
> [    0.660000] saif0_clk_set_rate: divider writing timeout                     
> [    0.670000] mxs-sgtl5000: probe of mxs-sgtl5000.0 failed with error -110    
> [    0.670000] ALSA device list:                                               
> [    0.680000]   No soundcards found.  
> 
> Audio is functional after this change.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-mxs/clock-mx28.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index 5d68e41..f85c09d 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -475,6 +475,8 @@ static int name##_set_rate(struct clk *clk, unsigned long rate)		\
>  		return -EINVAL;						\
>  									\
>  	reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
> +									\
> +	reg &= ~BM_CLKCTRL_##rs##_CLKGATE;				\
>  	reg &= ~BM_CLKCTRL_##rs##_DIV;					\
>  	reg |= div << BP_CLKCTRL_##rs##_DIV;				\
>  	__raw_writel(reg, CLKCTRL_BASE_ADDR + HW_CLKCTRL_##rs);		\
>
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.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

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);

 	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.

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

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Shawn Guo]

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.

-- 
Regards,
Shawn

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

2012/1/19 Shawn Guo <shawn.guo@linaro.org>:

> 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.

So something like:

--- 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);

        clk_set_parent(&lcdif_clk, &ref_pix_clk);
        clk_set_parent(&saif0_clk, &pll0_clk);
@@ -822,6 +824,8 @@ int __init mx28_clocks_init(void)
         */
        clk_set_rate(&saif0_clk, 24000000);
        clk_set_rate(&saif1_clk, 24000000);
+       clk_disable_unprepare(&saif0_clk);
+       clk_disable_unprepare(&saif1_clk);

        clkdev_add_table(lookups, ARRAY_SIZE(lookups));
---

didn't work on my tests (probe of mxs-saif fails with the same timeout message).

However if I use the clk_disable versions:

+       clk_disable(&saif0_clk);
+       clk_disable(&saif1_clk);

Then it works fine.

Regards,

Fabio Estevam

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Shawn Guo]

On Thu, Jan 19, 2012 at 01:57:45AM -0200, Fabio Estevam wrote:
> 2012/1/19 Shawn Guo <shawn.guo@linaro.org>:
> 
> > 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.
> 
> So something like:
> 
> --- 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);
> 
>         clk_set_parent(&lcdif_clk, &ref_pix_clk);
>         clk_set_parent(&saif0_clk, &pll0_clk);
> @@ -822,6 +824,8 @@ int __init mx28_clocks_init(void)
>          */
>         clk_set_rate(&saif0_clk, 24000000);
>         clk_set_rate(&saif1_clk, 24000000);
> +       clk_disable_unprepare(&saif0_clk);
> +       clk_disable_unprepare(&saif1_clk);
> 
>         clkdev_add_table(lookups, ARRAY_SIZE(lookups));
> ---
> 
> didn't work on my tests (probe of mxs-saif fails with the same timeout message).
> 
As far as I know, mxs-saif driver also has one clk_set_rate() being
called with the clock gated.

-- 
Regards,
Shawn

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

2012/1/19 Shawn Guo <shawn.guo@linaro.org>:

> As far as I know, mxs-saif driver also has one clk_set_rate() being
> called with the clock gated.

Yes, correct. Is this a good fix?

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index dccfb37..c5a29a8 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -124,6 +124,8 @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
         *
         * If MCLK is not used, we just set saif clk to 512*fs.
         */
+       clk_prepare_enable(master_saif->clk);
+
        if (master_saif->mclk_in_use) {
                if (mclk % 32 == 0) {
                        scr &= ~BM_SAIF_CTRL_BITCLK_BASE_RATE;
@@ -133,6 +135,7 @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
                        ret = clk_set_rate(master_saif->clk, 384 * rate);
                } else {
                        /* SAIF MCLK should be either 32x or 48x */
+                       clk_disable_unprepare(master_saif->clk);
                        return -EINVAL;
                }
        } else {
@@ -140,6 +143,8 @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
                scr &= ~BM_SAIF_CTRL_BITCLK_BASE_RATE;
        }

+       clk_disable_unprepare(master_saif->clk);
+
        if (ret)
                return ret;


Regards,

Fabio Estevam

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: Thursday, January 19, 2012 12:45 PM
> To: Shawn Guo
> Cc: Lothar Wa?mann; Estevam Fabio-R49496; w.sang at pengutronix.de;
> marek.vasut at gmail.com; linux-arm-kernel at lists.infradead.org;
> kernel at pengutronix.de; Guo Shawn-R65073; Dong Aisheng-B29396
> Subject: Re: [PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field
> Importance: High
> 
> 2012/1/19 Shawn Guo <shawn.guo@linaro.org>:
> 
> > As far as I know, mxs-saif driver also has one clk_set_rate() being
> > called with the clock gated.
> 
> Yes, correct. Is this a good fix?
> 
> diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index
> dccfb37..c5a29a8 100644
> --- a/sound/soc/mxs/mxs-saif.c
> +++ b/sound/soc/mxs/mxs-saif.c
> @@ -124,6 +124,8 @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
>          *
>          * If MCLK is not used, we just set saif clk to 512*fs.
>          */
> +       clk_prepare_enable(master_saif->clk);
> +
>         if (master_saif->mclk_in_use) {
>                 if (mclk % 32 == 0) {
>                         scr &= ~BM_SAIF_CTRL_BITCLK_BASE_RATE; @@ -133,6 +135,7
> @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
>                         ret = clk_set_rate(master_saif->clk, 384 * rate);
>                 } else {
>                         /* SAIF MCLK should be either 32x or 48x */
> +                       clk_disable_unprepare(master_saif->clk);
>                         return -EINVAL;
>                 }
>         } else {
> @@ -140,6 +143,8 @@ static int mxs_saif_set_clk(struct mxs_saif *saif,
>                 scr &= ~BM_SAIF_CTRL_BITCLK_BASE_RATE;
>         }
> 
> +       clk_disable_unprepare(master_saif->clk);
> +
>         if (ret)
>                 return ret;
> 
The change looks ok to me.
If the test is ok, I will ack this patch.

Regards
Dong Aisheng

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Fabio Estevam]

On 1/19/12, Dong Aisheng-B29396 <B29396@freescale.com> wrote:

> The change looks ok to me.
> If the test is ok, I will ack this patch.

Yes, audio works with this change. I submitted the patch to the alsa-devel list.

Regards,

Fabio Estevam

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Fabio Estevam [mailto:festevam at gmail.com]
> Sent: Thursday, January 19, 2012 11:58 AM
> To: Shawn Guo
> Cc: Lothar Wa?mann; Estevam Fabio-R49496; w.sang at pengutronix.de;
> marek.vasut at gmail.com; linux-arm-kernel at lists.infradead.org;
> kernel at pengutronix.de; Guo Shawn-R65073; Dong Aisheng-B29396
> Subject: Re: [PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field
> Importance: High
> 
> 2012/1/19 Shawn Guo <shawn.guo@linaro.org>:
> 
> > 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.
> 
> So something like:
> 
> --- 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);
> 
>         clk_set_parent(&lcdif_clk, &ref_pix_clk);
>         clk_set_parent(&saif0_clk, &pll0_clk); @@ -822,6 +824,8 @@ int __init
> mx28_clocks_init(void)
>          */
>         clk_set_rate(&saif0_clk, 24000000);
>         clk_set_rate(&saif1_clk, 24000000);
> +       clk_disable_unprepare(&saif0_clk);
> +       clk_disable_unprepare(&saif1_clk);
> 
>         clkdev_add_table(lookups, ARRAY_SIZE(lookups));
> ---
> 
> didn't work on my tests (probe of mxs-saif fails with the same timeout message).
> 
> However if I use the clk_disable versions:
> 
> +       clk_disable(&saif0_clk);
> +       clk_disable(&saif1_clk);
> 
> Then it works fine.
> 

It looks strange to me.
Is there any big difference between clk_disable and clk_disable_unprepare for MXS?
Why clk_disable works?

Regards
Dong Aisheng

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Russell King - ARM Linux]

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().

[PATCH] ARM: mx28: Clear CLKGATE bit prior to changing DIV field

[Shawn Guo]

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().
>