[v2] mtd: spi-nor: Return error when nor->addr_width does not match the device size
diff mbox series

Message ID 1552484753-3393-1-git-send-email-liu.xiang6@zte.com.cn
State Changes Requested
Delegated to: Ambarus Tudor
Headers show
Series
  • [v2] mtd: spi-nor: Return error when nor->addr_width does not match the device size
Related show

Commit Message

Liu Xiang March 13, 2019, 1:45 p.m. UTC
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@bootlin.com>
Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
---
 drivers/mtd/spi-nor/spi-nor.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Vignesh Raghavendra March 19, 2019, 5:22 a.m. UTC | #1
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@bootlin.com>
> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
> ---
>  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


>  	/* Fast Read settings. */
>  	for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
>  		const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];
>
Liu Xiang March 26, 2019, 2:20 p.m. UTC | #2
Hi, Vignesh

Thanks for your suggestion. I will send a new patch.






At 2019-03-19 13:22:15, "Vignesh Raghavendra" <vigneshr@ti.com> 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@bootlin.com>
>> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
>> ---
>>  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
>
>
>>  	/* Fast Read settings. */
>>  	for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
>>  		const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];
>> 
>
>-- 
>Regards
>Vignesh
Liu Xiang March 26, 2019, 3:20 p.m. UTC | #3
Hi, Vignesh






At 2019-03-19 13:22:15, "Vignesh Raghavendra" <vigneshr@ti.com> 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@bootlin.com>
>> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
>> ---
>>  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,
};


>
>
>>  	/* Fast Read settings. */
>>  	for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
>>  		const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];
>> 
>
>-- 
>Regards
>Vignesh
Vignesh Raghavendra March 27, 2019, 4:04 p.m. UTC | #4
On 26/03/19 8:50 PM, Liu Xiang wrote:
> At 2019-03-19 13:22:15, "Vignesh Raghavendra" <vigneshr@ti.com> 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@bootlin.com>
>>> Signed-off-by: Liu Xiang <liu.xiang6@zte.com.cn>
>>> ---
>>>  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.

Patch
diff mbox series

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.
+	 */
+	if (params->size > 0x1000000 && nor->addr_width == 3)
+		return -EINVAL;
+
 	/* Fast Read settings. */
 	for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
 		const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];