Re: [Qemu-devel] [PATCH v5 02/13] m25p80: Add support for SST READ ID 0x90/0xAB commands

[francisco iglesias]

On 29 October 2017 at 16:21, mar.krzeminski <address@hidden>
wrote:

>
>
> W dniu 29.10.2017 o 11:13, Francisco Iglesias pisze:
>
> Add support for SST READ ID 0x90/0xAB commands for reading out the flash
>> manufacuter ID and device ID.
>>
>> Signed-off-by: Francisco Iglesias <address@hidden>
>> Acked-by: Alistair Francis <address@hidden>
>> ---
>>   hw/block/m25p80.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 2971519..7a5c137 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -355,6 +355,8 @@ typedef enum {
>>       DPP = 0xa2,
>>       QPP = 0x32,
>>       QPP_4 = 0x34,
>> +    RDID_90 = 0x90,
>> +    RDID_AB = 0xab,
>>         ERASE_4K = 0x20,
>>       ERASE4_4K = 0x21,
>> @@ -405,6 +407,7 @@ typedef enum {
>>       MAN_MACRONIX,
>>       MAN_NUMONYX,
>>       MAN_WINBOND,
>> +    MAN_SST,
>>       MAN_GENERIC,
>>   } Manufacturer;
>>   @@ -476,6 +479,8 @@ static inline Manufacturer get_man(Flash *s)
>>           return MAN_SPANSION;
>>       case 0xC2:
>>           return MAN_MACRONIX;
>> +    case 0xBF:
>> +        return MAN_SST;
>>       default:
>>           return MAN_GENERIC;
>>       }
>> @@ -711,6 +716,22 @@ static void complete_collecting_data(Flash *s)
>>       case WEVCR:
>>           s->enh_volatile_cfg = s->data[0];
>>           break;
>> +    case RDID_90:
>> +    case RDID_AB:
>> +        if (get_man(s) == MAN_SST && s->cur_addr <= 1) {
>> +            if (s->cur_addr) {
>> +                s->data[0] = s->pi->id[2];
>> +                s->data[1] = s->pi->id[0];
>> +            } else {
>> +                s->data[0] = s->pi->id[0];
>> +                s->data[1] = s->pi->id[2];
>> +            }
>> +            s->pos = 0;
>> +            s->len = 2;
>> +            s->data_read_loop = true;
>> +            s->state = STATE_READING_DATA;
>>
> Do you know how the real HW will behave?
> When you get two bytes, it will send them once again or will wait for
> address?
>

Dear Marcin,

First of all thank you vey much for reviewing again! About your question
above, I have not tested it on HW but the SST flash datasheets for the
supported flashes in m25p80 state that "The manufacturer's and device ID
output stream is continuous until terminated by a low to high transition on
CE#.". Also in the diagram for the command in the datasheets it can be seen
that no address is needed (two continuous man/dev ids are read). This is
the reason to why data_read_loop is set to true above. Do you find this ok?



> +        }
>> +        break;
>>       default:
>>           break;
>>       }
>> @@ -926,6 +947,8 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>       case PP4:
>>       case PP4_4:
>>       case DIE_ERASE:
>> +    case RDID_90:
>> +    case RDID_AB:
>>           s->needed_bytes = get_addr_length(s);
>>
> If I understand correctly, for above commands allowed address length is 3
> bytes,
> but get_add_length can return 4. To avoid strange error I suggest to add
> case entry for them
> in get_addr_length.
>

About your suggestion above, I actually thought of this while creating the
patch but since I could not find support for 4 byte addresses in any of the
SST datasheets (and then no support for EN_4BYTE_ADDR (0xB7)), I could only
see get_addr_length to return 3 for these SST commands (and then the extra
cases shouldn't be needed), which is one of the reasons to why I decided to
add the commands to the above switch case. Do you find it ok to keep the
code as it is or would you like me to add the cases?

Best regards,
Francisco Iglesias


>
>           s->pos = 0;
>>           s->len = 0;
>>
> Regards,
> Marcin
>
>