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

[Insop Song]

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