Re: [PATCH v2] mtd: spi-nor: Return error when nor->addr_width does not match the device size

[Vignesh Raghavendra]

On 26/03/19 8:50 PM, Liu Xiang wrote:
> At 2019-03-19 13:22:15, "Vignesh Raghavendra" <vigneshr@xxxxxx> wrote:
>> Hi,
>>
>> On 13/03/19 7:15 PM, Liu Xiang wrote:
>>> In some is25lp256, the DWORD1 of JEDEC Basic Flash Parameter Header
>>> is 0xfff920e5. So the DWORD1[18:17] Address Bytes bits are 0b00,
>>> means that 3-Byte only addressing. But the device size is larger
>>> than 16MB, nor->addr_width must be 4 to access the whole address.
>>> An error should be returned when nor->addr_width does not match
>>> the device size in spi_nor_parse_bfpt(). Then it can go back to
>>> use spi_nor_ids[] for setting the right addr_width.
>>>
>>> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>
>>> Signed-off-by: Liu Xiang <liu.xiang6@xxxxxxxxxx>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index 6e13bbd..63933c7 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -2811,6 +2811,14 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>>  	}
>>>  	params->size >>= 3; /* Convert to bytes. */
>>>  
>>> +	/*
>>> +	 * If the device exceeds 16MiB, addr_width must be 4.
>>> +	 * addr_width == 3 means the Address Bytes we are
>>> +	 * reading from BFPT is wrong.
>>> +	 */
>>
>> JESD216 standard does not mandate flash devices >16MiB to always support
>> 4 byte addressing opcode. So, its okay for flash vendor to support
>>> 16MiB flash with 3 byte addressing and Bank/extended address register.
>>
>>> +	if (params->size > 0x1000000 && nor->addr_width == 3)
>>> +		return -EINVAL;
>>> +
>>
>> Assuming only DWORD1[18:17] bits are wrong, then returning from here
>> would mean we miss parsing Sector Erase settings, Quad Enable
>> Requirements etc from BFPT which is kind of bad.
>> I suggest to move the fix to[1], addr_width indicated in flash_info
>> struct of the device can take precedence over SFDP.
>>
>> [1]https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/spi-nor.c#L4106
> 
> Boris has added a fixup function, do you think this is more better:
> 
> static int
> is25lp256_post_bfpt_fixups(struct spi_nor *nor,
> 			    const struct sfdp_parameter_header *bfpt_header,
> 			    const struct sfdp_bfpt *bfpt,
> 			    struct spi_nor_flash_parameter *params)
> {
> 	/*
> 	 * IS25LP256 supports 4B opcodes.
> 	 * Unfortunately, some devices get BFPT_DWORD1_ADDRESS_BYTES_3_ONLY  
> 	 * from BFPT table for address width. We should fix it.
> 	 */
> 	if (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK == 
> 		BFPT_DWORD1_ADDRESS_BYTES_3_ONLY)
> 		nor->addr_width = 4;
> 
> 	return 0;
> }
> 
> static struct spi_nor_fixups is25lp256_fixups = {
> 	.post_bfpt = is25lp256_post_bfpt_fixups,
> };
> 
> 

Sounds fine to me as is25lp256 is the only part that needs this quirk at
the moment.


-- 
Regards
Vignesh