3.9 regression : CAM_XCLKA wrong frequency setting.

3.9 regression : CAM_XCLKA wrong frequency setting.

[jean-philippe francois]

Hi,

Laurent, you are on the recipient list because the symptom is visible
throug your driver. I found the other name looking in the clock
related files. Please apologize if you are not concerned by this mail.

Symptom :
External clock CAM_XCLKA frequency is wrong with 3.9, it was ok until
3.6. Everything else is working fine, except the fps are higher, and
the image quality sucks because the input clock is out of spec.
Oscilloscope measured freq is around 30 MHz instead of the 24 MHz the
sensor is asking for.

Hardware :
dm3730, with 19.2 MHz crystal, running at OPP100

The register frequency settings seems to be ok, and they are the same
as in 3.6.11 :
CM_CLKSEL2_PLL : 0x04816807
        19, 2 * 360 / 8 = 864 MHz
         this value is already set when I look at it in u-boot

CM_CLKSEL_CAM : 0x5
        864 / 5 = 172,8 MHz
        this value is set through the clock framework by the omap3isp
driver code

ISP.TCTRL_CTRL : 0x7
        172,8 / 7 = 24,685... MHz
         this value is set by the omap3isp driver code

However, as stated before, the actual output frequency is NOT 24 MHz
but around 30.

If dpll4 frequency is wrong, what other clock would be affected ?
What can I do to debug this ?
Laurent, I believe you have a camera add-on board for the beagle xm,
could you test and report wether you can reproduce this bug ?

Attached is the boot log with 3.9, with some debugging messages activated.

Thank you for your help,
Regards,

Jean-Philippe Fran?ois
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 3.9dmesg
Type: application/octet-stream
Size: 28444 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130517/4fd9064f/attachment-0001.obj>

3.9 regression : CAM_XCLKA wrong frequency setting.

[jean-philippe francois]

2013/5/17 jean-philippe francois <jp.francois@cynove.com>:
> Hi,
>
> Laurent, you are on the recipient list because the symptom is visible
> throug your driver. I found the other name looking in the clock
> related files. Please apologize if you are not concerned by this mail.
>
> Symptom :
> External clock CAM_XCLKA frequency is wrong with 3.9, it was ok until
> 3.6. Everything else is working fine, except the fps are higher, and
> the image quality sucks because the input clock is out of spec.
> Oscilloscope measured freq is around 30 MHz instead of the 24 MHz the
> sensor is asking for.
>
> Hardware :
> dm3730, with 19.2 MHz crystal, running at OPP100
>
> The register frequency settings seems to be ok, and they are the same
> as in 3.6.11 :
> CM_CLKSEL2_PLL : 0x04816807
>         19, 2 * 360 / 8 = 864 MHz
>          this value is already set when I look at it in u-boot
>
> CM_CLKSEL_CAM : 0x5
>         864 / 5 = 172,8 MHz
>         this value is set through the clock framework by the omap3isp
> driver code
>
> ISP.TCTRL_CTRL : 0x7
>         172,8 / 7 = 24,685... MHz
>          this value is set by the omap3isp driver code
>
> However, as stated before, the actual output frequency is NOT 24 MHz
> but around 30.
>
> If dpll4 frequency is wrong, what other clock would be affected ?

As usual, writing a mail brings up new idea. Sorry to use the lists
as rubber duck debug partner, but I still need your help.

Obviously, dpll4 frequency is correct, because it sources the 96 and
48 MHz clock.
I can check it measuring bit time on uart3tx : 8.6usec for 115200 => Ok

30 MHz is 1.25 * 24 MHz
CM_CLKSEL_CAM reset value is 4, programmed value is 5,
So I did the following, while looking at CAM_XCLKA on scope :

# devmem 0x48004f40
0x00000005
# devmem 0x48004f40 32 5  -> freq = 30 MHz
# devmem 0x48004f40 32 4  -> freq = 30 MHz
# devmem 0x48004f40 32 5  -> freq = 24 MHz

So there is something nasty in the setup sequence of cam_mclk and dpll4_m5ckx2
that prevents the new divisor value to be used by the hardware.
I will dive into the code to see where things went wrong, but I am a
bit lost here.

> What can I do to debug this ?
> Laurent, I believe you have a camera add-on board for the beagle xm,
> could you test and report wether you can reproduce this bug ?
>
> Attached is the boot log with 3.9, with some debugging messages activated.
>
> Thank you for your help,
> Regards,
>
> Jean-Philippe Fran?ois

[PATCH] 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.
Fix the function to use clk_divider. 
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
@@ -20,11 +20,12 @@
 
 #include <linux/kernel.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/io.h>
 
 #include "clock.h"
 #include "clock36xx.h"
-
+#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
 
 /**
  * omap36xx_pwrdn_clk_enable_with_hsdiv_restore - enable clocks suffering
@@ -39,29 +40,28 @@
  */
 int omap36xx_pwrdn_clk_enable_with_hsdiv_restore(struct clk_hw *clk)
 {
-	struct clk_hw_omap *parent;
+	struct clk_divider *parent;
 	struct clk_hw *parent_hw;
-	u32 dummy_v, orig_v, clksel_shift;
+	u32 dummy_v, orig_v;
 	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);
+	parent = to_clk_divider(parent_hw);
 
 	/* Restore the dividers */
 	if (!ret) {
-		clksel_shift = __ffs(parent->clksel_mask);
-		orig_v = __raw_readl(parent->clksel_reg);
+		orig_v = __raw_readl(parent->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);
+		dummy_v ^= (1 << parent->shift);
+		__raw_writel(dummy_v, parent->reg);
 
 		/* Write the original divider */
-		__raw_writel(orig_v, parent->clksel_reg);
+		__raw_writel(orig_v, parent->reg);
 	}
 
 	return ret;

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

[Mike Turquette]

Quoting Jean-Philippe Francois (2013-05-17 08:51:26)
> 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.
> Fix the function to use clk_divider. 
> 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
> @@ -20,11 +20,12 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/io.h>
>  
>  #include "clock.h"
>  #include "clock36xx.h"
> -
> +#define to_clk_divider(_hw) container_of(_hw, struct clk_divider, hw)
>  
>  /**
>   * omap36xx_pwrdn_clk_enable_with_hsdiv_restore - enable clocks suffering
> @@ -39,29 +40,28 @@
>   */
>  int omap36xx_pwrdn_clk_enable_with_hsdiv_restore(struct clk_hw *clk)
>  {
> -       struct clk_hw_omap *parent;
> +       struct clk_divider *parent;
>         struct clk_hw *parent_hw;
> -       u32 dummy_v, orig_v, clksel_shift;
> +       u32 dummy_v, orig_v;
>         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);
> +       parent = to_clk_divider(parent_hw);

Peaking inside of clock structures was OK back in the legacy clock days,
and even if the clocks are the same type (clk_hw_omap), but having omap
code dig into clk-divider structures is pretty gross.

How about reentrantly calling clk_set_rate here to achieve the same
effect?

	/* kick parent's clksel register after toggling PWRDN bit */
	struct clk *parent = clk_get_parent(clk->clk);
	unsigned long parent_rate = clk_get_rate(parent);
	clk_set_rate(parent, parent_rate/2);
	clk_set_rate(parent, parent_rate);

Regards,
Mike

>  
>         /* Restore the dividers */
>         if (!ret) {
> -               clksel_shift = __ffs(parent->clksel_mask);
> -               orig_v = __raw_readl(parent->clksel_reg);
> +               orig_v = __raw_readl(parent->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);
> +               dummy_v ^= (1 << parent->shift);
> +               __raw_writel(dummy_v, parent->reg);
>  
>                 /* Write the original divider */
> -               __raw_writel(orig_v, parent->clksel_reg);
> +               __raw_writel(orig_v, parent->reg);
>         }
>  
>         return ret;