Message-ID: <d3f03e392c060ff4ed4c2ae8a8999d9f@walle.cc>
Date:   Mon, 20 Jan 2020 16:55:55 +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-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
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

Powered by blists - more mailing lists