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

PATCH Superseded Fabio Estevam 7 participants 17 messages Jan 17, 2012 – Jan 20, 2012

Subsystems (5)

Arm (32-bit) Sound Arm sub-architectures Arm i.MX and Layerscape +1 more subsystems

lore

Share & tools

lore

Fabio Estevam

author

2012-01-17 17:15 UTC #0 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

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 <[email protected]>

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

arch/arm/mach-mxs/clock-mx28.c +2 -0

index 5d68e41..f85c09d 100644
@@ -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);		\

Signature · 1 line

1.7.1

Marek Vasut

2012-01-17 17:49 UTC #1 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 26 lines

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 <[email protected]>
---
 arch/arm/mach-mxs/clock-mx28.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/arch/arm/mach-mxs/clock-mx28.c index 5d68e41..f85c09d 100644

arch/arm/mach-mxs/clock-mx28.c static int name##_set_rate(struct clk *clk, unsigned · line 475

Quoted 1 line

arch/arm/mach-mxs/clock-mx28.c +0 -0

@@ -475,6 +475,8 @@ static int name##_set_rate(struct clk *clk, unsigned
 long rate)		\ return -EINVAL;

arch/arm/mach-mxs/clock-mx28.c static int name##_set_rate(struct clk *clk, unsigned long rate) \ · line 482

Quoted 7 lines

arch/arm/mach-mxs/clock-mx28.c +2 -0

 									\
 	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?

Fabio Estevam

reviewer

2012-01-17 18:00 UTC #2 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 3 lines · Marek Vasut

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

author

2012-01-17 17:58 UTC #3 lore raw

PATCH v2 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

In reply to 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 <[email protected]>

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

arch/arm/mach-mxs/clock-mx28.c +1 -1

index 5d68e41..0bcfd97 100644
@@ -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);		\
 									\

Signature · 1 line

1.7.1

Shawn Guo

2012-01-18 07:14 UTC #5 lore raw

PATCH v2 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

In reply to Fabio Estevam

Quoted 2 lines · Fabio Estevam

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.

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 9 lines

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.

Quoted 13 lines

Signed-off-by Fabio Estevam <[email protected]>
---
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(-)
arch/arm/mach-mxs/clock-mx28.c +0 -0
index 5d68e41..0bcfd97 100644

arch/arm/mach-mxs/clock-mx28.c static int name##_set_rate(struct clk *clk, unsigned long rate) \ · line 481

Quoted 8 lines

arch/arm/mach-mxs/clock-mx28.c +1 -1

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

Lothar Waßmann

2012-01-18 07:44 UTC #6 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

In reply to Fabio Estevam

Hi,

Quoted 26 lines · 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 <[email protected]>
---
 arch/arm/mach-mxs/clock-mx28.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/mach-mxs/clock-mx28.c +0 -0
index 5d68e41..f85c09d 100644

arch/arm/mach-mxs/clock-mx28.c static int name##_set_rate(struct clk *clk, unsigned long rate) \ · line 482

Quoted 8 lines

arch/arm/mach-mxs/clock-mx28.c +2 -0

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

Signature · 9 lines

___________________________________________________________

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
___________________________________________________________

Fabio Estevam

reviewer

2012-01-19 02:18 UTC #7 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 4 lines · Lothar Wa?mann

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:

arch/arm/mach-mxs/clock-mx28.c +2 -0

index 5d68e41..c697b4a 100644
@@ -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:

arch/arm/mach-mxs/clock-mx28.c +0 ~~-10~~

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

Shawn Guo

2012-01-19 03:17 UTC #8 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Add Aisheng into the thread ...

Quoted 22 lines · Fabio Estevam

Quoted 4 lines · Lothar Wa?mann
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:
arch/arm/mach-mxs/clock-mx28.c +2 -0
index 5d68e41..c697b4a 100644
@@ -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);
 

Quoted 1 line

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.

Quoted 25 lines

arch/arm/mach-mxs/clock-mx28.c +0 -0

 	clk_set_parent(&lcdif_clk, &ref_pix_clk);
 	clk_set_parent(&saif0_clk, &pll0_clk);

Or also the patch below instead:

arch/arm/mach-mxs/clock-mx28.c +0 ~~-10~~

@@ -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,

Quoted 3 lines

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.

Quoted 2 lines

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

Quoted 5 lines

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.

Signature · 2 lines

Regards,
Shawn

Fabio Estevam

reviewer

2012-01-19 03:57 UTC #9 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

2012/1/19 Shawn Guo <[email protected]>:

Quoted 4 lines

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:

arch/arm/mach-mxs/clock-mx28.c +4 -1

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

Shawn Guo

2012-01-19 04:20 UTC #10 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 30 lines · Fabio Estevam

2012/1/19 Shawn Guo <[email protected]>:

Quoted 4 lines

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:

arch/arm/mach-mxs/clock-mx28.c +4 -1

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

Quoted 2 lines

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.

Signature · 2 lines

Regards,
Shawn

Fabio Estevam

reviewer

2012-01-19 04:44 UTC #11 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

2012/1/19 Shawn Guo <[email protected]>:

Quoted 2 lines

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?

sound/soc/mxs/mxs-saif.c +5 -0

index dccfb37..c5a29a8 100644
@@ -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

Dong Aisheng-B29396

2012-01-19 05:14 UTC #12 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 39 lines

-----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 <[email protected]>:
Quoted 2 lines
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?
sound/soc/mxs/mxs-saif.c index renamed from sound/soc/mxs/mxs-saif.c +0 -0
dccfb37..c5a29a8 100644
sound/soc/mxs/mxs-saif.c +2 -0
@@ -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 */
sound/soc/mxs/mxs-saif.c +1 -0
+                       clk_disable_unprepare(master_saif->clk);
                        return -EINVAL;
                }
        } else {

sound/soc/mxs/mxs-saif.c static int mxs_saif_set_clk(struct mxs_saif *saif, · line 150

Quoted 8 lines

sound/soc/mxs/mxs-saif.c +2 -0

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

Fabio Estevam

reviewer

2012-01-19 14:04 UTC #13 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 2 lines · On 1/19/12, Dong Aisheng-B29396 <[email protected]> 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

Dong Aisheng-B29396

2012-01-19 04:55 UTC #14 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

In reply to Fabio Estevam

Quoted 47 lines

-----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 <[email protected]>:
Quoted 4 lines
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:
arch/arm/mach-mxs/clock-mx28.c +2 -0
@@ -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);
arch/arm/mach-mxs/clock-mx28.c +2 -1
+       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:
arch/arm/mach-mxs/clock-mx28.c +2 -0
+       clk_disable(&saif0_clk);
+       clk_disable(&saif1_clk);

Quoted 2 lines

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

Russell King - ARM Linux

2012-01-19 14:53 UTC #15 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

In reply to Shawn Guo

Quoted 4 lines · Shawn Guo

Quoted 1 line
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().

Shawn Guo

2012-01-20 03:56 UTC #16 lore raw

PATCH 1/1 ARM: mx28: Clear CLKGATE bit prior to changing DIV field

Quoted 11 lines · Russell King - ARM Linux

Quoted 4 lines · Shawn Guo
Quoted 1 line
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

Quoted 13 lines

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