[U-Boot] [PATCH v4 04/12] SPEAr : smi driver support for SPEAr SoCs

Tom
2010-01-14T15:22:32Z

Vipin Kumar wrote:
> Hello Tom,
>
>> +
>> +#define ST_M25Pxx_ID 0x00002020
>> +
>> +static struct flash_dev flash_ids[] = {
>> + {0x10, 64 * 1024, 2},
>> + {0x11, 128 * 1024, 4},
>> + {0x12, 256 * 1024, 4},
>> + {0x13, 512 * 1024, 8},
>> + {0x14, 1 * 1024 * 1024, 16},
>> + {0x15, 2 * 1024 * 1024, 32},
>> + {0x16, 4 * 1024 * 1024, 64},
>> + {0x17, 8 * 1024 * 1024, 128},
>> + {0x18, 16 * 1024 * 1024, 64},
>> + {0x00,}
>> +};
>> +
>> Could be accessed like
>> {
>> .densisty = xxx,
>> .size = xxx,
>> .sector_count = xxx,
>> }
>>
> This is meant to be a look up table, that's why I think the previous
> way is batter.
> Actually, I got the inspiration from drivers/mtd/nand/nand_ids.c
> Still, I have changed it to look as below
> {0x10, 0x10000, 2},
> {0x11, 0x20000, 4},
> {0x12, 0x40000, 4},
> {0x13, 0x80000, 8},
> size is changed to hex to stay vertically aligned
> Please let me know if this is OK

This is fine.
Improved readability a plus!

>
>> + *
>> + * Detect the SMI flash by reading the ID. Initializes the flash_info structure
>> + * with size, sector count etc.
>> + */
>> +static ulong flash_get_size(ulong base, int banknum)
>> +{
>> + flash_info_t *info = &flash_info[banknum];
>> + struct flash_dev *dev;
>> + unsigned int value;
>> + unsigned int density;
>> + int i;
>> +
>> + value = smi_read_id(info, banknum);
>> + density = (value >> 16) & 0xff;
>> +
>> + for (i = 0, dev = &flash_ids[0]; dev->density != 0x0;
>> + i++, dev = &flash_ids[i]) {
>> + if (dev->density == density) {
>> + info->size = dev->size;
>> + info->sector_count = dev->sector_count;
>> + break;
>> + }
>> + }
>> From the flash_ids's struct, it looks like 'density' field is
>> unique and increasing by 1. You may be able to replay this loop
>> with somthing like
>>
>> density -= DENSITY_START
>> if (density < 0)
>> bail
>> else if (density > DESITY_END)
>> bail
>>
>> then use desity as an index into the flash_ids array
>> info->size = flash_ids[density].size
>>
> density is one byte of the read id which is read from flash itself
> as of now, we are supporting consecutive values but these values
> may be rendom in future
> Thats why I used a look up table

Ok.

>
>> +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;
>> +
>> + /* Try again after 1m-sec */
>> + udelay(1000);
>> + }
>> + printf("SMI controller is still in wait, timeout=%d\n", timeout);
>> + return -EIO;
>>
>> Always failure ?
>>
> There is a return 0 in case WIP_BIT gets reset
> else if (!(sr & WIP_BIT))
> return 0;
>

Sorry missed that

Tom

> All the other comments are accepted and I would send a patchset v5 soon
>
> Regards
> Vipin