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

From: Michael Walle
Date: Tue Jan 21 2020 - 18:29:03 EST

Hi Tudor,

Am 2020-01-21 19:40, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
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.

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