From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tom <Tom.Rix@windriver.com>
Date: Thu, 14 Jan 2010 09:22:32 -0600
Subject: [U-Boot] [PATCH v4 04/12] SPEAr : smi driver support for SPEAr
 SoCs
In-Reply-To: <83d1d72b1001140250he7f3eb7tf85f034746ae0f25@mail.gmail.com>
References: <4B4DC5EB.9090507@windriver.com>
	<83d1d72b1001140250he7f3eb7tf85f034746ae0f25@mail.gmail.com>
Message-ID: <4B4F36B8.4010609@windriver.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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