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

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

[Jean-Philippe Francois]

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>

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

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

[Jean-Philippe Francois]

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>

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

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

[Mike Turquette]

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

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

[Mike Turquette]

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

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

[jean-philippe francois]

2013/5/31 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).
>
It is easier for my build system to work with tarball + quilt patches, so
I am not working with git. I will look into the alternative you provided.

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

Yes, it is exactly what happens on my board when using the camera, because
the sensor clock is provided by the SoC. So this patch works for this
particular clock.
If the asked frequency is the lower limit of the frequency range, then
asking for half the
frequency won't change the divider, but I think it is not a problem in practice.

> If that testing is done and everything looks good then please add:
>
> Acked-by: Mike Turquette <mturquette@linaro.org>
>
How should I proceed ? Should I just add the acked by below, or should
I resend the patch ?

Jean-Philippe François

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

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

[jean-philippe francois]

2013/5/31 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).
>
It is easier for my build system to work with tarball + quilt patches, so
I am not working with git. I will look into the alternative you provided.

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

Yes, it is exactly what happens on my board when using the camera, because
the sensor clock is provided by the SoC. So this patch works for this
particular clock.
If the asked frequency is the lower limit of the frequency range, then
asking for half the
frequency won't change the divider, but I think it is not a problem in practice.

> If that testing is done and everything looks good then please add:
>
> Acked-by: Mike Turquette <mturquette@linaro.org>
>
How should I proceed ? Should I just add the acked by below, or should
I resend the patch ?

Jean-Philippe Fran?ois

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

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

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3673 bytes --]

On Thu, 30 May 2013, Jean-Philippe Francois wrote:

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

Queued for 3.10-rc fixes with Mike's ack, after fixing the checkpatch.pl 
errors listed below.  Please don't forget to run scripts/checkpatch.pl on 
future patches -- Documentation/SubmittingPatches has more info about 
this.


ERROR: space required before the open parenthesis '('
#59: FILE: arch/arm/mach-omap2/clock36xx.c:52:
+		if(ret)

ERROR: space required before the open parenthesis '('
#62: FILE: arch/arm/mach-omap2/clock36xx.c:55:
+		if(ret)

WARNING: please, no spaces at the start of a line
#67: FILE: arch/arm/mach-omap2/clock36xx.c:60:
+ badfreq :$

ERROR: space prohibited before that ':' (ctx:WxE)
#67: FILE: arch/arm/mach-omap2/clock36xx.c:60:
+ badfreq :
          ^

Updated patch follows.


- Paul

From: Jean-Philippe Francois <jp.francois@cynove.com>
Date: Thu, 30 May 2013 10:50:27 +0200
Subject: [PATCH] ARM: omap3: clock: fix wrong container_of in clock36xx.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
Acked-by: Mike Turquette <mturquette@linaro.org>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/clock36xx.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/clock36xx.c b/arch/arm/mach-omap2/clock36xx.c
index 8f3bf4e..7f69fbb 100644
--- 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;
 }
-- 
1.7.10.4

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

[Paul Walmsley]

On Thu, 30 May 2013, Jean-Philippe Francois wrote:

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

Queued for 3.10-rc fixes with Mike's ack, after fixing the checkpatch.pl 
errors listed below.  Please don't forget to run scripts/checkpatch.pl on 
future patches -- Documentation/SubmittingPatches has more info about 
this.


ERROR: space required before the open parenthesis '('
#59: FILE: arch/arm/mach-omap2/clock36xx.c:52:
+		if(ret)

ERROR: space required before the open parenthesis '('
#62: FILE: arch/arm/mach-omap2/clock36xx.c:55:
+		if(ret)

WARNING: please, no spaces at the start of a line
#67: FILE: arch/arm/mach-omap2/clock36xx.c:60:
+ badfreq :$

ERROR: space prohibited before that ':' (ctx:WxE)
#67: FILE: arch/arm/mach-omap2/clock36xx.c:60:
+ badfreq :
          ^

Updated patch follows.


- Paul

From: Jean-Philippe Francois <jp.francois@cynove.com>
Date: Thu, 30 May 2013 10:50:27 +0200
Subject: [PATCH] ARM: omap3: clock: fix wrong container_of in clock36xx.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
Acked-by: Mike Turquette <mturquette@linaro.org>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/clock36xx.c |   29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-omap2/clock36xx.c b/arch/arm/mach-omap2/clock36xx.c
index 8f3bf4e..7f69fbb 100644
--- 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;
 }
-- 
1.7.10.4

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

[Paul Walmsley]

On Mon, 3 Jun 2013, jean-philippe francois wrote:

> How should I proceed ? Should I just add the acked by below, or should
> I resend the patch ?

It's been fixed up locally here.  Mike, please speak up if I shouldn't 
apply your ack.


- Paul

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

[Paul Walmsley]

On Mon, 3 Jun 2013, jean-philippe francois wrote:

> How should I proceed ? Should I just add the acked by below, or should
> I resend the patch ?

It's been fixed up locally here.  Mike, please speak up if I shouldn't 
apply your ack.


- Paul

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

[Paul Walmsley]

Hi

On Thu, 30 May 2013, Jean-Philippe Francois wrote:

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

Tested this patch before applying, and noticed that it causes the 
retention dynamic idle power management test to fail here on the 
3730beaglexm:

http://www.pwsan.com/omap/testlogs/fixes_b_v3.10-rc/20130605113443/pm/3730beaglexm/3730beaglexm_log.txt

Not sure at this point if this is caused by the patch, or if the patch is 
just unmasking another bug.

Jean, Mike, Rajendra, care to take a look at this?


- Paul

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

[Paul Walmsley]

Hi

On Thu, 30 May 2013, Jean-Philippe Francois wrote:

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

Tested this patch before applying, and noticed that it causes the 
retention dynamic idle power management test to fail here on the 
3730beaglexm:

http://www.pwsan.com/omap/testlogs/fixes_b_v3.10-rc/20130605113443/pm/3730beaglexm/3730beaglexm_log.txt

Not sure at this point if this is caused by the patch, or if the patch is 
just unmasking another bug.

Jean, Mike, Rajendra, care to take a look at this?


- Paul

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

[jean-philippe francois]

2013/6/5 Paul Walmsley <paul@pwsan.com>:
> Hi
>
> On Thu, 30 May 2013, Jean-Philippe Francois wrote:
>
>> 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>
>
> Tested this patch before applying, and noticed that it causes the
> retention dynamic idle power management test to fail here on the
> 3730beaglexm:
>
> http://www.pwsan.com/omap/testlogs/fixes_b_v3.10-rc/20130605113443/pm/3730beaglexm/3730beaglexm_log.txt
>
> Not sure at this point if this is caused by the patch, or if the patch is
> just unmasking another bug.
>
> Jean, Mike, Rajendra, care to take a look at this?
>

I am booting with nohlt on my board, because I otherwise have memory
corruption error.
I never looked into it because power consumption is currently a non
issue for us, and the board
will always be working anyway.

However I have a beaglexm too, so I can probably help with testing.

Does the first version [1] of the patch, that only touch the MSB of
the divider also trigger the
bug ?

[1]  https://patchwork.kernel.org/patch/2609681/

Jean-Philippe
>
> - Paul
> --
> 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

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

[jean-philippe francois]

2013/6/5 Paul Walmsley <paul@pwsan.com>:
> Hi
>
> On Thu, 30 May 2013, Jean-Philippe Francois wrote:
>
>> 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>
>
> Tested this patch before applying, and noticed that it causes the
> retention dynamic idle power management test to fail here on the
> 3730beaglexm:
>
> http://www.pwsan.com/omap/testlogs/fixes_b_v3.10-rc/20130605113443/pm/3730beaglexm/3730beaglexm_log.txt
>
> Not sure at this point if this is caused by the patch, or if the patch is
> just unmasking another bug.
>
> Jean, Mike, Rajendra, care to take a look at this?
>

I am booting with nohlt on my board, because I otherwise have memory
corruption error.
I never looked into it because power consumption is currently a non
issue for us, and the board
will always be working anyway.

However I have a beaglexm too, so I can probably help with testing.

Does the first version [1] of the patch, that only touch the MSB of
the divider also trigger the
bug ?

[1]  https://patchwork.kernel.org/patch/2609681/

Jean-Philippe
>
> - Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Paul Walmsley]

On Thu, 6 Jun 2013, jean-philippe francois wrote:

> Does the first version [1] of the patch, that only touch the MSB of
> the divider also trigger the
> bug ?
> 
> [1]  https://patchwork.kernel.org/patch/2609681/

That one passes the PM test here.  Will take this one for v3.10-rc fixes 
instead, since it fixes a known DSS problem and the original code wasn't 
correct.

Jean, could you work with Mike to come up with a better approach for 
v3.11 or v3.12?


- Paul

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

[Paul Walmsley]

On Thu, 6 Jun 2013, jean-philippe francois wrote:

> Does the first version [1] of the patch, that only touch the MSB of
> the divider also trigger the
> bug ?
> 
> [1]  https://patchwork.kernel.org/patch/2609681/

That one passes the PM test here.  Will take this one for v3.10-rc fixes 
instead, since it fixes a known DSS problem and the original code wasn't 
correct.

Jean, could you work with Mike to come up with a better approach for 
v3.11 or v3.12?


- Paul

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

[jean-philippe francois]

2013/6/6 Paul Walmsley <paul@pwsan.com>:
> On Thu, 6 Jun 2013, jean-philippe francois wrote:
>
>> Does the first version [1] of the patch, that only touch the MSB of
>> the divider also trigger the
>> bug ?
>>
>> [1]  https://patchwork.kernel.org/patch/2609681/
>
> That one passes the PM test here.  Will take this one for v3.10-rc fixes
> instead, since it fixes a known DSS problem and the original code wasn't
> correct.
>
> Jean, could you work with Mike to come up with a better approach for
> v3.11 or v3.12?
>
Hi,

I am ok to work on it, however could you explain to me what is the
expected output of your PM tests, and how to reproduce them ?
The board I used to test the clock frequency was correct suffered from
heavy handed oscilloscope probing and is currently out of order :(

Jean-Philippe François.

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

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

[jean-philippe francois]

2013/6/6 Paul Walmsley <paul@pwsan.com>:
> On Thu, 6 Jun 2013, jean-philippe francois wrote:
>
>> Does the first version [1] of the patch, that only touch the MSB of
>> the divider also trigger the
>> bug ?
>>
>> [1]  https://patchwork.kernel.org/patch/2609681/
>
> That one passes the PM test here.  Will take this one for v3.10-rc fixes
> instead, since it fixes a known DSS problem and the original code wasn't
> correct.
>
> Jean, could you work with Mike to come up with a better approach for
> v3.11 or v3.12?
>
Hi,

I am ok to work on it, however could you explain to me what is the
expected output of your PM tests, and how to reproduce them ?
The board I used to test the clock frequency was correct suffered from
heavy handed oscilloscope probing and is currently out of order :(

Jean-Philippe Fran?ois.

>
> - Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Paul Walmsley]

Hello Jean-Philippe,

On Mon, 10 Jun 2013, jean-philippe francois wrote:

> I am ok to work on it, however could you explain to me what is the
> expected output of your PM tests, and how to reproduce them ?

Here is the expected output for my ancient 3730 ES1.0 Beagle-XM:

http://www.pwsan.com/omap/testlogs/test_v3.10-rc5/20130609031534/pm/3730beaglexm/3730beaglexm_log.txt

When I ran that same test on the second version of your patch, the system 
froze during the retention dynamic idle test:

----------------

%% Start retention dynamic idle UART wakeup test

echo 3000 > /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
root@beagleboard:~# 
root@beagleboard:~# echo 3000 > 
/sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
root@beagleboard:~# 
root@beagleboard:~# echo 3000 > 
/sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
root@beagleboard:~# 
root@beagleboard:~# echo 3000 > 
/sys/devices/platform/omap_uart.3/power/autosuspend_delay_ms
root@beagleboard:~# 
root@beagleboard:~# 

-------------------

That is where the log stopped.

Sorry to hear about your board - I can certainly sympathize,

regards,

- Paul

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

[Paul Walmsley]

Hello Jean-Philippe,

On Mon, 10 Jun 2013, jean-philippe francois wrote:

> I am ok to work on it, however could you explain to me what is the
> expected output of your PM tests, and how to reproduce them ?

Here is the expected output for my ancient 3730 ES1.0 Beagle-XM:

http://www.pwsan.com/omap/testlogs/test_v3.10-rc5/20130609031534/pm/3730beaglexm/3730beaglexm_log.txt

When I ran that same test on the second version of your patch, the system 
froze during the retention dynamic idle test:

----------------

%% Start retention dynamic idle UART wakeup test

echo 3000 > /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
root at beagleboard:~# 
root at beagleboard:~# echo 3000 > 
/sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
root at beagleboard:~# 
root at beagleboard:~# echo 3000 > 
/sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
root at beagleboard:~# 
root at beagleboard:~# echo 3000 > 
/sys/devices/platform/omap_uart.3/power/autosuspend_delay_ms
root at beagleboard:~# 
root at beagleboard:~# 

-------------------

That is where the log stopped.

Sorry to hear about your board - I can certainly sympathize,

regards,

- Paul