Message ID | 20201202143037.24110-1-bmeng.cn@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/block: m25p80: Implement AAI-WP command support for SST flashes | expand |
Hello Bin, On [2020 Dec 02] Wed 22:30:37, Bin Meng wrote: > From: Xuzhou Cheng <xuzhou.cheng@windriver.com> > > Auto Address Increment (AAI) Word-Program is a special command of > SST flashes. AAI-WP allows multiple bytes of data to be programmed > without re-issuing the next sequential address location. > > Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com> > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > hw/block/m25p80.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index 9b36762df9..f225d9c96d 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -359,6 +359,7 @@ typedef enum { > QPP_4 = 0x34, > RDID_90 = 0x90, > RDID_AB = 0xab, > + AAI_WP = 0xad, > > ERASE_4K = 0x20, > ERASE4_4K = 0x21, > @@ -449,6 +450,7 @@ struct Flash { > bool four_bytes_address_mode; > bool reset_enable; > bool quad_enable; > + bool aai_enable; We need to add above addition also into the vmstate. > uint8_t ear; > > int64_t dirty_page; > @@ -661,6 +663,7 @@ static void complete_collecting_data(Flash *s) > case PP: > case PP4: > case PP4_4: > + case AAI_WP: > s->state = STATE_PAGE_PROGRAM; > break; > case READ: > @@ -1010,6 +1013,9 @@ static void decode_new_cmd(Flash *s, uint32_t value) Since only 3 cmds are allowed while within AAI programming sequence [1] I think a warning migt be good have before the command switch case, similar to: if (get_man(s) == MAN_SST && s->aai_enable && !is_valid_aai_cmd(value)) { qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Invalid cmd within AAI programming sequence"); } > > case WRDI: > s->write_enable = false; > + if (get_man(s) == MAN_SST) { > + s->aai_enable = false; > + } > break; > case WREN: > s->write_enable = true; > @@ -1162,6 +1168,17 @@ static void decode_new_cmd(Flash *s, uint32_t value) > case RSTQIO: > s->quad_enable = false; > break; > + case AAI_WP: > + if (get_man(s) == MAN_SST && s->write_enable) { > + if (s->aai_enable) { > + s->state = STATE_PAGE_PROGRAM; > + } else { > + s->aai_enable = true; > + s->needed_bytes = get_addr_length(s); > + s->state = STATE_COLLECTING_DATA; > + } Perhaps a qemu_log_mask in an 'else' could be useful here also: } else { qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %s" (get_man(s) == MAN_SST) ? "AAI_WP with write protect" : "Unknown CMD: 0xAD\n"); Lastly, [1] also says that the address shouldn't wrapp around when in AAI mode, so we need a check before doing that also I think. Best regards, Francisco Iglesias [1] http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf > + } > + break; > default: > s->pos = 0; > s->len = 1; > -- > 2.25.1 > >
Hi Francisco, On Thu, Dec 3, 2020 at 8:54 PM Francisco Iglesias <frasse.iglesias@gmail.com> wrote: > > Hello Bin, > > On [2020 Dec 02] Wed 22:30:37, Bin Meng wrote: > > From: Xuzhou Cheng <xuzhou.cheng@windriver.com> > > > > Auto Address Increment (AAI) Word-Program is a special command of > > SST flashes. AAI-WP allows multiple bytes of data to be programmed > > without re-issuing the next sequential address location. > > > > Signed-off-by: Xuzhou Cheng <xuzhou.cheng@windriver.com> > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > > > hw/block/m25p80.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > > index 9b36762df9..f225d9c96d 100644 > > --- a/hw/block/m25p80.c > > +++ b/hw/block/m25p80.c > > @@ -359,6 +359,7 @@ typedef enum { > > QPP_4 = 0x34, > > RDID_90 = 0x90, > > RDID_AB = 0xab, > > + AAI_WP = 0xad, > > > > ERASE_4K = 0x20, > > ERASE4_4K = 0x21, > > @@ -449,6 +450,7 @@ struct Flash { > > bool four_bytes_address_mode; > > bool reset_enable; > > bool quad_enable; > > + bool aai_enable; > > We need to add above addition also into the vmstate. > > > uint8_t ear; > > > > int64_t dirty_page; > > @@ -661,6 +663,7 @@ static void complete_collecting_data(Flash *s) > > case PP: > > case PP4: > > case PP4_4: > > + case AAI_WP: > > s->state = STATE_PAGE_PROGRAM; > > break; > > case READ: > > @@ -1010,6 +1013,9 @@ static void decode_new_cmd(Flash *s, uint32_t value) > > Since only 3 cmds are allowed while within AAI programming sequence [1] I think > a warning migt be good have before the command switch case, similar to: > > if (get_man(s) == MAN_SST && s->aai_enable && !is_valid_aai_cmd(value)) { > qemu_log_mask(LOG_GUEST_ERROR, > "M25P80: Invalid cmd within AAI programming sequence"); > } > > > > > case WRDI: > > s->write_enable = false; > > + if (get_man(s) == MAN_SST) { > > + s->aai_enable = false; > > + } > > break; > > case WREN: > > s->write_enable = true; > > @@ -1162,6 +1168,17 @@ static void decode_new_cmd(Flash *s, uint32_t value) > > case RSTQIO: > > s->quad_enable = false; > > break; > > + case AAI_WP: > > + if (get_man(s) == MAN_SST && s->write_enable) { > > + if (s->aai_enable) { > > + s->state = STATE_PAGE_PROGRAM; > > + } else { > > + s->aai_enable = true; > > + s->needed_bytes = get_addr_length(s); > > + s->state = STATE_COLLECTING_DATA; > > + } > > Perhaps a qemu_log_mask in an 'else' could be useful here also: > > } else { > qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %s" > (get_man(s) == MAN_SST) ? "AAI_WP with write protect" : > "Unknown CMD: 0xAD\n"); > > Lastly, [1] also says that the address shouldn't wrapp around when in AAI mode, > so we need a check before doing that also I think. > > Best regards, > Francisco Iglesias > > [1] http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf Thanks for the review. All of your comments will be addressed in v2. Regards, Bin
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 9b36762df9..f225d9c96d 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -359,6 +359,7 @@ typedef enum { QPP_4 = 0x34, RDID_90 = 0x90, RDID_AB = 0xab, + AAI_WP = 0xad, ERASE_4K = 0x20, ERASE4_4K = 0x21, @@ -449,6 +450,7 @@ struct Flash { bool four_bytes_address_mode; bool reset_enable; bool quad_enable; + bool aai_enable; uint8_t ear; int64_t dirty_page; @@ -661,6 +663,7 @@ static void complete_collecting_data(Flash *s) case PP: case PP4: case PP4_4: + case AAI_WP: s->state = STATE_PAGE_PROGRAM; break; case READ: @@ -1010,6 +1013,9 @@ static void decode_new_cmd(Flash *s, uint32_t value) case WRDI: s->write_enable = false; + if (get_man(s) == MAN_SST) { + s->aai_enable = false; + } break; case WREN: s->write_enable = true; @@ -1162,6 +1168,17 @@ static void decode_new_cmd(Flash *s, uint32_t value) case RSTQIO: s->quad_enable = false; break; + case AAI_WP: + if (get_man(s) == MAN_SST && s->write_enable) { + if (s->aai_enable) { + s->state = STATE_PAGE_PROGRAM; + } else { + s->aai_enable = true; + s->needed_bytes = get_addr_length(s); + s->state = STATE_COLLECTING_DATA; + } + } + break; default: s->pos = 0; s->len = 1;