[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support

Jagan Teki jagannadh.teki at gmail.com
Mon Jun 10 17:27:36 CEST 2013


Hi Simon,

On Sun, Jun 9, 2013 at 9:36 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagan,
>>
>> On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <sjg at chromium.org> wrote:
>>> > Hi Jagan,
>>> >
>>> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki at gmail.com>
>>> > wrote:
>>> >>
>>> >> Hi Simon,
>>> >>
>>> >> Please let know your comments.
>>> >>
>>> >> I have changed the logic, but removed spi_flash_cmd_poll_bit() use
>>> >> poll code on spi_flash_cmd_wait_ready()
>>> >> as no other call for spi_flash_cmd_poll_bit() this.
>>> >>
>>> >> And also for read_status the check_status i assigned as 0,earlier it
>>> >> has direct 0 (w/o check_status variable).
>>> >>
>>> >> To add the support for flag status on the same code, i define this
>>> >> check_status.
>>> >> I don't see any coding functionality change for now, compared to
>>> >> before.
>>> >
>>> >
>>> > This is not the right patch, but in one of them you remove
>>> > spi_flash_cmd_poll_bit(), so that it no longer works the same way. You
>>> > will
>>> > get lots of individual SPI transactions on the bus instead of a single
>>> > one
>>> > that reads the status byte continuously.
>>>
>>> I couldn't understand you clearly.
>>>
>>> Earlier all erase/write they were calling spi_flash_cmd_wait_ready
>>> with flash, timeout values.
>>> spi_flash_cmd_wait_ready() is intern call to spi_flash_cmd_poll_bit()
>>> with CMD_READ_STATUS and STATUS_WIP.
>>>
>>> In my changes I have removed spi_flash_cmd_poll_bit() as there is no
>>> other caller except spi_flash_cmd_wait_ready()
>>> so all polling stuff on spi_flash_cmd_wait_ready() means, still
>>> erase/program can call spi_flash_cmd_wait_ready() with
>>> flash, timeout and CMD_READ_STATUS and STAUS_WIP were use directly
>>> used on spi_flash_cmd_wait_ready() instead
>>> of passing through spi_flash_cmd_poll_bit().
>>>
>>> Any wrong here, please comment.
>>
>>
>> You have changed the implementation of spi_flash_cmd_wait_ready() so that
>> instead of polling for the bit within a single SPI transaction (CS staying
>> asserted) you are now doing multiple transactions.
>>
>> The old code for spi_flash_cmd_wait_ready() ended up with an algorithm like
>> this, when you look inside the nested functions:
>>
>> spi_claim_bus(spi);
>> ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>> do {
>> ret = spi_xfer(spi, 8, NULL, &status, 0);
>>         ...
>> }
>> spi_xfer(spi, 0, NULL, NULL, SPI_XFER_END);
>> spi_release_bus(spi);
>>
>> The new code is:
>>
>> do {
>> spi_claim_bus(spi);
>>         ret = spi_xfer(spi, cmd_len * 8, cmd, NULL, SPI_XFER_BEGIN);
>>         ret = spi_xfer(spi, data_len * 8, data_out, data_in, SPI_XFER_END);
>> spi_release_bus(spi);
>>         ...
>> } while (...)
>>
>>
>> Can you see the difference? The bus clain and SPI_XFER_BEGIN/END is now
>> inside the loop.
>
> Understand, i thought as we do _END transfer in old code as well.
> It could be fine to go with using common function as it will do _BEGIN and _END.
>
> What could be the problem you see if we do _BEGIN and _END on loop...?
>
>>
>> Is there a reason why this change is needed? Does updating the bank address
>> not work with the old code?
>
> It doesn't effect with banking.
>
> Thanks,
> Jagan.
>
>>
>> It is find to move the spi_flash_cmd_poll_bit() function into
>> spi_flash_cmd_wait_ready() - I am not worried about that. I am only worried
>> about your change of algorithm as above.
>>
>> Regards,
>> Simon
>>
>>>
>>>
>>> Thanks,
>>> Jagan.
>>>
>>> >
>>> > Do we need to change this?
>>> >
>>> >
>>> >>
>>> >>
>>> >> --
>>> >> Thanks,
>>> >> Jagan.
>>> >>
>>> >> On Fri, May 31, 2013 at 6:22 PM, Jagannadha Sutradharudu Teki
>>> >> <jagannadha.sutradharudu-teki at xilinx.com> wrote:
>>> >> > Flag status register polling is required for micron 512Mb flash
>>> >> > devices onwards, for performing erase/program operations.
>>> >> >
>>> >> > Like polling for WIP(Write-In-Progress) bit in read status register,
>>> >> > spi_flash_cmd_wait_ready will poll for PEC(Program-Erase-Control)
>>> >> > bit in flag status register.
>>> >> >
>>> >> > Signed-off-by: Jagannadha Sutradharudu Teki <jaganna at xilinx.com>
>>> >> > ---
>>> >> > Changes for v2:
>>> >> >         - none
>>> >> >
>>> >> >  drivers/mtd/spi/spi_flash.c          | 15 ++++++++++++---
>>> >> >  drivers/mtd/spi/spi_flash_internal.h |  3 +++
>>> >> >  2 files changed, 15 insertions(+), 3 deletions(-)
>>> >> >
>>> >> > diff --git a/drivers/mtd/spi/spi_flash.c
>>> >> > b/drivers/mtd/spi/spi_flash.c
>>> >> > index 527423d..8cd2988 100644
>>> >> > --- a/drivers/mtd/spi/spi_flash.c
>>> >> > +++ b/drivers/mtd/spi/spi_flash.c
>>> >> > @@ -195,25 +195,34 @@ int spi_flash_cmd_wait_ready(struct spi_flash
>>> >> > *flash, unsigned long timeout)
>>> >> >         unsigned long timebase;
>>> >> >         int ret;
>>> >> >         u8 status;
>>> >> > +       u8 check_status = 0x0;
>>> >> >         u8 poll_bit = STATUS_WIP;
>>> >> >         u8 cmd = CMD_READ_STATUS;
>>> >> >
>>> >> > +       if ((flash->idcode0 == 0x20) &&
>>> >> > +                       (flash->size >= SPI_FLASH_512MB_STMIC)) {
>>> >> > +               poll_bit = STATUS_PEC;
>>> >> > +               check_status = poll_bit;
>>> >> > +               cmd = CMD_FLAG_STATUS;
>>> >> > +       }
>>> >> > +
>>> >> >         timebase = get_timer(0);
>>> >> >         do {
>>> >> >                 WATCHDOG_RESET();
>>> >> >
>>> >> >                 ret = spi_flash_read_common(flash, &cmd, 1, &status,
>>> >> > 1);
>>> >> >                 if (ret < 0) {
>>> >> > -                       debug("SF: fail to read read status
>>> >> > register\n");
>>> >> > +                       debug("SF: fail to read %s status
>>> >> > register\n",
>>> >> > +                               cmd == CMD_READ_STATUS ? "read" :
>>> >> > "flag");
>>> >> >                         return ret;
>>> >> >                 }
>>> >> >
>>> >> > -               if ((status & poll_bit) == 0)
>>> >> > +               if ((status & poll_bit) == check_status)
>>> >> >                         break;
>>> >> >
>>> >> >         } while (get_timer(timebase) < timeout);
>>> >> >
>>> >> > -       if ((status & poll_bit) == 0)
>>> >> > +       if ((status & poll_bit) == check_status)
>>> >> >                 return 0;
>>> >> >
>>> >> >         /* Timed out */
>>> >> > diff --git a/drivers/mtd/spi/spi_flash_internal.h
>>> >> > b/drivers/mtd/spi/spi_flash_internal.h
>>> >> > index ac4530f..cb7a505 100644
>>> >> > --- a/drivers/mtd/spi/spi_flash_internal.h
>>> >> > +++ b/drivers/mtd/spi/spi_flash_internal.h
>>> >> > @@ -13,6 +13,7 @@
>>> >> >  #define SPI_FLASH_SECTOR_ERASE_TIMEOUT (10 * CONFIG_SYS_HZ)
>>> >> >
>>> >> >  #define SPI_FLASH_16MB_BOUN            0x1000000
>>> >> > +#define SPI_FLASH_512MB_STMIC          0x4000000
>>> >> >
>>> >> >  /* Common commands */
>>> >> >  #define CMD_READ_ID                    0x9f
>>> >> > @@ -24,6 +25,7 @@
>>> >> >  #define CMD_PAGE_PROGRAM               0x02
>>> >> >  #define CMD_WRITE_DISABLE              0x04
>>> >> >  #define CMD_READ_STATUS                        0x05
>>> >> > +#define CMD_FLAG_STATUS                        0x70
>>> >> >  #define CMD_WRITE_ENABLE               0x06
>>> >> >  #define CMD_ERASE_4K                   0x20
>>> >> >  #define CMD_ERASE_32K                  0x52
>>> >> > @@ -38,6 +40,7 @@
>>> >> >
>>> >> >  /* Common status */
>>> >> >  #define STATUS_WIP                     0x01
>>> >> > +#define STATUS_PEC                     0x80
>>> >> >
>>> >> >  /* Send a single-byte command to the device and read the response */
>>> >> >  int spi_flash_cmd(struct spi_slave *spi, u8 cmd, void *response,
>>> >> > size_t
>>> >> > len);
>>> >> > --
>>> >> > 1.8.3
>>> >>
>>

May be I will explore much for this, update the changes later.
and i will remains the code as before as of now.

Do you have any more comments on this series.
I will going to send v3 and planning to apply.

--
Thanks,
Jagan.


More information about the U-Boot mailing list