[U-Boot] [PATCH v02 1/2] OMAP3+: introduce generic ABB support

[Andrii Tseglytskyi <andrii.tseglytskyi@ti.com>]

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