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

Wolfgang Denk
2011-01-09T22:48:46Z

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?

And what exactly are "key maintenance operations"? Looks as if you
were talking about basic cache 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.

> --- 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.

...
> +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.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
No one can guarantee the actions of another.
-- Spock, "Day of the Dove", stardate unknown