From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benoit Cousson Date: Tue, 25 Feb 2014 17:07:54 +0000 Subject: Re: [PATCH] clk: shmobile: rcar-gen2: Use kick bit to allow Z clock frequency change Message-Id: <530CCDEA.40203@baylibre.com> List-Id: References: <530C9D2A.8030409@baylibre.com> In-Reply-To: <530C9D2A.8030409@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: linux-sh@vger.kernel.org Hi Laurent, On 25/02/2014 17:01, Laurent Pinchart wrote: > Hi Benoit, > > Thank you for the patch. You're welcome. Thanks for your comments. > On Tuesday 25 February 2014 14:39:54 Benoit Cousson wrote: >> The Z clock frequency change is effective only after setting the kick >> bit located in the FRQCRB register. >> Without that, the CA15 CPUs clock rate will never change. >> >> Fix that by checking if the kick bit is cleared and enable it to make >> the clock rate change effective. The bit is cleared automatically upon >> completion. >> >> Note: The kick bit is used as well to change 3 other emulation clocks: >> Debug Trace port clock (ZTR), Debug Trace bus clock (ZT), and Debug >> clock (ZTRD2). It is not clear from the spec [1] if there >> is any relation between the CPU clock rate and these emulation clocks. >> Moreover, these clocks are probably supposed to be controled by an >> external debugger / tracer like Lauterbach PowerTrace. >> For all these reasons, the current implementation does not expose these >> clock nodes and thus does not have to consider the multiple accesses >> to the kick bit. >> >> Signed-off-by: Benoit Cousson >> Cc: Mike Turquette >> Cc: Laurent Pinchart >> >> [1] R-Car-H2-v0.6-en.pdf - page 186 >> --- >> >> Salut Laurent, >> >> If you have any information about these emulation clocks, please let me >> know. > > I don't I'm afraid. All I know is that we don't use them for now, so I > wouldn't care :-) > >> Moreover, the CCF clock driver is shared with the r8a7791, so a test on >> that platform will be good. > > I can do that. I assume you're working on Lager then ? Yes I am. I don't have any Koelsch board. > How can I exercise the code ? Using cpufreq to change the CA15 clock freq= uency > ? A brief test procedure would be appreciated. Sure. First you can use dhrystone to check that the CPU frequency is=20 changing or not. So far, changing the clock rate will not affect the dhrystone value at=20 all becasue the magic kick bit is not set. debugfs does not seems to allow write access to the clk_rate, so using=20 cpufreq is propably the best way to test it. I have a small series to add DVFS for Lager (Multiplatform boot only), I=20 can share you that code if you want. Naive question: How did you test that z-clock code originally ? >> drivers/clk/shmobile/clk-rcar-gen2.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) > > By the way the patch was corrupted with line wraps and additional spaces. Gosh! I edited it slightly by hand with thunderbird before submitting...=20 I probably messed it up. Sorry :-( > >> diff --git a/drivers/clk/shmobile/clk-rcar-gen2.c >> b/drivers/clk/shmobile/clk-rcar-gen2.c >> index a59ec21..9f12746 100644 >> --- a/drivers/clk/shmobile/clk-rcar-gen2.c >> +++ b/drivers/clk/shmobile/clk-rcar-gen2.c >> @@ -26,6 +26,8 @@ struct rcar_gen2_cpg { >> void __iomem *reg; >> }; >> >> +#define CPG_FRQCRB 0x00000004 >> +#define CPG_FRQCRB_KICK BIT(31) >> #define CPG_SDCKCR 0x00000074 >> #define CPG_PLL0CR 0x000000d8 >> #define CPG_FRQCRC 0x000000e0 >> @@ -45,6 +47,7 @@ struct rcar_gen2_cpg { >> struct cpg_z_clk { >> struct clk_hw hw; >> void __iomem *reg; >> + void __iomem *kick_reg; >> }; >> >> #define to_z_clk(_hw) container_of(_hw, struct cpg_z_clk, hw) >> @@ -83,17 +86,35 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, >> unsigned long rate, >> { >> struct cpg_z_clk *zclk =3D to_z_clk(hw); >> unsigned int mult; >> - u32 val; >> + u32 val, kick; >> + int i; > > The loop counter will always be positive, please use an unsigned int. OK. >> mult =3D div_u64((u64)rate * 32, parent_rate); >> mult =3D clamp(mult, 1U, 32U); >> >> + if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK) >> + return -EBUSY; >> + >> val =3D clk_readl(zclk->reg); >> val &=3D ~CPG_FRQCRC_ZFC_MASK; >> val |=3D (32 - mult) << CPG_FRQCRC_ZFC_SHIFT; >> clk_writel(val, zclk->reg); >> >> - return 0;ir: cannot create directory `/home/ubuntu/projects/ti-glsdk_o= map5-uevm_6_03_00_01/targetfs': Permission denied >> + /* >> + * Set KICK bit in FRQCRB to update hardware setting and >> + * wait for completion. >> + */ >> + kick =3D clk_readl(zclk->kick_reg); >> + kick |=3D CPG_FRQCRB_KICK; >> + clk_writel(kick, zclk->kick_reg); > > Does CCF guarantee that two set_rate calls for different clocks will not = occur > concurrently ? If so a brief comment would be nice. Otherwise we need a l= ock > around this. So far, this is the only user of the register. That's why there is no=20 lock. It is explained in the changelog. But if you want I can re-use the=20 same comment here. > >> + >> + for (i =3D 1000; i; i--) >> + if (clk_readl(zclk->kick_reg) & CPG_FRQCRB_KICK) >> + cpu_relax(); >> + else >> + return 0; > > Please put braces around the for() content. OK, no problem, if you prefer that. > How many iterations does this need in practice ? I don't have any clue :-) I don't like these kind of magic numbers as well, but without further=20 information, it is hard to know. Based on TI experience, there is no way to get any accurate information=20 about this kind of transition time even from HW enginners. Most of the=20 time the best you can get is a tens of micro-sec or a hundred of clock=20 cycles. FWIW, this "value" is based on what was already done and merged for the=20 zlock inside the clock-r8a73a4.c code (non-CCF). > I also wonder if we really > need to wait or if we could just return and assume the clock frequency wi= ll > change soon enough. We do need to wait, that the recommended procedure from the spec, and=20 this is the only way to ensure the frequency is properly changed. In the case of DVFS sequence, you do not want to start reducing the=20 voltage before you are 100% sure the frequency was reduced before. So=20 that's the only safe way to do that. Regards, Benoit --=20 Beno=EEt Cousson BayLibre Embedded Linux Technology Lab www.baylibre.com