qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] hw/block: m25p80: Don't write to flash if write is disabled
@ 2020-12-11  6:37 Bin Meng
  2020-12-11  6:37 ` [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes Bin Meng
  0 siblings, 1 reply; 5+ messages in thread
From: Bin Meng @ 2020-12-11  6:37 UTC (permalink / raw)
  To: Alistair Francis, Francisco Iglesias, Kevin Wolf, Max Reitz,
	qemu-block, qemu-devel
  Cc: Bin Meng

From: Bin Meng <bin.meng@windriver.com>

When write is disabled, the write to flash should be avoided
in flash_write8().

Fixes: 82a2499011a7 ("m25p80: Initial implementation of SPI flash device")
Signed-off-by: Bin Meng <bin.meng@windriver.com>

---

(no changes since v2)

Changes in v2:
- new patch: honor write enable flag in flash write

 hw/block/m25p80.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 483925f..236e1b4 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -594,6 +594,7 @@ void flash_write8(Flash *s, uint32_t addr, uint8_t data)
 
     if (!s->write_enable) {
         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
+        return;
     }
 
     if ((prev ^ data) & data) {
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes
  2020-12-11  6:37 [PATCH v3 1/2] hw/block: m25p80: Don't write to flash if write is disabled Bin Meng
@ 2020-12-11  6:37 ` Bin Meng
  2020-12-19 13:52   ` Bin Meng
  2020-12-21 15:52   ` Francisco Iglesias
  0 siblings, 2 replies; 5+ messages in thread
From: Bin Meng @ 2020-12-11  6:37 UTC (permalink / raw)
  To: Alistair Francis, Francisco Iglesias, Kevin Wolf, Max Reitz,
	qemu-block, qemu-devel
  Cc: Xuzhou Cheng, Bin Meng

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;
+    }
+}
+
 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) {
+            /*
+             * 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),
         VMSTATE_UINT8(spansion_cr1nv, Flash),
         VMSTATE_UINT8(spansion_cr2nv, Flash),
         VMSTATE_UINT8(spansion_cr3nv, Flash),
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes
  2020-12-11  6:37 ` [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes Bin Meng
@ 2020-12-19 13:52   ` Bin Meng
  2020-12-21 15:52   ` Francisco Iglesias
  1 sibling, 0 replies; 5+ messages in thread
From: Bin Meng @ 2020-12-19 13:52 UTC (permalink / raw)
  To: Alistair Francis, Francisco Iglesias, Kevin Wolf, Max Reitz,
	Qemu-block, qemu-devel@nongnu.org Developers
  Cc: Xuzhou Cheng, Bin Meng

Hi Francisco,

On Fri, Dec 11, 2020 at 2:37 PM Bin Meng <bmeng.cn@gmail.com> 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(+)
>

Any comments for this series?

Regards,
Bin


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes
  2020-12-11  6:37 ` [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes Bin Meng
  2020-12-19 13:52   ` Bin Meng
@ 2020-12-21 15:52   ` Francisco Iglesias
  2020-12-22  6:41     ` Bin Meng
  1 sibling, 1 reply; 5+ messages in thread
From: Francisco Iglesias @ 2020-12-21 15:52 UTC (permalink / raw)
  To: Bin Meng
  Cc: Kevin Wolf, qemu-block, Xuzhou Cheng, Bin Meng, qemu-devel,
	Max Reitz, Alistair Francis

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
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes
  2020-12-21 15:52   ` Francisco Iglesias
@ 2020-12-22  6:41     ` Bin Meng
  0 siblings, 0 replies; 5+ messages in thread
From: Bin Meng @ 2020-12-22  6:41 UTC (permalink / raw)
  To: Francisco Iglesias
  Cc: Kevin Wolf, Qemu-block, Xuzhou Cheng, Bin Meng,
	qemu-devel@nongnu.org Developers, Max Reitz, Alistair Francis

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-12-22  6:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11  6:37 [PATCH v3 1/2] hw/block: m25p80: Don't write to flash if write is disabled Bin Meng
2020-12-11  6:37 ` [PATCH v3 2/2] hw/block: m25p80: Implement AAI-WP command support for SST flashes Bin Meng
2020-12-19 13:52   ` Bin Meng
2020-12-21 15:52   ` Francisco Iglesias
2020-12-22  6:41     ` Bin Meng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).