* [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model
@ 2016-12-16 13:27 marcin.krzeminski
2016-12-16 13:27 ` [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op marcin.krzeminski
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: marcin.krzeminski @ 2016-12-16 13:27 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, peter.maydell, rfsw-patches, clg
From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
This series introduce some improvememnt targeting mt25qu01g.
Marcin Krzeminski (2):
block: m25p80: Add Quad Page Program 4byte version op
block: m25p80: Introduce Die Erase command
hw/block/m25p80.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op 2016-12-16 13:27 [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model marcin.krzeminski @ 2016-12-16 13:27 ` marcin.krzeminski 2016-12-16 16:18 ` Edgar E. Iglesias 2016-12-16 13:27 ` [Qemu-devel] [PATCH 2/2] block: m25p80: Introduce Die Erase command marcin.krzeminski 2016-12-16 16:09 ` [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model no-reply 2 siblings, 1 reply; 9+ messages in thread From: marcin.krzeminski @ 2016-12-16 13:27 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-arm, peter.maydell, rfsw-patches, clg From: Marcin Krzeminski <marcin.krzeminski@nokia.com> Some flash chips has additional page program opcode that takes only 4 byte address. This commit adds support for such command in Qemu. Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> --- hw/block/m25p80.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index d29ff4c..2bc7028 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -325,6 +325,7 @@ typedef enum { PP4_4 = 0x3e, DPP = 0xa2, QPP = 0x32, + QPP_4 = 0x34, ERASE_4K = 0x20, ERASE4_4K = 0x21, @@ -573,6 +574,7 @@ static inline int get_addr_length(Flash *s) switch (s->cmd_in_progress) { case PP4: case PP4_4: + case QPP_4: case READ4: case QIOR4: case ERASE4_4K: @@ -606,6 +608,7 @@ static void complete_collecting_data(Flash *s) switch (s->cmd_in_progress) { case DPP: case QPP: + case QPP_4: case PP: case PP4: case PP4_4: @@ -873,6 +876,7 @@ static void decode_new_cmd(Flash *s, uint32_t value) case READ4: case DPP: case QPP: + case QPP_4: case PP: case PP4: case PP4_4: -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op 2016-12-16 13:27 ` [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op marcin.krzeminski @ 2016-12-16 16:18 ` Edgar E. Iglesias 0 siblings, 0 replies; 9+ messages in thread From: Edgar E. Iglesias @ 2016-12-16 16:18 UTC (permalink / raw) To: marcin.krzeminski; +Cc: qemu-devel, peter.maydell, rfsw-patches, qemu-arm, clg On Fri, Dec 16, 2016 at 02:27:41PM +0100, marcin.krzeminski@nokia.com wrote: > From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > Some flash chips has additional page program opcode that > takes only 4 byte address. This commit adds support > for such command in Qemu. > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > hw/block/m25p80.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index d29ff4c..2bc7028 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -325,6 +325,7 @@ typedef enum { > PP4_4 = 0x3e, > DPP = 0xa2, > QPP = 0x32, > + QPP_4 = 0x34, > > ERASE_4K = 0x20, > ERASE4_4K = 0x21, > @@ -573,6 +574,7 @@ static inline int get_addr_length(Flash *s) > switch (s->cmd_in_progress) { > case PP4: > case PP4_4: > + case QPP_4: > case READ4: > case QIOR4: > case ERASE4_4K: > @@ -606,6 +608,7 @@ static void complete_collecting_data(Flash *s) > switch (s->cmd_in_progress) { > case DPP: > case QPP: > + case QPP_4: > case PP: > case PP4: > case PP4_4: > @@ -873,6 +876,7 @@ static void decode_new_cmd(Flash *s, uint32_t value) > case READ4: > case DPP: > case QPP: > + case QPP_4: > case PP: > case PP4: > case PP4_4: > -- > 2.7.4 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/2] block: m25p80: Introduce Die Erase command 2016-12-16 13:27 [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model marcin.krzeminski 2016-12-16 13:27 ` [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op marcin.krzeminski @ 2016-12-16 13:27 ` marcin.krzeminski 2016-12-16 16:35 ` [Qemu-devel] [Qemu-arm] " Edgar E. Iglesias 2016-12-16 16:09 ` [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model no-reply 2 siblings, 1 reply; 9+ messages in thread From: marcin.krzeminski @ 2016-12-16 13:27 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-arm, peter.maydell, rfsw-patches, clg From: Marcin Krzeminski <marcin.krzeminski@nokia.com> Big flash chips (like mt25qu01g) are consisted from dies. Because of that some manufactures remove support for Chip Erase giving Die Erase command instead.To avoid unnecessary code complication, support for chip erase for mt25qu01g is not removed. Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> --- hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 2bc7028..0bc1fbf 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = { { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, - { INFO("mt25ql01g", 0x20ba21, 0, 64 << 10, 2048, ER_4K) }, - { INFO("mt25qu01g", 0x20bb21, 0, 64 << 10, 2048, ER_4K) }, + { INFO("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K) }, + { INFO("mt25qu01g", 0x20bb21, 0x1040, 64 << 10, 2048, ER_4K) }, /* Spansion -- single (large) sector size only, at least * for the chips listed here (without boot sectors). @@ -358,6 +358,8 @@ typedef enum { REVCR = 0x65, WEVCR = 0x61, + + DIE_ERASE = 0xC4, } FlashCMD; typedef enum { @@ -411,6 +413,7 @@ typedef struct Flash { bool reset_enable; bool quad_enable; uint8_t ear; + uint32_t die_cnt; int64_t dirty_page; @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, int64_t off, int64_t len) static void flash_erase(Flash *s, int offset, FlashCMD cmd) { - uint32_t len; + uint32_t len = 0; uint8_t capa_to_assert = 0; switch (cmd) { @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd) case BULK_ERASE: len = s->size; break; + case DIE_ERASE: + if (s->die_cnt) { + len = s->size / s->die_cnt; + offset = offset & (~(len-1)); + } else { + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not supported by" + " device\n", len); + return; + } + break; default: abort(); } @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s) case ERASE4_32K: case ERASE_SECTOR: case ERASE4_SECTOR: + case DIE_ERASE: flash_erase(s, s->cur_addr, s->cmd_in_progress); break; case WRSR: @@ -684,6 +698,7 @@ static void reset_memory(Flash *s) s->write_enable = false; s->reset_enable = false; s->quad_enable = false; + s->die_cnt = 0; switch (get_man(s)) { case MAN_NUMONYX: @@ -716,7 +731,15 @@ static void reset_memory(Flash *s) s->four_bytes_address_mode = true; } if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) { - s->ear = s->size / MAX_3BYTES_SIZE - 1; + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0); + } + /* 1GiB devices */ + if (s->pi->id[2] == 0x21) { + /* MT25Q00 has 2 dies N25Q00 has 4 */ + if (s->pi->id[4] & BIT(6)) + s->die_cnt = 2; + else + s->die_cnt = 4; } break; case MAN_MACRONIX: @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t value) case PP: case PP4: case PP4_4: + case DIE_ERASE: s->needed_bytes = get_addr_length(s); s->pos = 0; s->len = 0; @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80 = { VMSTATE_UINT8(spansion_cr2nv, Flash), VMSTATE_UINT8(spansion_cr3nv, Flash), VMSTATE_UINT8(spansion_cr4nv, Flash), + VMSTATE_UINT32(die_cnt, Flash), VMSTATE_END_OF_LIST() } }; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command 2016-12-16 13:27 ` [Qemu-devel] [PATCH 2/2] block: m25p80: Introduce Die Erase command marcin.krzeminski @ 2016-12-16 16:35 ` Edgar E. Iglesias 2016-12-19 6:21 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) 0 siblings, 1 reply; 9+ messages in thread From: Edgar E. Iglesias @ 2016-12-16 16:35 UTC (permalink / raw) To: marcin.krzeminski; +Cc: qemu-devel, peter.maydell, rfsw-patches, qemu-arm, clg On Fri, Dec 16, 2016 at 02:27:42PM +0100, marcin.krzeminski@nokia.com wrote: > From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > Big flash chips (like mt25qu01g) are consisted from dies. > Because of that some manufactures remove support for Chip > Erase giving Die Erase command instead.To avoid unnecessary > code complication, support for chip erase for mt25qu01g > is not removed. > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > --- > hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++---- > 1 file changed, 29 insertions(+), 4 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index 2bc7028..0bc1fbf 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = { > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > - { INFO("mt25ql01g", 0x20ba21, 0, 64 << 10, 2048, ER_4K) }, > - { INFO("mt25qu01g", 0x20bb21, 0, 64 << 10, 2048, ER_4K) }, > + { INFO("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K) }, > + { INFO("mt25qu01g", 0x20bb21, 0x1040, 64 << 10, 2048, ER_4K) }, > > /* Spansion -- single (large) sector size only, at least > * for the chips listed here (without boot sectors). > @@ -358,6 +358,8 @@ typedef enum { > > REVCR = 0x65, > WEVCR = 0x61, > + > + DIE_ERASE = 0xC4, > } FlashCMD; > > typedef enum { > @@ -411,6 +413,7 @@ typedef struct Flash { > bool reset_enable; > bool quad_enable; > uint8_t ear; > + uint32_t die_cnt; > > int64_t dirty_page; > > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, int64_t off, int64_t len) > > static void flash_erase(Flash *s, int offset, FlashCMD cmd) > { > - uint32_t len; > + uint32_t len = 0; Do you really need this? > uint8_t capa_to_assert = 0; > > switch (cmd) { > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd) > case BULK_ERASE: > len = s->size; > break; > + case DIE_ERASE: > + if (s->die_cnt) { > + len = s->size / s->die_cnt; > + offset = offset & (~(len-1)); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not supported by" > + " device\n", len); > + return; > + } > + break; > default: > abort(); > } > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s) > case ERASE4_32K: > case ERASE_SECTOR: > case ERASE4_SECTOR: > + case DIE_ERASE: > flash_erase(s, s->cur_addr, s->cmd_in_progress); > break; > case WRSR: > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s) > s->write_enable = false; > s->reset_enable = false; > s->quad_enable = false; > + s->die_cnt = 0; > > switch (get_man(s)) { > case MAN_NUMONYX: > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s) > s->four_bytes_address_mode = true; > } > if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) { > - s->ear = s->size / MAX_3BYTES_SIZE - 1; > + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0); > + } > + /* 1GiB devices */ > + if (s->pi->id[2] == 0x21) { > + /* MT25Q00 has 2 dies N25Q00 has 4 */ > + if (s->pi->id[4] & BIT(6)) > + s->die_cnt = 2; > + else > + s->die_cnt = 4; Decoding of die_cnt should probably be done in realize(). > } > break; > case MAN_MACRONIX: > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t value) > case PP: > case PP4: > case PP4_4: > + case DIE_ERASE: > s->needed_bytes = get_addr_length(s); > s->pos = 0; > s->len = 0; > @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80 = { > VMSTATE_UINT8(spansion_cr2nv, Flash), > VMSTATE_UINT8(spansion_cr3nv, Flash), > VMSTATE_UINT8(spansion_cr4nv, Flash), > + VMSTATE_UINT32(die_cnt, Flash), die_cnt is fixed per instance. Since it doesn't change, it doesn't need to be part of VMSTATE. > VMSTATE_END_OF_LIST() > } > }; > -- > 2.7.4 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command 2016-12-16 16:35 ` [Qemu-devel] [Qemu-arm] " Edgar E. Iglesias @ 2016-12-19 6:21 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) 2016-12-19 7:28 ` Edgar E. Iglesias 0 siblings, 1 reply; 9+ messages in thread From: Krzeminski, Marcin (Nokia - PL/Wroclaw) @ 2016-12-19 6:21 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-devel, peter.maydell, rfsw-patches, qemu-arm, clg > -----Original Message----- > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > Sent: Friday, December 16, 2016 5:36 PM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > <marcin.krzeminski@nokia.com> > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw- > patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org > Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase > command > > On Fri, Dec 16, 2016 at 02:27:42PM +0100, marcin.krzeminski@nokia.com > wrote: > > From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > > > Big flash chips (like mt25qu01g) are consisted from dies. > > Because of that some manufactures remove support for Chip Erase giving > > Die Erase command instead.To avoid unnecessary code complication, > > support for chip erase for mt25qu01g is not removed. > > > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > --- > > hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++---- > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > > 2bc7028..0bc1fbf 100644 > > --- a/hw/block/m25p80.c > > +++ b/hw/block/m25p80.c > > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = { > > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > > { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > > { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > > - { INFO("mt25ql01g", 0x20ba21, 0, 64 << 10, 2048, ER_4K) }, > > - { INFO("mt25qu01g", 0x20bb21, 0, 64 << 10, 2048, ER_4K) }, > > + { INFO("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K) }, > > + { INFO("mt25qu01g", 0x20bb21, 0x1040, 64 << 10, 2048, ER_4K) }, > > > > /* Spansion -- single (large) sector size only, at least > > * for the chips listed here (without boot sectors). > > @@ -358,6 +358,8 @@ typedef enum { > > > > REVCR = 0x65, > > WEVCR = 0x61, > > + > > + DIE_ERASE = 0xC4, > > } FlashCMD; > > > > typedef enum { > > @@ -411,6 +413,7 @@ typedef struct Flash { > > bool reset_enable; > > bool quad_enable; > > uint8_t ear; > > + uint32_t die_cnt; > > > > int64_t dirty_page; > > > > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, > > int64_t off, int64_t len) > > > > static void flash_erase(Flash *s, int offset, FlashCMD cmd) { > > - uint32_t len; > > + uint32_t len = 0; > > Do you really need this? Compilation warning. > > > uint8_t capa_to_assert = 0; > > > > switch (cmd) { > > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, > FlashCMD cmd) > > case BULK_ERASE: > > len = s->size; > > break; > > + case DIE_ERASE: > > + if (s->die_cnt) { > > + len = s->size / s->die_cnt; > > + offset = offset & (~(len-1)); > > + } else { > > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not > supported by" > > + " device\n", len); > > + return; > > + } > > + break; > > default: > > abort(); > > } > > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s) > > case ERASE4_32K: > > case ERASE_SECTOR: > > case ERASE4_SECTOR: > > + case DIE_ERASE: > > flash_erase(s, s->cur_addr, s->cmd_in_progress); > > break; > > case WRSR: > > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s) > > s->write_enable = false; > > s->reset_enable = false; > > s->quad_enable = false; > > + s->die_cnt = 0; > > > > switch (get_man(s)) { > > case MAN_NUMONYX: > > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s) > > s->four_bytes_address_mode = true; > > } > > if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) { > > - s->ear = s->size / MAX_3BYTES_SIZE - 1; > > + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE > - 1 : 0); > > + } > > + /* 1GiB devices */ > > + if (s->pi->id[2] == 0x21) { > > + /* MT25Q00 has 2 dies N25Q00 has 4 */ > > + if (s->pi->id[4] & BIT(6)) > > + s->die_cnt = 2; > > + else > > + s->die_cnt = 4; > > Decoding of die_cnt should probably be done in realize(). > > > } > > break; > > case MAN_MACRONIX: > > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t > value) > > case PP: > > case PP4: > > case PP4_4: > > + case DIE_ERASE: > > s->needed_bytes = get_addr_length(s); > > s->pos = 0; > > s->len = 0; > > @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80 > = { > > VMSTATE_UINT8(spansion_cr2nv, Flash), > > VMSTATE_UINT8(spansion_cr3nv, Flash), > > VMSTATE_UINT8(spansion_cr4nv, Flash), > > + VMSTATE_UINT32(die_cnt, Flash), > > die_cnt is fixed per instance. Since it doesn't change, it doesn't need to be > part of VMSTATE. Yes, you are right. Even more, number of a dies is fixed attribute of chip, so it could Be a part of FlashPartInfo. I'll up v2. Thanks, Marcin > > > VMSTATE_END_OF_LIST() > > } > > }; > > -- > > 2.7.4 > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command 2016-12-19 6:21 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) @ 2016-12-19 7:28 ` Edgar E. Iglesias 2016-12-19 7:31 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) 0 siblings, 1 reply; 9+ messages in thread From: Edgar E. Iglesias @ 2016-12-19 7:28 UTC (permalink / raw) To: Krzeminski, Marcin (Nokia - PL/Wroclaw) Cc: qemu-devel, peter.maydell, rfsw-patches, qemu-arm, clg On Mon, Dec 19, 2016 at 06:21:13AM +0000, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote: > > > > -----Original Message----- > > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > > Sent: Friday, December 16, 2016 5:36 PM > > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > > <marcin.krzeminski@nokia.com> > > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw- > > patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org > > Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase > > command > > > > On Fri, Dec 16, 2016 at 02:27:42PM +0100, marcin.krzeminski@nokia.com > > wrote: > > > From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > > > > > Big flash chips (like mt25qu01g) are consisted from dies. > > > Because of that some manufactures remove support for Chip Erase giving > > > Die Erase command instead.To avoid unnecessary code complication, > > > support for chip erase for mt25qu01g is not removed. > > > > > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > > --- > > > hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++---- > > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > > > 2bc7028..0bc1fbf 100644 > > > --- a/hw/block/m25p80.c > > > +++ b/hw/block/m25p80.c > > > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = { > > > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > > > { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > > > { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > > > - { INFO("mt25ql01g", 0x20ba21, 0, 64 << 10, 2048, ER_4K) }, > > > - { INFO("mt25qu01g", 0x20bb21, 0, 64 << 10, 2048, ER_4K) }, > > > + { INFO("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K) }, > > > + { INFO("mt25qu01g", 0x20bb21, 0x1040, 64 << 10, 2048, ER_4K) }, > > > > > > /* Spansion -- single (large) sector size only, at least > > > * for the chips listed here (without boot sectors). > > > @@ -358,6 +358,8 @@ typedef enum { > > > > > > REVCR = 0x65, > > > WEVCR = 0x61, > > > + > > > + DIE_ERASE = 0xC4, > > > } FlashCMD; > > > > > > typedef enum { > > > @@ -411,6 +413,7 @@ typedef struct Flash { > > > bool reset_enable; > > > bool quad_enable; > > > uint8_t ear; > > > + uint32_t die_cnt; > > > > > > int64_t dirty_page; > > > > > > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, > > > int64_t off, int64_t len) > > > > > > static void flash_erase(Flash *s, int offset, FlashCMD cmd) { > > > - uint32_t len; > > > + uint32_t len = 0; > > > > Do you really need this? > > Compilation warning. Of course, because you are logging len in a code path that doesn't use the variable. Let me be more clear below: > > > > > uint8_t capa_to_assert = 0; > > > > > > switch (cmd) { > > > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, > > FlashCMD cmd) > > > case BULK_ERASE: > > > len = s->size; > > > break; > > > + case DIE_ERASE: > > > + if (s->die_cnt) { > > > + len = s->size / s->die_cnt; > > > + offset = offset & (~(len-1)); > > > + } else { > > > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not > > supported by" > > > + " device\n", len); Don't log len here... > > > + return; > > > + } > > > + break; > > > default: > > > abort(); > > > } > > > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s) > > > case ERASE4_32K: > > > case ERASE_SECTOR: > > > case ERASE4_SECTOR: > > > + case DIE_ERASE: > > > flash_erase(s, s->cur_addr, s->cmd_in_progress); > > > break; > > > case WRSR: > > > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s) > > > s->write_enable = false; > > > s->reset_enable = false; > > > s->quad_enable = false; > > > + s->die_cnt = 0; > > > > > > switch (get_man(s)) { > > > case MAN_NUMONYX: > > > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s) > > > s->four_bytes_address_mode = true; > > > } > > > if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) { > > > - s->ear = s->size / MAX_3BYTES_SIZE - 1; > > > + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE > > - 1 : 0); > > > + } > > > + /* 1GiB devices */ > > > + if (s->pi->id[2] == 0x21) { > > > + /* MT25Q00 has 2 dies N25Q00 has 4 */ > > > + if (s->pi->id[4] & BIT(6)) > > > + s->die_cnt = 2; > > > + else > > > + s->die_cnt = 4; > > > > Decoding of die_cnt should probably be done in realize(). > > > > > } > > > break; > > > case MAN_MACRONIX: > > > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t > > value) > > > case PP: > > > case PP4: > > > case PP4_4: > > > + case DIE_ERASE: > > > s->needed_bytes = get_addr_length(s); > > > s->pos = 0; > > > s->len = 0; > > > @@ -1217,6 +1241,7 @@ static const VMStateDescription vmstate_m25p80 > > = { > > > VMSTATE_UINT8(spansion_cr2nv, Flash), > > > VMSTATE_UINT8(spansion_cr3nv, Flash), > > > VMSTATE_UINT8(spansion_cr4nv, Flash), > > > + VMSTATE_UINT32(die_cnt, Flash), > > > > die_cnt is fixed per instance. Since it doesn't change, it doesn't need to be > > part of VMSTATE. > > Yes, you are right. Even more, number of a dies is fixed attribute of chip, so it could > Be a part of FlashPartInfo. I'll up v2. > > Thanks, > Marcin > > > > > > VMSTATE_END_OF_LIST() > > > } > > > }; > > > -- > > > 2.7.4 > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase command 2016-12-19 7:28 ` Edgar E. Iglesias @ 2016-12-19 7:31 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) 0 siblings, 0 replies; 9+ messages in thread From: Krzeminski, Marcin (Nokia - PL/Wroclaw) @ 2016-12-19 7:31 UTC (permalink / raw) To: Edgar E. Iglesias; +Cc: qemu-devel, peter.maydell, rfsw-patches, qemu-arm, clg > -----Original Message----- > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > Sent: Monday, December 19, 2016 8:28 AM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > <marcin.krzeminski@nokia.com> > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw- > patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org > Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die Erase > command > > On Mon, Dec 19, 2016 at 06:21:13AM +0000, Krzeminski, Marcin (Nokia - > PL/Wroclaw) wrote: > > > > > > > -----Original Message----- > > > From: Edgar E. Iglesias [mailto:edgar.iglesias@gmail.com] > > > Sent: Friday, December 16, 2016 5:36 PM > > > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > > > <marcin.krzeminski@nokia.com> > > > Cc: qemu-devel@nongnu.org; peter.maydell@linaro.org; rfsw- > > > patches@mlist.nokia.com; qemu-arm@nongnu.org; clg@kaod.org > > > Subject: Re: [Qemu-arm] [PATCH 2/2] block: m25p80: Introduce Die > > > Erase command > > > > > > On Fri, Dec 16, 2016 at 02:27:42PM +0100, > > > marcin.krzeminski@nokia.com > > > wrote: > > > > From: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > > > > > > > Big flash chips (like mt25qu01g) are consisted from dies. > > > > Because of that some manufactures remove support for Chip Erase > > > > giving Die Erase command instead.To avoid unnecessary code > > > > complication, support for chip erase for mt25qu01g is not removed. > > > > > > > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > > > --- > > > > hw/block/m25p80.c | 33 +++++++++++++++++++++++++++++---- > > > > 1 file changed, 29 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > > > > 2bc7028..0bc1fbf 100644 > > > > --- a/hw/block/m25p80.c > > > > +++ b/hw/block/m25p80.c > > > > @@ -216,8 +216,8 @@ static const FlashPartInfo known_devices[] = { > > > > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > > > > { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > > > > { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > > > > - { INFO("mt25ql01g", 0x20ba21, 0, 64 << 10, 2048, ER_4K) }, > > > > - { INFO("mt25qu01g", 0x20bb21, 0, 64 << 10, 2048, ER_4K) }, > > > > + { INFO("mt25ql01g", 0x20ba21, 0x1040, 64 << 10, 2048, ER_4K) }, > > > > + { INFO("mt25qu01g", 0x20bb21, 0x1040, 64 << 10, 2048, ER_4K) }, > > > > > > > > /* Spansion -- single (large) sector size only, at least > > > > * for the chips listed here (without boot sectors). > > > > @@ -358,6 +358,8 @@ typedef enum { > > > > > > > > REVCR = 0x65, > > > > WEVCR = 0x61, > > > > + > > > > + DIE_ERASE = 0xC4, > > > > } FlashCMD; > > > > > > > > typedef enum { > > > > @@ -411,6 +413,7 @@ typedef struct Flash { > > > > bool reset_enable; > > > > bool quad_enable; > > > > uint8_t ear; > > > > + uint32_t die_cnt; > > > > > > > > int64_t dirty_page; > > > > > > > > @@ -492,7 +495,7 @@ static inline void flash_sync_area(Flash *s, > > > > int64_t off, int64_t len) > > > > > > > > static void flash_erase(Flash *s, int offset, FlashCMD cmd) { > > > > - uint32_t len; > > > > + uint32_t len = 0; > > > > > > Do you really need this? > > > > Compilation warning. > > Of course, because you are logging len in a code path that doesn't use the > variable. > > Let me be more clear below: > > > > > > > > uint8_t capa_to_assert = 0; > > > > > > > > switch (cmd) { > > > > @@ -513,6 +516,16 @@ static void flash_erase(Flash *s, int offset, > > > FlashCMD cmd) > > > > case BULK_ERASE: > > > > len = s->size; > > > > break; > > > > + case DIE_ERASE: > > > > + if (s->die_cnt) { > > > > + len = s->size / s->die_cnt; > > > > + offset = offset & (~(len-1)); > > > > + } else { > > > > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase > > > > + not > > > supported by" > > > > + " device\n", len); > > Don't log len here... Sure, I did not get your intention :) Thanks, Marcin > > > > > > > + return; > > > > + } > > > > + break; > > > > default: > > > > abort(); > > > > } > > > > @@ -634,6 +647,7 @@ static void complete_collecting_data(Flash *s) > > > > case ERASE4_32K: > > > > case ERASE_SECTOR: > > > > case ERASE4_SECTOR: > > > > + case DIE_ERASE: > > > > flash_erase(s, s->cur_addr, s->cmd_in_progress); > > > > break; > > > > case WRSR: > > > > @@ -684,6 +698,7 @@ static void reset_memory(Flash *s) > > > > s->write_enable = false; > > > > s->reset_enable = false; > > > > s->quad_enable = false; > > > > + s->die_cnt = 0; > > > > > > > > switch (get_man(s)) { > > > > case MAN_NUMONYX: > > > > @@ -716,7 +731,15 @@ static void reset_memory(Flash *s) > > > > s->four_bytes_address_mode = true; > > > > } > > > > if (!(s->nonvolatile_cfg & NVCFG_LOWER_SEGMENT_MASK)) { > > > > - s->ear = s->size / MAX_3BYTES_SIZE - 1; > > > > + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / > > > > + MAX_3BYTES_SIZE > > > - 1 : 0); > > > > + } > > > > + /* 1GiB devices */ > > > > + if (s->pi->id[2] == 0x21) { > > > > + /* MT25Q00 has 2 dies N25Q00 has 4 */ > > > > + if (s->pi->id[4] & BIT(6)) > > > > + s->die_cnt = 2; > > > > + else > > > > + s->die_cnt = 4; > > > > > > Decoding of die_cnt should probably be done in realize(). > > > > > > > } > > > > break; > > > > case MAN_MACRONIX: > > > > @@ -880,6 +903,7 @@ static void decode_new_cmd(Flash *s, uint32_t > > > value) > > > > case PP: > > > > case PP4: > > > > case PP4_4: > > > > + case DIE_ERASE: > > > > s->needed_bytes = get_addr_length(s); > > > > s->pos = 0; > > > > s->len = 0; > > > > @@ -1217,6 +1241,7 @@ static const VMStateDescription > > > > vmstate_m25p80 > > > = { > > > > VMSTATE_UINT8(spansion_cr2nv, Flash), > > > > VMSTATE_UINT8(spansion_cr3nv, Flash), > > > > VMSTATE_UINT8(spansion_cr4nv, Flash), > > > > + VMSTATE_UINT32(die_cnt, Flash), > > > > > > die_cnt is fixed per instance. Since it doesn't change, it doesn't > > > need to be part of VMSTATE. > > > > Yes, you are right. Even more, number of a dies is fixed attribute of > > chip, so it could Be a part of FlashPartInfo. I'll up v2. > > > > Thanks, > > Marcin > > > > > > > > > VMSTATE_END_OF_LIST() > > > > } > > > > }; > > > > -- > > > > 2.7.4 > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model 2016-12-16 13:27 [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model marcin.krzeminski 2016-12-16 13:27 ` [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op marcin.krzeminski 2016-12-16 13:27 ` [Qemu-devel] [PATCH 2/2] block: m25p80: Introduce Die Erase command marcin.krzeminski @ 2016-12-16 16:09 ` no-reply 2 siblings, 0 replies; 9+ messages in thread From: no-reply @ 2016-12-16 16:09 UTC (permalink / raw) To: marcin.krzeminski Cc: famz, qemu-devel, peter.maydell, rfsw-patches, qemu-arm, clg Hi, Your series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model Message-id: 1481894862-14102-1-git-send-email-marcin.krzeminski@nokia.com === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1481894862-14102-1-git-send-email-marcin.krzeminski@nokia.com -> patchew/1481894862-14102-1-git-send-email-marcin.krzeminski@nokia.com Switched to a new branch 'test' 44ae298 block: m25p80: Introduce Die Erase command 1c50e2e block: m25p80: Add Quad Page Program 4byte version op === OUTPUT BEGIN === Checking PATCH 1/2: block: m25p80: Add Quad Page Program 4byte version op... Checking PATCH 2/2: block: m25p80: Introduce Die Erase command... ERROR: spaces required around that '-' (ctx:VxV) #63: FILE: hw/block/m25p80.c:522: + offset = offset & (~(len-1)); ^ WARNING: line over 80 characters #65: FILE: hw/block/m25p80.c:524: + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %d die erase not supported by" WARNING: line over 80 characters #94: FILE: hw/block/m25p80.c:734: + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0); ERROR: space prohibited after that open parenthesis '(' #94: FILE: hw/block/m25p80.c:734: + s->ear = ( s->size > MAX_3BYTES_SIZE ? s->size / MAX_3BYTES_SIZE - 1 : 0); ERROR: braces {} are necessary for all arms of this statement #99: FILE: hw/block/m25p80.c:739: + if (s->pi->id[4] & BIT(6)) [...] + else [...] total: 3 errors, 2 warnings, 93 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-12-19 8:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-16 13:27 [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model marcin.krzeminski 2016-12-16 13:27 ` [Qemu-devel] [PATCH 1/2] block: m25p80: Add Quad Page Program 4byte version op marcin.krzeminski 2016-12-16 16:18 ` Edgar E. Iglesias 2016-12-16 13:27 ` [Qemu-devel] [PATCH 2/2] block: m25p80: Introduce Die Erase command marcin.krzeminski 2016-12-16 16:35 ` [Qemu-devel] [Qemu-arm] " Edgar E. Iglesias 2016-12-19 6:21 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) 2016-12-19 7:28 ` Edgar E. Iglesias 2016-12-19 7:31 ` Krzeminski, Marcin (Nokia - PL/Wroclaw) 2016-12-16 16:09 ` [Qemu-devel] [PATCH 0/2]block: m25p80: Improve mt25qu01g chip model no-reply
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.