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

[Michael Walle]

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.

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