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

[Vignesh Raghavendra]

Hi,

On 06/11/19 2:03 PM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> 
> 
> 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.
> 

I was not suggesting on fixing up spi_nor_clear_sr_bp(), but pointing
out that single byte writes SR1 can happen until patch 20/20.

But now I see that these patches are fixing related but different bugs.

> 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. 

Ok, sounds fine, no need to change...

> 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.
> 

Alright.

Reviewed-by: Vignesh Raghavendra <vigneshr@xxxxxx>

Regards
Vignesh