On 10/29/20 12:15 AM, Joe Komlodi wrote: > 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-de...@nongnu.org > Cc: Francisco Eduardo Iglesias <figle...@xilinx.com>; kw...@redhat.com; > alist...@alistair23.me; qemu-block@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.
It is OK to not implement unused registers/fields, but please use qemu_log_mask(LOG_UNIMP,...) there, so we can: - notice the guest is accessing unimplemented registers at runtime - easily find the missing place in the code.
