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

[Jagan Teki]

On Wed, Jun 5, 2013 at 10:10 AM, 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: Monday, June 03, 2013 10:31 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
>> >
>> > I've made a change and add spi_flash_cmd_wait_program to align with
>> existing code structure.
>> > Please see the patch below.
>> > Erase is okay without checking, so I didn't add the check.
>>
>> No i see without the check in erase when i erase and read back i couldn't see
>> 0xff Please check.
>
> I've tested on the N25Q512A erase without the flag check went okay.
> However, adding the check to the erase would be safe, and I've tested erase with flag status check.
> Here is the diff of this change. I will use the format-patch when we finalize the change.
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 4240e9d..9cf5aaf 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -247,6 +247,10 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
>                 if (ret)
>                         goto out;
>
> +         ret = spi_flash_cmd_wait_program(flash, SPI_FLASH_PROG_TIMEOUT);
> +         if (ret)
> +                 break;
> +
>                 ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PAGE_ERASE_TIMEOUT);
>                 if (ret)
>                         goto out;
>
>
>> >
>> > I am not sure I'd agree with you on that you are putting the device check in
>> spi_flash_cmd_poll_bit().
>> > I am more inclined to make the change at the level where we call
>> spi_flash_cmd_poll_bit() if we have to distinguish per devices.
>>
>> spi_flash_cmd_poll_bit() is common call for poll for read_status, as write call
>> is common to all making changes to
>> spi_flash_cmd_poll_bit() is valid i guess.
>> Write call is a global which is used by
>> so many user, i don't like to add the flag status there and make the confusion
>> user.
>>
>
> To your comment on "confusion to users", I agree on that.
> With that argument, your patch is better since it hides the flag status check.
>
> And how about this?  can we add "flag_status_check" flag somewhere, and if the device is needed then we set the flag during device discovery.
> Then call the flag check function if the flag is set.
> I think this way might be more generic then what you did in your patch.
>
> What do you think?
>
> Thank you,

Can u just send pseudo code about your logic, i couldn't get u exactly sorry.

--
Thanks,
Jagan.