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

Reply via email to