Re: [PATCH v2,1/2] mtd: spi-nor: giga: gd25lq64c: Disable quad mode according to bus width

[Tudor Ambarus]

On 8/16/23 12:51, Michael Walle wrote:
> Am 2023-08-16 12:38, schrieb Hsin-Yi Wang:
>> gd25lq64c has Quad Enable Requirement flag parsed as
>> BFPT_DWORD15_QER_SR2_BIT1_BUGGY in BFPT, even if spi-{rx/tx}-bus-width
>> set as non QUAD, eg. 0, 1, 2... Thus quad_enable will not be NULL and
>> quad enable (QE) bit will be set to 1 by default. According to
>> datasheet[1], if QE bit is enabled, WP pin will become IO pin and the
>> system can't use write protection feature, and it's also not recommended
>> to set QE bit to 1[1].
>>
>> Add a post_bfpt fixup that reads spi-rx-bus-width to remove quad_enable
>> if the width is set to below QUAD mode.
>>
>> [1]
>> https://www.gigadevice.com.cn/Public/Uploads/uploadfile/files/20220714/DS-00012-GD25LQ64C-Rev3.4.pdf
>> page 13
>>
>> Signed-off-by: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/gigadevice.c | 28 +++++++++++++++++++++++++++-
>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/gigadevice.c b/drivers/mtd/spi-nor/gigadevice.c
>> index d57ddaf1525b3..8ea89e1858f9b 100644
>> --- a/drivers/mtd/spi-nor/gigadevice.c
>> +++ b/drivers/mtd/spi-nor/gigadevice.c
>> @@ -33,6 +33,31 @@ static const struct spi_nor_fixups gd25q256_fixups = {
>>      .post_bfpt = gd25q256_post_bfpt,
>>  };
>>
>> +static int
>> +gd25lq64c_post_bfpt(struct spi_nor *nor,
>> +            const struct sfdp_parameter_header *bfpt_header,
>> +            const struct sfdp_bfpt *bfpt)
>> +{
>> +    struct device_node *np = spi_nor_get_flash_node(nor);
>> +    u32 value;
>> +
>> +    /*
>> +     * Even if spi-{tx,rx}-bus-width is set to DUAL mode, due to the QER
>> +     * flag parsed from BFPT is BFPT_DWORD15_QER_SR2_BIT1_BUGGY, so the
>> +     * quad_enable will be set and QE bit set to 1.
>> +     */
>> +    if (!of_property_read_u32(np, "spi-rx-bus-width", &value)) {
>> +        if (value <= 2)
>> +            nor->params->quad_enable = NULL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static struct spi_nor_fixups gd25lq64c_fixups = {
>> +    .post_bfpt = gd25lq64c_post_bfpt,
> 
> No. Please fix it in the core and not just for this part. To me it seems

The core seems fine to me. We already adjust the hw caps by keeping just the
hardware capabilities supported by both the SPI controller and the flash,
see spi_nor_spimem_adjust_hwcaps(). If you set spi-rx-bus-width = <2>; 
(spi_nor_get_protocol_width(nor->read_proto) will be 2, thus the quad enable
method will not be called. Are you sure you don't have the quad enable bit
set by the bootloaders? Please add some prints and check whether the
quad_enable method is called or not.

> like a fundamental problem and that commit 39d1e3340c73 ("mtd: spi-nor:
> Fix clearing of QE bit on lock()/unlock()") is broken in that regard.

what's wrong with the mentioned commit?

Cheers,
ta
> Tudor?
> 
> -michael