[PATCH 09/15] ARM: mxs: Add clock support

[Shawn Guo]

2010/12/2 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> On Fri, Nov 26, 2010 at 02:49:08PM +0800, Shawn Guo wrote:
>> +
>> +/*
>> + * ref_clk
>> + */
>> +#define _CLK_GET_RATE_REF(name, sr, ss)                                      \
>> +static unsigned long name##_get_rate(struct clk *clk)                        \
>> +{                                                                    \
>> +     unsigned long parent_rate;                                      \
>> +     u32 reg, div;                                                   \
>> +                                                                     \
>> +     reg = __raw_readl(CLKCTRL_BASE_ADDR + HW_CLKCTRL_##sr);         \
>> +     div = (reg >> BP_CLKCTRL_##sr##_##ss##FRAC) & 0x3f;             \
>> +     parent_rate = clk_get_rate(clk->parent);                        \
>> +                                                                     \
>> +     return parent_rate * 18 / div;                                  \
>> +}
>> +
>> +_CLK_GET_RATE_REF(ref_cpu_clk, FRAC, CPU)
>> +_CLK_GET_RATE_REF(ref_emi_clk, FRAC, EMI)
>> +_CLK_GET_RATE_REF(ref_pix_clk, FRAC, PIX)
>> +_CLK_GET_RATE_REF(ref_io_clk, FRAC, IO)
>> +
>> +#define _DEFINE_CLOCK_REF(name, er, es)                                      \
>> +     static struct clk name = {                                      \
>> +             .enable_reg     = CLKCTRL_BASE_ADDR + HW_CLKCTRL_##er,  \
>> +             .enable_shift   = BP_CLKCTRL_##er##_CLKGATE##es,        \
>> +             .get_rate       = name##_get_rate,                      \
>> +             .enable         = _raw_clk_enable,                      \
>> +             .disable        = _raw_clk_disable,                     \
>> +             .parent         = &pll_clk,                             \
>> +     }
>> +
>> +_DEFINE_CLOCK_REF(ref_cpu_clk, FRAC, CPU);
> What happens if get_clock_rate(ref_cpu_clk) is called?:
>
>        ref_cpu_clk_get_rate
>                reg = something
>                div = something else
>                parent_rate = clk_get_rate(clk->parent)
>                        = pll_clk_get_rate()
>                        = 480000000;
>                return parent_rate * 18 / div
>                        = (480000000 * 18) / div
>                        = 0x202fbf000 / div
>                        = ...
>
> Note that 0x202fbf000 is too big for an unsigned long so (AFAIK) this is
> truncated to 0x02fbf000 / div which is wrong.
>
> The same overflow happens in at least one set_rate function, too.
>
> Either you have to switch to long long or (IMHO preferable) use shifted
> values.
>
Excellent catch.  It solved another issue that I ran into, which
caused by imprecise mdelay.

Thanks a million, Uwe.

-- 
Regards,
Shawn