Hi Francisco, -----Original Message----- From: Francisco Iglesias <francisco.igles...@xilinx.com> Sent: Monday, November 16, 2020 7:59 AM To: Joe Komlodi <koml...@xilinx.com> Cc: qemu-de...@nongnu.org; philippe.mathieu.da...@gmail.com; Francisco Eduardo Iglesias <figle...@xilinx.com>; alist...@alistair23.me; qemu-block@nongnu.org; mre...@redhat.com Subject: Re: [PATCH v4 3/4] hw/block/m25p80: Check SPI mode before running some Numonyx commands
Hi Joe, On Thu, Nov 12, 2020 at 07:10:54PM -0800, Joe Komlodi wrote: > Some Numonyx flash commands cannot be executed in DIO and QIO mode, > such as trying to do DPP or DOR when in QIO mode. > > Signed-off-by: Joe Komlodi <koml...@xilinx.com> > --- > hw/block/m25p80.c | 134 > +++++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 112 insertions(+), 22 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > eb6539f..2552f2c 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -413,6 +413,12 @@ typedef enum { > MAN_GENERIC, > } Manufacturer; > > +typedef enum { > + MODE_STD = 0, > + MODE_DIO = 1, > + MODE_QIO = 2 > +} SPIMode; > + > #define M25P80_INTERNAL_DATA_BUFFER_SZ 16 > > struct Flash { > @@ -820,6 +826,17 @@ static void reset_memory(Flash *s) > trace_m25p80_reset_done(s); > } > > +static uint8_t numonyx_get_mode(Flash *s) { > + if (!(s->enh_volatile_cfg & EVCFG_QUAD_IO_DISABLED)) { > + return MODE_QIO; > + } else if (!(s->enh_volatile_cfg & EVCFG_DUAL_IO_DISABLED)) { > + return MODE_DIO; > + } else { > + return MODE_STD; > + } > +} > + > static void decode_fast_read_cmd(Flash *s) { > s->needed_bytes = get_addr_length(s); @@ -827,9 +844,11 @@ static > void decode_fast_read_cmd(Flash *s) > /* Dummy cycles - modeled with bytes writes instead of bits */ > case MAN_WINBOND: > s->needed_bytes += 8; > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_NUMONYX: > s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_MACRONIX: > if (extract32(s->volatile_cfg, 6, 2) == 1) { @@ -837,19 > +856,21 @@ static void decode_fast_read_cmd(Flash *s) > } else { > s->needed_bytes += 8; > } > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_SPANSION: > s->needed_bytes += extract32(s->spansion_cr2v, > SPANSION_DUMMY_CLK_POS, > SPANSION_DUMMY_CLK_LEN > ); > + s->state = STATE_COLLECTING_DATA; > break; > default: > + s->state = STATE_COLLECTING_DATA; > break; > } > s->pos = 0; > s->len = 0; > - s->state = STATE_COLLECTING_DATA; Above change in this function and the similar ones in below two functions don't seem to be needed anymore (s->state = STATE_COLLECTING_DATA is being done in all cases). [Joe] Oops, I'll simplify that. > } > > static void decode_dio_read_cmd(Flash *s) @@ -859,6 +880,7 @@ static > void decode_dio_read_cmd(Flash *s) > switch (get_man(s)) { > case MAN_WINBOND: > s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN; > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_SPANSION: > s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN; > @@ -866,9 +888,11 @@ static void decode_dio_read_cmd(Flash *s) > SPANSION_DUMMY_CLK_POS, > SPANSION_DUMMY_CLK_LEN > ); > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_NUMONYX: > s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_MACRONIX: > switch (extract32(s->volatile_cfg, 6, 2)) { @@ -882,13 > +906,14 @@ static void decode_dio_read_cmd(Flash *s) > s->needed_bytes += 4; > break; > } > + s->state = STATE_COLLECTING_DATA; > break; > default: > + s->state = STATE_COLLECTING_DATA; > break; > } > s->pos = 0; > s->len = 0; > - s->state = STATE_COLLECTING_DATA; > } > > static void decode_qio_read_cmd(Flash *s) @@ -899,6 +924,7 @@ static > void decode_qio_read_cmd(Flash *s) > case MAN_WINBOND: > s->needed_bytes += WINBOND_CONTINUOUS_READ_MODE_CMD_LEN; > s->needed_bytes += 4; > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_SPANSION: > s->needed_bytes += SPANSION_CONTINUOUS_READ_MODE_CMD_LEN; > @@ -906,9 +932,11 @@ static void decode_qio_read_cmd(Flash *s) > SPANSION_DUMMY_CLK_POS, > SPANSION_DUMMY_CLK_LEN > ); > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_NUMONYX: > s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + s->state = STATE_COLLECTING_DATA; > break; > case MAN_MACRONIX: > switch (extract32(s->volatile_cfg, 6, 2)) { @@ -922,13 > +950,14 @@ static void decode_qio_read_cmd(Flash *s) > s->needed_bytes += 6; > break; > } > + s->state = STATE_COLLECTING_DATA; > break; > default: > + s->state = STATE_COLLECTING_DATA; > break; > } > s->pos = 0; > s->len = 0; > - s->state = STATE_COLLECTING_DATA; > } > > static void decode_new_cmd(Flash *s, uint32_t value) @@ -950,14 > +979,8 @@ static void decode_new_cmd(Flash *s, uint32_t value) > case ERASE4_32K: > case ERASE_SECTOR: > case ERASE4_SECTOR: > - case READ: > - case READ4: > - case DPP: > - case QPP: > - case QPP_4: > case PP: > case PP4: > - case PP4_4: > case DIE_ERASE: > case RDID_90: > case RDID_AB: > @@ -966,24 +989,85 @@ static void decode_new_cmd(Flash *s, uint32_t value) > s->len = 0; > s->state = STATE_COLLECTING_DATA; > break; > + case READ: > + case READ4: > + if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO > && > + numonyx_get_mode(s) != MODE_QIO)) { Above seems easier to check if we are in the correct mode here instead: if (get_man(s) != MAN_NUMONYX || numonyx_get_mode(s) == MODE_STD) { > + s->needed_bytes = get_addr_length(s); > + s->pos = 0; > + s->len = 0; > + s->state = STATE_COLLECTING_DATA; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "DIO or QIO mode\n", s->cmd_in_progress); > + } > + break; > + case DPP: > + if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) > { > + s->needed_bytes = get_addr_length(s); > + s->pos = 0; > + s->len = 0; > + s->state = STATE_COLLECTING_DATA; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "QIO mode\n", s->cmd_in_progress); > + } > + break; > + case QPP: > + case QPP_4: > + case PP4_4: > + if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) > { > + s->needed_bytes = get_addr_length(s); > + s->pos = 0; > + s->len = 0; > + s->state = STATE_COLLECTING_DATA; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "DIO mode\n", s->cmd_in_progress); > + } > + break; > > case FAST_READ: > case FAST_READ4: > + decode_fast_read_cmd(s); > + break; > case DOR: > case DOR4: > + if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) > { > + decode_fast_read_cmd(s); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "QIO mode\n", s->cmd_in_progress); > + } > + break; > case QOR: > case QOR4: > - decode_fast_read_cmd(s); > + if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) > { > + decode_fast_read_cmd(s); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "DIO mode\n", s->cmd_in_progress); > + } > break; > > case DIOR: > case DIOR4: > - decode_dio_read_cmd(s); > + if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_QIO) > { > + decode_dio_read_cmd(s); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "QIO mode\n", s->cmd_in_progress); > + } > break; > > case QIOR: > case QIOR4: > - decode_qio_read_cmd(s); > + if (!(get_man(s) == MAN_NUMONYX) || numonyx_get_mode(s) != MODE_DIO) > { > + decode_qio_read_cmd(s); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute cmd %x in > " > + "DIO mode\n", s->cmd_in_progress); > + } > break; > > case WRSR: > @@ -1035,17 +1119,23 @@ static void decode_new_cmd(Flash *s, uint32_t value) > break; > > case JEDEC_READ: > - trace_m25p80_populated_jedec(s); > - for (i = 0; i < s->pi->id_len; i++) { > - s->data[i] = s->pi->id[i]; > - } > - for (; i < SPI_NOR_MAX_ID_LEN; i++) { > - s->data[i] = 0; > - } > + if (!(get_man(s) == MAN_NUMONYX) || (numonyx_get_mode(s) != MODE_DIO > && > + numonyx_get_mode(s) != MODE_QIO)) { The same here as above: if (get_man(s) != MAN_NUMONYX || numonyx_get_mode(s) == MODE_STD) { [Joe] Agreed, that's much easier and keeps it on one line, not sure how I missed that... Thanks! Joe Best regards, Francisco Iglesias > + trace_m25p80_populated_jedec(s); > + for (i = 0; i < s->pi->id_len; i++) { > + s->data[i] = s->pi->id[i]; > + } > + for (; i < SPI_NOR_MAX_ID_LEN; i++) { > + s->data[i] = 0; > + } > > - s->len = SPI_NOR_MAX_ID_LEN; > - s->pos = 0; > - s->state = STATE_READING_DATA; > + s->len = SPI_NOR_MAX_ID_LEN; > + s->pos = 0; > + s->state = STATE_READING_DATA; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Cannot execute JEDEC > read " > + "in DIO or QIO mode\n"); > + } > break; > > case RDCR: > -- > 2.7.4 >