Hi Jagan,
On Mon, Jun 10, 2013 at 8:27 AM, Jagan Teki jagannadh.teki@gmail.comwrote:
Hi Simon,
On Sun, Jun 9, 2013 at 9:36 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki jagannadh.teki@gmail.com
wrote:
Hi Simon,
On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki <jagannadh.teki@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@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@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?
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.