Re: [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes

[Bin Meng]

Hi Francisco,

On Mon, Dec 21, 2020 at 11:52 PM Francisco Iglesias
<frasse.iglesias@gmail.com> wrote:
>
> Hi Bin,
>
> On [2020 Dec 11] Fri 14:37:20, 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>
> >
> > ---
> >
> > Changes in v3:
> > - initialize aai_enable to false in reset_memory()
> >
> > Changes in v2:
> > - add aai_enable into the vmstate
> > - validate AAI command before decoding a new command
> > - log guest errors during AAI_WP command handling
> > - report AAI status in the status register
> > - abort AAI programming when address is wrapped
> > - make sure AAI programming starts from the even address
> >
> >  hw/block/m25p80.c | 61 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 236e1b4..802a21d 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;
> > @@ -664,6 +666,11 @@ static void complete_collecting_data(Flash *s)
> >      case PP4_4:
> >          s->state = STATE_PAGE_PROGRAM;
> >          break;
> > +    case AAI_WP:
> > +        /* AAI programming starts from the even address */
> > +        s->cur_addr &= ~BIT(0);
> > +        s->state = STATE_PAGE_PROGRAM;
> > +        break;
> >      case READ:
> >      case READ4:
> >      case FAST_READ:
> > @@ -762,6 +769,7 @@ static void reset_memory(Flash *s)
> >      s->write_enable = false;
> >      s->reset_enable = false;
> >      s->quad_enable = false;
> > +    s->aai_enable = false;
> >
> >      switch (get_man(s)) {
> >      case MAN_NUMONYX:
> > @@ -932,6 +940,15 @@ static void decode_qio_read_cmd(Flash *s)
> >      s->state = STATE_COLLECTING_DATA;
> >  }
> >
> > +static bool is_valid_aai_cmd(uint32_t value)
> > +{
> > +    if (value == AAI_WP || value == WRDI || value == RDSR) {
> > +        return true;
> > +    } else {
> > +        return false;
> > +    }
>
> For fewer lines (perhaps also rename value -> cmd):
>
> return value == AAI_WP || value == WRDI || value == RDSR;

Sure!

>
>
> > +}
> > +
> >  static void decode_new_cmd(Flash *s, uint32_t value)
> >  {
> >      int i;
> > @@ -943,6 +960,11 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >          s->reset_enable = false;
> >      }
> >
> > +    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");
> > +    }
> > +
> >      switch (value) {
> >
> >      case ERASE_4K:
> > @@ -1008,6 +1030,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;
> > @@ -1018,6 +1043,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
> >          if (get_man(s) == MAN_MACRONIX) {
> >              s->data[0] |= (!!s->quad_enable) << 6;
> >          }
> > +        if (get_man(s) == MAN_SST) {
> > +            s->data[0] |= (!!s->aai_enable) << 6;
> > +        }
> > +
> >          s->pos = 0;
> >          s->len = 1;
> >          s->data_read_loop = true;
> > @@ -1160,6 +1189,24 @@ 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) {
> > +            if (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;
> > +                }
> > +            } else {
> > +                qemu_log_mask(LOG_GUEST_ERROR,
> > +                              "M25P80: AAI_WP with write protect\n");
> > +            }
> > +        } else {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", 
> > value);
> > +        }
> > +        break;
> >      default:
> >          s->pos = 0;
> >          s->len = 1;
> > @@ -1205,6 +1252,19 @@ static uint32_t m25p80_transfer8(SSISlave *ss, 
> > uint32_t tx)
> >          trace_m25p80_page_program(s, s->cur_addr, (uint8_t)tx);
> >          flash_write8(s, s->cur_addr, (uint8_t)tx);
> >          s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> > +
> > +        if (get_man(s) == MAN_SST && s->aai_enable && s->cur_addr == 0) {
>
> I had a another look at above and think [1] actually speeks about wrapping
> when hitting into protected areas. Since we do not handle those at the
> moment we can just remove this portion in case you agree with me.
>

The datasheet actually says:

1. "An AAI Word program instruction pointing to a protected memory
area will be ignored."
2. "There is no wrap mode during AAI programming once the highest
unprotected memory address is reached."

In QEMU protected block is implemented for m25p80 so 1 can be ignored.
2 indicates that AAI stops at the highest unprotected memory address,
ie. no wrap mode like what READ or FAST READ does.

We've also tested on a real SST flash and tests showed that AAI stops
at the highest memory address (note the whole flash is unprotected)

>
> > +            /*
> > +             * There is no wrap mode during AAI programming once the 
> > highest
> > +             * unprotected memory address is reached. The 
> > Write-Enable-Latch
> > +             * bit is automatically reset, and AAI programming mode aborts.
> > +             */
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "M25P80: AAI highest memory address reached");
> > +            s->write_enable = false;
> > +            s->aai_enable = false;
> > +        }
> > +
> >          break;
> >
> >      case STATE_READ:
> > @@ -1372,6 +1432,7 @@ static const VMStateDescription vmstate_m25p80 = {
> >          VMSTATE_UINT32(volatile_cfg, Flash),
> >          VMSTATE_UINT32(enh_volatile_cfg, Flash),
> >          VMSTATE_BOOL(quad_enable, Flash),
> > +        VMSTATE_BOOL(aai_enable, Flash),
>
> Above we will have to be put into a subsection (for not breaking
> migration). In 'docs/devel/migration.rst' one can read more about those
> (and in the file there's one already that can be looked into).
>

Will do in v4.

> The rest of the patch looks good to me!

Thanks for the review!

Regards,
Bin