[U-Boot] [PATCH v02 1/2] OMAP3+: introduce generic ABB support
Andrii Tseglytskyi
andrii.tseglytskyi at ti.com
Mon May 20 13:06:38 CEST 2013
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
More information about the U-Boot
mailing list