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

Vipin Kumar
2009-12-19T06:44:00Z

Dear Wolfgang,

>> Signed-off-by: Vipin <vipin.kumar@st.com>
> ...
>> +static ulong flash_get_size(ulong base, int banknum)
>> +{
>> + ? ? flash_info_t *info = &flash_info[banknum];
>> + ? ? unsigned int value = 0;
>> + ? ? unsigned int density = 0;
>
> remove useless initialization.

Ok
I will send out a fresh patchset v2 with your review comments
incorporated.
Please let me know if it is ok

>> + ? ? int i;
>> +
>> + ? ? value = smi_read_id(info, banknum);
>> + ? ? density = (value >> 16) & 0xff;
>> +
>> + ? ? 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

>> + ? ? /* 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

>
>> +static int smi_wait_till_ready(int bank, int timeout)
>> +{
>> + ? ? int count;
>> + ? ? int sr;
>> +
>> + ? ? /* One chip guarantees max 5 msec wait here after page writes,
>> + ? ? ? ?but potentially three seconds (!) after page erase. */
>> + ? ? for (count = 0; count < timeout; count++) {
>> + ? ? ? ? ? ? sr = smi_read_sr(bank);
>> + ? ? ? ? ? ? if (sr < 0)
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? else if (!(sr & WIP_BIT))
>> + ? ? ? ? ? ? ? ? ? ? return 0;
>
> Use braces here.

Ok. braces added

Best Regards
Vipin