| Message ID | 1361949350-22241-12-git-send-email-dantesu@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 27 February 2013 07:15, Kuo-Jung Su <dantesu@gmail.com> wrote: > From: Kuo-Jung Su <dantesu@faraday-tech.com> Your subject line could be made a little more specific, like this: "hw/nand.c: correct the sense of the BUSY/READY status bit". > The BIT6 of Status Register(SR): > > SR[6] behaves the same as R/B# pin > SR[6] = 0 indicates the device is busy; > SR[6] = 1 means the device is ready > > Some NAND flash controller (i.e. ftnandc021) relies on the SR[6] > to determine if the NAND flash erase/program is success or error timeout. > > P.S: > The exmaple NAND flash datasheet could be found at following link: > http://www.mxic.com.tw/QuickPlace/hq/PageLibrary4825740B00298A3B.nsf/h_Index/8FEA549237D2F7674825795800104C26/$File/MX30LF1G08AA,%203V,%201Gb,%20v1.1.pdf > > Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com> > --- > hw/nand.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/hw/nand.c b/hw/nand.c > index 4a71265..7f40ebf 100644 > --- a/hw/nand.c > +++ b/hw/nand.c > @@ -46,7 +46,7 @@ > # define NAND_IOSTATUS_PLANE1 (1 << 2) > # define NAND_IOSTATUS_PLANE2 (1 << 3) > # define NAND_IOSTATUS_PLANE3 (1 << 4) > -# define NAND_IOSTATUS_BUSY (1 << 6) > +# define NAND_IOSTATUS_READY (1 << 6) > # define NAND_IOSTATUS_UNPROTCT (1 << 7) > > # define MAX_PAGE 0x800 > @@ -231,6 +231,7 @@ static void nand_reset(DeviceState *dev) > s->iolen = 0; > s->offset = 0; > s->status &= NAND_IOSTATUS_UNPROTCT; > + s->status |= NAND_IOSTATUS_READY; > } These two parts look good. > > static inline void nand_pushio_byte(NANDFlashState *s, uint8_t value) > @@ -647,6 +648,8 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) > if (PAGE(s->addr) >= s->pages) > return; > > + s->status &= ~NAND_IOSTATUS_READY; > + > if (!s->bdrv) { > mem_and(s->storage + PAGE_START(s->addr) + (s->addr & PAGE_MASK) + > s->offset, s->io, s->iolen); > @@ -656,7 +659,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) > soff = SECTOR_OFFSET(s->addr); > if (bdrv_read(s->bdrv, sector, iobuf, PAGE_SECTORS) < 0) { > printf("%s: read error in sector %" PRIu64 "\n", __func__, sector); > - return; > + goto blkw_out; > } > > mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, PAGE_SIZE - off)); > @@ -675,7 +678,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) > soff = off & 0x1ff; > if (bdrv_read(s->bdrv, sector, iobuf, PAGE_SECTORS + 2) < 0) { > printf("%s: read error in sector %" PRIu64 "\n", __func__, sector); > - return; > + goto blkw_out; > } > > mem_and(iobuf + soff, s->io, s->iolen); > @@ -685,6 +688,9 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) > } > } > s->offset = 0; > + > +blkw_out: > + s->status |= NAND_IOSTATUS_READY; > } How is it ever possible for a guest to observe the status register in the state where the READY bit is cleared? There's no point in clearing the bit on entry to this function and setting it again on every exit path when nobody will ever read the status register while we're inside the function. (Same comments apply for read and erase.) thanks -- PMM
2013/3/1 Peter Maydell <peter.maydell@linaro.org>: > On 27 February 2013 07:15, Kuo-Jung Su <dantesu@gmail.com> wrote: >> From: Kuo-Jung Su <dantesu@faraday-tech.com> > > Your subject line could be made a little more specific, like this: > "hw/nand.c: correct the sense of the BUSY/READY status bit". > Got it, thanks >> The BIT6 of Status Register(SR): >> >> SR[6] behaves the same as R/B# pin >> SR[6] = 0 indicates the device is busy; >> SR[6] = 1 means the device is ready >> >> Some NAND flash controller (i.e. ftnandc021) relies on the SR[6] >> to determine if the NAND flash erase/program is success or error timeout. >> >> P.S: >> The exmaple NAND flash datasheet could be found at following link: >> http://www.mxic.com.tw/QuickPlace/hq/PageLibrary4825740B00298A3B.nsf/h_Index/8FEA549237D2F7674825795800104C26/$File/MX30LF1G08AA,%203V,%201Gb,%20v1.1.pdf >> >> Signed-off-by: Kuo-Jung Su <dantesu@faraday-tech.com> >> --- >> hw/nand.c | 20 +++++++++++++++++--- >> 1 file changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/hw/nand.c b/hw/nand.c >> index 4a71265..7f40ebf 100644 >> --- a/hw/nand.c >> +++ b/hw/nand.c >> @@ -46,7 +46,7 @@ >> # define NAND_IOSTATUS_PLANE1 (1 << 2) >> # define NAND_IOSTATUS_PLANE2 (1 << 3) >> # define NAND_IOSTATUS_PLANE3 (1 << 4) >> -# define NAND_IOSTATUS_BUSY (1 << 6) >> +# define NAND_IOSTATUS_READY (1 << 6) >> # define NAND_IOSTATUS_UNPROTCT (1 << 7) >> >> # define MAX_PAGE 0x800 >> @@ -231,6 +231,7 @@ static void nand_reset(DeviceState *dev) >> s->iolen = 0; >> s->offset = 0; >> s->status &= NAND_IOSTATUS_UNPROTCT; >> + s->status |= NAND_IOSTATUS_READY; >> } > > These two parts look good. > >> >> static inline void nand_pushio_byte(NANDFlashState *s, uint8_t value) >> @@ -647,6 +648,8 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) >> if (PAGE(s->addr) >= s->pages) >> return; >> >> + s->status &= ~NAND_IOSTATUS_READY; >> + >> if (!s->bdrv) { >> mem_and(s->storage + PAGE_START(s->addr) + (s->addr & PAGE_MASK) + >> s->offset, s->io, s->iolen); >> @@ -656,7 +659,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) >> soff = SECTOR_OFFSET(s->addr); >> if (bdrv_read(s->bdrv, sector, iobuf, PAGE_SECTORS) < 0) { >> printf("%s: read error in sector %" PRIu64 "\n", __func__, sector); >> - return; >> + goto blkw_out; >> } >> >> mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, PAGE_SIZE - off)); >> @@ -675,7 +678,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) >> soff = off & 0x1ff; >> if (bdrv_read(s->bdrv, sector, iobuf, PAGE_SECTORS + 2) < 0) { >> printf("%s: read error in sector %" PRIu64 "\n", __func__, sector); >> - return; >> + goto blkw_out; >> } >> >> mem_and(iobuf + soff, s->io, s->iolen); >> @@ -685,6 +688,9 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) >> } >> } >> s->offset = 0; >> + >> +blkw_out: >> + s->status |= NAND_IOSTATUS_READY; >> } > > How is it ever possible for a guest to observe the status register > in the state where the READY bit is cleared? There's no point in > clearing the bit on entry to this function and setting it again > on every exit path when nobody will ever read the status register > while we're inside the function. (Same comments apply for read > and erase.) > Got it, thanks. I'll remove the code for clear/set SR[6] from the read/erase/write functions. > thanks > -- PMM
diff --git a/hw/nand.c b/hw/nand.c index 4a71265..7f40ebf 100644 --- a/hw/nand.c +++ b/hw/nand.c @@ -46,7 +46,7 @@ # define NAND_IOSTATUS_PLANE1 (1 << 2) # define NAND_IOSTATUS_PLANE2 (1 << 3) # define NAND_IOSTATUS_PLANE3 (1 << 4) -# define NAND_IOSTATUS_BUSY (1 << 6) +# define NAND_IOSTATUS_READY (1 << 6) # define NAND_IOSTATUS_UNPROTCT (1 << 7) # define MAX_PAGE 0x800 @@ -231,6 +231,7 @@ static void nand_reset(DeviceState *dev) s->iolen = 0; s->offset = 0; s->status &= NAND_IOSTATUS_UNPROTCT; + s->status |= NAND_IOSTATUS_READY; } static inline void nand_pushio_byte(NANDFlashState *s, uint8_t value) @@ -647,6 +648,8 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) if (PAGE(s->addr) >= s->pages) return; + s->status &= ~NAND_IOSTATUS_READY; + if (!s->bdrv) { mem_and(s->storage + PAGE_START(s->addr) + (s->addr & PAGE_MASK) + s->offset, s->io, s->iolen); @@ -656,7 +659,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) soff = SECTOR_OFFSET(s->addr); if (bdrv_read(s->bdrv, sector, iobuf, PAGE_SECTORS) < 0) { printf("%s: read error in sector %" PRIu64 "\n", __func__, sector); - return; + goto blkw_out; } mem_and(iobuf + (soff | off), s->io, MIN(s->iolen, PAGE_SIZE - off)); @@ -675,7 +678,7 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) soff = off & 0x1ff; if (bdrv_read(s->bdrv, sector, iobuf, PAGE_SECTORS + 2) < 0) { printf("%s: read error in sector %" PRIu64 "\n", __func__, sector); - return; + goto blkw_out; } mem_and(iobuf + soff, s->io, s->iolen); @@ -685,6 +688,9 @@ static void glue(nand_blk_write_, PAGE_SIZE)(NANDFlashState *s) } } s->offset = 0; + +blkw_out: + s->status |= NAND_IOSTATUS_READY; } /* Erase a single block */ @@ -697,6 +703,8 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s) if (PAGE(addr) >= s->pages) return; + s->status &= ~NAND_IOSTATUS_READY; + if (!s->bdrv) { memset(s->storage + PAGE_START(addr), 0xff, (PAGE_SIZE + OOB_SIZE) << s->erase_shift); @@ -738,6 +746,8 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s) printf("%s: write error in sector %" PRIu64 "\n", __func__, page); } } + + s->status |= NAND_IOSTATUS_READY; } static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s, @@ -746,6 +756,8 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s, if (PAGE(addr) >= s->pages) return; + s->status &= ~NAND_IOSTATUS_READY; + if (s->bdrv) { if (s->mem_oob) { if (bdrv_read(s->bdrv, SECTOR(addr), s->io, PAGE_SECTORS) < 0) { @@ -769,6 +781,8 @@ static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s, offset, PAGE_SIZE + OOB_SIZE - offset); s->ioaddr = s->io; } + + s->status |= NAND_IOSTATUS_READY; } static void glue(nand_init_, PAGE_SIZE)(NANDFlashState *s)