| Message ID | 1369903827-2025-1-git-send-email-jp.francois@cynove.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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; >> }
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
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
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
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
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
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
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; }
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>