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

Tudor.Ambarus
2022-03-07T09:31:48Z

On 3/7/22 09:12, Tudor.Ambarus@microchip.com 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@walle.cc>
>> Tested-by: Heiko Thiery <heiko.thiery@gmail.com>
>> ---
>> 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/