Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 4:57 AM,  <address@hidden> wrote:
> From: Marcin Krzeminski <address@hidden>
>
> Signed-off-by: Pawel Lenkow <address@hidden>

Same comments as for earlier patches. Need to clarify authorship and
add subsystem prefix and commit paragraph.

> ---
>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 6fc55a3..25ec666 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -255,19 +255,24 @@ typedef enum {
>      BULK_ERASE = 0xc7,
>
>      READ = 0x3,
> +    READ4 = 0x13,
>      FAST_READ = 0xb,
>      DOR = 0x3b,
>      QOR = 0x6b,
>      DIOR = 0xbb,
>      QIOR = 0xeb,
> +    QIOR4 = 0xec,
>
>      PP = 0x2,
> +    PP4 = 0x12,
>      DPP = 0xa2,
>      QPP = 0x32,
>
>      ERASE_4K = 0x20,
> +    ERASE4_4K = 0x21,
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
> +    ERASE4_SECTOR = 0xdc,
>
>      EN_4BYTE_ADDR = 0xB7,
>      EX_4BYTE_ADDR = 0xE9,
> @@ -307,6 +312,7 @@ typedef struct Flash {
>      bool write_enable;
>      bool four_bytes_address_mode;
>      bool reset_enable;
> +    bool quad_enable;

What is this used for?

>      bool initialized;
>      uint8_t reset_pin;
>      uint8_t ear;
> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD 
> cmd)
>
>      switch (cmd) {
>      case ERASE_4K:
> +    case ERASE4_4K:
>          len = 4 << 10;
>          capa_to_assert = ER_4K;
>          break;
> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD 
> cmd)
>          capa_to_assert = ER_32K;
>          break;
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>          len = s->pi->sector_size;
>          break;
>      case BULK_ERASE:
> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>
>  static inline int is_4bytes(Flash *s)
>  {
> +   switch (s->cmd_in_progress) {
> +   case PP4:
> +   case READ4:
> +   case QIOR4:
> +   case ERASE4_4K:
> +   case ERASE4_SECTOR:
> +       return 1;
> +   default:
>         return s->four_bytes_address_mode;
>     }
>  }
> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
>      case DPP:
>      case QPP:
>      case PP:
> +    case PP4:
>          s->state = STATE_PAGE_PROGRAM;
>          break;
>      case READ:
> +    case READ4:
>      case FAST_READ:
>      case DOR:
>      case QOR:
>      case DIOR:
>      case QIOR:
> +    case QIOR4:
>          s->state = STATE_READ;
>          break;
>      case ERASE_4K:
> +    case ERASE4_4K:
>      case ERASE_32K:
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>          break;
>      case WRSR:
> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
>      s->state = STATE_IDLE;
>      s->write_enable = false;
>      s->reset_enable = false;
> +    s->quad_enable = false;
>
>      DB_PRINT_L(0, "Reset done.\n");
>  }
> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>      s->cmd_in_progress = value;
> +    int i;

I can't see where i is used?

>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>
>      switch (value) {
>
>      case ERASE_4K:
> +    case ERASE4_4K:
>      case ERASE_32K:
>      case ERASE_SECTOR:
> +    case ERASE4_SECTOR:
>      case READ:
> +    case READ4:
>      case DPP:
>      case QPP:
>      case PP:
> -        s->needed_bytes = 3;
> +    case PP4:
> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
>          s->pos = 0;
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          s->len = 0;
>          s->state = STATE_COLLECTING_DATA;
>          break;
> -
> +    case QIOR4:
> +        /* 4 address + 1 dummy */
> +        s->needed_bytes = 5;
> +        s->pos = 0;
> +        s->len = 0;
> +        s->state = STATE_COLLECTING_DATA;
> +        break;

Blank line needed here. The blank is omitted between between logical
groupings of commands (such as read-write pairs or different types of
the same op) but there is no grouping between the data RW ops and
WRSR.

Regards,
Peter

>      case WRSR:
>          if (s->write_enable) {
>              s->needed_bytes = 1;
> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
>          VMSTATE_UINT8(ear, Flash),
>          VMSTATE_BOOL(reset_enable, Flash),
> +        VMSTATE_BOOL(quad_enable, Flash),
>          VMSTATE_BOOL(initialized, Flash),
>          VMSTATE_UINT8(reset_pin, Flash),
>          VMSTATE_END_OF_LIST()
> --
> 2.5.0
>
>