Message-ID: <2088283.tnVAhDz6N7@192.168.0.113>
Date:   Wed, 22 Jan 2020 06:48:07 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
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, Michael,

On Wednesday, January 22, 2020 1:28:52 AM 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-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

The flash unlock at probe was a bad decision, but we can't be backward 
compatible. kconfig or module param will solve the problem without breaking 
backward compatibility.

> actually uses flash_lock. Please, don't take this personally, I'll buy
> you a
> beer at FOSDEM :p back to the technical stuff.

I don't take this personally, but I think the discussion went in a wrong 
direction, and I feel that we waste time on theoretical problems.

> 
> >> 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.

I don't want to introduce code which I don't know if it will ever be used.

> > 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

- generic compatible eases the use. I don't have to check the dtb each time I 
change a plug-able flash, to see if I gave a wrong hint for the flash in the 
extended compatible
- people will prefer to use the extended compatible instead of trying to auto-
detect the differences at run-time (it's easier, but wrong).

> 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.
> 

If you possess 2 flashes that we can't correctly detect at run-time and you 
care to fix them, then your suggestion has a good change to be implemented. I 
will not introduce code that is not used, in the hope that it will be used. No 
compatible extension if we have a way to auto-detect the flash.

Cheers,
ta

> 
> > 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