> -----Original Message-----
> From: Eduardo Valentin [mailto:[email protected]]
> Sent: Tuesday, January 05, 2010 1:06 PM
> To: Sripathy, Vishwanath
> Cc: [email protected]; 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.
> > 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 <[email protected]>
> >
> > Signed-off-by: Richard Woodruff <[email protected]>
> > Signed-off-by: Nishanth Menon <[email protected]>
> > Signed-off-by: Vishwanath BS <[email protected]>
> > ---
> > 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
>
> > 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
>
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
