Re: [Qemu-devel] Odp.: [PATCH 6/9] m25p80: Introduce quad and equad modes.

[Cédric Le Goater]

On 06/15/2016 07:40 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
> W dniu 15.06.2016 o 16:25, Cédric Le Goater pisze:
>> On 06/15/2016 03:41 PM, address@hidden wrote:
>>> From: Marcin Krzeminski <address@hidden>
>>>
>>> Quad and Equad modes for Spansion and Macronix flash devices.
>>> This commit also includes modification and new command to manipulate
>>> quad mode (status registers and dedicated commands).
>>> This work is based on Pawel Lenkow work.
>>>
>>> Signed-off-by: Marcin Krzeminski <address@hidden>
>>> ---
>>>  hw/block/m25p80.c | 70 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 65 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>>> index 55b4377..d1c4d46 100644
>>> --- a/hw/block/m25p80.c
>>> +++ b/hw/block/m25p80.c
>>> @@ -281,6 +281,7 @@ typedef enum {
>>>      JEDEC_READ = 0x9f,
>>>      BULK_ERASE = 0xc7,
>>>      READ_FSR = 0x70,
>>> +    RDCR = 0x15,
>>>
>>>      READ = 0x03,
>>>      READ4 = 0x13,
>>> @@ -317,6 +318,13 @@ typedef enum {
>>>      RESET_ENABLE = 0x66,
>>>      RESET_MEMORY = 0x99,
>>>
>>> +    /*
>>> +     * Micron: 0x35 - enable QPI
>>> +     * Spansion: 0x35 - read control register
>>> +     */
>>> +    RDCR_EQIO = 0x35,
>>> +    RSTQIO = 0xf5,
>>> +
>>>      RNVCR = 0xB5,
>>>      WNVCR = 0xB1,
>>>
>>> @@ -366,6 +374,7 @@ typedef struct Flash {
>>>      bool write_enable;
>>>      bool four_bytes_address_mode;
>>>      bool reset_enable;
>>> +    bool quad_enable;
>>>      uint8_t ear;
>>>
>>>      int64_t dirty_page;
>>> @@ -586,6 +595,16 @@ static void complete_collecting_data(Flash *s)
>>>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
>>>          break;
>>>      case WRSR:
>>> +        switch (get_man(s)) {
>>> +        case MAN_SPANSION:
>>> +            s->quad_enable = !!(s->data[1] & 0x02);
>>> +            break;
>>> +        case MAN_MACRONIX:
>>> +            s->quad_enable = extract32(s->data[0], 6, 1);
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>>          if (s->write_enable) {
>>>              s->write_enable = false;
>>>          }
>>> @@ -619,6 +638,7 @@ static void reset_memory(Flash *s)
>>>      s->state = STATE_IDLE;
>>>      s->write_enable = false;
>>>      s->reset_enable = false;
>>> +    s->quad_enable = false;
>>>
>>>      switch (get_man(s)) {
>>>      case MAN_NUMONYX:
>>> @@ -747,10 +767,20 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>>
>>>      case WRSR:
>>>          if (s->write_enable) {
>>> -            s->needed_bytes = 1;
>>> +            switch (get_man(s)) {
>>> +            case MAN_SPANSION:
>>> +                s->needed_bytes = 2;
>>> +                s->state = STATE_COLLECTING_DATA;
>>> +                break;
>>> +            case MAN_MACRONIX:
>>> +                s->needed_bytes = 2;
>>> +                s->state = STATE_COLLECTING_VAR_LEN_DATA;
>>> +                break;
>>
>> ah. I am glad you address this topic because I am having a little issue
>> with the mx25l25635e and the mx25l25635f.
>>
>> mx25l25635e is a macronix which supports the WRSR command with one byte 
>> and the mx25l25635f needs two. The fun part is that they have the same
>> JEDEC : 0xc22019.
>>
>> So not all macronix need 2 bytes. Should we add a flag for that ? 
>>
> I think this case should be partially handled with path 4 and this one.
> The new state (patch 4) was introduced to allow use variable write cmds.
> As in my case mx66u51235f allow write up to 2 bytes, but linux writes only 
> one,
> and that is allowed and works on real HW. From the datasheet mx25l25635f
> should behave exactly same way.
> In case you mention, the problem might pop up, when guest will try
> to write 2 bytes to mx25l25635e. Real HW will treat second byte as new 
> command,
> but this model accept this situation.

qemu complains a bit but that's OK.

> Generally my approach and I think original author was to not emulate exactly
> real hw behaviour but allow to model it in such way the quest can boot up and 
> use it.
> If we go witch very restricted command behaviour we will end up in unreadable 
> code
> (eg. newest Spansions/Cypress family does not support extended address 
> register,
> but such commands will work in this model). The question is if you really 
> need to flag
> and support this. If yes additional if is not a problem.

I will wait for your patchset to be merged and then see if the mx25l25635f
needs anything more. This is really minor. It should be a onliner.

Thanks,

C. 

>>
>>> +            default:
>>> +                s->needed_bytes = 1;
>>> +                s->state = STATE_COLLECTING_DATA;
>>> +            }
>>>              s->pos = 0;
>>> -            s->len = 0;
>>> -            s->state = STATE_COLLECTING_DATA;
>>>          }
>>>          break;
>>>
>>> @@ -763,6 +793,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>>
>>>      case RDSR:
>>>          s->data[0] = (!!s->write_enable) << 1;
>>> +        if (get_man(s) == MAN_MACRONIX) {
>>> +            s->data[0] |= (!!s->quad_enable) << 6;
>>> +        }
>>>          s->pos = 0;
>>>          s->len = 1;
>>>          s->state = STATE_READING_DATA;
>>> @@ -789,6 +822,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>>          s->state = STATE_READING_DATA;
>>>          break;
>>>
>>> +    case RDCR:
>>> +        s->data[0] = s->volatile_cfg & 0xFF;
>>> +        s->data[0] |= (!!s->four_bytes_address_mode) << 5;
>>> +        s->pos = 0;
>>> +        s->len = 1;
>>> +        s->state = STATE_READING_DATA;
>>> +        break;
>>> +
>>>      case BULK_ERASE:
>>>          if (s->write_enable) {
>>>              DB_PRINT_L(0, "chip erase\n");
>>> @@ -828,7 +869,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>>          s->state = STATE_READING_DATA;
>>>          break;
>>>      case WNVCR:
>>> -        if (s->write_enable) {
>>> +        if (s->write_enable && get_man(s) == MAN_NUMONYX) {
>>>              s->needed_bytes = 2;
>>>              s->pos = 0;
>>>              s->len = 0;
>>> @@ -871,6 +912,24 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>>              reset_memory(s);
>>>          }
>>>          break;
>>> +    case RDCR_EQIO:
>>> +        switch (get_man(s)) {
>>> +        case MAN_SPANSION:
>>> +            s->data[0] = (!!s->quad_enable) << 1;
>>> +            s->pos = 0;
>>> +            s->len = 1;
>>> +            s->state = STATE_READING_DATA;
>>> +            break;
>>> +        case MAN_MACRONIX:
>>> +            s->quad_enable = true;
>>> +            break;
>>> +        default:
>>> +            break;
>>> +        }
>>> +        break;
>>> +    case RSTQIO:
>>> +        s->quad_enable = false;
>>> +        break;
>>>      default:
>>>          qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>>>          break;
>>> @@ -999,7 +1058,7 @@ static Property m25p80_properties[] = {
>>>
>>>  static const VMStateDescription vmstate_m25p80 = {
>>>      .name = "xilinx_spi",
>>> -    .version_id = 2,
>>> +    .version_id = 3,
>>>      .minimum_version_id = 1,
>>>      .pre_save = m25p80_pre_save,
>>>      .fields = (VMStateField[]) {
>>> @@ -1017,6 +1076,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>>          VMSTATE_UINT32_V(nonvolatile_cfg, Flash, 2),
>>>          VMSTATE_UINT32_V(volatile_cfg, Flash, 2),
>>>          VMSTATE_UINT32_V(enh_volatile_cfg, Flash, 2),
>>> +        VMSTATE_BOOL_V(quad_enable, Flash, 3),
>>>          VMSTATE_END_OF_LIST()
>>>      }
>>>  };
>>>
>>
>>
>>
>