From mboxrd@z Thu Jan  1 00:00:00 1970
From: Aneesh V <aneesh@ti.com>
Date: Mon, 10 Jan 2011 19:11:53 +0530
Subject: [U-Boot] [PATCH 5/8] armv7: add PL310 support to u-boot
In-Reply-To: <20110109224846.B1FA7150A44@gemini.denx.de>
References: <1293018898-13253-1-git-send-email-aneesh@ti.com>
	<1293018898-13253-6-git-send-email-aneesh@ti.com>
	<20110109224846.B1FA7150A44@gemini.denx.de>
Message-ID: <4D2B0CA1.5050604@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

Dear Wolfgang,

On Monday 10 January 2011 04:18 AM, Wolfgang Denk wrote:
> Dear Aneesh V,
>
> In message<1293018898-13253-6-git-send-email-aneesh@ti.com>  you wrote:
>> Add support for some of the key maintenance operations
>> 	- Invalidate all
>> 	- Invalidate range
>> 	- Flush(clean&  invalidate) all
>> 	- Flush range
>
> Can you please use a more descriptive subject, and commit message?
>
> I have no idea what a "PL310" might be - is this a new board? Or a new
> SoC? or a new Ethernet controller?

Sure. I will add a more descriptive message.
PL310 is an L2 cache controller from ARM.

>
>
> And what exactly are "key maintenance operations"?  Looks as if you
> were talking about basic cache operations?

Yes. I was talking about basic cache operations. In ARM terminology the
cache operations are called "cache maintenance operations".

>
>> --- /dev/null
>> +++ b/arch/arm/include/asm/pl310.h
> ...
>> +/* Register offsets */
>> +#define PL310_CACHE_TYPE		0x004
>> +#define PL310_AUX_CTRL			0x104
>> +
>> +#define PL310_CACHE_SYNC		0x730
>> +#define PL310_INVAL_LINE_PA		0x770
>> +#define PL310_INVAL_WAY			0x77C
>> +#define PL310_CLEAN_LINE_PA		0x7B0
>> +#define PL310_CLEAN_INVAL_WAY		0x7FC
>> +#define PL310_CLEAN_INVAL_LINE_PA	0x7F0
>
> NAK.  Please use a C struct instead.

Ok.

>
>
>> --- a/arch/arm/lib/Makefile
>> +++ b/arch/arm/lib/Makefile
>> @@ -42,6 +42,7 @@ COBJS-y	+= cache.o
>>   ifndef CONFIG_SYS_NO_CP15_CACHE
>>   COBJS-y	+= cache-cp15.o
>>   endif
>> +COBJS-$(CONFIG_SYS_USE_PL310) += cache-pl310.o
>>   COBJS-y	+= interrupts.o
>>   COBJS-y	+= reset.o
>
> There is no documentation for CONFIG_SYS_USE_PL310, and there is no
> use of this variable.
>
> Also, it seems CONFIG_SYS_PL310 would be more appropriate.

I shall make it CONFIG_SYS_PL310 and add documentation.

>
> ...
>> +static void pl310_cache_sync(void)
>> +{
>> +	__raw_writel(0, CONFIG_SYS_PL310_BASE + PL310_CACHE_SYNC);
>> +}
>
> Please use a proper C struct instead of base address plus offset.
> Please fix globally.
>
> ...
>> +	for (pa = start; pa<  stop; pa = pa + line_size)
>> +		__raw_writel(pa, CONFIG_SYS_PL310_BASE +
>> +			     PL310_CLEAN_INVAL_LINE_PA);
>
> Please use braces for multiline statements.

ok.

>
>
> Best regards,
>
> Wolfgang Denk
>

Best regards,
Aneesh