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 <[email protected]>
Signed-off-by: Liu Xiang <[email protected]>
---
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.
+ */
+ 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];
--
1.9.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 <[email protected]>
> Signed-off-by: Liu Xiang <[email protected]>
> ---
> 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
Hi, Vignesh
Thanks for your suggestion. I will send a new patch.
At 2019-03-19 13:22:15, "Vignesh Raghavendra" <[email protected]> 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 <[email protected]>
>> Signed-off-by: Liu Xiang <[email protected]>
>> ---
>> 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
Hi, Vignesh
At 2019-03-19 13:22:15, "Vignesh Raghavendra" <[email protected]> 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 <[email protected]>
>> Signed-off-by: Liu Xiang <[email protected]>
>> ---
>> 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
On 26/03/19 8:50 PM, Liu Xiang wrote:
> At 2019-03-19 13:22:15, "Vignesh Raghavendra" <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Liu Xiang <[email protected]>
>>> ---
>>> 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