RE: [PATCH v2 1/1] hw/block/m25p80: Fix Numonyx fast read dummy cycle count

[Joe Komlodi]

Hi Philippe,

Comments marked inline with [Joe]

-----Original Message-----
From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of 
Philippe Mathieu-Daudé
Sent: Wednesday, October 28, 2020 2:27 AM
To: Joe Komlodi <komlodi@xilinx.com>; qemu-devel@nongnu.org
Cc: Francisco Eduardo Iglesias <figlesia@xilinx.com>; kwolf@redhat.com; 
alistair@alistair23.me; qemu-block@nongnu.org; mreitz@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 <komlodi@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)) {
>