[Qemu-devel] [PATCH v1 0/5] Nand Cleanup

[Qemu-devel] [PATCH v1 0/5] Nand Cleanup

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>


Misc refactorings and cleanup of NAND.


Peter Crosthwaite (5):
  block/nand: Factor out common code
  block/nand: Formatting sweep
  block/nand: QOM casting sweep
  block/nand: Convert Sysbus::init to Device::realize
  nand: Don't inherit from Sysbus

 hw/block/nand.c | 89 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 49 insertions(+), 40 deletions(-)

-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 1/5] block/nand: Factor out common code

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Most of this computation of s->iolen is the same for both the if and
else paths here. Factor out the common parts outside the if.

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/block/nand.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 087ca14..6309f93 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)
             break;
         offset = s->addr & ((1 << s->addr_shift) - 1);
         s->blk_load(s, s->addr, offset);
-        if (s->gnd)
-            s->iolen = (1 << s->page_shift) - offset;
-        else
-            s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
+        s->iolen = (1 << s->page_shift) - offset;
+        if (!s->gnd) {
+            s->iolen += 1 << s->oob_shift;
+        }
         break;
 
     case NAND_CMD_RESET:
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 2/5] block/nand: Formatting sweep

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Make this code closer to passing checkpatch. Mostly missing braces, but
a few rogue tabs in there as well.

Cc: qemu-trivial@nongnu.org

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/block/nand.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 6309f93..de0a022 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -298,10 +298,8 @@ static void nand_command(NANDFlashState *s)
 
     case NAND_CMD_BLOCKERASE2:
         s->addr &= (1ull << s->addrlen * 8) - 1;
-        if (nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP)
-            s->addr <<= 16;
-        else
-            s->addr <<= 8;
+        s->addr <<= nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP ?
+                                                                    16 : 8;
 
         if (s->wp) {
             s->blk_erase(s);
@@ -464,10 +462,11 @@ void nand_setpins(DeviceState *dev, uint8_t cle, uint8_t ale,
     s->ce = ce;
     s->wp = wp;
     s->gnd = gnd;
-    if (wp)
+    if (wp) {
         s->status |= NAND_IOSTATUS_UNPROTCT;
-    else
+    } else {
         s->status &= ~NAND_IOSTATUS_UNPROTCT;
+    }
 }
 
 void nand_getpins(DeviceState *dev, int *rb)
@@ -489,13 +488,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
                 return;
             }
         }
-        if (value == NAND_CMD_READ0)
+        if (value == NAND_CMD_READ0) {
             s->offset = 0;
-	else if (value == NAND_CMD_READ1) {
+        } else if (value == NAND_CMD_READ1) {
             s->offset = 0x100;
             value = NAND_CMD_READ0;
-        }
-	else if (value == NAND_CMD_READ2) {
+        } else if (value == NAND_CMD_READ2) {
             s->offset = 1 << s->page_shift;
             value = NAND_CMD_READ0;
         }
@@ -508,8 +506,9 @@ void nand_setio(DeviceState *dev, uint32_t value)
                 s->cmd == NAND_CMD_BLOCKERASE2 ||
                 s->cmd == NAND_CMD_NOSERIALREAD2 ||
                 s->cmd == NAND_CMD_RANDOMREAD2 ||
-                s->cmd == NAND_CMD_RESET)
+                s->cmd == NAND_CMD_RESET) {
             nand_command(s);
+        }
 
         if (s->cmd != NAND_CMD_RANDOMREAD2) {
             s->addrlen = 0;
@@ -596,8 +595,9 @@ uint32_t nand_getio(DeviceState *dev)
             s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
     }
 
-    if (s->ce || s->iolen <= 0)
+    if (s->ce || s->iolen <= 0) {
         return 0;
+    }
 
     for (offset = s->buswidth; offset--;) {
         x |= s->ioaddr[offset] << (offset << 3);
@@ -696,8 +696,9 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
     uint8_t iobuf[0x200] = { [0 ... 0x1ff] = 0xff, };
     addr = s->addr & ~((1 << (ADDR_SHIFT + s->erase_shift)) - 1);
 
-    if (PAGE(addr) >= s->pages)
+    if (PAGE(addr) >= s->pages) {
         return;
+    }
 
     if (!s->bdrv) {
         memset(s->storage + PAGE_START(addr),
@@ -725,11 +726,12 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
         memset(iobuf, 0xff, 0x200);
         i = (addr & ~0x1ff) + 0x200;
         for (addr += ((PAGE_SIZE + OOB_SIZE) << s->erase_shift) - 0x200;
-                        i < addr; i += 0x200)
+                        i < addr; i += 0x200) {
             if (bdrv_write(s->bdrv, i >> 9, iobuf, 1) < 0) {
                 printf("%s: write error in sector %" PRIu64 "\n",
                        __func__, i >> 9);
             }
+        }
 
         page = i >> 9;
         if (bdrv_read(s->bdrv, page, iobuf, 1) < 0) {
@@ -745,8 +747,9 @@ static void glue(nand_blk_erase_, PAGE_SIZE)(NANDFlashState *s)
 static void glue(nand_blk_load_, PAGE_SIZE)(NANDFlashState *s,
                 uint64_t addr, int offset)
 {
-    if (PAGE(addr) >= s->pages)
+    if (PAGE(addr) >= s->pages) {
         return;
+    }
 
     if (s->bdrv) {
         if (s->mem_oob) {
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 3/5] block/nand: QOM casting sweep

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Define and use standard QOM cast macro. Remove usages of DO_UPCAST and
direct -> style casting.

Cc: afaerber@suse.de

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/block/nand.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index de0a022..d2469f5 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -82,6 +82,11 @@ struct NANDFlashState {
     uint32_t ioaddr_vmstate;
 };
 
+#define TYPE_NAND "nand"
+
+#define NAND(obj) \
+    OBJECT_CHECK(NANDFlashState, (obj), TYPE_NAND)
+
 static void mem_and(uint8_t *dest, const uint8_t *src, size_t n)
 {
     /* Like memcpy() but we logical-AND the data into the destination */
@@ -224,7 +229,7 @@ static const struct {
 
 static void nand_reset(DeviceState *dev)
 {
-    NANDFlashState *s = FROM_SYSBUS(NANDFlashState, SYS_BUS_DEVICE(dev));
+    NANDFlashState *s = NAND(dev);
     s->cmd = NAND_CMD_READ0;
     s->addr = 0;
     s->addrlen = 0;
@@ -279,7 +284,7 @@ static void nand_command(NANDFlashState *s)
         break;
 
     case NAND_CMD_RESET:
-        nand_reset(&s->busdev.qdev);
+        nand_reset(DEVICE(s));
         break;
 
     case NAND_CMD_PAGEPROGRAM1:
@@ -319,14 +324,14 @@ static void nand_command(NANDFlashState *s)
 
 static void nand_pre_save(void *opaque)
 {
-    NANDFlashState *s = opaque;
+    NANDFlashState *s = NAND(opaque);
 
     s->ioaddr_vmstate = s->ioaddr - s->io;
 }
 
 static int nand_post_load(void *opaque, int version_id)
 {
-    NANDFlashState *s = opaque;
+    NANDFlashState *s = NAND(opaque);
 
     if (s->ioaddr_vmstate > sizeof(s->io)) {
         return -EINVAL;
@@ -365,7 +370,7 @@ static const VMStateDescription vmstate_nand = {
 static int nand_device_init(SysBusDevice *dev)
 {
     int pagesize;
-    NANDFlashState *s = FROM_SYSBUS(NANDFlashState, dev);
+    NANDFlashState *s = NAND(dev);
 
     s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
     s->size = nand_flash_ids[s->chip_id].size << 20;
@@ -436,7 +441,7 @@ static void nand_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo nand_info = {
-    .name          = "nand",
+    .name          = TYPE_NAND,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(NANDFlashState),
     .class_init    = nand_class_init,
@@ -456,7 +461,8 @@ static void nand_register_types(void)
 void nand_setpins(DeviceState *dev, uint8_t cle, uint8_t ale,
                   uint8_t ce, uint8_t wp, uint8_t gnd)
 {
-    NANDFlashState *s = (NANDFlashState *) dev;
+    NANDFlashState *s = NAND(dev);
+
     s->cle = cle;
     s->ale = ale;
     s->ce = ce;
@@ -477,7 +483,8 @@ void nand_getpins(DeviceState *dev, int *rb)
 void nand_setio(DeviceState *dev, uint32_t value)
 {
     int i;
-    NANDFlashState *s = (NANDFlashState *) dev;
+    NANDFlashState *s = NAND(dev);
+
     if (!s->ce && s->cle) {
         if (nand_flash_ids[s->chip_id].options & NAND_SAMSUNG_LP) {
             if (s->cmd == NAND_CMD_READ0 && value == NAND_CMD_LPREAD2)
@@ -581,7 +588,7 @@ uint32_t nand_getio(DeviceState *dev)
 {
     int offset;
     uint32_t x = 0;
-    NANDFlashState *s = (NANDFlashState *) dev;
+    NANDFlashState *s = NAND(dev);
 
     /* Allow sequential reading */
     if (!s->iolen && s->cmd == NAND_CMD_READ0) {
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 4/5] block/nand: Convert Sysbus::init to Device::realize

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

The prescribed transition from Sysbus::init function to a
Device::realize.

Cc: afaerber@suse.de

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/block/nand.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index d2469f5..8dca3bc 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -367,7 +367,7 @@ static const VMStateDescription vmstate_nand = {
     }
 };
 
-static int nand_device_init(SysBusDevice *dev)
+static void nand_realize(DeviceState *dev, Error **errp)
 {
     int pagesize;
     NANDFlashState *s = NAND(dev);
@@ -393,16 +393,17 @@ static int nand_device_init(SysBusDevice *dev)
         nand_init_2048(s);
         break;
     default:
-        error_report("Unsupported NAND block size");
-        return -1;
+        error_setg(errp, "Unsupported NAND block size %#x\n",
+                   1 << s->page_shift);
+        return;
     }
 
     pagesize = 1 << s->oob_shift;
     s->mem_oob = 1;
     if (s->bdrv) {
         if (bdrv_is_read_only(s->bdrv)) {
-            error_report("Can't use a read-only drive");
-            return -1;
+            error_setg(errp, "Can't use a read-only drive");
+            return;
         }
         if (bdrv_getlength(s->bdrv) >=
                 (s->pages << s->page_shift) + (s->pages << s->oob_shift)) {
@@ -418,8 +419,6 @@ static int nand_device_init(SysBusDevice *dev)
     }
     /* Give s->ioaddr a sane value in case we save state before it is used. */
     s->ioaddr = s->io;
-
-    return 0;
 }
 
 static Property nand_properties[] = {
@@ -432,9 +431,8 @@ static Property nand_properties[] = {
 static void nand_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = nand_device_init;
+    dc->realize = nand_realize;
     dc->reset = nand_reset;
     dc->vmsd = &vmstate_nand;
     dc->props = nand_properties;
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH v1 5/5] nand: Don't inherit from Sysbus

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Nand chips are not sysbus devices - they do not have any sense of MMIO,
nor interrupts. Re-parent to TYPE_DEVICE accordingly.

Cc: afaerber@suse.de

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/block/nand.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/block/nand.c b/hw/block/nand.c
index 8dca3bc..072de7c 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -21,7 +21,7 @@
 # include "hw/hw.h"
 # include "hw/block/flash.h"
 # include "sysemu/blockdev.h"
-# include "hw/sysbus.h"
+#include "hw/qdev.h"
 #include "qemu/error-report.h"
 
 # define NAND_CMD_READ0		0x00
@@ -54,7 +54,8 @@
 
 typedef struct NANDFlashState NANDFlashState;
 struct NANDFlashState {
-    SysBusDevice busdev;
+    DeviceState parent_obj;
+
     uint8_t manf_id, chip_id;
     uint8_t buswidth; /* in BYTES */
     int size, pages;
@@ -440,7 +441,7 @@ static void nand_class_init(ObjectClass *klass, void *data)
 
 static const TypeInfo nand_info = {
     .name          = TYPE_NAND,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_DEVICE,
     .instance_size = sizeof(NANDFlashState),
     .class_init    = nand_class_init,
 };
-- 
1.8.3.rc1.44.gb387c77.dirty

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 2/5] block/nand: Formatting sweep

[Michael Tokarev]

18.06.2013 15:08, peter.crosthwaite@xilinx.com пишет:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Make this code closer to passing checkpatch. Mostly missing braces, but
> a few rogue tabs in there as well.

Thanks, applied to the trivial patches queue.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code

[Michael Tokarev]

18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Most of this computation of s->iolen is the same for both the if and
> else paths here. Factor out the common parts outside the if.
> 
> Cc: qemu-trivial@nongnu.org
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/block/nand.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 087ca14..6309f93 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)
>              break;
>          offset = s->addr & ((1 << s->addr_shift) - 1);
>          s->blk_load(s, s->addr, offset);
> -        if (s->gnd)
> -            s->iolen = (1 << s->page_shift) - offset;
> -        else
> -            s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
> +        s->iolen = (1 << s->page_shift) - offset;
> +        if (!s->gnd) {
> +            s->iolen += 1 << s->oob_shift;
> +        }

Hm.  Can s->iolen become negative here?

addr_shift can be either less or greather than page_shift.  When addr_shift
is larger than page_shift (addr_shift=16, page_shift=11), offset may be up
to 65535, in which case s->iolen may be -63487, even without additional
oob_shift.

I dunno if this is a concern, just.. asking.

Besides, exactly the same expression is used in nand_getio() down this file,
maybe a common function is in order? :)

And looking at this file, I think it deserves some good type changes.  For
example, all these _shift are unsigned, and most are actually used as masks,
in form (1<<shift), instead of directly (except of erase_shift).

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code

[Peter Crosthwaite]

Hi Michael,

On Wed, Jun 19, 2013 at 6:09 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Most of this computation of s->iolen is the same for both the if and
>> else paths here. Factor out the common parts outside the if.
>>
>> Cc: qemu-trivial@nongnu.org
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/block/nand.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/block/nand.c b/hw/block/nand.c
>> index 087ca14..6309f93 100644
>> --- a/hw/block/nand.c
>> +++ b/hw/block/nand.c
>> @@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)
>>              break;
>>          offset = s->addr & ((1 << s->addr_shift) - 1);
>>          s->blk_load(s, s->addr, offset);
>> -        if (s->gnd)
>> -            s->iolen = (1 << s->page_shift) - offset;
>> -        else
>> -            s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
>> +        s->iolen = (1 << s->page_shift) - offset;
>> +        if (!s->gnd) {
>> +            s->iolen += 1 << s->oob_shift;
>> +        }
>
> Hm.  Can s->iolen become negative here?
>
> addr_shift can be either less or greather than page_shift.  When addr_shift
> is larger than page_shift (addr_shift=16, page_shift=11), offset may be up
> to 65535, in which case s->iolen may be -63487, even without additional
> oob_shift.
>
> I dunno if this is a concern, just.. asking.
>
> Besides, exactly the same expression is used in nand_getio() down this file,
> maybe a common function is in order? :)
>
> And looking at this file, I think it deserves some good type changes.  For
> example, all these _shift are unsigned, and most are actually used as masks,
> in form (1<<shift), instead of directly (except of erase_shift).
>

All sounds fair. Ill have another look in the context of type
changing, and factoring out some commonality. Your able to take P2
without conflict? :)

Regards,
Peter

> Thanks,
>
> /mjt
>

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code

[Michael Tokarev]

19.06.2013 12:13, Peter Crosthwaite wrote:
[]

> All sounds fair. Ill have another look in the context of type
> changing, and factoring out some commonality. Your able to take P2
> without conflict? :)

Thanks!

And sure, either with or without your next reformatting
(block/nand: Formatting sweep) -- either on top of it or
without it -- is okay.

/mjt

Re: [Qemu-devel] [PATCH v1 1/5] block/nand: Factor out common code

[Peter Maydell]

On 18 June 2013 12:08,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> Most of this computation of s->iolen is the same for both the if and
> else paths here. Factor out the common parts outside the if.
>
> Cc: qemu-trivial@nongnu.org

Please don't cc qemu-trivial on half a patchset: it makes
it awkward to commit to a submaintainer tree if half of
it has already gone somewhere else.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 3/5] block/nand: QOM casting sweep

[Andreas Färber]

Am 18.06.2013 13:10, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Define and use standard QOM cast macro. Remove usages of DO_UPCAST and
> direct -> style casting.
> 
> Cc: afaerber@suse.de
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/block/nand.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index de0a022..d2469f5 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
[...]
> @@ -319,14 +324,14 @@ static void nand_command(NANDFlashState *s)
>  
>  static void nand_pre_save(void *opaque)
>  {
> -    NANDFlashState *s = opaque;
> +    NANDFlashState *s = NAND(opaque);
>  
>      s->ioaddr_vmstate = s->ioaddr - s->io;
>  }
>  
>  static int nand_post_load(void *opaque, int version_id)
>  {
> -    NANDFlashState *s = opaque;
> +    NANDFlashState *s = NAND(opaque);
>  
>      if (s->ioaddr_vmstate > sizeof(s->io)) {
>          return -EINVAL;
[snip]

These two are not strictly necessary, but either way:

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v1 4/5] block/nand: Convert Sysbus::init to Device::realize

[Andreas Färber]

Am 18.06.2013 13:11, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> The prescribed transition from Sysbus::init function to a
> Device::realize.
> 
> Cc: afaerber@suse.de
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/block/nand.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)

Due to chip_id property we can't seem to split anything off from realize.

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v1 5/5] nand: Don't inherit from Sysbus

[Andreas Färber]

Am 18.06.2013 13:12, schrieb peter.crosthwaite@xilinx.com:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Nand chips are not sysbus devices - they do not have any sense of MMIO,
> nor interrupts. Re-parent to TYPE_DEVICE accordingly.
> 
> Cc: afaerber@suse.de
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/block/nand.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

CPUs are bus-less devices and work fine so far, so

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v1 0/5] Nand Cleanup

[Peter Maydell]

On 18 June 2013 12:07,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
>
> Misc refactorings and cleanup of NAND.
>
>
> Peter Crosthwaite (5):
>   block/nand: Factor out common code
>   block/nand: Formatting sweep
>   block/nand: QOM casting sweep
>   block/nand: Convert Sysbus::init to Device::realize
>   nand: Don't inherit from Sysbus

I've applied patches 3..5 to arm-devs.next.

Thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 0/5] Nand Cleanup

[Peter Crosthwaite]

On Wed, Jun 26, 2013 at 4:16 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 June 2013 12:07,  <peter.crosthwaite@xilinx.com> wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>>
>> Misc refactorings and cleanup of NAND.
>>
>>
>> Peter Crosthwaite (5):
>>   block/nand: Factor out common code
>>   block/nand: Formatting sweep
>>   block/nand: QOM casting sweep
>>   block/nand: Convert Sysbus::init to Device::realize
>>   nand: Don't inherit from Sysbus
>
> I've applied patches 3..5 to arm-devs.next.
>

Thanks,

Regards
Peter

> Thanks
> -- PMM
>

Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code

[Peter Crosthwaite]

On Wed, Jun 19, 2013 at 6:09 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 18.06.2013 15:08, peter.crosthwaite@xilinx.com wrote:
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Most of this computation of s->iolen is the same for both the if and
>> else paths here. Factor out the common parts outside the if.
>>
>> Cc: qemu-trivial@nongnu.org
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/block/nand.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/block/nand.c b/hw/block/nand.c
>> index 087ca14..6309f93 100644
>> --- a/hw/block/nand.c
>> +++ b/hw/block/nand.c
>> @@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)
>>              break;
>>          offset = s->addr & ((1 << s->addr_shift) - 1);
>>          s->blk_load(s, s->addr, offset);
>> -        if (s->gnd)
>> -            s->iolen = (1 << s->page_shift) - offset;
>> -        else
>> -            s->iolen = (1 << s->page_shift) + (1 << s->oob_shift) - offset;
>> +        s->iolen = (1 << s->page_shift) - offset;
>> +        if (!s->gnd) {
>> +            s->iolen += 1 << s->oob_shift;
>> +        }
>
> Hm.  Can s->iolen become negative here?
>
> addr_shift can be either less or greather than page_shift.  When addr_shift
> is larger than page_shift (addr_shift=16, page_shift=11), offset may be up
> to 65535, in which case s->iolen may be -63487, even without additional
> oob_shift.
>
> I dunno if this is a concern, just.. asking.
>
> Besides, exactly the same expression is used in nand_getio() down this file,
> maybe a common function is in order? :)
>

Factored this out. Just added it to nand_blk_load_XXX().

Regards,
Peter

> And looking at this file, I think it deserves some good type changes.  For
> example, all these _shift are unsigned, and most are actually used as masks,
> in form (1<<shift), instead of directly (except of erase_shift).
>
> Thanks,
>
> /mjt
>