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@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[snip]
Regards,
Andrii
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
