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

[Cédric Le Goater]

On 07/04/2016 06:03 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
> 
> 
> W dniu 04.07.2016 o 17:48, Cédric Le Goater pisze:
>> On 07/04/2016 05:23 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) wrote:
>>>
>>>
>>> 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 :)
>>
>> np.
>>
>>
>>>>>> 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...).
>>
>> Yes.  
>>
>> Linux still does not know about the mx25l25635f and it is behaving 
>> fine using the mx25l25635e definition. We should send a patch for 
>> it I guess. I have the HW, I will look into that when I have some 
>> time.
> Linux detects flash type by JEDEC code, so if the JEDEC is the same
> it will be hard to patch this. They are working on detection by JEDEC216,
> and then probably it will be possible to properly identify the flash.
>
>>>>> 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.
>>
>> ah yes, this is true. so we are fine on that side.
>>
>>>> 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?
>>
>> Before your patchset, we could get these:
>>
>>         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
>>
>> you have solved that. 
>>
>> So I guess we could just add : 
>>
>>  +    { INFO("mx25l25635f", 0xc22019,      0,  64 << 10, 512, 0) },
>>
>> but as we don't have any errors without them, I will just drop 
>> those oneliner patches. 
>
> I think you should add this line, since this model is supported.

OK. Next round then.

Thanks,

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