| Message ID | 1467634738-28642-3-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Qemu-devel [mailto:qemu-devel- > bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Cédric Le > Goater > Sent: Monday, July 04, 2016 2:19 PM > To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite > <crosthwaite.peter@gmail.com> > Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu- > devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater > <clg@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. > Signed-off-by: Cédric Le Goater <clg@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? 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. Could you rebase to master and check if it will boot without your this change? Thanks, Marcin > case RNVCR: > -- > 2.1.4 >
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.com@nongnu.org] On Behalf Of Cédric Le >> Goater >> Sent: Monday, July 04, 2016 2:19 PM >> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite >> <crosthwaite.peter@gmail.com> >> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu- >> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater >> <clg@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. >> Signed-off-by: Cédric Le Goater <clg@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 > 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.
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.com@nongnu.org] On Behalf Of Cédric Le >>> Goater >>> Sent: Monday, July 04, 2016 2:19 PM >>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite >>> <crosthwaite.peter@gmail.com> >>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu- >>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater >>> <clg@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 :) > > >>> Signed-off-by: Cédric Le Goater <clg@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...). > > >> 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. >
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.com@nongnu.org] On Behalf Of Cédric Le >>>> Goater >>>> Sent: Monday, July 04, 2016 2:19 PM >>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite >>>> <crosthwaite.peter@gmail.com> >>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu- >>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater >>>> <clg@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 <clg@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. >>> 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. Thanks, C.
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.com@nongnu.org] On Behalf Of Cédric Le >>>>> Goater >>>>> Sent: Monday, July 04, 2016 2:19 PM >>>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite >>>>> <crosthwaite.peter@gmail.com> >>>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu- >>>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater >>>>> <clg@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 <clg@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. Thanks, Marcin > > > Thanks, > > C. >
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.com@nongnu.org] On Behalf Of Cédric Le >>>>>> Goater >>>>>> Sent: Monday, July 04, 2016 2:19 PM >>>>>> To: Peter Maydell <peter.maydell@linaro.org>; Peter Crosthwaite >>>>>> <crosthwaite.peter@gmail.com> >>>>>> Cc: Andrew Jeffery <andrew@aj.id.au>; qemu-arm@nongnu.org; qemu- >>>>>> devel@nongnu.org; mar.krzeminski@gmail.com; Cédric Le Goater >>>>>> <clg@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 <clg@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. >> >
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; case RNVCR:
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. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/block/m25p80.c | 4 ++++ 1 file changed, 4 insertions(+)