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- >>>>>> bounces+marcin.krzeminski=nokia....@nongnu.org] On Behalf Of Cédric Le >>>>>> Goater >>>>>> Sent: Monday, July 04, 2016 2:19 PM >>>>>> To: Peter Maydell <peter.mayd...@linaro.org>; Peter Crosthwaite >>>>>> <crosthwaite.pe...@gmail.com> >>>>>> Cc: Andrew Jeffery <and...@aj.id.au>; qemu-...@nongnu.org; qemu- >>>>>> de...@nongnu.org; mar.krzemin...@gmail.com; Cédric Le Goater >>>>>> <c...@kaod.org> >>>>>> 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 <c...@kaod.org> >>>>>> --- >>>>>> 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. >> >
