Message ID | 20200103223423.14025-1-michael@walle.cc |
---|---|
State | Superseded |
Delegated to: | Ambarus Tudor |
Headers | show |
Series | mtd: spi-nor: Add support for w25qNNjwim | expand |
Hi, Michael, On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote: > Add support for the Winbond W25QnnJW-IM flashes. These have a > programmable QE bit. There are also the W25QnnJW-IQ variant which shares > the ID with the W25QnnFW parts. These have the QE bit hard strapped to > 1, thus don't support hardware write protection. There are few flavors of hw write protection supported by this flash, the Q version does not disable them all. How about saying just that the /HOLD function is disabled? When we receive new flash id patches, we ask the contributors to specify if they test the flash, in which modes (single, quad), and with which controller. Ideally all the flash's flags should be tested, but there are cases in which the controllers do not support quad read for example, and we accept the patches even if tested in single read mode. SPI_NOR_HAS_LOCK and SPI_NOR_HAS_TB must be tested as well. Even if the patches are rather simple, we ask for this to be sure that we don't add a flash that is broken from day one. So, would you please tell us what flashes did you test, what flags, and with which controller? > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index addb6319fcbb..3fa8a81bdab0 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = { > SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) > }, > + { > + "w25q16jwim", INFO(0xef8015, 0, 64 * 1024, 32, "i" is for the temperature range, which is not a fixed characteristic. Usually there are flashes with the same jedec-id, but with different temperature ranges. Let's drop the "i" and rename it to "w25q16jwm" Cheers, ta
Hi Tudor, Am 2020-01-11 15:19, schrieb Tudor.Ambarus@microchip.com: > Hi, Michael, > > On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote: >> Add support for the Winbond W25QnnJW-IM flashes. These have a >> programmable QE bit. There are also the W25QnnJW-IQ variant which >> shares >> the ID with the W25QnnFW parts. These have the QE bit hard strapped to >> 1, thus don't support hardware write protection. > > There are few flavors of hw write protection supported by this flash, > the Q > version does not disable them all. How about saying just that the /HOLD > function is disabled? I don't get your point here ;) My understanding is that HOLD# and WP# will be disabled. Thus there is no "hardware write protection". What other hw write protection do you have in mind? > When we receive new flash id patches, we ask the contributors to > specify if > they test the flash, in which modes (single, quad), and with which > controller. > Ideally all the flash's flags should be tested, but there are cases in > which > the controllers do not support quad read for example, and we accept the > patches even if tested in single read mode. SPI_NOR_HAS_LOCK and > SPI_NOR_HAS_TB must be tested as well. > > Even if the patches are rather simple, we ask for this to be sure that > we > don't add a flash that is broken from day one. So, would you please > tell us > what flashes did you test, what flags, and with which controller? Ok will add that to the commit message. Just to make sure. I've only tested the 32mbit part. So is it still ok to include all other flashes of this family? For now. tested with the NXP FlexSPI, single and dual (no quad since we are using the write protection feature and IO2 and IO3 are not connected to the CPU). So write protection is also tested. I will retest the TB bit. >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c >> b/drivers/mtd/spi-nor/spi-nor.c >> index addb6319fcbb..3fa8a81bdab0 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = >> { >> SECT_4K | SPI_NOR_DUAL_READ | > SPI_NOR_QUAD_READ | >> SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) >> }, >> + { >> + "w25q16jwim", INFO(0xef8015, 0, 64 * 1024, 32, > > "i" is for the temperature range, which is not a fixed characteristic. > Usually > there are flashes with the same jedec-id, but with different > temperature > ranges. Let's drop the "i" and rename it to "w25q16jwm" Only that there is no flash with that part name :( according to the datasheet there is only this one temp range available. From what I've seen for now (esp looking at the macronix parts) it seems to first come first serve ;) That being said, I don't insist on keeping that name, I'm fine with any name, since I've learned you cannot rely on it in any way. Eg. the w25q32jwiq will be discovered as w25q32dw. Or some Macronix flashes will be discovered as ancient ones. Btw. is renaming the flashes also considered a backwards incomaptible change? And can there be two flashes with the same name? Because IMHO it would be better to just have the name "w25q16" regardless whether its an FW/JW/JV etc. It's better to show an ambiguous name than a wrong name. -michael
Hi, Michael, On Sunday, January 12, 2020 1:16:12 AM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, > > Am 2020-01-11 15:19, schrieb Tudor.Ambarus@microchip.com: > > Hi, Michael, > > > > On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote: > >> Add support for the Winbond W25QnnJW-IM flashes. These have a > >> programmable QE bit. There are also the W25QnnJW-IQ variant which > >> shares > >> the ID with the W25QnnFW parts. These have the QE bit hard strapped to > >> 1, thus don't support hardware write protection. > > > > There are few flavors of hw write protection supported by this flash, > > the Q > > version does not disable them all. How about saying just that the /HOLD > > function is disabled? > > I don't get your point here ;) My understanding is that HOLD# and WP# > will > be disabled. Thus there is no "hardware write protection". What other hw > write protection do you have in mind? Time delay write disable after Power-up for example. > > > When we receive new flash id patches, we ask the contributors to > > specify if > > they test the flash, in which modes (single, quad), and with which > > controller. > > Ideally all the flash's flags should be tested, but there are cases in > > which > > the controllers do not support quad read for example, and we accept the > > patches even if tested in single read mode. SPI_NOR_HAS_LOCK and > > SPI_NOR_HAS_TB must be tested as well. > > > > Even if the patches are rather simple, we ask for this to be sure that > > we > > don't add a flash that is broken from day one. So, would you please > > tell us > > what flashes did you test, what flags, and with which controller? > > Ok will add that to the commit message. Just to make sure. I've only > tested the > 32mbit part. So is it still ok to include all other flashes of this > family? No, just the ones that you can test please. > > For now. tested with the NXP FlexSPI, single and dual (no quad since we > are > using the write protection feature and IO2 and IO3 are not connected to > the > CPU). So write protection is also tested. I will retest the TB bit. Great, thanks. > > >> Signed-off-by: Michael Walle <michael@walle.cc> > >> --- > >> > >> drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/drivers/mtd/spi-nor/spi-nor.c > >> b/drivers/mtd/spi-nor/spi-nor.c > >> index addb6319fcbb..3fa8a81bdab0 100644 > >> --- a/drivers/mtd/spi-nor/spi-nor.c > >> +++ b/drivers/mtd/spi-nor/spi-nor.c > >> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = > >> { > >> > >> SECT_4K | SPI_NOR_DUAL_READ | > > > > SPI_NOR_QUAD_READ | > > > >> SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) > >> > >> }, > >> > >> + { > >> + "w25q16jwim", INFO(0xef8015, 0, 64 * 1024, 32, > > > > "i" is for the temperature range, which is not a fixed characteristic. > > Usually > > there are flashes with the same jedec-id, but with different > > temperature > > ranges. Let's drop the "i" and rename it to "w25q16jwm" > > Only that there is no flash with that part name :( according to the > datasheet The datasheet describes the W25Q32JW flash (check the first page of the datasheet). There are two flavors of this flash, each with its own jedec-id: Q version uses 156016h, M 158016h. We should name this flashes as "w25q32jwq" and "w25q32jwm". Please notice that I skipped intentionally the "i" that stands for temperature range. Manufacturers can provide better temperature ranges for the same flash without changing the jedec-id. See this datasheet: https://ro.mouser.com/datasheet/2/949/w25q128jv_revf_03272018_plus-1489608.pdf > there is only this one temp range available. From what I've seen for now > (esp > looking at the macronix parts) it seems to first come first serve ;) > That being said, I don't insist on keeping that name, I'm fine with any > name, you should be fine just with the name that best describes the flash :) > since I've learned you cannot rely on it in any way. Eg. the w25q32jwiq > will > be discovered as w25q32dw. Or some Macronix flashes will be discovered > as > ancient ones. Would you please study what's wrong with these names and provide a patch to fix them? > > Btw. is renaming the flashes also considered a backwards incomaptible > change? No, we can fix the names. > And can there be two flashes with the same name? Because IMHO it would > be I would prefer that we don't. Why would you have two different jedec-ids with the same name? Cheers, ta > better to just have the name "w25q16" regardless whether its an FW/JW/JV > etc. > It's better to show an ambiguous name than a wrong name. > > -michael
Hi Tudor, Am 2020-01-13 10:06, schrieb Tudor.Ambarus@microchip.com: > Hi, Michael, > > On Sunday, January 12, 2020 1:16:12 AM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> >> Hi Tudor, >> >> Am 2020-01-11 15:19, schrieb Tudor.Ambarus@microchip.com: >> > Hi, Michael, >> > >> > On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote: >> >> Add support for the Winbond W25QnnJW-IM flashes. These have a >> >> programmable QE bit. There are also the W25QnnJW-IQ variant which >> >> shares >> >> the ID with the W25QnnFW parts. These have the QE bit hard strapped to >> >> 1, thus don't support hardware write protection. >> > >> > There are few flavors of hw write protection supported by this flash, >> > the Q >> > version does not disable them all. How about saying just that the /HOLD >> > function is disabled? >> >> I don't get your point here ;) My understanding is that HOLD# and WP# >> will >> be disabled. Thus there is no "hardware write protection". What other >> hw >> write protection do you have in mind? > > Time delay write disable after Power-up for example. That is the usual "write enable" mechanism. while marketing may seem to see that as write protection, I do not, esp. not hardware write protection. What about changing it to the following? "These have the QE bit hard strapped to 1, thus don't the /HOLD and /WP pins". >> >> > When we receive new flash id patches, we ask the contributors to >> > specify if >> > they test the flash, in which modes (single, quad), and with which >> > controller. >> > Ideally all the flash's flags should be tested, but there are cases in >> > which >> > the controllers do not support quad read for example, and we accept the >> > patches even if tested in single read mode. SPI_NOR_HAS_LOCK and >> > SPI_NOR_HAS_TB must be tested as well. >> > >> > Even if the patches are rather simple, we ask for this to be sure that >> > we >> > don't add a flash that is broken from day one. So, would you please >> > tell us >> > what flashes did you test, what flags, and with which controller? >> >> Ok will add that to the commit message. Just to make sure. I've only >> tested the >> 32mbit part. So is it still ok to include all other flashes of this >> family? > > No, just the ones that you can test please. ok >> For now. tested with the NXP FlexSPI, single and dual (no quad since >> we >> are >> using the write protection feature and IO2 and IO3 are not connected >> to >> the >> CPU). So write protection is also tested. I will retest the TB bit. > > Great, thanks. > >> >> Signed-off-by: Michael Walle <michael@walle.cc> >> >> --- >> >> >> >> drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++ >> >> 1 file changed, 22 insertions(+) >> >> >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c >> >> b/drivers/mtd/spi-nor/spi-nor.c >> >> index addb6319fcbb..3fa8a81bdab0 100644 >> >> --- a/drivers/mtd/spi-nor/spi-nor.c >> >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> >> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = >> >> { >> >> >> >> SECT_4K | SPI_NOR_DUAL_READ | >> > >> > SPI_NOR_QUAD_READ | >> > >> >> SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) >> >> >> >> }, >> >> >> >> + { >> >> + "w25q16jwim", INFO(0xef8015, 0, 64 * 1024, 32, >> > >> > "i" is for the temperature range, which is not a fixed characteristic. >> > Usually >> > there are flashes with the same jedec-id, but with different >> > temperature >> > ranges. Let's drop the "i" and rename it to "w25q16jwm" >> >> Only that there is no flash with that part name :( according to the >> datasheet > > The datasheet describes the W25Q32JW flash (check the first page of the > datasheet). There are two flavors of this flash, each with its own > jedec-id: Q > version uses 156016h, M 158016h. We should name this flashes as > "w25q32jwq" > and "w25q32jwm". You mean ef6016 and ef8016, yes that is correct. My point was there is no "w25q15jwm": If linux kernel messages write "detected w25q32jwm" nobody will know what that part is, because there is no such part. There is a w25q32jwim or maybe w25q32jwXm but no w25q32jwm. And naming the Q version w25q32jwq is not possible, because the id is shared with the w25q32dw, which is already in spi-nor.c. > Please notice that I skipped intentionally the "i" that > stands for temperature range. Manufacturers can provide better > temperature > ranges for the same flash without changing the jedec-id. See this > datasheet: I know that and get your point. But I fear that this will confuse others. > > https://ro.mouser.com/datasheet/2/949/w25q128jv_revf_03272018_plus-1489608.pdf > >> there is only this one temp range available. From what I've seen for >> now >> (esp >> looking at the macronix parts) it seems to first come first serve ;) >> That being said, I don't insist on keeping that name, I'm fine with >> any >> name, > > you should be fine just with the name that best describes the flash :) > >> since I've learned you cannot rely on it in any way. Eg. the >> w25q32jwiq >> will >> be discovered as w25q32dw. Or some Macronix flashes will be discovered >> as >> ancient ones. > > Would you please study what's wrong with these names and provide a > patch to > fix them? Well, I've did that last week. But TBH I don't know if I want to go down that road. Its not only the names, its also the flags. There is a mix of old and new flashes in spi-nor.c; for example the newer ones supports dual and quad mode. But the crux is, Macronix shares the same id over different generations of the flash chip. For example look at the MX25L8005 (as it is supported in the kernel) [1]. It doesn't support dual I/O mode. But the newer generation MX25L8006E, which has the same id, supports it. So even I could come up with something to fix it, I doubt it will be accepted without testing. >> >> Btw. is renaming the flashes also considered a backwards incomaptible >> change? > > No, we can fix the names. > >> And can there be two flashes with the same name? Because IMHO it would >> be > > I would prefer that we don't. Why would you have two different > jedec-ids with > the same name? Because as pointed out in the Winbond example you cannot distiguish between W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 and MX25L8006E. Thus my reasoning was to show only the common part, ie W25Q32 or MX25L80 which should be the same for this particular ID. Like I said, I'd prefer showing an ambiguous name instead of a wrong one. But then you may have different IDs with the same ambiguous name. >> better to just have the name "w25q16" regardless whether its an >> FW/JW/JV >> etc. >> It's better to show an ambiguous name than a wrong name. >> >> -michael -michael [1] http://web.archive.org/web/20180712194807/https://www.mct.net/download/macronix/mx25l8005.pdf
Hi Tudor, Am 2020-01-13 11:07, schrieb Michael Walle: >>> >>> Btw. is renaming the flashes also considered a backwards incomaptible >>> change? >> >> No, we can fix the names. >> >>> And can there be two flashes with the same name? Because IMHO it >>> would >>> be >> >> I would prefer that we don't. Why would you have two different >> jedec-ids with >> the same name? > > Because as pointed out in the Winbond example you cannot distiguish > between > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 > and > MX25L8006E. Thus my reasoning was to show only the common part, ie > W25Q32 > or MX25L80 which should be the same for this particular ID. Like I > said, I'd > prefer showing an ambiguous name instead of a wrong one. But then you > may > have different IDs with the same ambiguous name. Another solution would be to have the device tree provide a hint for the actual flash chip. There would be multiple entries in the spi_nor_ids with the same flash id. By default the first one is used (keeping the current behaviour). If there is for example compatible = "jedec,spi-nor", "w25q32jwq"; the flash_info for the w25q32jwq will be chosen. I know this will conflict with the new rule that there should only be compatible = "jedec,spi-nor"; without the actual flash chip. But it seems that it is not always possible to just use the jedec id to match the correct chip. Also see for example mx25l25635_post_bfpt_fixups() which tries to figure out different behaviour by looking at "some" SFDP data. In this case we might have been lucky, but I fear that this won't work in all cases and for older flashes it won't work at all. BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. I guess that would be a less invasive way to fix different flashes with same jedec ids. -michael
On Monday, January 13, 2020 3:15:15 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, Hi, Michael, > > Am 2020-01-13 11:07, schrieb Michael Walle: > >>> Btw. is renaming the flashes also considered a backwards incomaptible > >>> change? > >> > >> No, we can fix the names. > >> > >>> And can there be two flashes with the same name? Because IMHO it > >>> would > >>> be > >> > >> I would prefer that we don't. Why would you have two different > >> jedec-ids with > >> the same name? > > > > Because as pointed out in the Winbond example you cannot distiguish > > between > > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 > > and > > MX25L8006E. Thus my reasoning was to show only the common part, ie > > W25Q32 > > or MX25L80 which should be the same for this particular ID. Like I > > said, I'd > > prefer showing an ambiguous name instead of a wrong one. But then you > > may > > have different IDs with the same ambiguous name. > > Another solution would be to have the device tree provide a hint for the > actual flash chip. There would be multiple entries in the spi_nor_ids > with the > same flash id. By default the first one is used (keeping the current > behaviour). If there is for example > > compatible = "jedec,spi-nor", "w25q32jwq"; > > the flash_info for the w25q32jwq will be chosen. This won't work for plug-able flashes. You will influence the name in dt to be chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will end up with a wrong name for w25q32dw, thus the same problem. If the flashes are identical but differ just in terms of name, we can rename the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences between these flashes; if you want to fix them, send a patch and I'll try to help. Cheers, ta > > I know this will conflict with the new rule that there should only be > > compatible = "jedec,spi-nor"; > > without the actual flash chip. But it seems that it is not always > possible > to just use the jedec id to match the correct chip. > > Also see for example mx25l25635_post_bfpt_fixups() which tries to figure > out different behaviour by looking at "some" SFDP data. In this case we > might have been lucky, but I fear that this won't work in all cases and > for older flashes it won't work at all. > > BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. > > I guess that would be a less invasive way to fix different flashes with > same jedec ids. > > -michael
Hi Tudor, >> Am 2020-01-13 11:07, schrieb Michael Walle: >> >>> Btw. is renaming the flashes also considered a backwards incomaptible >> >>> change? >> >> >> >> No, we can fix the names. >> >> >> >>> And can there be two flashes with the same name? Because IMHO it >> >>> would >> >>> be >> >> >> >> I would prefer that we don't. Why would you have two different >> >> jedec-ids with >> >> the same name? >> > >> > Because as pointed out in the Winbond example you cannot distiguish >> > between >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 >> > and >> > MX25L8006E. Thus my reasoning was to show only the common part, ie >> > W25Q32 >> > or MX25L80 which should be the same for this particular ID. Like I >> > said, I'd >> > prefer showing an ambiguous name instead of a wrong one. But then you >> > may >> > have different IDs with the same ambiguous name. >> >> Another solution would be to have the device tree provide a hint for >> the >> actual flash chip. There would be multiple entries in the spi_nor_ids >> with the >> same flash id. By default the first one is used (keeping the current >> behaviour). If there is for example >> >> compatible = "jedec,spi-nor", "w25q32jwq"; >> >> the flash_info for the w25q32jwq will be chosen. > > This won't work for plug-able flashes. You will influence the name in > dt to be > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will > end up > with a wrong name for w25q32dw, thus the same problem. No, because then the device tree is wrong and doesn't fit the hardware. You'd have to some instance which could change the device tree node, like the bootloader or some device tree overlay for plugable flashes. We should try to solve the actual problem at hand first.. It is just not possible to autodetect the SPI flash, just because the vendors reuse the same IDs for flashes with different features (and the SFDP is likely not enough). Therefore, you need to have a hint in some place to use the flash properly. > If the flashes are identical but differ just in terms of name, we can > rename > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences > between > these flashes; if you want to fix them, send a patch and I'll try to > help. It is not only the name, here are two examples which differ in functionality: (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports dual/quad mode (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit. well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third example. -michael > > Cheers, > ta > >> >> I know this will conflict with the new rule that there should only be >> >> compatible = "jedec,spi-nor"; >> >> without the actual flash chip. But it seems that it is not always >> possible >> to just use the jedec id to match the correct chip. >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to >> figure >> out different behaviour by looking at "some" SFDP data. In this case >> we >> might have been lucky, but I fear that this won't work in all cases >> and >> for older flashes it won't work at all. >> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. >> >> I guess that would be a less invasive way to fix different flashes >> with >> same jedec ids. >> >> -michael
On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, Hi, Michael, > > >> Am 2020-01-13 11:07, schrieb Michael Walle: > >> >>> Btw. is renaming the flashes also considered a backwards incomaptible > >> >>> change? > >> >> > >> >> No, we can fix the names. > >> >> > >> >>> And can there be two flashes with the same name? Because IMHO it > >> >>> would > >> >>> be > >> >> > >> >> I would prefer that we don't. Why would you have two different > >> >> jedec-ids with > >> >> the same name? > >> > > >> > Because as pointed out in the Winbond example you cannot distiguish > >> > between > >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 > >> > and > >> > MX25L8006E. Thus my reasoning was to show only the common part, ie > >> > W25Q32 > >> > or MX25L80 which should be the same for this particular ID. Like I > >> > said, I'd > >> > prefer showing an ambiguous name instead of a wrong one. But then you > >> > may > >> > have different IDs with the same ambiguous name. > >> > >> Another solution would be to have the device tree provide a hint for > >> the > >> actual flash chip. There would be multiple entries in the spi_nor_ids > >> with the > >> same flash id. By default the first one is used (keeping the current > >> behaviour). If there is for example > >> > >> compatible = "jedec,spi-nor", "w25q32jwq"; > >> > >> the flash_info for the w25q32jwq will be chosen. > > > > This won't work for plug-able flashes. You will influence the name in > > dt to be > > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will > > end up > > with a wrong name for w25q32dw, thus the same problem. > > No, because then the device tree is wrong and doesn't fit the hardware. > You'd > have to some instance which could change the device tree node, like the > bootloader or some device tree overlay for plugable flashes. We should > try to > solve the actual problem at hand first.. > > It is just not possible to autodetect the SPI flash, just because > the vendors reuse the same IDs for flashes with different features (and > the > SFDP is likely not enough). Therefore, you need to have a hint in some > place > to use the flash properly. > > > If the flashes are identical but differ just in terms of name, we can > > rename > > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences > > between > > these flashes; if you want to fix them, send a patch and I'll try to > > help. > > It is not only the name, here are two examples which differ in > functionality: > (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports > dual/quad > mode > (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit. > > well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third sorry if this exhausted you. > example. > Flash auto-detection is nice and we should preserve it if possible. I would prefer having a post bfpt fixup than giving a hint about the flash in the compatible. The flashes that you mention are quite old and I don't know if it is worth to harm the auto-detection for them. A compromise has to be made. You can gain traction in your endeavor if you have such a flash and there's nothing auto-detectable that differentiates it from some other flash that shares the sama jedec-id. If you have such a flash and you care about it, send a patch and I'll try to help. > -michael > > > Cheers, > > ta > > > >> I know this will conflict with the new rule that there should only be > >> > >> compatible = "jedec,spi-nor"; > >> > >> without the actual flash chip. But it seems that it is not always > >> possible > >> to just use the jedec id to match the correct chip. > >> > >> Also see for example mx25l25635_post_bfpt_fixups() which tries to > >> figure > >> out different behaviour by looking at "some" SFDP data. In this case > >> we > >> might have been lucky, but I fear that this won't work in all cases > >> and > >> for older flashes it won't work at all. > >> > >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. > >> > >> I guess that would be a less invasive way to fix different flashes > >> with > >> same jedec ids. > >> > >> -michael
Hi Tudor, Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com: > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> >> Hi Tudor, > > Hi, Michael, > >> >> >> Am 2020-01-13 11:07, schrieb Michael Walle: >> >> >>> Btw. is renaming the flashes also considered a backwards incomaptible >> >> >>> change? >> >> >> >> >> >> No, we can fix the names. >> >> >> >> >> >>> And can there be two flashes with the same name? Because IMHO it >> >> >>> would >> >> >>> be >> >> >> >> >> >> I would prefer that we don't. Why would you have two different >> >> >> jedec-ids with >> >> >> the same name? >> >> > >> >> > Because as pointed out in the Winbond example you cannot distiguish >> >> > between >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005 >> >> > and >> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie >> >> > W25Q32 >> >> > or MX25L80 which should be the same for this particular ID. Like I >> >> > said, I'd >> >> > prefer showing an ambiguous name instead of a wrong one. But then you >> >> > may >> >> > have different IDs with the same ambiguous name. >> >> >> >> Another solution would be to have the device tree provide a hint for >> >> the >> >> actual flash chip. There would be multiple entries in the spi_nor_ids >> >> with the >> >> same flash id. By default the first one is used (keeping the current >> >> behaviour). If there is for example >> >> >> >> compatible = "jedec,spi-nor", "w25q32jwq"; >> >> >> >> the flash_info for the w25q32jwq will be chosen. >> > >> > This won't work for plug-able flashes. You will influence the name in >> > dt to be >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will >> > end up >> > with a wrong name for w25q32dw, thus the same problem. >> >> No, because then the device tree is wrong and doesn't fit the >> hardware. >> You'd >> have to some instance which could change the device tree node, like >> the >> bootloader or some device tree overlay for plugable flashes. We should >> try to >> solve the actual problem at hand first.. >> >> It is just not possible to autodetect the SPI flash, just because >> the vendors reuse the same IDs for flashes with different features >> (and >> the >> SFDP is likely not enough). Therefore, you need to have a hint in some >> place >> to use the flash properly. >> >> > If the flashes are identical but differ just in terms of name, we can >> > rename >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences >> > between >> > these flashes; if you want to fix them, send a patch and I'll try to >> > help. >> >> It is not only the name, here are two examples which differ in >> functionality: >> (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports >> dual/quad >> mode >> (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit. >> >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third > > sorry if this exhausted you. TBH, this is no fun (and I'm doing this on my spare time because I like open source). I guess our opinions differ waaay too much. I don't really like band-aid fixes; eg. with vague information "it seems that the F version adveritses support for Fast Read 4-4-4", what about other flashes with that idcode and this property. This might break at any time or with anyone trying support for other flashes with that ID. That's what I've meant with first come first serve, I'm lucky now that there was no flash with the same jedec id as the W25Q32JW. To add the MX25U3232F I could check the JEDEC revision (or the BFPT length) because it differers from the MX25U3235F. But I don't feel well doing that. Who says Macronix won't update their description for the MX25U3235F to the new revision.. FYI the Winbond guys apparently use the first OTP region to store the JEDEC data, which is clever because they can update it during production. >> example. >> > > Flash auto-detection is nice and we should preserve it if possible. I > would > prefer having a post bfpt fixup than giving a hint about the flash in > the > compatible. see above. > The flashes that you mention are quite old and I don't know if it > is worth to harm the auto-detection for them. A compromise has to be > made. so you'd drop support for them? because SFDP is never read if there is no DUAL_READ or QUAD_READ flag. > You can gain traction in your endeavor if you have such a flash and > there's > nothing auto-detectable that differentiates it from some other flash > that > shares the sama jedec-id. > > If you have such a flash and you care about it, send a patch and I'll > try to > help. Given my reasoning above.. well maybe in the future. The Macronix would be a second source candidate. For now we are using the Winbond flash. I would rather like to have the flash protection topic and OTP support sorted out, because that is something we are actually using. -michael > >> -michael >> >> > Cheers, >> > ta >> > >> >> I know this will conflict with the new rule that there should only be >> >> >> >> compatible = "jedec,spi-nor"; >> >> >> >> without the actual flash chip. But it seems that it is not always >> >> possible >> >> to just use the jedec id to match the correct chip. >> >> >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to >> >> figure >> >> out different behaviour by looking at "some" SFDP data. In this case >> >> we >> >> might have been lucky, but I fear that this won't work in all cases >> >> and >> >> for older flashes it won't work at all. >> >> >> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. >> >> >> >> I guess that would be a less invasive way to fix different flashes >> >> with >> >> same jedec ids. >> >> >> >> -michael
Hi, Michael, On Monday, January 20, 2020 5:55:55 PM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, > > Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com: > > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Hi Tudor, > > > > Hi, Michael, > > > >> >> Am 2020-01-13 11:07, schrieb Michael Walle: > >> >> >>> Btw. is renaming the flashes also considered a backwards > >> >> >>> incomaptible > >> >> >>> change? > >> >> >> > >> >> >> No, we can fix the names. > >> >> >> > >> >> >>> And can there be two flashes with the same name? Because IMHO it > >> >> >>> would > >> >> >>> be > >> >> >> > >> >> >> I would prefer that we don't. Why would you have two different > >> >> >> jedec-ids with > >> >> >> the same name? > >> >> > > >> >> > Because as pointed out in the Winbond example you cannot distiguish > >> >> > between > >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between > >> >> > MX25L8005 > >> >> > and > >> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie > >> >> > W25Q32 > >> >> > or MX25L80 which should be the same for this particular ID. Like I > >> >> > said, I'd > >> >> > prefer showing an ambiguous name instead of a wrong one. But then > >> >> > you > >> >> > may > >> >> > have different IDs with the same ambiguous name. > >> >> > >> >> Another solution would be to have the device tree provide a hint for > >> >> the > >> >> actual flash chip. There would be multiple entries in the spi_nor_ids > >> >> with the > >> >> same flash id. By default the first one is used (keeping the current > >> >> behaviour). If there is for example > >> >> > >> >> compatible = "jedec,spi-nor", "w25q32jwq"; > >> >> > >> >> the flash_info for the w25q32jwq will be chosen. > >> > > >> > This won't work for plug-able flashes. You will influence the name in > >> > dt to be > >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will > >> > end up > >> > with a wrong name for w25q32dw, thus the same problem. > >> > >> No, because then the device tree is wrong and doesn't fit the > >> hardware. > >> You'd > >> have to some instance which could change the device tree node, like > >> the > >> bootloader or some device tree overlay for plugable flashes. We should > >> try to > >> solve the actual problem at hand first.. > >> > >> It is just not possible to autodetect the SPI flash, just because > >> the vendors reuse the same IDs for flashes with different features > >> (and > >> the > >> SFDP is likely not enough). Therefore, you need to have a hint in some > >> place > >> to use the flash properly. > >> > >> > If the flashes are identical but differ just in terms of name, we can > >> > rename > >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences > >> > between > >> > these flashes; if you want to fix them, send a patch and I'll try to > >> > help. > >> > >> It is not only the name, here are two examples which differ in > >> > >> functionality: > >> (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports > >> > >> dual/quad > >> > >> mode > >> > >> (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit. > >> > >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third > > > > sorry if this exhausted you. > > TBH, this is no fun (and I'm doing this on my spare time because I like It's not my fault that you're not having fun when someone disagrees with you. > open source). I guess our opinions differ waaay too much. I don't Up to a point, yes, our opinions differ. I'm not rejecting your suggestion, I just say that we should implement it as a last resort, when there's nothing auto-detectable at run-time that can differentiate between two flashes that share the same id. > really like band-aid fixes; eg. with vague information "it seems that > the F version adveritses support for Fast Read 4-4-4", what about other We can update the comment to clear the incertitude: "The F version advertises support for Fast Read 4-4-4"" > flashes with that idcode and this property. This might break at any time > or with anyone trying support for other flashes with that ID. The jedec-id should be unique in the first place, manufacturers that use the same jedec-id for different flavors of flashes are doing a bad thing. A third flash with the same jedec-id is unlikely to happen. > > That's what I've meant with first come first serve, I'm lucky now that > there was no flash with the same jedec id as the W25Q32JW. > > To add the MX25U3232F I could check the JEDEC revision (or the BFPT > length) because it differers from the MX25U3235F. But I don't feel well I prefer this because it's auto-detectable. If you don't feel well doing it, don't do it. > doing that. Who says Macronix won't update their description for the > MX25U3235F to the new revision.. FYI the Winbond guys apparently use the You are raising theoretical problems. We can fix this when we will encounter it. > first OTP region to store the JEDEC data, which is clever because they > can update it during production. If you say so. > > >> example. > > > > Flash auto-detection is nice and we should preserve it if possible. I > > would > > prefer having a post bfpt fixup than giving a hint about the flash in > > the > > compatible. > > see above. > > > The flashes that you mention are quite old and I don't know if it > > is worth to harm the auto-detection for them. A compromise has to be > > made. > > so you'd drop support for them? because SFDP is never read if there is > no > DUAL_READ or QUAD_READ flag. mx25l8006e defines bfpt, while mx25l8005 doesn't. We can differentiate these too. > > > You can gain traction in your endeavor if you have such a flash and > > there's > > nothing auto-detectable that differentiates it from some other flash > > that > > shares the sama jedec-id. > > > > If you have such a flash and you care about it, send a patch and I'll > > try to > > help. > > Given my reasoning above.. well maybe in the future. The Macronix would ok > be > a second source candidate. For now we are using the Winbond flash. > > I would rather like to have the flash protection topic and OTP support > sorted out, because that is something we are actually using. You can speed up the process by reviewing/testing the BP3 support. In turn, maybe Jungseung will review your OTP patches. To sum up: the flash auto-detection (with capabilities) greatly ease the device tree node description and it allows us to plug and play different manufacturer flashes using the same dtb. I have a connector on one of my boards, to which I connect different types of flashes (assuming they have similar frequency and modes). So I would always prefer to have a post bfpt hook to differentiate between flashes which share the same jedec-id, than compromising the generic compatible. Of course, if there's nothing auto- detectable that can differentiate between the flashes, then your idea can be implemented, but I would do this as a last resort. There's also the idea of compromise. The jedec-id should be unique in the first place, manufacturers that use the same jedec-id for different flavors of flashes are taking a wrong design decision. Do I want to cripple the generic compatible just for an old flash with a bad jedec-id? I don't know yet. Also, the flashes that share the same id are quite old, and if nobody screamed about this until now, it's fine by me. You raised some theoretical questions, you don't really use the macronix flashes, what I say is that we should consider fixing them when it's actually required. And that the extension of the compatible should be done as a last resort, as of now it has more disadvantages than advantages. Cheers, ta > > -michael > > >> -michael > >> > >> > Cheers, > >> > ta > >> > > >> >> I know this will conflict with the new rule that there should only be > >> >> > >> >> compatible = "jedec,spi-nor"; > >> >> > >> >> without the actual flash chip. But it seems that it is not always > >> >> possible > >> >> to just use the jedec id to match the correct chip. > >> >> > >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to > >> >> figure > >> >> out different behaviour by looking at "some" SFDP data. In this case > >> >> we > >> >> might have been lucky, but I fear that this won't work in all cases > >> >> and > >> >> for older flashes it won't work at all. > >> >> > >> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. > >> >> > >> >> I guess that would be a less invasive way to fix different flashes > >> >> with > >> >> same jedec ids. > >> >> > >> >> -michael
Hi Tudor, Am 2020-01-21 19:40, schrieb Tudor.Ambarus@microchip.com: > Hi, Michael, > > On Monday, January 20, 2020 5:55:55 PM EET Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the >> content is safe >> >> Hi Tudor, >> >> Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com: >> > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote: >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> >> the >> >> content is safe >> >> >> >> Hi Tudor, >> > >> > Hi, Michael, >> > >> >> >> Am 2020-01-13 11:07, schrieb Michael Walle: >> >> >> >>> Btw. is renaming the flashes also considered a backwards >> >> >> >>> incomaptible >> >> >> >>> change? >> >> >> >> >> >> >> >> No, we can fix the names. >> >> >> >> >> >> >> >>> And can there be two flashes with the same name? Because IMHO it >> >> >> >>> would >> >> >> >>> be >> >> >> >> >> >> >> >> I would prefer that we don't. Why would you have two different >> >> >> >> jedec-ids with >> >> >> >> the same name? >> >> >> > >> >> >> > Because as pointed out in the Winbond example you cannot distiguish >> >> >> > between >> >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between >> >> >> > MX25L8005 >> >> >> > and >> >> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie >> >> >> > W25Q32 >> >> >> > or MX25L80 which should be the same for this particular ID. Like I >> >> >> > said, I'd >> >> >> > prefer showing an ambiguous name instead of a wrong one. But then >> >> >> > you >> >> >> > may >> >> >> > have different IDs with the same ambiguous name. >> >> >> >> >> >> Another solution would be to have the device tree provide a hint for >> >> >> the >> >> >> actual flash chip. There would be multiple entries in the spi_nor_ids >> >> >> with the >> >> >> same flash id. By default the first one is used (keeping the current >> >> >> behaviour). If there is for example >> >> >> >> >> >> compatible = "jedec,spi-nor", "w25q32jwq"; >> >> >> >> >> >> the flash_info for the w25q32jwq will be chosen. >> >> > >> >> > This won't work for plug-able flashes. You will influence the name in >> >> > dt to be >> >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will >> >> > end up >> >> > with a wrong name for w25q32dw, thus the same problem. >> >> >> >> No, because then the device tree is wrong and doesn't fit the >> >> hardware. >> >> You'd >> >> have to some instance which could change the device tree node, like >> >> the >> >> bootloader or some device tree overlay for plugable flashes. We should >> >> try to >> >> solve the actual problem at hand first.. >> >> >> >> It is just not possible to autodetect the SPI flash, just because >> >> the vendors reuse the same IDs for flashes with different features >> >> (and >> >> the >> >> SFDP is likely not enough). Therefore, you need to have a hint in some >> >> place >> >> to use the flash properly. >> >> >> >> > If the flashes are identical but differ just in terms of name, we can >> >> > rename >> >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences >> >> > between >> >> > these flashes; if you want to fix them, send a patch and I'll try to >> >> > help. >> >> >> >> It is not only the name, here are two examples which differ in >> >> >> >> functionality: >> >> (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports >> >> >> >> dual/quad >> >> >> >> mode >> >> >> >> (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit. >> >> >> >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third >> > >> > sorry if this exhausted you. >> >> TBH, this is no fun (and I'm doing this on my spare time because I >> like > > It's not my fault that you're not having fun when someone disagrees > with you. The reason is not the disagreement, but how you're (not) answering my arguments. Like in the other thread, the question about the uselessness of the flash_lock and flash_unlock tools with SPI-NOR and the (IMHO) bad behaviour when the user actually uses flash_lock. Please, don't take this personally, I'll buy you a beer at FOSDEM :p back to the technical stuff. > >> open source). I guess our opinions differ waaay too much. I don't > > Up to a point, yes, our opinions differ. I'm not rejecting your > suggestion, I > just say that we should implement it as a last resort, when there's > nothing > auto-detectable at run-time that can differentiate between two flashes > that > share the same id. > >> really like band-aid fixes; eg. with vague information "it seems that >> the F version adveritses support for Fast Read 4-4-4", what about >> other > > We can update the comment to clear the incertitude: "The F version > advertises > support for Fast Read 4-4-4"" > >> flashes with that idcode and this property. This might break at any >> time >> or with anyone trying support for other flashes with that ID. > > The jedec-id should be unique in the first place, manufacturers that > use the > same jedec-id for different flavors of flashes are doing a bad thing. A > third > flash with the same jedec-id is unlikely to happen. MX25U3232F, MX25U3235F, MX25U3273F, MX25U3235E all use the same 0x2c2536 identification. And these are only the active ones. I bet there are a bunch of older 32MBit flashes. MX25U6432F, MX25U6472F, MX25U6433F, MX25U6435F, MX25U6473F all use the same 0x2c2537 id. W25Q32JW-IQ, W25Q32DW, W25Q32FW all use the same 0x156016 id. btw. thats why I argued to just have MX25U32 or W25Q32 as a name for the flashes. >> >> That's what I've meant with first come first serve, I'm lucky now that >> there was no flash with the same jedec id as the W25Q32JW. >> >> To add the MX25U3232F I could check the JEDEC revision (or the BFPT >> length) because it differers from the MX25U3235F. But I don't feel >> well > > I prefer this because it's auto-detectable. If you don't feel well > doing it, > don't do it. ok, I'll do so for the MX25U3232F support. >> doing that. Who says Macronix won't update their description for the >> MX25U3235F to the new revision.. FYI the Winbond guys apparently use >> the > > You are raising theoretical problems. We can fix this when we will > encounter > it. > >> first OTP region to store the JEDEC data, which is clever because they >> can update it during production. > > If you say so. > >> >> >> example. >> > >> > Flash auto-detection is nice and we should preserve it if possible. I >> > would >> > prefer having a post bfpt fixup than giving a hint about the flash in >> > the >> > compatible. >> >> see above. >> >> > The flashes that you mention are quite old and I don't know if it >> > is worth to harm the auto-detection for them. A compromise has to be >> > made. >> >> so you'd drop support for them? because SFDP is never read if there is >> no >> DUAL_READ or QUAD_READ flag. > > mx25l8006e defines bfpt, while mx25l8005 doesn't. We can differentiate > these > too. >> >> > You can gain traction in your endeavor if you have such a flash and >> > there's >> > nothing auto-detectable that differentiates it from some other flash >> > that >> > shares the sama jedec-id. >> > >> > If you have such a flash and you care about it, send a patch and I'll >> > try to >> > help. >> >> Given my reasoning above.. well maybe in the future. The Macronix >> would > > ok > >> be >> a second source candidate. For now we are using the Winbond flash. >> >> I would rather like to have the flash protection topic and OTP support >> sorted out, because that is something we are actually using. > > You can speed up the process by reviewing/testing the BP3 support. In > turn, > maybe Jungseung will review your OTP patches. > > To sum up: the flash auto-detection (with capabilities) greatly ease > the > device tree node description and it allows us to plug and play > different > manufacturer flashes using the same dtb. I have a connector on one of > my > boards, to which I connect different types of flashes (assuming they > have > similar frequency and modes). So I would always prefer to have a post > bfpt > hook to differentiate between flashes which share the same jedec-id, > than > compromising the generic compatible. and making assumptions which are true for the flashes you currently know about. > Of course, if there's nothing auto- > detectable that can differentiate between the flashes, then your idea > can be > implemented, but I would do this as a last resort. > > There's also the idea of compromise. The jedec-id should be unique in > the > first place, manufacturers that use the same jedec-id for different > flavors of > flashes are taking a wrong design decision. Do I want to cripple the > generic > compatible just for an old flash with a bad jedec-id? I don't know yet. > Also, > the flashes that share the same id are quite old, and if nobody > screamed about > this until now, it's fine by me. See above, the assumption that newer flashes have differnet jedec-ids is wrong. > You raised some theoretical questions, you > don't really use the macronix flashes, what I say is that we should > consider > fixing them when it's actually required. And that the extension of the > compatible should be done as a last resort, as of now it has more > disadvantages than advantages. Well what are the disadvantages? I don't argue against the autodetection. I argue to have a mechanism _already_ in place when the autodetection fails. If you don't specify the hint, everything stays the same. We could have the advantages of both worlds, have a generic "w25q32" which tries its best for autodetection and a specific "w25q32fw" which could can be hinted. Same for like "mx25u32" and "mx25u3232f", "mx25u3235f" etc. -michael > > Cheers, > ta > >> >> -michael >> >> >> -michael >> >> >> >> > Cheers, >> >> > ta >> >> > >> >> >> I know this will conflict with the new rule that there should only be >> >> >> >> >> >> compatible = "jedec,spi-nor"; >> >> >> >> >> >> without the actual flash chip. But it seems that it is not always >> >> >> possible >> >> >> to just use the jedec id to match the correct chip. >> >> >> >> >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to >> >> >> figure >> >> >> out different behaviour by looking at "some" SFDP data. In this case >> >> >> we >> >> >> might have been lucky, but I fear that this won't work in all cases >> >> >> and >> >> >> for older flashes it won't work at all. >> >> >> >> >> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[]. >> >> >> >> >> >> I guess that would be a less invasive way to fix different flashes >> >> >> with >> >> >> same jedec ids. >> >> >> >> >> >> -michael
Hi, Michael, On Wednesday, January 22, 2020 1:28:52 AM EET Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi Tudor, > > Am 2020-01-21 19:40, schrieb Tudor.Ambarus@microchip.com: > > Hi, Michael, > > > > On Monday, January 20, 2020 5:55:55 PM EET Michael Walle wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> the > >> content is safe > >> > >> Hi Tudor, > >> > >> Am 2020-01-20 12:03, schrieb Tudor.Ambarus@microchip.com: > >> > On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote: > >> >> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >> >> the > >> >> content is safe > >> >> > >> >> Hi Tudor, > >> > > >> > Hi, Michael, > >> > > >> >> >> Am 2020-01-13 11:07, schrieb Michael Walle: > >> >> >> >>> Btw. is renaming the flashes also considered a backwards > >> >> >> >>> incomaptible > >> >> >> >>> change? > >> >> >> >> > >> >> >> >> No, we can fix the names. > >> >> >> >> > >> >> >> >>> And can there be two flashes with the same name? Because IMHO > >> >> >> >>> it > >> >> >> >>> would > >> >> >> >>> be > >> >> >> >> > >> >> >> >> I would prefer that we don't. Why would you have two different > >> >> >> >> jedec-ids with > >> >> >> >> the same name? > >> >> >> > > >> >> >> > Because as pointed out in the Winbond example you cannot > >> >> >> > distiguish > >> >> >> > between > >> >> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between > >> >> >> > MX25L8005 > >> >> >> > and > >> >> >> > MX25L8006E. Thus my reasoning was to show only the common part, > >> >> >> > ie > >> >> >> > W25Q32 > >> >> >> > or MX25L80 which should be the same for this particular ID. Like > >> >> >> > I > >> >> >> > said, I'd > >> >> >> > prefer showing an ambiguous name instead of a wrong one. But then > >> >> >> > you > >> >> >> > may > >> >> >> > have different IDs with the same ambiguous name. > >> >> >> > >> >> >> Another solution would be to have the device tree provide a hint > >> >> >> for > >> >> >> the > >> >> >> actual flash chip. There would be multiple entries in the > >> >> >> spi_nor_ids > >> >> >> with the > >> >> >> same flash id. By default the first one is used (keeping the > >> >> >> current > >> >> >> behaviour). If there is for example > >> >> >> > >> >> >> compatible = "jedec,spi-nor", "w25q32jwq"; > >> >> >> > >> >> >> the flash_info for the w25q32jwq will be chosen. > >> >> > > >> >> > This won't work for plug-able flashes. You will influence the name > >> >> > in > >> >> > dt to be > >> >> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you > >> >> > will > >> >> > end up > >> >> > with a wrong name for w25q32dw, thus the same problem. > >> >> > >> >> No, because then the device tree is wrong and doesn't fit the > >> >> hardware. > >> >> You'd > >> >> have to some instance which could change the device tree node, like > >> >> the > >> >> bootloader or some device tree overlay for plugable flashes. We should > >> >> try to > >> >> solve the actual problem at hand first.. > >> >> > >> >> It is just not possible to autodetect the SPI flash, just because > >> >> the vendors reuse the same IDs for flashes with different features > >> >> (and > >> >> the > >> >> SFDP is likely not enough). Therefore, you need to have a hint in some > >> >> place > >> >> to use the flash properly. > >> >> > >> >> > If the flashes are identical but differ just in terms of name, we > >> >> > can > >> >> > rename > >> >> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the > >> >> > differences > >> >> > between > >> >> > these flashes; if you want to fix them, send a patch and I'll try to > >> >> > help. > >> >> > >> >> It is not only the name, here are two examples which differ in > >> >> > >> >> functionality: > >> >> (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports > >> >> > >> >> dual/quad > >> >> > >> >> mode > >> >> > >> >> (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit. > >> >> > >> >> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third > >> > > >> > sorry if this exhausted you. > >> > >> TBH, this is no fun (and I'm doing this on my spare time because I > >> like > > > > It's not my fault that you're not having fun when someone disagrees > > with you. > > The reason is not the disagreement, but how you're (not) answering my > arguments. > Like in the other thread, the question about the uselessness of the > flash_lock > and flash_unlock tools with SPI-NOR and the (IMHO) bad behaviour when > the user The flash unlock at probe was a bad decision, but we can't be backward compatible. kconfig or module param will solve the problem without breaking backward compatibility. > actually uses flash_lock. Please, don't take this personally, I'll buy > you a > beer at FOSDEM :p back to the technical stuff. I don't take this personally, but I think the discussion went in a wrong direction, and I feel that we waste time on theoretical problems. > > >> open source). I guess our opinions differ waaay too much. I don't > > > > Up to a point, yes, our opinions differ. I'm not rejecting your > > suggestion, I > > just say that we should implement it as a last resort, when there's > > nothing > > auto-detectable at run-time that can differentiate between two flashes > > that > > share the same id. > > > >> really like band-aid fixes; eg. with vague information "it seems that > >> the F version adveritses support for Fast Read 4-4-4", what about > >> other > > > > We can update the comment to clear the incertitude: "The F version > > advertises > > support for Fast Read 4-4-4"" > > > >> flashes with that idcode and this property. This might break at any > >> time > >> or with anyone trying support for other flashes with that ID. > > > > The jedec-id should be unique in the first place, manufacturers that > > use the > > same jedec-id for different flavors of flashes are doing a bad thing. A > > third > > flash with the same jedec-id is unlikely to happen. > > MX25U3232F, MX25U3235F, MX25U3273F, MX25U3235E all use the same 0x2c2536 > identification. And these are only the active ones. I bet there are a > bunch of older 32MBit flashes. > > MX25U6432F, MX25U6472F, MX25U6433F, MX25U6435F, MX25U6473F all use the > same 0x2c2537 id. > > W25Q32JW-IQ, W25Q32DW, W25Q32FW all use the same 0x156016 id. > > btw. thats why I argued to just have MX25U32 or W25Q32 as a name for the > flashes. > > >> That's what I've meant with first come first serve, I'm lucky now that > >> there was no flash with the same jedec id as the W25Q32JW. > >> > >> To add the MX25U3232F I could check the JEDEC revision (or the BFPT > >> length) because it differers from the MX25U3235F. But I don't feel > >> well > > > > I prefer this because it's auto-detectable. If you don't feel well > > doing it, > > don't do it. > > ok, I'll do so for the MX25U3232F support. > > >> doing that. Who says Macronix won't update their description for the > >> MX25U3235F to the new revision.. FYI the Winbond guys apparently use > >> the > > > > You are raising theoretical problems. We can fix this when we will > > encounter > > it. > > > >> first OTP region to store the JEDEC data, which is clever because they > >> can update it during production. > > > > If you say so. > > > >> >> example. > >> > > >> > Flash auto-detection is nice and we should preserve it if possible. I > >> > would > >> > prefer having a post bfpt fixup than giving a hint about the flash in > >> > the > >> > compatible. > >> > >> see above. > >> > >> > The flashes that you mention are quite old and I don't know if it > >> > is worth to harm the auto-detection for them. A compromise has to be > >> > made. > >> > >> so you'd drop support for them? because SFDP is never read if there is > >> no > >> DUAL_READ or QUAD_READ flag. > > > > mx25l8006e defines bfpt, while mx25l8005 doesn't. We can differentiate > > these > > too. > > > >> > You can gain traction in your endeavor if you have such a flash and > >> > there's > >> > nothing auto-detectable that differentiates it from some other flash > >> > that > >> > shares the sama jedec-id. > >> > > >> > If you have such a flash and you care about it, send a patch and I'll > >> > try to > >> > help. > >> > >> Given my reasoning above.. well maybe in the future. The Macronix > >> would > > > > ok > > > >> be > >> a second source candidate. For now we are using the Winbond flash. > >> > >> I would rather like to have the flash protection topic and OTP support > >> sorted out, because that is something we are actually using. > > > > You can speed up the process by reviewing/testing the BP3 support. In > > turn, > > maybe Jungseung will review your OTP patches. > > > > To sum up: the flash auto-detection (with capabilities) greatly ease > > the > > device tree node description and it allows us to plug and play > > different > > manufacturer flashes using the same dtb. I have a connector on one of > > my > > boards, to which I connect different types of flashes (assuming they > > have > > similar frequency and modes). So I would always prefer to have a post > > bfpt > > hook to differentiate between flashes which share the same jedec-id, > > than > > compromising the generic compatible. > > and making assumptions which are true for the flashes you currently know > about. I don't want to introduce code which I don't know if it will ever be used. > > Of course, if there's nothing auto- > > detectable that can differentiate between the flashes, then your idea > > can be > > implemented, but I would do this as a last resort. > > > > There's also the idea of compromise. The jedec-id should be unique in > > the > > first place, manufacturers that use the same jedec-id for different > > flavors of > > flashes are taking a wrong design decision. Do I want to cripple the > > generic > > compatible just for an old flash with a bad jedec-id? I don't know yet. > > Also, > > the flashes that share the same id are quite old, and if nobody > > screamed about > > this until now, it's fine by me. > > See above, the assumption that newer flashes have differnet jedec-ids is > wrong. > > > You raised some theoretical questions, you > > don't really use the macronix flashes, what I say is that we should > > consider > > fixing them when it's actually required. And that the extension of the > > compatible should be done as a last resort, as of now it has more > > disadvantages than advantages. > > Well what are the disadvantages? I don't argue against the > autodetection. I - generic compatible eases the use. I don't have to check the dtb each time I change a plug-able flash, to see if I gave a wrong hint for the flash in the extended compatible - people will prefer to use the extended compatible instead of trying to auto- detect the differences at run-time (it's easier, but wrong). > argue to have a mechanism _already_ in place when the autodetection > fails. > If you don't specify the hint, everything stays the same. > > We could have the advantages of both worlds, have a generic "w25q32" > which tries > its best for autodetection and a specific "w25q32fw" which could can be > hinted. > Same for like "mx25u32" and "mx25u3232f", "mx25u3235f" etc. > If you possess 2 flashes that we can't correctly detect at run-time and you care to fix them, then your suggestion has a good change to be implemented. I will not introduce code that is not used, in the hope that it will be used. No compatible extension if we have a way to auto-detect the flash. Cheers, ta > > > Cheers, > > ta > > > >> -michael > >> > >> >> -michael > >> >> > >> >> > Cheers, > >> >> > ta > >> >> > > >> >> >> I know this will conflict with the new rule that there should only > >> >> >> be > >> >> >> > >> >> >> compatible = "jedec,spi-nor"; > >> >> >> > >> >> >> without the actual flash chip. But it seems that it is not always > >> >> >> possible > >> >> >> to just use the jedec id to match the correct chip. > >> >> >> > >> >> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to > >> >> >> figure > >> >> >> out different behaviour by looking at "some" SFDP data. In this > >> >> >> case > >> >> >> we > >> >> >> might have been lucky, but I fear that this won't work in all cases > >> >> >> and > >> >> >> for older flashes it won't work at all. > >> >> >> > >> >> >> BTW I do not suggest to add the strings to the the > >> >> >> spi_nor_dev_ids[]. > >> >> >> > >> >> >> I guess that would be a less invasive way to fix different flashes > >> >> >> with > >> >> >> same jedec ids. > >> >> >> > >> >> >> -michael
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index addb6319fcbb..3fa8a81bdab0 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] = { SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, + { + "w25q16jwim", INFO(0xef8015, 0, 64 * 1024, 32, + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) + }, { "w25x32", INFO(0xef3016, 0, 64 * 1024, 64, SECT_4K) }, { "w25q16jv-im/jm", INFO(0xef7015, 0, 64 * 1024, 32, @@ -2647,6 +2652,11 @@ static const struct flash_info spi_nor_ids[] = { SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, + { + "w25q32jwim", INFO(0xef8016, 0, 64 * 1024, 64, + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) + }, { "w25x64", INFO(0xef3017, 0, 64 * 1024, 128, SECT_4K) }, { "w25q64", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) }, { @@ -2654,6 +2664,11 @@ static const struct flash_info spi_nor_ids[] = { SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, + { + "w25q64jwim", INFO(0xef8017, 0, 64 * 1024, 128, + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) + }, { "w25q128fw", INFO(0xef6018, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | @@ -2664,6 +2679,11 @@ static const struct flash_info spi_nor_ids[] = { SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, + { + "w25q128jwim", INFO(0xef8018, 0, 64 * 1024, 256, + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) + }, { "w25q80", INFO(0xef5014, 0, 64 * 1024, 16, SECT_4K) }, { "w25q80bl", INFO(0xef4014, 0, 64 * 1024, 16, SECT_4K) }, { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) }, @@ -2674,6 +2694,8 @@ static const struct flash_info spi_nor_ids[] = { SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "w25q256jw", INFO(0xef6019, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, + { "w25q256jwim", INFO(0xef8019, 0, 64 * 1024, 512, + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) }, { "w25m512jv", INFO(0xef7119, 0, 64 * 1024, 1024, SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_DUAL_READ) },
Add support for the Winbond W25QnnJW-IM flashes. These have a programmable QE bit. There are also the W25QnnJW-IQ variant which shares the ID with the W25QnnFW parts. These have the QE bit hard strapped to 1, thus don't support hardware write protection. Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)