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

On Thu, Jun 6, 2013 at 12:18 PM, Insop Song Insop.Song@cohere.net wrote:

Hi Jagan,

Thank you for your feedback,

...

-----Original Message----- From: Jagan Teki [mailto:jagannadh.teki@gmail.com] Sent: Wednesday, June 05, 2013 11:34 PM To: Insop Song Cc: u-boot@lists.denx.de; yorksun@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.