Hello Vishwanath, On Tue, Jan 05, 2010 at 09:16:22AM +0100, ext Sripathy, Vishwanath wrote: > > > > -----Original Message----- > > From: Eduardo Valentin [mailto:eduardo.valentin@xxxxxxxxx] > > Sent: Tuesday, January 05, 2010 1:06 PM > > To: Sripathy, Vishwanath > > Cc: linux-omap@xxxxxxxxxxxxxxx; Paul Walmsley; Woodruff, Richard; Menon, Nishanth > > Subject: Re: [PATCHV4 1/3] OMAP3: introduce DPLL4 Jtype > > > > Hello, > > > > Few little comments bellow. > > > > On Tue, Jan 05, 2010 at 07:36:08AM +0100, ext Vishwanath BS wrote: > > > DPLL4 for 3630 introduces a changed block requiring special divisor > > > bits and additional reg fields. To allow for silicons to use this, this > > > is introduced as a omap3_has_jtype_dpll4() and is enabled for 3630 > > ^ > > I don't see anywhere in this patch a reference to this function/macro > > > Yes, omap3_has_jtype_dpll4 is no longer used. This description is borrowed from previous version of the patch. I will correct it. OK. Nice. > > > silicon. Also FREQSEL field is no longer valid for OMAP3630. So reference > > > to this is removed for 3630. > > > > > > Tested with 3630 ZOOM3 and OMAP3430 ZOOM2 > > > > > > Cc: Paul Walmsley <paul@xxxxxxxxx> > > > > > > Signed-off-by: Richard Woodruff <r-woodruff2@xxxxxx> > > > Signed-off-by: Nishanth Menon <nm@xxxxxx> > > > Signed-off-by: Vishwanath BS <Vishwanath.bs@xxxxxx> > > > --- > > > arch/arm/mach-omap2/clock.h | 3 ++ > > > arch/arm/mach-omap2/clock34xx_data.c | 2 + > > > arch/arm/mach-omap2/clock44xx_data.c | 1 + > > > arch/arm/mach-omap2/cm-regbits-34xx.h | 6 +++- > > > arch/arm/mach-omap2/dpll.c | 53 > > ++++++++++++++++++++++++++++-- > > > arch/arm/plat-omap/include/plat/clock.h | 1 + > > > 6 files changed, 61 insertions(+), 5 deletions(-) > > > > > > diff --git a/arch/arm/mach-omap2/clock.h b/arch/arm/mach-omap2/clock.h > > > index 93c48df..14f73e7 100644 > > > --- a/arch/arm/mach-omap2/clock.h > > > +++ b/arch/arm/mach-omap2/clock.h > > > @@ -47,6 +47,9 @@ > > > #define DPLL_LOW_POWER_BYPASS 0x5 > > > #define DPLL_LOCKED 0x7 > > > > > > +/*DPLL Type and DCO Selection Flags*/ > > > +#define DPLL_J_TYPE 0x1 > > > +#define DPLL_NO_DCO_SEL 0x2 > > > > Add blank line here, just to make it cleaner. > > > > > int omap2_clk_init(void); > > > int omap2_clk_enable(struct clk *clk); > > > void omap2_clk_disable(struct clk *clk); > > > diff --git a/arch/arm/mach-omap2/clock34xx_data.c b/arch/arm/mach- > > omap2/clock34xx_data.c > > > index 043caed..9aac354 100644 > > > --- a/arch/arm/mach-omap2/clock34xx_data.c > > > +++ b/arch/arm/mach-omap2/clock34xx_data.c > > > @@ -3241,6 +3241,8 @@ int __init omap2_clk_init(void) > > > cpu_clkflg |= CK_3430ES2; > > > } > > > } > > > + if (cpu_is_omap3630()) > > > + dpll4_ck.dpll_data->flags |= DPLL_J_TYPE; > > > > > > clk_init(&omap2_clk_functions); > > > > > > diff --git a/arch/arm/mach-omap2/clock44xx_data.c b/arch/arm/mach- > > omap2/clock44xx_data.c > > > index 2210e22..7347246 100644 > > > --- a/arch/arm/mach-omap2/clock44xx_data.c > > > +++ b/arch/arm/mach-omap2/clock44xx_data.c > > > @@ -2736,6 +2736,7 @@ int __init omap2_clk_init(void) > > > if (cpu_is_omap44xx()) { > > > cpu_mask = RATE_IN_4430; > > > cpu_clkflg = CK_443X; > > > + dpll_usb_ck.dpll_data->flags |= DPLL_NO_DCO_SEL | DPLL_J_TYPE; > > > } > > > > > > clk_init(&omap2_clk_functions); > > > diff --git a/arch/arm/mach-omap2/cm-regbits-34xx.h b/arch/arm/mach-omap2/cm- > > regbits-34xx.h > > > index 6923deb..6f2802b 100644 > > > --- a/arch/arm/mach-omap2/cm-regbits-34xx.h > > > +++ b/arch/arm/mach-omap2/cm-regbits-34xx.h > > > @@ -516,9 +516,13 @@ > > > > > > /* CM_CLKSEL2_PLL */ > > > #define OMAP3430_PERIPH_DPLL_MULT_SHIFT 8 > > > -#define OMAP3430_PERIPH_DPLL_MULT_MASK (0x7ff << 8) > > > +#define OMAP3430_PERIPH_DPLL_MULT_MASK (0xfff << 8) > > > #define OMAP3430_PERIPH_DPLL_DIV_SHIFT 0 > > > #define OMAP3430_PERIPH_DPLL_DIV_MASK (0x7f << 0) > > > +#define OMAP3630_PERIPH_DPLL_DCO_SEL_SHIFT 21 > > > +#define OMAP3630_PERIPH_DPLL_DCO_SEL_MASK (0x7 << 21) > > > +#define OMAP3630_PERIPH_DPLL_SD_DIV_SHIFT 24 > > > +#define OMAP3630_PERIPH_DPLL_SD_DIV_MASK (0xff << 24) > > > > > > /* CM_CLKSEL3_PLL */ > > > #define OMAP3430_DIV_96M_SHIFT 0 > > > diff --git a/arch/arm/mach-omap2/dpll.c b/arch/arm/mach-omap2/dpll.c > > > index f6055b4..d52ab37 100644 > > > --- a/arch/arm/mach-omap2/dpll.c > > > +++ b/arch/arm/mach-omap2/dpll.c > > > @@ -238,6 +238,42 @@ static int _omap3_noncore_dpll_stop(struct clk *clk) > > > } > > > > > > /** > > > + * lookup_dco_sddiv - Set j-type DPLL4 compensation variables > > > + * @clk: pointer to a DPLL struct clk > > > + * @dco: digital control oscillator selector > > > + * @sd_div: target sigma-delta divider > > > + * @m: DPLL multiplier to set > > > + * @n: DPLL divider to set > > > + */ > > > +static void lookup_dco_sddiv(struct clk *clk, u8 *dco, u8 *sd_div, u16 > > > + m, u8 n) > > > + { > > > + unsigned long fint, clkinp, sd; /* watch out for overflow */ > > > + int mod1, mod2; > > > + > > > + clkinp = clk->parent->rate; > > > + fint = (clkinp / n) * m; > > > + > > > + if (fint < 1000000000) > > > + *dco = 2; > > > + else > > > + *dco = 4; > > > + /* > > > + * target sigma-delta to near 250MHz > > > + * sd = ceil[(m/(n+1)) * (clkinp_MHz / 250)] > > > + */ > > > + clkinp /= 100000; /* shift from MHz to 10*Hz for 38.4 and 19.2*/ > > > + mod1 = (clkinp * m) % (250 * n); > > > + sd = (clkinp * m) / (250 * n); > > > + mod2 = sd % 10; > > > + sd /= 10; > > > + > > > + if (mod1 + mod2) > > > + sd++; > > > > I think would make sense to change the above condition to a real boolean expression. > > I know this is not required, but increases code readability. > > > OK > > > + *sd_div = sd; > > > +} > > > + > > > +/** > > > * omap3_noncore_dpll_enable - instruct a DPLL to enter bypass or lock mode > > > * @clk: pointer to a DPLL struct clk > > > * > > > @@ -311,7 +347,7 @@ int omap3_noncore_dpll_program(struct clk *clk, u16 m, > > u8 n, u16 freqsel) > > > _omap3_noncore_dpll_bypass(clk); > > > > > > /* Set jitter correction */ > > > - if (!cpu_is_omap44xx()) { > > > + if (!cpu_is_omap44xx() && !cpu_is_omap3630()) { > > > > Is the above really related to J_TYPE of DPLL4 (as patch subject)? > No, this is specific to OMAP3630 and OMAP4. Basically FREQSEL is no longer available in 3630 and OMAP4 > Right. Then I'd suggest to split the change some how. Or improve the patch description. But would be nicer if you do not put in same patch changes which do not have much relation. > > > > > v = __raw_readl(dd->control_reg); > > > v &= ~dd->freqsel_mask; > > > v |= freqsel << __ffs(dd->freqsel_mask); > > > @@ -323,6 +359,15 @@ int omap3_noncore_dpll_program(struct clk *clk, u16 m, > > u8 n, u16 freqsel) > > > v &= ~(dd->mult_mask | dd->div1_mask); > > > v |= m << __ffs(dd->mult_mask); > > > v |= (n - 1) << __ffs(dd->div1_mask); > > > + > > > + if ((dd->flags & DPLL_J_TYPE) && !(dd->flags & DPLL_NO_DCO_SEL)) { > > > + u8 dco, sd_div; > > > + lookup_dco_sddiv(clk, &dco, &sd_div, m, n); > > > + v &= ~(OMAP3630_PERIPH_DPLL_DCO_SEL_MASK > > > + | OMAP3630_PERIPH_DPLL_SD_DIV_MASK); > > > + v |= dco << __ffs(OMAP3630_PERIPH_DPLL_DCO_SEL_MASK); > > > + v |= sd_div << __ffs(OMAP3630_PERIPH_DPLL_SD_DIV_MASK); > > > + } > > > __raw_writel(v, dd->mult_div1_reg); > > > > > > /* We let the clock framework set the other output dividers later */ > > > @@ -384,8 +429,8 @@ int omap3_noncore_dpll_set_rate(struct clk *clk, unsigned > > long rate) > > > if (dd->last_rounded_rate == 0) > > > return -EINVAL; > > > > > > - /* No freqsel on OMAP4 */ > > > - if (!cpu_is_omap44xx()) { > > > + /* No freqsel on OMAP4 and OMAP3630 */ > > > + if (!cpu_is_omap44xx() && !cpu_is_omap3630()) { > > > > dito > > > > > freqsel = _omap3_dpll_compute_freqsel(clk, > > > dd->last_rounded_n); > > > if (!freqsel) > > > @@ -530,7 +575,7 @@ unsigned long omap3_clkoutx2_recalc(struct clk *clk) > > > > > > v = __raw_readl(dd->control_reg) & dd->enable_mask; > > > v >>= __ffs(dd->enable_mask); > > > - if (v != OMAP3XXX_EN_DPLL_LOCKED) > > > + if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > > > rate = clk->parent->rate; > > > else > > > rate = clk->parent->rate * 2; > > > diff --git a/arch/arm/plat-omap/include/plat/clock.h b/arch/arm/plat- > > omap/include/plat/clock.h > > > index 309b6d1..15d0d9a 100644 > > > --- a/arch/arm/plat-omap/include/plat/clock.h > > > +++ b/arch/arm/plat-omap/include/plat/clock.h > > > @@ -62,6 +62,7 @@ struct dpll_data { > > > void __iomem *idlest_reg; > > > u32 autoidle_mask; > > > u32 freqsel_mask; > > > + u8 flags; > > > > > > It would be nice to add some comment for this new field, as it is generically named > > "flags". > ok Nice. > > > > > u32 idlest_mask; > > > u8 auto_recal_bit; > > > u8 recal_en_bit; > > > -- > > > 1.5.6.3 > > > > > > -- > > > 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 > > > > -- > > Eduardo Valentin -- Eduardo Valentin -- 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
