On Thu, Jun 6, 2013 at 12:18 PM, Insop Song <[email protected]> wrote: > Hi Jagan, > > Thank you for your feedback, > >> -----Original Message----- >> From: Jagan Teki [mailto:[email protected]] >> Sent: Wednesday, June 05, 2013 11:34 PM >> To: Insop Song >> Cc: [email protected]; [email protected] >> 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. _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
