Re: [Qemu-arm] [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip

[Cédric Le Goater]

On 07/04/2016 02:57 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
>> -----Original Message-----
>> From: Qemu-devel [mailto:qemu-devel-
>> address@hidden On Behalf Of Cédric Le
>> Goater
>> Sent: Monday, July 04, 2016 2:19 PM
>> To: Peter Maydell <address@hidden>; Peter Crosthwaite
>> <address@hidden>
>> Cc: Andrew Jeffery <address@hidden>; address@hidden; qemu-
>> address@hidden; address@hidden; Cédric Le Goater
>> <address@hidden>
>> Subject: [Qemu-devel] [PATCH 2/7] m25p80: add mx25l25635f chip
>>
>> The Macronix chip mx25l25635f used on some OpenPower boxes is very
>> similar to the mx25l25635e. They share the same JEDEC identifier but the
>> WRSR instruction requires 2 bytes in the mx25l25635f case.
>>
>> To prevent some warnings on guests, let's introduce a new chip identifying
>> JEDEC 0xc22019 as a MX25L25635F chip.
>>
> Hello,
> 
> Adding fake JEDEC id is not good idea at all, we should somehow handle this 
> in the code.

It is not a fake JEDEC id. It is a real one from the datasheet :)

mx25l25635f and mx25l25635e share the same 0xc22019 JEDEC. 

>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/block/m25p80.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index
>> d9b27939dde6..aa964280beab 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -199,6 +199,7 @@ static const FlashPartInfo known_devices[] = {
>>      { INFO("mx25l12805d", 0xc22018,      0,  64 << 10, 256, 0) },
>>      { INFO("mx25l12855e", 0xc22618,      0,  64 << 10, 256, 0) },
>>      { INFO("mx25l25635e", 0xc22019,      0,  64 << 10, 512, 0) },
>> +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>      { INFO("mx25l25655e", 0xc22619,      0,  64 << 10, 512, 0) },
>>      { INFO("mx66u51235f", 0xc2253a,      0,  64 << 10, 1024, ER_4K | 
>> ER_32K) },
>>      { INFO("mx66u1g45g",  0xc2253b,      0,  64 << 10, 2048, ER_4K | 
>> ER_32K) },
>> @@ -991,6 +992,9 @@ static void decode_new_cmd(Flash *s, uint32_t
>> value)
>>              s->pos = 0;
>>              s->len = 0;
>>              s->state = STATE_COLLECTING_DATA;
>> +            if (!strcmp(s->pi->part_name, "mx25l25635f")) {
>> +                s->needed_bytes = 2;
>> +            }
>>          }
>>          break;
> Is it based on current master?

You are right. The patch has bit rotted, it still applies on qemu's HEAD
but at the wrong place :/ But, I don't think it is needed anymore, see 
below.

> In my last series (and current master) this case should be handled by this 
> code:
>             case MAN_MACRONIX:
>                 s->needed_bytes = 2;
>                 s->state = STATE_COLLECTING_VAR_LEN_DATA;
>                 break;
> I do not know what quest you are using, but linux mainline (at least 4.4.0) 
> is 
> sending only one byte, in WRSR so this will not work.

It is not Linux, it is a user space tool :

        
https://github.com/open-power/skiboot/blob/master/hw/ast-bmc/ast-sf-ctrl.c#L465

> Could you rebase to master and check if it will boot without your this change?

So the current code is fine for the mx25l25635f, which takes 2 bytes 
for the WRSR. But, now, how would the m25p80 object behave with the 
mx25l25635e ? Only one byte will be written.

Should we deprecate mx25l25635e ? It is not used in qemu.

Thanks,

C.