Hi Philippe, Comments marked inline with [Joe]
-----Original Message----- From: Philippe Mathieu-Daudé <philippe.mathieu.da...@gmail.com> On Behalf Of Philippe Mathieu-Daudé Sent: Wednesday, October 28, 2020 2:27 AM To: Joe Komlodi <koml...@xilinx.com>; qemu-devel@nongnu.org Cc: Francisco Eduardo Iglesias <figle...@xilinx.com>; kw...@redhat.com; alist...@alistair23.me; qemu-bl...@nongnu.org; mre...@redhat.com Subject: Re: [PATCH v2 1/1] hw/block/m25p80: Fix Numonyx fast read dummy cycle count Hi Joe, On 10/28/20 12:43 AM, Joe Komlodi wrote: > Numonyx chips determine the number of cycles to wait based on bits 7:4 > in the volatile configuration register. > > However, if these bits are 0x0 or 0xF, the number of dummy cycles to > wait is > 10 on a QIOR or QIOR4 command, or 8 on any other currently supported > fast read command. [1] > > [1] > https://www.micron.com/-/media/client/global/documents/products/data-s > heet/ nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf > ?rev=9b167fbf2b3645efba6385949a72e453 Please use single line for URL (even longer than 80 characters): https://www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf Or use Datasheet: "MT25QU02GCBB Rev. H 05/19 EN" [Joe] Ah, sorry about that, I'll put it all on one line in v3. > Page 34, page 39 note 5 The note 5 is not restricted to QIOR/QIOR4 commands, so your patch seems incomplete. [Joe] Right. Right now it's only checking QIOR/QIOR4 because we don't have a way to put Numonyx chips in QIO mode (s->quad_enable == true), and we don't model DDR commands. Because of those restrictions, all the read commands need 8 cycles, except for QIOR/QIOR4. > > Signed-off-by: Joe Komlodi <koml...@xilinx.com> > --- > hw/block/m25p80.c | 26 +++++++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > 483925f..302ed9d 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -820,6 +820,26 @@ static void reset_memory(Flash *s) > trace_m25p80_reset_done(s); > } > > +static uint8_t numonyx_extract_cfg_num_dummies(Flash *s) { > + uint8_t cycle_count; > + uint8_t num_dummies; > + assert(get_man(s) == MAN_NUMONYX); > + > + cycle_count = extract32(s->volatile_cfg, 4, 4); Could be easier to review as: num_dummies = extract32(s->volatile_cfg, 4, 4); switch (s->cmd_in_progress) { /* note 5 comment ... */ case FAST_READ: case ... /* field erased or reset in NVRAM */ if (cycle_count == 0x0 || cycle_count == 0xf) { switch (s->cmd_in_progress) { case FAST_READ: case ...: num_dummies = 10; break; case ...: case ...: /* assert(s->quad_enable); */ num_dummies = 8; break; default: qemu_log_mask(GUEST_ERROR, ...); break; } } break; default: break; } return num_dummies; > + if (cycle_count == 0x0 || cycle_count == 0x0F) { > + if (s->cmd_in_progress == QIOR || s->cmd_in_progress == QIOR4) { > + num_dummies = 10; > + } else { > + num_dummies = 8; Note, this is only true if s->quad_enable. Maybe clever to use the dumbest approach and copy the table... static uint8_t numonyx_extract_cfg_num_dummies(Flash *s) { static const unsigned dummy_clock_cycles[0x100][3] = { [FAST_READ] = {8, 8, 10}, ... }; uint8_t num_dummies = extract32(s->volatile_cfg, 4, 4); if (cycle_count == 0x0 || cycle_count == 0xf) { num_dummies = dummy_clock_cycles[s->cmd_in_progress][mode]; } return num_dummies; } [Joe] I think either this or the switch statement would work fine, it just depends if we want to add a way to set s->quad_enable and s->dual_enable (doesn't exist in the model) for Numonyx chips. To be the most accurate, it probably would be best to add a way to enable/disable QIO and DIO mode for Numonyx chips, then change the cycles needed accordingly. Let me know what you think. Thanks! Joe > + } > + } else { > + num_dummies = cycle_count; > + } > + > + return num_dummies; > +} > + > static void decode_fast_read_cmd(Flash *s) { > s->needed_bytes = get_addr_length(s); @@ -829,7 +849,7 @@ static > void decode_fast_read_cmd(Flash *s) > s->needed_bytes += 8; > break; > case MAN_NUMONYX: > - s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + s->needed_bytes += numonyx_extract_cfg_num_dummies(s); > break; > case MAN_MACRONIX: > if (extract32(s->volatile_cfg, 6, 2) == 1) { @@ -868,7 +888,7 > @@ static void decode_dio_read_cmd(Flash *s) > ); > break; > case MAN_NUMONYX: > - s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + s->needed_bytes += numonyx_extract_cfg_num_dummies(s); > break; > case MAN_MACRONIX: > switch (extract32(s->volatile_cfg, 6, 2)) { @@ -908,7 +928,7 > @@ static void decode_qio_read_cmd(Flash *s) > ); > break; > case MAN_NUMONYX: > - s->needed_bytes += extract32(s->volatile_cfg, 4, 4); > + s->needed_bytes += numonyx_extract_cfg_num_dummies(s); > break; > case MAN_MACRONIX: > switch (extract32(s->volatile_cfg, 6, 2)) { >
