[U-Boot] [PATCH v3 11/18] arm: Add control over cachability of memory regions

[Simon Glass]

Hi Albert and Mike,

On Thu, Jul 19, 2012 at 7:10 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Thursday 12 July 2012 14:12:41 Albert ARIBAUD wrote:
>> On Thu, 12 Jul 2012 08:25:11 -0700, Simon Glass wrote:
>> > --- a/arch/arm/include/asm/system.h
>> > +++ b/arch/arm/include/asm/system.h
>> >
>> > +/* options available for data cache on each page */
>> > +enum dcache_option {
>> > +   DCACHE_OFF,
>> > +   DCACHE_WRITETHROUGH,
>> > +   DCACHE_WRITEBACK,
>> > +};
>> >
>> > --- a/arch/arm/lib/cache-cp15.c
>> > +++ b/arch/arm/lib/cache-cp15.c
>> >
>> > -static inline void dram_bank_mmu_setup(int bank)
>> > +void set_section_dcache(int section, enum dcache_option option)
>> >  {
>> > +   u32 value = section << MMU_SECTION_SHIFT | (3 << 10);
>> >     u32 *page_table = (u32 *)gd->tlb_addr;
>> > +
>> > +   switch (option) {
>> > +   case DCACHE_WRITETHROUGH:
>> > +           value |= 0x1a;
>> > +           break;
>> > +
>> > +   case DCACHE_WRITEBACK:
>> > +           value |= 0x1e;
>> > +           break;
>> > +
>> > +   case DCACHE_OFF:
>> > +           value |= 0x12;
>> > +           break;
>> > +   }
>>
>> what's the benefit of introducing an arbitrary enum rather than
>> defining DCACHE_WRITETHROUGH, DCACHE_WRITEBACK and DCACHE_OFF equal
>> respecitvely to 0x1a, 0x1e and 0x12? All it does is force this
>> switch case instead of a simple 'value |= option;' statement.
>
> if these magic bitmasks are going to be the same for all arm cores (the enum
> is in common arm header), then you can get the advantages of both:
> enum dcache_option {
>         DCACHE_OFF = 0x12,
>         DCACHE_WRITETHROUGH = 0x1a,
>         DCACHE_WRITEBACK = 0x1e,
> };
>
> then just do:
>         value |= option;

Yes you are both saying the same thing - sounds good. I will change it.

Regards,
Simon

> -mike