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

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

W dniu 21.12.2015 o 12:52, Peter Crosthwaite pisze:
> 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?
Should be in patch 09/12.
I will correct this.
Thanks,
Marcin
>
>
>>      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
>>
>>
>
>