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

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

W dniu 04.07.2016 o 15:41, Cédric Le Goater pisze:
> 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. 
Sorry, I misunderstand and haven't check in data-sheet :)
>
>
>>> 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
I meant spi-nor framework in Linux, if you are using different tool it is 
different story,
but should work for both (if real hw works for both...).
>
>
>> 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.
It should behave properly (as in my case spi-nor framework in Linux writes only
one byte for Macronix in WSRS). COLLECTING_VAR_LEN_DATA state apply changes
(call complete_collecting_data) when CS go high. If it takes one byte instead 
of two,
one byte will be written to SR.
>
>
> Should we deprecate mx25l25635e ? It is not used in qemu.
That is good question, I do not see any flash with same JEDEC in the code, so
this one could be the first if you leave both of them.
You mentioned about warnings when you configure Qemu to use mx25l25635e,
instead of mx25l25635f, in my it should not matter, what are those warnings?

Regards,
Marcin
>
>
> Thanks,
>
> C. 
>