Thank you very much for your interest on this.
>> > What do you think?
>> >
>> > Thank you,
>>
>> Can u just send pseudo code about your logic, i couldn't get u exactly sorry.
>>
>
> Here is what I think, it is similar with your patch in the sense that "do the
> different status" check for different device.
> However, the difference is that your patch checks based on device type at the
> status checking function.
> Mine is proposing to add the information to the devices' struct (spi_flash)
> instead, so each device can have a flexibility to call flag status check. In
> this way if new device use this flag_status check, you can just hide the
> setting in devices' probe function, and not worrying about updating generic
> function call (spi_flash_cmd_poll_bit() as in your patch)
Your idea seems to be clean and reasonable, but
>
> I am not so happy to add "flag_status" in struct spi_flash; so I am open to
> your idea.
> Please see below and let me know what you think?
>
>
>
>
> // 1. adding flag status
> struct spi_flash {
> struct spi_slave *spi;
>
> const char *name;
>
> /* Total flash size */
> u32 size;
> /* Write (page) size */
> u32 page_size;
> /* Erase (sector) size */
> u32 sector_size;
>
> /* if flag_status is use or not */
> u8 flag_status;
>
> int (*read)(struct spi_flash *flash, u32 offset,
> size_t len, void *buf);
> int (*write)(struct spi_flash *flash, u32 offset,
> size_t len, const void *buf);
> int (*erase)(struct spi_flash *flash, u32 offset,
> size_t len);
> };
>
> // 2. in probe function, set the flag_status per vendor
>
>
> struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 * idcode)
> {
>
> flash->spi = spi;
> flash->name = params->name;
>
> flash->write = spi_flash_cmd_write_multi;
> flash->erase = spi_flash_cmd_erase;
> flash->read = spi_flash_cmd_read_fast;
> flash->page_size = 256;
> flash->sector_size = 256 * params->pages_per_sector;
> flash->size = flash->sector_size * params->nr_sectors;
>
> // we do this for Micron, and will do the same if any other device
> needs this
> flash->flag_status = 1;
>
> return flash;
> }
>
>
> // 3. call flag status check if flash->flag_status is set
>
>
> int spi_flash_cmd_wait_ready(struct spi_flash *flash, unsigned long timeout)
> {
> if (flash->flag_status)
> return spi_flash_cmd_poll_bit(flash, timeout,
> CMD_READ_FLAG_STATUS, STATUS_PEC, STATUS_PEC);
> else
> return spi_flash_cmd_poll_bit(flash, timeout,
> CMD_READ_STATUS, STATUS_WIP, 0);
> }
APAIK. flag status required only for stmicro flashes which has >= 512MB flashes.
And as this is specific to a particular flash with particular size, i
don't see any reason to add extra variable in spi_flash
and then initialized at probe.
Your idea seems to be reasonable if we have more numbers of flash
vendors require this with respective sizes.
ie, the reason I have gave a condition for the particular state like
+ if ((flash->idcode0 == 0x20) &&
+ (flash->size >= SPI_FLASH_512MB_STMIC))
And I have removed spi_flash_cmd_poll_bit as these is no separate
caller for this except the spi_flash_cmd_wait_ready()
and did everything on spi_flash_cmd_wait_ready().
Comments.
--
Thanks,
Jagan.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
