Re: [PATCH v3 3/3] hw/block/m25p80: Fix Numonyx fast read dummy cycle count

[Francisco Iglesias <francisco.iglesias@xilinx.com>]

Hi Joe,

On Thu, Nov 05, 2020 at 05:32:58PM -0800, 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-sheet/nor-flash/serial-nor/mt25q/die-rev-b/mt25q_qlkt_u_02g_cbb_0.pdf?rev=9b167fbf2b3645efba6385949a72e453
> 
> Signed-off-by: Joe Komlodi <komlodi@xilinx.com>
> ---
>  hw/block/m25p80.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8a1b684..a2cdfb6 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -841,6 +841,43 @@ static uint8_t numonyx_get_mode(Flash *s)
>      return mode;
>  }
>  
> +static uint8_t numonyx_extract_cfg_num_dummies(Flash *s)
> +{
> +    uint8_t cycle_count;

We can remove some lines and shrink the function if we remove above variable
and work on num_dummies directly instead.

> +    uint8_t num_dummies;
> +    uint8_t mode;
> +    uint8_t cycle_table[0x100][3] = {
> +        [FAST_READ] = {8, 8, 10},
> +        [FAST_READ4] = {8, 8, 10},
> +        [DOR] = {8, 8, 0xff},
> +        [DOR4] = {8, 8, 0xff},
> +        [QOR] = {8, 0xff, 10},
> +        [QOR4] = {8, 0xff, 10},
> +        [DIOR] = {8, 8, 0xff},
> +        [DIOR4] = {8, 8, 0xff},
> +        [QIOR] = {10, 0xff, 10},
> +        [QIOR4] = {10, 0xff, 10},
> +    };
> +    assert(get_man(s) == MAN_NUMONYX);
> +
> +    mode = numonyx_get_mode(s);
> +
> +    cycle_count = extract32(s->volatile_cfg, 4, 4);
> +    if (cycle_count == 0x0 || cycle_count == 0xf) {
> +        num_dummies = cycle_table[s->cmd_in_progress][mode];

Instead of the table i think it is easier to do a switch here:

switch (s->cmd_in_progress) {
case QIOR:
case QIOR4:
    num_dummies = 10;
    break;
default:
    num_dummies = (mode == MODE_QIO) = 10 : 8;
    break;

};

Best regards,
Francisco Iglesias

> +    } else {
> +        num_dummies = cycle_count;
> +    }
> +
> +    /*
> +     * Validation if the command can be executed should be done outside of
> +     * this function. e.g. trying to execute DIOR in QIO mode.
> +     */
> +    assert(num_dummies != 0xff);
> +
> +    return num_dummies;
> +}
> +
>  static bool numonyx_check_cmd_mode(Flash *s)
>  {
>      uint8_t mode;
> @@ -901,7 +938,7 @@ static void decode_fast_read_cmd(Flash *s)
>          break;
>      case MAN_NUMONYX:
>          if (numonyx_check_cmd_mode(s)) {
> -            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>              s->state = STATE_COLLECTING_DATA;
>          }
>          break;
> @@ -947,7 +984,7 @@ static void decode_dio_read_cmd(Flash *s)
>          break;
>      case MAN_NUMONYX:
>          if (numonyx_check_cmd_mode(s)) {
> -            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>              s->state = STATE_COLLECTING_DATA;
>          }
>          break;
> @@ -993,7 +1030,7 @@ static void decode_qio_read_cmd(Flash *s)
>          break;
>      case MAN_NUMONYX:
>          if (numonyx_check_cmd_mode(s)) {
> -            s->needed_bytes += extract32(s->volatile_cfg, 4, 4);
> +            s->needed_bytes += numonyx_extract_cfg_num_dummies(s);
>              s->state = STATE_COLLECTING_DATA;
>          }
>          break;
> -- 
> 2.7.4
>