Hi Francisco,
On Tue, Dec 22, 2020 at 11:43 PM Francisco Iglesias
<frasse.igles...@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.ch...@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.ch...@windriver.com>
> > Signed-off-by: Bin Meng <bin.m...@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
