Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

[Tudor.Ambarus]

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@xxxxxxxxxxxxx:
> > 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