Re: [PATCH v1] mtd: spi-nor: unset quad_enable if SFDP doesn't specify it

[Tudor.Ambarus]

On 3/7/22 09:12, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 3/4/22 20:51, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> While the first version of JESD216 specify the opcode for 4 bit I/O
>> accesses, it lacks information on how to actually enable this mode.
>>
>> For now, the one set in spi_nor_init_default_params() will be used.
>> But this one is likely wrong for some flashes, in particular the
>> Macronix MX25L12835F. Thus we need to clear the enable method when
>> parsing the SFDP. Flashes with such an SFDP revision will have to use a
>> flash (and SFDP revision) specific fixup.
>>
>> This might break quad I/O for some flashes which relied on the
>> spi_nor_sr2_bit1_quad_enable() that was formerly set. If your bisect
>> turns up this commit, you'll probably have to set the proper
>> quad_enable method in a post_bfpt() fixup for your flash.
>>
> 
> Right, I meant adding a paragraph such as the one from above.
> 
>> Signed-off-by: Michael Walle <michael@xxxxxxxx>
>> Tested-by: Heiko Thiery <heiko.thiery@xxxxxxxxx>
>> ---
>> changes since RFC:
>>  - reworded commit message
>>  - added comment about post_bfpt hook
>>
>> Tudor, I'm not sure what you meant with
>>   Maybe you can update the commit message and explain why would some
>>   flashes fail to enable quad mode, similar to what I did.
>>
>> It doesn't work because the wrong method is chosen? ;)
>>
>>  drivers/mtd/spi-nor/sfdp.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index a5211543d30d..6bba9b601846 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -549,6 +549,16 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>         map->uniform_erase_type = map->uniform_region.offset &
>>                                   SNOR_ERASE_TYPE_MASK;
>>
>> +       /*
>> +        * The first JESD216 revision doesn't specify a method to enable
>> +        * quad mode. spi_nor_init_default_params() will set a legacy
>> +        * default method to enable quad mode. We have to disable it
>> +        * again.
>> +        * Flashes with this JESD216 revision need to set the quad_enable
>> +        * method in their post_bfpt() fixup if they want to use quad I/O.
>> +        */
> 
> Great. Looks good to me. I'll change the subject to "mtd: spi-nor: sfdp:"
> when applying.

As we talked on the meeting, we can instead move the default quad mode init
to the deprecated way of initializing the params, or/and to where SKIP_SFDP
is used. This way you'll no longer need to clear it here.

> 
> Cheers,
> ta
>> +       params->quad_enable = NULL;
>> +
>>         /* Stop here if not JESD216 rev A or later. */
>>         if (bfpt_header->length == BFPT_DWORD_MAX_JESD216)
>>                 return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
>> @@ -562,7 +572,6 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>         /* Quad Enable Requirements. */
>>         switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>>         case BFPT_DWORD15_QER_NONE:
>> -               params->quad_enable = NULL;
>>                 break;
>>
>>         case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
>> --
>> 2.30.2
>>
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/