Message-ID: <1e78fbe87c0d6dc19a27210551b02af4@walle.cc>
Date:   Wed, 22 Jan 2020 00:28:52 +0100
From:   Michael Walle <michael@...le.cc>
To:     Tudor.Ambarus@...rochip.com
Cc:     linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        richard@....at, vigneshr@...com, miquel.raynal@...tlin.com
Subject: Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

Hi Tudor,

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

Powered by blists - more mailing lists