[PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Pratyush Yadav]

On 04/03/22 03:36PM, Michael Walle wrote:
> Am 2022-03-04 01:36, schrieb Tudor.Ambarus at microchip.com:
> > On 3/3/22 18:45, Michael Walle wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > Am 2022-03-03 17:31, schrieb Heiko Thiery:
> > > ..
> > > 
> > > > > > > > # xxd -p mx25l3233f-sfdp
> > > > > > > > 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> > > > > > > > ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
> > > > > > > > 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
> > > > > > > > ffffffffffff003650269cf97764fecfffffffffffff
> > > > > > > 
> > > > > > > Is quad enable working or has this the same problem as
> > > > > > > the macronix flash in patch 4? Judging by the length of the SFDP
> > > > > > > this also lacks the required information to select an
> > > > > > > appropriate enable method. I haven't had closer look though.
> > > > > > 
> > > > > > it worked, yes. As I specified in the commit message, I tested it
> > > > > and
> > > > > > it used
> > > > > > SPINOR_OP_READ_1_4_4 0xeb opcode for reads.
> > > > > 
> > > > > I'm confused, why is Heiko reporting that the CR/SR writing isn't
> > > > > working because a wrong quad_enable method is chosen, but here it
> > > > > will work. What am I missing?
> > > > 
> > > > I suppose that the flash that supports the RSSFDP is JEDES216B
> > > > compatible including DWORD[15]. The flash that I have is only
> > > > JEDES216
> > > > compatible and has not the DWORD[15] defined.
> > > 
> > > That was why I wrote "Judging by the length of the SFDP". I've
> > > converted both the mx25l12835f and mx25l3233f to binary and both
> > > are 112 bytes long. Both seem to have the short BFPT table, ie.
> > > no DWORD(15). Both seem to have a second table at offset 60h.
> > > 
> > 
> > I've just redone the test, I see:
> > root at sama5d2-xplained:~# mtd_debug read /dev/mtd1 0 65536 read
> > atmel_qspi f0020000.spi: op->cmd.opcode = 00eb, so
> > SPINOR_OP_READ_1_4_4 as I said.
> > 
> > Michael, you have the eyes of an eagle, only the first 9 BFPT dwords
> > are defined:
> > spi-nor spi1.0: bfpt_header->length = 9
> > spi-nor spi1.0: BFPT[DWORD(1)] = fff120e5
> > spi-nor spi1.0: BFPT[DWORD(2)] = 01ffffff
> > spi-nor spi1.0: BFPT[DWORD(3)] = 6b08eb44
> > spi-nor spi1.0: BFPT[DWORD(4)] = bb043b08
> > spi-nor spi1.0: BFPT[DWORD(5)] = ffffffee
> > spi-nor spi1.0: BFPT[DWORD(6)] = ff00ffff
> > spi-nor spi1.0: BFPT[DWORD(7)] = ff00ffff
> > spi-nor spi1.0: BFPT[DWORD(8)] = 520f200c
> > spi-nor spi1.0: BFPT[DWORD(9)] = ff00d810
> > 
> > What happens is that the QE bit is non volatile and it's already set.
> > 
> > spi-nor spi1.0: spi_nor_quad_enable
> > spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable
> > atmel_qspi f0020000.spi: op->cmd.opcode = 0035
> > spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable cr = ff
> > atmel_qspi f0020000.spi: op->cmd.opcode = 0005
> > spi-nor spi1.0: sr = 40
> > 
> > spi_nor_sr2_bit1_quad_enable is called, RDCR is ignored so 0xff,
> > but I did a read of the SR and surprise, it's value is 0x40, so QE set.
> > This is a new kind of bug :). So yes, this patch has the same problem
> > as Heiko's, I will update it. Thanks for the heads up!
> 
> I've given this a little bit more thought. I'm pretty sure
> the QE bit is non-volatile on most flashes which share IO2
> IO3 with hold#/reset#/wp#. Why is the QE bit needed in the
> first place? Because it will determine the function of the
> pins. For example, all our products will have WP# pin enabled
> and pulled low to make sure (parts of) the flash is
> write-protected. Needless to say, we cannot use quad mode.

For those cases you should set spi-{rx,tx}-bus-width to 1 or 2 in device 
tree so SPI NOR does not attempt quad mode. If it does despite that, 
this is a bug which we should fix.

> 
> There might be edge cases where we accidentally disable the
> write protection pin. I'm not saying it is likely, though.
> E.g. on our boards we have a jumper to disable the write
> protection, now if linux decides for whatever reason to fiddle
> with the QE bit and set it to 1 that jumper might very well
> be useless after you booted linux once. Now that does ring a
> bell with "linux will just disable the write protection for
> no good reason on any flash", if you still remember ;)
> 
> Anyways, just to let you know. I don't have any better
> idea.
> 
> -michael

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.