On Tue, Jun 11, 2013 at 2:01 AM, Simon Glass <[email protected]> wrote: > Hi Jagan, > > On Mon, Jun 10, 2013 at 8:27 AM, Jagan Teki <[email protected]> > wrote: >> >> Hi Simon, >> >> On Sun, Jun 9, 2013 at 9:36 PM, Jagan Teki <[email protected]> >> wrote: >> > Hi Simon, >> > >> > On Sun, Jun 9, 2013 at 7:43 PM, Simon Glass <[email protected]> wrote: >> >> Hi Jagan, >> >> >> >> On Sat, Jun 8, 2013 at 7:44 AM, Jagan Teki <[email protected]> >> >> wrote: >> >>> >> >>> Hi Simon, >> >>> >> >>> On Sat, Jun 8, 2013 at 8:02 PM, Simon Glass <[email protected]> wrote: >> >>> > Hi Jagan, >> >>> > >> >>> > On Sat, Jun 8, 2013 at 1:32 AM, Jagan Teki >> >>> > <[email protected]> >> >>> > 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 >> >>> >> <[email protected]> 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 <[email protected]> >> >>> >> > --- >> >>> >> > 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. > > _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
