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

[Jagan Teki]

On Thu, Jun 6, 2013 at 12:18 PM, Insop Song <Insop.Song at cohere.net> wrote:
> Hi Jagan,
>
> Thank you for your feedback,
>
>> -----Original Message-----
>> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
>> Sent: Wednesday, June 05, 2013 11:34 PM
>> To: Insop Song
>> Cc: u-boot at lists.denx.de; yorksun at freescale.com
>> Subject: Re: [U-Boot] [PATCH] Add SPI Flash STMicro's N25Q512A & add flag
>> status check during SPI Flash write
>>
>> 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.
>
> Agree with you, that's why I also was not so happy to put flag_status to a generic struct.
>
>> 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().
>>
>
> Sounds good. I think your updated patch is coming on line now, so I will take a look at your v2 patch and I will comment on that thread directly.
>
> I think we could close this discussion here.
>
> Thank you.
>
> Insop

Thank you, if possible please test with new sf framework updates.

--
Thanks,
Jagan.