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

[Bin Meng]

Hi Francisco,

On Thu, Dec 3, 2020 at 8:54 PM Francisco Iglesias
 wrote:
>
> Hello Bin,
>
> On [2020 Dec 02] Wed 22:30:37, Bin Meng wrote:
> > From: Xuzhou Cheng 
> >
> > 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 
> > Signed-off-by: Bin Meng 
> > ---
> >
> >  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

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

[Francisco Iglesias]

Hello Bin,

On [2020 Dec 02] Wed 22:30:37, Bin Meng wrote:
> From: Xuzhou Cheng 
> 
> 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 
> Signed-off-by: Bin Meng 
> ---
> 
>  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

> +}
> +break;
>  default:
>  s->pos = 0;
>  s->len = 1;
> -- 
> 2.25.1
> 
> 

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

[Bin Meng]

From: Xuzhou Cheng 

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 
Signed-off-by: Bin Meng 
---

 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;
 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)
 
 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;
+}
+}
+break;
 default:
 s->pos = 0;
 s->len = 1;
-- 
2.25.1