Nand Cleanup
cover Nand Cleanup
From: Peter Crosthwaite <[email protected]>
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(-)
Signature · 1 line
1.8.3.rc1.44.gb387c77.dirty
PATCH 1/5 block/nand: Factor out common code
From: Peter Crosthwaite <[email protected]>
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.
---
hw/block/nand.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
hw/block/nand.c +4 -4
@@ -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 :
Signature · 1 line
1.8.3.rc1.44.gb387c77.dirty
Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code
Quoted 19 lines · 18.06.2013 15:08, [email protected] wrote:
From: Peter Crosthwaite <[email protected]>
Most of this computation of
s->iolenis the same for both the if and else paths here. Factor out the common parts outside the if.Signed-off-by Peter Crosthwaite <[email protected]>---
hw/block/nand.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)hw/block/nand.c +0
-0@@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)break ;
hw/block/nand.c static void nand_command(NANDFlashState *s) · line 278
Quoted 10 lines
hw/block/nand.c +4
-4offset = 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
Hi Michael,
Quoted 38 lines · Michael Tokarev
Quoted 19 lines · 18.06.2013 15:08, [email protected] wrote:
From: Peter Crosthwaite <[email protected]>
Most of this computation of
s->iolenis the same for both the if and else paths here. Factor out the common parts outside the if.Signed-off-by Peter Crosthwaite <[email protected]>---
hw/block/nand.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)hw/block/nand.c +0
-0@@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)break ;hw/block/nand.c static void nand_command(NANDFlashState *s) · line 278
Quoted 10 lines
hw/block/nand.c +4
-4offset = 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->iolenbecome negative here?
addr_shiftcan be either less or greather thanpage_shift. Whenaddr_shiftis larger thanpage_shift(addr_shift=16, page_shift=11), offset may be up to 65535, in which cases->iolenmay be -63487, even without additionaloob_shift.
Quoted 9 lines
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
_shiftare unsigned, and most are actually used as masks, in form (1<<shift), instead of directly (except oferase_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
Quoted 4 lines
Thanks,
/mjt
Re: [Qemu-devel] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code
19.06.2013 12:13, Peter Crosthwaite wrote:
[]
Quoted 3 lines
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] [Qemu-trivial] [PATCH v1 1/5] block/nand: Factor out common code
In reply to Michael TokarevQuoted 38 lines · Michael Tokarev
Quoted 19 lines · 18.06.2013 15:08, [email protected] wrote:
From: Peter Crosthwaite <[email protected]>
Most of this computation of
s->iolenis the same for both the if and else paths here. Factor out the common parts outside the if.Signed-off-by Peter Crosthwaite <[email protected]>---
hw/block/nand.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)hw/block/nand.c +0
-0@@ -272,10 +272,10 @@ static void nand_command(NANDFlashState *s)break ;hw/block/nand.c static void nand_command(NANDFlashState *s) · line 278
Quoted 10 lines
hw/block/nand.c +4
-4offset = 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->iolenbecome negative here?
addr_shiftcan be either less or greather thanpage_shift. Whenaddr_shiftis larger thanpage_shift(addr_shift=16, page_shift=11), offset may be up to 65535, in which cases->iolenmay be -63487, even without additionaloob_shift.
Quoted 5 lines
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
Quoted 8 lines
And looking at this file, I think it deserves some good type changes. For example, all these
_shiftare unsigned, and most are actually used as masks, in form (1<<shift), instead of directly (except oferase_shift).Thanks,
/mjt
Re: [Qemu-devel] [PATCH v1 1/5] block/nand: Factor out common code
In reply to [email protected]Quoted 6 lines · On 18 June 2013 12:08, <[email protected]> wrote:
From: Peter Crosthwaite <[email protected]>
Most of this computation of
s->iolenis the same for both the if and else paths here. Factor out the common parts outside the if.
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
PATCH 2/5 block/nand: Formatting sweep
In reply to [email protected]From: Peter Crosthwaite <[email protected]>
Make this code closer to passing checkpatch. Mostly missing braces, but a few rogue tabs in there as well.
---
hw/block/nand.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)
hw/block/nand.c +18 -15
@@ -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) {
Signature · 1 line
1.8.3.rc1.44.gb387c77.dirty
Michael Tokarev
AppliedRe: [Qemu-devel] [Qemu-trivial] [PATCH v1 2/5] block/nand: Formatting sweep
18.06.2013 15:08, [email protected] пишет:
Quoted 4 lines
From: Peter Crosthwaite <[email protected]>
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
PATCH 3/5 block/nand: QOM casting sweep
In reply to [email protected]From: Peter Crosthwaite <[email protected]>
Define and use standard QOM cast macro. Remove usages of DO_UPCAST and direct -> style casting.
---
hw/block/nand.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
hw/block/nand.c +16 -9
@@ -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 ) {
Signature · 1 line
1.8.3.rc1.44.gb387c77.dirty
Re: [Qemu-devel] [PATCH v1 3/5] block/nand: QOM casting sweep
Am 18.06.2013 13:10, schrieb [email protected]:
Quoted 17 lines
From: Peter Crosthwaite <[email protected]>
Define and use standard QOM cast macro. Remove usages of
DO_UPCASTand direct -> style casting.Signed-off-by Peter Crosthwaite <[email protected]>---
hw/block/nand.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)hw/block/nand.c +0
-0
[...]
Quoted 7 lines
hw/block/nand.c +1
-1@@ -319,14 +324,14 @@ static void nand_command(NANDFlashState *s)static void nand_pre_save (void *opaque) { -NANDFlashState *s = opaque; +NANDFlashState *s =NAND (opaque);
hw/block/nand.c static void nand_command(NANDFlashState *s) · line 337
Quoted 10 lines
hw/block/nand.c +1
-1s->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:
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
PATCH 4/5 block/nand: Convert Sysbus::init to Device::realize
In reply to [email protected]From: Peter Crosthwaite <[email protected]>
The prescribed transition from Sysbus::init function to a Device::realize.
---
hw/block/nand.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
hw/block/nand.c +7 -9
@@ -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;
Signature · 1 line
1.8.3.rc1.44.gb387c77.dirty
Re: [Qemu-devel] [PATCH v1 4/5] block/nand: Convert Sysbus::init to Device::realize
Am 18.06.2013 13:11, schrieb [email protected]:
Quoted 5 lines
From: Peter Crosthwaite <[email protected]>
The prescribed transition from
Sysbus::initfunction to aDevice::realize.
Quoted 7 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
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.
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
PATCH 5/5 nand: Don't inherit from Sysbus
In reply to [email protected]From: Peter Crosthwaite <[email protected]>
Nand chips are not sysbus devices - they do not have any sense of MMIO, nor interrupts. Re-parent to TYPE_DEVICE accordingly.
---
hw/block/nand.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
hw/block/nand.c +4 -3
@@ -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, };
Signature · 1 line
1.8.3.rc1.44.gb387c77.dirty
Re: [Qemu-devel] [PATCH v1 5/5] nand: Don't inherit from Sysbus
Am 18.06.2013 13:12, schrieb [email protected]:
Quoted 5 lines
From: Peter Crosthwaite <[email protected]>
Nand chips are not sysbus devices - they do not have any sense of MMIO, nor interrupts. Re-parent to
TYPE_DEVICEaccordingly.
Quoted 7 lines
Signed-off-by Peter Crosthwaite <[email protected]>---
hw/block/nand.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
CPUs are bus-less devices and work fine so far, so
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Peter Maydell
AppliedQuoted 6 lines · On 18 June 2013 12:07, <[email protected]> wrote:
From: Peter Crosthwaite <[email protected]>
Misc refactorings and cleanup of NAND.
Quoted 6 lines
Peter Crosthwaite (5):
block/nand: Factor out common code block/nand: Formatting sweep
block/nand: QOM casting sweep
block/nand: ConvertSysbus::inittoDevice::realize
nand: Don't inherit from Sysbus
I've applied patches 3..5 to arm-devs.next.
Thanks
-- PMM
Quoted 7 lines · Peter Maydell
Quoted 6 lines · On 18 June 2013 12:07, <[email protected]> wrote:
From: Peter Crosthwaite <[email protected]>
Misc refactorings and cleanup of NAND.
Quoted 9 lines
Quoted 6 lines
Peter Crosthwaite (5):
block/nand: Factor out common code block/nand: Formatting sweep
block/nand: QOM casting sweep
block/nand: ConvertSysbus::inittoDevice::realize
nand: Don't inherit from SysbusI've applied patches 3..5 to arm-devs.next.
Thanks,
Regards
Peter
Quoted 3 lines
Thanks
-- PMM