Hi Francisco,
On Thu, Dec 3, 2020 at 8:54 PM Francisco Iglesias
<frasse.igles...@gmail.com> wrote:
>
> Hello Bin,
>
> On [2020 Dec 02] Wed 22:30:37, Bin Meng wrote:
> > From: Xuzhou Cheng <xuzhou.ch...@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.ch...@windriver.com>
> > Signed-off-by: Bin Meng <bin.m...@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
