[U-Boot] [PATCH 5/8] armv7: add PL310 support to u-boot

Aneesh V
2011-01-10T13:41:53Z

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