Re: [PATCH v4 13/20] mtd: spi-nor: Fix clearing of QE bit on lock()/unlock()

[Tudor.Ambarus]

On 11/05/2019 07:07 PM, Vignesh Raghavendra wrote:
> On 02-Nov-19 4:53 PM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
>> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
>>
>> Make sure that when doing a lock() or an unlock() operation we don't clear
>> the QE bit from Status Register 2.
>>
>> JESD216 revB or later offers information about the *default* Status
>> Register commands to use (see BFPT DWORDS[15], bits 22:20). In this
>> standard, Status Register 1 refers to the first data byte transferred on a
>> Read Status (05h) or Write Status (01h) command. Status register 2 refers
>> to the byte read using instruction 35h. Status register 2 is the second
>> byte transferred in a Write Status (01h) command.
>>
>> Industry naming and definitions of these Status Registers may differ.
>> The definitions are described in JESD216B, BFPT DWORDS[15], bits 22:20.
>> There are cases in which writing only one byte to the Status Register 1
>> has the side-effect of clearing Status Register 2 and implicitly the Quad
>> Enable bit. This side-effect is hit just by the
>> BFPT_DWORD15_QER_SR2_BIT1_BUGGY and BFPT_DWORD15_QER_SR2_BIT1 cases.
>>
> But I see that 1 byte SR1 write still happens as part of
> spi_nor_clear_sr_bp() until patch 20/20. So is this only a partial fix?

Fixing spi_nor_clear_sr_bp() would mean to add dead code that will be removed
anyway with patch 20/20. This patch fixes the clearing of the QE bit, while in
20/20 the QE bit is already zero when the one byte SR1 write is used, so the
quad mode is not affected. 20/20 fixes indirectly the clearing of all the bits
from SR2 but QE bit, because it's already zero. I would say it's not a partial
fix, but a different bug.

There are different angles to look at this, I chose the modifying of the quad
mode angle. Given the two arguments from above (avoid adding dead code and
changing of quad mode approach), I would prefer to keep things as they are. But
I get your approach too, so if you still want yours, I can do it. Please let me
know.

> Should this patch be rearranged to appear along with 20/20?

Not necessarily (different bugs) but I can bring 20/20 immediately after this
one if you want.

> 
> 
>> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 120 ++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/mtd/spi-nor.h   |   3 ++
>>  2 files changed, 118 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 725dab241271..f96bc80c0ed1 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -959,12 +959,19 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len)
>>  	return spi_nor_wait_till_ready(nor);
>>  }
>>  
> [...]
>> +/**
>>   * spi_nor_write_sr2() - Write the Status Register 2 using the
>>   * SPINOR_OP_WRSR2 (3eh) command.
>>   * @nor:	pointer to 'struct spi_nor'.
>> @@ -3634,19 +3723,38 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>  		break;
>>  
>>  	case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
>> +		/*
>> +		 * Writing only one byte to the Status Register has the
>> +		 * side-effect of clearing Status Register 2.
>> +		 */
>>  	case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
>> +		/*
>> +		 * Read Configuration Register (35h) instruction is not
>> +		 * supported.
>> +		 */
>> +		nor->flags |= SNOR_F_HAS_16BIT_SR | SNOR_F_NO_READ_CR;
> Since SNOR_F_HAS_16BIT_SR is set by default in
> spi_nor_info_init_params(), no need to set the flag here again
> 

I did this on purpose. I set SNOR_F_HAS_16BIT_SR here based on SFDP standard, I
want to indicate where the standard requires the 16 bit SR write .
spi_nor_info_init_params() initializes data based on info, but that data can be
overwritten (even with the same data) when parsing SFDP.

Thanks,
ta