Re: [U-Boot] [PATCH v02 1/2] OMAP3+: introduce generic ABB support
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 */
Re: [U-Boot] [PATCH v02 1/2] OMAP3+: introduce generic ABB support
On Mon, May 20, 2013 at 6:06 AM, Andrii Tseglytskyi
andrii.tseglyts...@ti.com wrote:
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.
PRM register may be reset OR not depending on type of reset and SoC,
Though this might be the first line of code to execute in bootloader
that touches this register, there is no guarentee that there is no
pending TRANXDONE interrupt in the register. if there is TRANXDONE
interrupt, it is not something we desire. Hence the suggestion to
clear.
+
+ /* 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?
Case 1) if we have been given control by another bootloader which had
already setup ABB, writing 0 will not impact the current setting as
the transition trigger is OPP_CHANGE
case 2) we are the only bootloader in the system. writing 0 is ok as well.
So, am ok if you reset the registers prior to programming them.
--
Regards,
Nishanth Menon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v02 1/2] OMAP3+: introduce generic ABB support
On 19:49-20130513, Andrii Tseglytskyi wrote:
[...]
+void abb_setup(u32 fuse, u32 ldovbb, u32 setup, u32 control,
+u32 txdone, u32 txdone_mask, u32 opp)
+{
+ u32 abb_type_mask, opp_sel_mask;
+
+ /* sanity check */
+ if (!setup || !control || !txdone)
+ return;
+
+ /* setup ABB only in case of Fast or Slow OPP */
+ switch (opp) {
+ case OMAP_ABB_FAST_OPP:
+ abb_type_mask = OMAP_ABB_SETUP_ACTIVE_FBB_SEL_MASK;
+ opp_sel_mask = OMAP_ABB_CONTROL_FAST_OPP_SEL_MASK;
+ break;
+ case OMAP_ABB_SLOW_OPP:
+ abb_type_mask = OMAP_ABB_SETUP_ACTIVE_RBB_SEL_MASK;
+ opp_sel_mask = OMAP_ABB_CONTROL_SLOW_OPP_SEL_MASK;
+ break;
+ default:
+return;
+ }
+
+ /*
+ * For some OMAP silicons additional setup for LDOVBB register is
+ * required. This is determined by data retrieved from corresponding
+ * OPP EFUSE register. Data, which is retrieved from EFUSE - is
+ * ABB enable/disable flag and VSET value, which must be copied
+ * to LDOVBB register. If function call fails - return quietly,
+ * it means no ABB is required for such silicon.
+ *
+ * For silicons, which don't require LDOVBB setup fuse and
+ * ldovbb offsets are not defined. ABB will be initialized in
+ * the common way for them.
+ */
+ 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.
+
+ /* 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:
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
