[U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag status check during SPI Flash write

[Jagan Teki]

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.