lkml.org 
[lkml]   [2020]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2020-01-22 00:29    [W:0.057 / U:0.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site