> -----Original Message----- > From: Paul Walmsley [mailto:paul@xxxxxxxxx] > Sent: Tuesday, January 19, 2010 11:39 PM > To: Turquette, Mike > Cc: Kauppi Ari (EXT-Ixonos/Oulu); G.N, Vijayakumar; linux-omap@xxxxxxxxxxxxxxx; > khilman@xxxxxxxxxxxxxxxxxxx; Sripathy, Vishwanath > Subject: Re: [PATCHV2 2/2] OMAP3630: Clock: Fixing HSDivider Limitation > > Hi, > > On Tue, 19 Jan 2010, Mike Turquette wrote: > > > Kauppi Ari (EXT-Ixonos/Oulu) wrote: > > > On Mon, 2010-01-18 at 19:04 +0100, ext Paul Walmsley wrote: > > (some missing attribution here) > > > > > > +int omap3_pwrdn_clk_enable_with_hsdiv_restore(struct clk *clk) > > > > > +{ > > > > > + u32 v; > > > > > + int ret; > > > > > + > > > > > + /* enable the clock */ > > > > > + ret = omap2_dflt_clk_enable(clk); > > > > > + > > > > > + /* Restore the dividers */ > > > > > + if (!ret) { > > > > > + v = __raw_readl(clk->parent->clksel_reg); > > > > > + v += (1 << clk->parent->clksel_shift); > > > > Using += here could affect many bits in the register if the add carries. > > > > This doesn't seem like what you want. > > > > > > > > Also, isn't there a risk of side-effects here, that if this bit was not > > > > already set, that everything further down the clock tree from this could > > > > be affected? Wouldn't it be best to just rewrite the correct clksel > > > > value? > > > > > > The assumption is that the divider is correct in the register so I guess > > > the clock tree should be fine? The proper clksel/divider has been > > > earlier set but for some reason HW loses the divider if PWRDN is > > > toggled. > > > > In my testing that assumption was always correct. In fact that is what made > > this bug very difficult to track down: the register values were always the > > correct value programmed initially but the actual clocks were wrong. > > > > > Our experiments with this issue show that just read/write of same value > > > is not enough to refresh the HW. The clksel has to be written with a > > > different divider first and then write the original divider back. > > > > ACK to the above comment. Registers need to be changed to a different value > > and back. > > Please put that into the comments on the function. Right now it says "Any > write to the corresponding CM_CLKSEL register will refresh the dividers." > > Also, in that case the v += / v -= seems more appropriate. The concern > then is, what happens if the divider is already at its maximum value? > Won't it wrap around, which could potentially result in excessively high > clocks further down the tree? In this case it might be best to step down > to the next lowest divider, rather than wrapping. Maybe something like > this? (untested) > > u32 orig_v, v, c, clksel_shift, max_div; > > [...] > > v = __raw_readl(clk->parent->clksel_reg); > orig_v = v; > > clksel_shift = __ffs(clk->parent->clksel_mask); > > max_div = clk->parent->clksel_mask >> clksel_shift; > > /* Isolate the current divider */ > c = v & clk->parent->clksel_mask; > c >>= clksel_shift; > > /* Prevent excessively high clock rates if divider would wrap */ > c += (c == max_div) ? -1 : 1; > > /* Write the temporarily altered divider back */ > c <<= clksel_shift; > v &= ~c; > v |= c; > __raw_writel(v, clk->parent->clksel_reg); > > /* Write the original divider */ > __raw_writel(orig_v, clk->parent->clksel_reg); > > This seems to better than which currently I have; I will incorporate and test it for the same. I will add few more comments to have a better clarity in the same function. Thanks Vijay > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
