RE: [PATCH 1/2] hw/block/m25p80: Fix Numonyx dummy cycle register behavior

[Joe Komlodi]

Hi Francisco,

Comments marked with [Joe]

-----Original Message-----
From: Francisco Iglesias <francisco.iglesias@xilinx.com> 
Sent: Tuesday, October 20, 2020 6:50 AM
To: Joe Komlodi <komlodi@xilinx.com>
Cc: qemu-devel@nongnu.org; alistair@alistair23.me; kwolf@redhat.com; 
mreitz@redhat.com; qemu-block@nongnu.org
Subject: Re: [PATCH 1/2] hw/block/m25p80: Fix Numonyx dummy cycle register 
behavior

Hi Joe,

On Tue, Sep 29, 2020 at 05:28:35PM -0700, 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] http://www.micron.com/-/media/client/global/documents/products/
> data-sheet/nor-flash/serial-nor/n25q/n25q_512mb_1_8v_65nm.pdf
> 
> Page 22 note 2, and page 30 notes 5 and 10.
> 
> 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..43830c9 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_fast_read_num_dummies(Flash *s)

Should we rename the function to something like 
'numonyx_extract_cfg_num_dummies' (since it is not only used inside 
'decode_fast_read_cmd')?

[Joe] Yeah, that name makes more sense.

> +{
> +    uint8_t cycle_count;
> +    uint8_t num_dummies;
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    cycle_count = extract32(s->volatile_cfg, 4, 4);
> +    if (cycle_count == 0x0 || cycle_count == 0x0F) {
> +        if (s->cmd_in_progress == QIOR || s->cmd_in_progress == 
> + QIOR4) {

QOR and QOR4 also has 10 dummy cycles on default so we will have to check for 
those aswell, perhaps something similar like below migth work:  

uint8_t n_dummies = extract32(s->volatile_cfg, 4, 4);

if (!n_dummies || n_dummies == 0xF) {
    switch(s->cmd_in_progress){
    case QOR:
    case QOR4
    case QIOR:
    case QIOR4:
        n_dummies = 10;
        break;
    default:
        n_dummies = 8;
        break;
    }
}

return n_dummies;

[Joe] As talked about offline, the datasheet in the commit message just has 
confusing wording.
8 dummies for QOR seems to be correct, and I'll update the datasheet in the 
commit message with one that's more clear.

Thanks!
Joe

Best regards,
Francisco Iglesias

> +            num_dummies = 10;
> +        } else {
> +            num_dummies = 8;
> +        }
> +    } 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_fast_read_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_fast_read_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_fast_read_num_dummies(s);
>          break;
>      case MAN_MACRONIX:
>          switch (extract32(s->volatile_cfg, 6, 2)) {
> --
> 2.7.4
>