From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrii Tseglytskyi Date: Mon, 20 May 2013 14:06:38 +0300 Subject: [U-Boot] [PATCH v02 1/2] OMAP3+: introduce generic ABB support In-Reply-To: <20130517131101.GB4892@kahuna> References: <1368463777-31980-1-git-send-email-andrii.tseglytskyi@ti.com> <1368463777-31980-2-git-send-email-andrii.tseglytskyi@ti.com> <20130517131101.GB4892@kahuna> Message-ID: <519A03BE.60902@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi, Thank you for review. On 05/17/2013 04:11 PM, Nishanth Menon wrote: [snip] > On 19:49-20130513, Andrii Tseglytskyi wrote: > [...] >> + if (fuse && ldovbb) { >> + if (abb_setup_ldovbb(fuse, ldovbb)) >> + return; >> + } >> + >> + /* configure timings, based on oscillator value */ >> + abb_setup_timings(setup); > Still missing txdone clear.. > If txdone was set on entry, you'd escape a bit waiting for transition > completion bit in SR2, However, by clearing TXDONE here, you can just wait > for TXDONE. We touch ABB first time here. I can add check/clear txdone here to double check that everything is fine, but this may be superfluous. >> + >> + /* select ABB type */ >> + clrsetbits_le32(setup, >> + abb_type_mask | OMAP_ABB_SETUP_SR2EN_MASK, >> + abb_type_mask | OMAP_ABB_SETUP_SR2EN_MASK); > This is no better than set_bits! clearbits (addr, clr, set) -> the clr > bits should clear *all* bits of the field. in this example ABB_TYPE > should both be cleared, same in OPP_SEL. > See the following change on top of this series: Yep. should be: clrsetbits_le32(setup, OMAP_ABB_SETUP_ACTIVE_FBB_SEL_MASK |OMAP_ABB_SETUP_ACTIVE_RBB_SEL_MASK | OMAP_ABB_SETUP_SR2EN_MASK, abb_type_mask | OMAP_ABB_SETUP_SR2EN_MASK) But I propose to rework this in the following way: - at the beginning of setup function clear both ABB registers (setup and control), writel(0, setup); writel(0, control); - use setbits_le32 everywhere. This guarantees that there will be no trash values in ABB registers and increases code readability. Your opinion? > diff --git a/arch/arm/cpu/armv7/omap-common/abb.c b/arch/arm/cpu/armv7/omap-common/abb.c > index 73eadb2..31332fb 100644 > --- a/arch/arm/cpu/armv7/omap-common/abb.c > +++ b/arch/arm/cpu/armv7/omap-common/abb.c > @@ -115,14 +115,22 @@ void abb_setup(u32 fuse, u32 ldovbb, u32 setup, u32 control, > /* configure timings, based on oscillator value */ > abb_setup_timings(setup); > > + /* > + * Clear any pending ABB tranxdones before we wait for txdone. > + * We do not know the mode in which we have been handed over > + * the system (warm/cold reboot), ROM code operations etc.. > + * on a cold boot, we do not expect to see this set. > + */ > + setbits_le32(txdone, OMAP_ABB_MPU_TXDONE_MASK); > + > /* select ABB type */ > - clrsetbits_le32(setup, > - abb_type_mask | OMAP_ABB_SETUP_SR2EN_MASK, > + clrsetbits_le32(setup, OMAP_ABB_SETUP_ABB_SEL_MASK | > + OMAP_ABB_SETUP_SR2EN_MASK, > abb_type_mask | OMAP_ABB_SETUP_SR2EN_MASK); > > /* initiate ABB ldo change */ > - clrsetbits_le32(control, > - opp_sel_mask | OMAP_ABB_CONTROL_OPP_CHANGE_MASK, > + clrsetbits_le32(control, OMAP_ABB_CONTROL_OPP_SEL_MASK | > + OMAP_ABB_CONTROL_OPP_CHANGE_MASK, > opp_sel_mask | OMAP_ABB_CONTROL_OPP_CHANGE_MASK); > > /* wait until transition complete */ > diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h > index 4892c0a..c2fc180 100644 > --- a/arch/arm/include/asm/omap_common.h > +++ b/arch/arm/include/asm/omap_common.h > @@ -565,13 +565,17 @@ s8 abb_setup_ldovbb(u32 fuse, u32 ldovbb); > #define OMAP_ABB_NOMINAL_OPP 0 > #define OMAP_ABB_FAST_OPP 1 > #define OMAP_ABB_SLOW_OPP 3 > +#define OMAP_ABB_CONTROL_OPP_SEL_MASK (0x3 << 0) > #define OMAP_ABB_CONTROL_FAST_OPP_SEL_MASK (0x1 << 0) > -#define OMAP_ABB_CONTROL_SLOW_OPP_SEL_MASK (0x1 << 1) > +#define OMAP_ABB_CONTROL_SLOW_OPP_SEL_MASK (0x3 << 0) > +#define OMAP_ABB_CONTROL_NOMINAL_OPP_SEL_MASK (0x0 << 0) > #define OMAP_ABB_CONTROL_OPP_CHANGE_MASK (0x1 << 2) > #define OMAP_ABB_CONTROL_SR2_IN_TRANSITION_MASK (0x1 << 6) > #define OMAP_ABB_SETUP_SR2EN_MASK (0x1 << 0) > #define OMAP_ABB_SETUP_ACTIVE_FBB_SEL_MASK (0x1 << 2) > #define OMAP_ABB_SETUP_ACTIVE_RBB_SEL_MASK (0x1 << 1) > +#define OMAP_ABB_SETUP_ABB_SEL_MASK (OMAP_ABB_SETUP_ACTIVE_RBB_SEL_MASK | \ > + OMAP_ABB_SETUP_ACTIVE_FBB_SEL_MASK) > #define OMAP_ABB_SETUP_SR2_WTCNT_VALUE_MASK (0xff << 8) > > static inline u32 omap_revision(void) > >> + >> + /* initiate ABB ldo change */ >> + clrsetbits_le32(control, >> + opp_sel_mask | OMAP_ABB_CONTROL_OPP_CHANGE_MASK, >> + opp_sel_mask | OMAP_ABB_CONTROL_OPP_CHANGE_MASK); >> + >> + /* wait until transition complete */ >> + if (!wait_on_value(OMAP_ABB_CONTROL_SR2_IN_TRANSITION_MASK, 0, >> + (void *)control, LDELAY)) >> + puts("Error: ABB is in transition\n"); > superfluous if you wait for txdone. Agree. Can be removed. [snip] > + /* setup LDOVBB using fused value */ > + clrsetbits_le32(ldovbb, vset, vset); > here as well -> please why clrbits need to be clearing all field - > suggest looking in all usages. This is a mistake :( Should be clrsetbits_le32(ldovbb, OMAP5_ABB_LDOVBBMPU_VSET_OUT_MASK, vset) Thank you for catching this! >> + >> + return 0; >> +} >> ____________________ >> U-Boot mailing list >> U-Boot at lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot [snip] Regards, Andrii