Messages in this thread | | | Date | Wed, 22 Jan 2020 00:28:52 +0100 | From | Michael Walle <> | Subject | Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim |
| |
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
| |