[U-Boot] [PATCH v2 16/16] sf: Add Flag status register polling support
Jagan Teki
jagannadh.teki at gmail.com
Tue Jun 11 06:39:03 CEST 2013
On Tue, Jun 11, 2013 at 2:01 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Jagan,
>
> On Mon, Jun 10, 2013 at 8:27 AM, Jagan Teki <jagannadh.teki at gmail.com>
> wrote:
>>
>> 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 mean you will keep the old status-reading algorithm, or change to
> your new one?
Will keep the old status-read algo.
--
Thanks,
Jagan.
>
>>
>>
>> Do you have any more comments on this series.
>> I will going to send v3 and planning to apply.
>
>
> I don't have any other comments.
>
>>
>>
>> --
>> Thanks,
>> Jagan.
>
>
More information about the U-Boot
mailing list