diff mbox series

[v4,2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes

Message ID 1608619520-62782-2-git-send-email-bmeng.cn@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v4,1/2] hw/block: m25p80: Don't write to flash if write is disabled | expand

Commit Message

Bin Meng Dec. 22, 2020, 6:45 a.m. UTC 
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 v4:
- simplify is_valid_aai_cmd()
- use a subsection for s->aai_enable vm state

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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

Comments

Francisco Iglesias Dec. 22, 2020, 3:43 p.m. UTC | #1 
Hi Bin,

A couple of minor comments only!

On [2020 Dec 22] Tue 14:45: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 v4:
> - simplify is_valid_aai_cmd()
> - use a subsection for s->aai_enable vm state
> 
> 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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 236e1b4..d37b4d8 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,11 @@ static void decode_qio_read_cmd(Flash *s)
>      s->state = STATE_COLLECTING_DATA;
>  }
>  
> +static bool is_valid_aai_cmd(uint32_t cmd)
> +{
> +    return cmd == AAI_WP || cmd == WRDI || cmd == RDSR;
> +}
> +
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>      int i;
> @@ -943,6 +956,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 +1026,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 +1039,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 +1185,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 +1248,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) {
> +            /*
> +             * 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");

Above message will be printed after writing the highest addressed byte but
before trying to write a byte after wrapping. Since it wouldn't be a guest
error (to write the final byte) perhaps we should remove it?
(An option is to swap it to a trace event instead also)

> +            s->write_enable = false;
> +            s->aai_enable = false;
> +        }
> +
>          break;
>  
>      case STATE_READ:
> @@ -1350,6 +1406,24 @@ static const VMStateDescription vmstate_m25p80_data_read_loop = {
>      }
>  };
>  
> +static bool m25p80_aai_enable_needed(void *opaque)
> +{
> +    Flash *s = (Flash *)opaque;
> +
> +    return get_man(s) == MAN_SST;

For only using the subsection if really needed (and restricting further)
we can swap above to:

return s->aai_enable;

Best regards,
Francisco Iglesias

> +}
> +
> +static const VMStateDescription vmstate_m25p80_aai_enable = {
> +    .name = "m25p80/aai_enable",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = m25p80_aai_enable_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(aai_enable, Flash),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_m25p80 = {
>      .name = "m25p80",
>      .version_id = 0,
> @@ -1380,6 +1454,7 @@ static const VMStateDescription vmstate_m25p80 = {
>      },
>      .subsections = (const VMStateDescription * []) {
>          &vmstate_m25p80_data_read_loop,
> +        &vmstate_m25p80_aai_enable,
>          NULL
>      }
>  };
> -- 
> 2.7.4
>
Bin Meng Dec. 22, 2020, 3:51 p.m. UTC | #2 
Hi Francisco,

On Tue, Dec 22, 2020 at 11:43 PM Francisco Iglesias
<frasse.iglesias@gmail.com> wrote:
>
> Hi Bin,
>
> A couple of minor comments only!
>
> On [2020 Dec 22] Tue 14:45: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 v4:
> > - simplify is_valid_aai_cmd()
> > - use a subsection for s->aai_enable vm state
> >
> > 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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 236e1b4..d37b4d8 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,11 @@ static void decode_qio_read_cmd(Flash *s)
> >      s->state = STATE_COLLECTING_DATA;
> >  }
> >
> > +static bool is_valid_aai_cmd(uint32_t cmd)
> > +{
> > +    return cmd == AAI_WP || cmd == WRDI || cmd == RDSR;
> > +}
> > +
> >  static void decode_new_cmd(Flash *s, uint32_t value)
> >  {
> >      int i;
> > @@ -943,6 +956,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 +1026,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 +1039,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 +1185,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 +1248,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) {
> > +            /*
> > +             * 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");
>
> Above message will be printed after writing the highest addressed byte but
> before trying to write a byte after wrapping. Since it wouldn't be a guest
> error (to write the final byte) perhaps we should remove it?
> (An option is to swap it to a trace event instead also)

Agree. This is not an error. Will update in the next version.

>
> > +            s->write_enable = false;
> > +            s->aai_enable = false;
> > +        }
> > +
> >          break;
> >
> >      case STATE_READ:
> > @@ -1350,6 +1406,24 @@ static const VMStateDescription vmstate_m25p80_data_read_loop = {
> >      }
> >  };
> >
> > +static bool m25p80_aai_enable_needed(void *opaque)
> > +{
> > +    Flash *s = (Flash *)opaque;
> > +
> > +    return get_man(s) == MAN_SST;
>
> For only using the subsection if really needed (and restricting further)
> we can swap above to:
>
> return s->aai_enable;

Will do. Thanks!

Regards,
Bin
diff mbox series

Patch

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 236e1b4..d37b4d8 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,11 @@  static void decode_qio_read_cmd(Flash *s)
     s->state = STATE_COLLECTING_DATA;
 }
 
+static bool is_valid_aai_cmd(uint32_t cmd)
+{
+    return cmd == AAI_WP || cmd == WRDI || cmd == RDSR;
+}
+
 static void decode_new_cmd(Flash *s, uint32_t value)
 {
     int i;
@@ -943,6 +956,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 +1026,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 +1039,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 +1185,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 +1248,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) {
+            /*
+             * 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:
@@ -1350,6 +1406,24 @@  static const VMStateDescription vmstate_m25p80_data_read_loop = {
     }
 };
 
+static bool m25p80_aai_enable_needed(void *opaque)
+{
+    Flash *s = (Flash *)opaque;
+
+    return get_man(s) == MAN_SST;
+}
+
+static const VMStateDescription vmstate_m25p80_aai_enable = {
+    .name = "m25p80/aai_enable",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = m25p80_aai_enable_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(aai_enable, Flash),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_m25p80 = {
     .name = "m25p80",
     .version_id = 0,
@@ -1380,6 +1454,7 @@  static const VMStateDescription vmstate_m25p80 = {
     },
     .subsections = (const VMStateDescription * []) {
         &vmstate_m25p80_data_read_loop,
+        &vmstate_m25p80_aai_enable,
         NULL
     }
 };