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

[Francisco Iglesias <frasse.iglesias@gmail.com>]

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;


> +}
> +
>  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.


> +            /*
> +             * 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).

The rest of the patch looks good to me!

Best regards,
Francisco Iglesias

>          VMSTATE_UINT8(spansion_cr1nv, Flash),
>          VMSTATE_UINT8(spansion_cr2nv, Flash),
>          VMSTATE_UINT8(spansion_cr3nv, Flash),
> -- 
> 2.7.4
>