Re: [PATCH] mtd: spi-nor: Fix quad-enable for flashes with QER bit in SR1

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

On Fri Sep 11, 2026 at 11:34 AM CEST, Miquel Raynal wrote:
> On 11/09/2026 at 11:19:39 +02, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
>>>> -	ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>> +	if (qe_mask[0])
>>>> +		ret = spi_nor_read_sr1(nor, sr);
>>>> +	else
>>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>>
>>> Shouldn't that be the other way around?
>>>
>>> +	if (qe_mask[1])
>>> +		ret = spi_nor_read_sr1_and_sr2(nor, sr);
>>> +	else
>>> +		ret = spi_nor_read_sr1(nor, sr);
>>>
>>> I know qe_mask won't be spread across two SRs. Just to be
>>> correct and make it easier to grok.
>>
>> Why is the second version easier to parse? First version is maybe more
>> future proof since we are treating a specific case. Because either it is
>> a specific chip (Mxic) or we end up in the more common case. There is
>> only one QER entry for qe_mask[0], many for qe_mask[1], and chances are
>> that in the future we will mostly see new chips only using a qe_mask[1]
>> mask. So having this in the else doesn't sound so strange to me? I also
>> don't think we will ever see masks spanning the two registers.
>
> The problem still persists because there is the check which also fails
> reading SR2.

I don't get it. Why would you read SR2 if bit6 is set in qe_mask[0]?
Or do you mean in the spi_nor_write_sr1_and_sr2_and_check(). Yeah
that should also probably only write SR1 if qe_mask[1] == 0, no?

> Michael, we moved to swp.c that handling, introducing a "careful"
> revision, was this actually relevant?

Yes, because I eventually want to get rid of that sr2 QE bit
guessing. I really can't imagine that this is necessary.

> the lack of SR2 read back into the main core like before?
>
> Possible steps forward:
> 1- Add extra logic in the "and_check" helper, kind of duplicating what
>    is in the swp.c file (not my favourite)
> 2- Move the "careful" handling (which does not return errors when a
>    register read is absent) back from swp.c into core.c.
>
> What do you prefer?

3- make the generic_quad_enable differentiate between "qe bit is in
SR1" or "qe bit is in SR2". If it's in SR1, just use
spi_nor_{read,write}_sr1. if it's in SR2, keep the current handling.

Do I miss something?

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]