[U-Boot] [PATCH 2/9] smi driver support for SPEAr SoCs

[Vipin Kumar]

Dear Wolfgang,

> In message <83d1d72b0912182244yb303c6ai3b61d7b896020c00 at mail.gmail.com> you wrote:
>>
>> >> +     switch (density) {
>> >> +     case 0x10:
>> >> +             info->size = 64 * 1024;
>> >> +             info->sector_count = 2;
>> >> +             break;
>> >> +     case 0x11:
>> >> +             info->size = 128 * 1024;
>> >> +             info->sector_count = 4;
>> >> +             break;
>> >> +     case 0x12:
>> >> +             info->size = 256 * 1024;
>> >> +             info->sector_count = 4;
>> >> +             break;
>> >> +     case 0x13:
>> >> +             info->size = 512 * 1024;
>> >> +             info->sector_count = 8;
>> >> +             break;
>> >> +     case 0x14:
>> >> +             info->size = 1 * 1024 * 1024;
>> >> +             info->sector_count = 16;
>> >> +             break;
>> >> +     case 0x15:
>> >> +             info->size = 2 * 1024 * 1024;
>> >> +             info->sector_count = 32;
>> >> +             break;
>> >> +     case 0x16:
>> >> +             info->size = 4 * 1024 * 1024;
>> >> +             info->sector_count = 64;
>> >> +             break;
>> >> +     case 0x17:
>> >> +             info->size = 8 * 1024 * 1024;
>> >> +             info->sector_count = 128;
>> >> +             break;
>> >> +     case 0x18:
>> >> +             info->size = 16 * 1024 * 1024;
>> >> +             info->sector_count = 64;
>> >> +             break;
>> >> +     default:
>> >> +             return 0x0;
>> >> +     }
>> >
>> > Consider using lookup tables?
>>
>> Currently supported flashes have consequent values of density.
>> It may have random values supported in future. That's the reason
>> I feel it's better to keep the code this way
>
> That's why I did not recommend to calculate this witha simple
> formula, (which would be trivially possible for "size", but not so
> for "sector_count"), but using a lookup table - this is independent
> of the actual values.

Ok. I would implement the code using lookup tables

>> >> +     /* Assume that all sectors are unprotected by default */
>> >> +     for (i = 0; i < CONFIG_SYS_MAX_FLASH_SECT; i++)
>> >> +             info->protect[i] = 0;
>> >
>> > Um... is this assumption correct?
>>
>> It is intentional
>
> Why don;t you protect sectors where the U-Bootimage and environment
> are stored?

Since the code is being developed for a development board, erasing and
flashing the uboot is frequent. This is done only to save unprotect every
time before erasing/flashing uboot

Best Regards
Vipin