Re: [Qemu-devel] [PATCH v5 11/24] hw/nand.c: bug fix to BUSY/READY status bit

[Peter Maydell]

On 27 February 2013 07:15, Kuo-Jung Su <address@hidden> wrote:
> From: Kuo-Jung Su <address@hidden>

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 <address@hidden>
> ---
>  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