Patchwork [v5,11/24] hw/nand.c: bug fix to BUSY/READY status bit

login
register
mail settings
Submitter Kuo-Jung Su
Date Feb. 27, 2013, 7:15 a.m.
Message ID <1361949350-22241-12-git-send-email-dantesu@gmail.com>
Download mbox | patch
Permalink /patch/223549/
State New
Headers show

Comments

Kuo-Jung Su - Feb. 27, 2013, 7:15 a.m.
From: Kuo-Jung Su <dantesu@faraday-tech.com>

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(-)
Peter Maydell - Feb. 28, 2013, 4:16 p.m.
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
Kuo-Jung Su - March 1, 2013, 1:13 a.m.
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

Patch

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)