From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrii Tseglytskyi <andrii.tseglytskyi@ti.com>
Date: Mon, 13 May 2013 18:07:31 +0300
Subject: [U-Boot] [PATCH v1 1/2] OMAP3+: introduce generic ABB support
In-Reply-To: <20130513145800.GA2501@kahuna>
References: <1368454519-23197-1-git-send-email-andrii.tseglytskyi@ti.com>
	<1368454519-23197-2-git-send-email-andrii.tseglytskyi@ti.com>
	<20130513145800.GA2501@kahuna>
Message-ID: <519101B3.4010400@ti.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 05/13/2013 05:58 PM, Nishanth Menon wrote:
> Few minor comments follow:
> On 17:15-20130513, Andrii Tseglytskyi wrote:
> <snip>
>> diff --git a/arch/arm/cpu/armv7/omap-common/abb.c b/arch/arm/cpu/armv7/omap-common/abb.c
>> new file mode 100644
>> index 0000000..7ade110
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/omap-common/abb.c
>> @@ -0,0 +1,139 @@
> <snip>
>> +	/* - On OMAP5+ silicons some ABB setting are fused
> /*
>   *
> please
>> +	 *  in corresponding OPP control registers. Also additional
>> +	 *  setup for LDOVBB is required. Initialization
>> +	 *  sequence contains specific part which handles this.
>> +	 *  If function call fails - return quitely, it means
>> +	 *  no ABB is required for silicon.
>> +	 *
>> +	 * - OMAP3 and OMAP4 don't have any fused settings for ABB.
>> +	 *   EFUSE and LDOVBB registers are also not defined for them.
>> +	 *   ABB will be initialized in the common way without
>> +	 *   ldovbb setup.
> OMAP4 does have ABB efuse offset for OPP_TURBO. LDOVBB override is not
> used. please rephrase accordingly.

OK.

>> +	 */
>> +	if (fuse && ldovbb) {
>> +		if (abb_setup_ldovbb(fuse, ldovbb))
>> +			return;
>> +	}
> <snip>
>> +s8 abb_setup_ldovbb(u32 fuse, u32 ldovbb)
>> +{
>> +	u32 vset;
>> +
>> +	/* ABB parameters must be properly fused
>> +	 * otherwise ABB should be disabled */
> please fix comment style.

OK

>> +	vset = readl(fuse);
>> +	if (!(vset & OMAP5_ABB_FUSE_ENABLE_MASK))
>> +		return -1;
>> +
>> +	/* prepare VSET value for LDOVBB mux register */
>> +	vset &= OMAP5_ABB_FUSE_VSET_MASK;
>> +	vset >>= ffs(OMAP5_ABB_FUSE_VSET_MASK) - 1;
>> +	vset <<= ffs(OMAP5_ABB_LDOVBBMPU_VSET_OUT_MASK) - 1;
>> +	vset |= OMAP5_ABB_LDOVBBMPU_MUX_CTRL_MASK;
>> +
>> +	/* setup LDOVBB using fused value */
>> +	clrsetbits_le32(ldovbb, vset, vset);
>> +
>> +	return 0;
>> +}
>> diff --git a/arch/arm/include/asm/arch-omap3/omap3.h b/arch/arm/include/asm/arch-omap3/omap3.h
>> index 2b5e9ae..66361d5 100644
>> --- a/arch/arm/include/asm/arch-omap3/omap3.h
>> +++ b/arch/arm/include/asm/arch-omap3/omap3.h
>> @@ -253,4 +253,13 @@ struct gpio {
>>   
>>   #define OMAP3_EMU_HAL_START_HAL_CRITICAL	4
>>   
>> +/*
>> + * ABB settings
>> + */
> could be one line :)
Yeah it could. The reason why I did this - all comments in this file 
have such style.
Anyway, agree with you.


>> +#define OMAP_ABB_SETTLING_TIME		30
>> +#define OMAP_ABB_CLOCK_CYCLES		8
> <snip>
> otherwise looks ok to me.
>
>

Thank you,

Regards,
Andrii