[PATCH] mtd: spi-nor: Return error when nor->addr

Discussion:

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

Liu Xiang

2018-11-14 12:56:05 UTC

In 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 not match
the device size in spi_nor_parse_sfdp().

Suggested-by: Boris Brezillon <***@bootlin.com>
Signed-off-by: Liu Xiang <***@zte.com.cn>
---
drivers/mtd/spi-nor/spi-nor.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 3eba13a..77eaf22 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2669,6 +2669,10 @@ 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*/
+ 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

Boris Brezillon

2018-11-14 13:51:29 UTC

Permalink

On Wed, 14 Nov 2018 20:56:05 +0800

Post by Liu Xiang
In 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.

According to your other patch this NOR supports 4B opcode, which means
the SFDP table is wrong.

Post by Liu Xiang
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 not match

^does not

Post by Liu Xiang
the device size in spi_nor_parse_sfdp().
---
drivers/mtd/spi-nor/spi-nor.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 3eba13a..77eaf22 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2669,6 +2669,10 @@ 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*/

Please add a white space after '/*' and before '*/':

/* If the device exceeds 16MiB, ->addr_width must be 4. */

Post by Liu Xiang
+ if ((params->size > 0x1000000) && (nor->addr_width == 3))

Parens are not needed around sub-conditions:

if (params->size > 0x1000000 && nor->addr_width == 3)

Post by Liu Xiang
+ return -EINVAL;
+

I'm not sure this is correct. Looks like some NORs only support 3B
opcodes but have a "4-byte addressing" mode (see set_4byte() [1]).
Don't know what's reported by the BFPT section in this case though
(BFPT_DWORD1_ADDRESS_BYTES_3_ONLY or BFPT_DWORD1_ADDRESS_BYTES_3_OR_4).

Anyway, I think this check should be moved here [2] to cover the
non-SFDP case.

Post by Liu Xiang
/* Fast Read settings. */
for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];

[1]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L278
[2]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L3758

Liu Xiang

2018-11-14 14:46:03 UTC

Permalink

Hi,
If the check is moved to
[2]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L3758,
the nor->addr_width selection logic may be needed to rework.

I have sent an email to ISSI for the register value, but haven't got the reply.
Do you think it is better to add this check until I get the right answer from ISSI?

Post by Boris Brezillon
On Wed, 14 Nov 2018 20:56:05 +0800

Post by Liu Xiang
In 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.

According to your other patch this NOR supports 4B opcode, which means
the SFDP table is wrong.

Post by Liu Xiang
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 not match

^does not

/* If the device exceeds 16MiB, ->addr_width must be 4. */

Post by Liu Xiang
+ if ((params->size > 0x1000000) && (nor->addr_width == 3))

if (params->size > 0x1000000 && nor->addr_width == 3)

Post by Liu Xiang
+ return -EINVAL;
+

I'm not sure this is correct. Looks like some NORs only support 3B
opcodes but have a "4-byte addressing" mode (see set_4byte() [1]).
Don't know what's reported by the BFPT section in this case though
(BFPT_DWORD1_ADDRESS_BYTES_3_ONLY or BFPT_DWORD1_ADDRESS_BYTES_3_OR_4).
Anyway, I think this check should be moved here [2] to cover the
non-SFDP case.

Post by Liu Xiang
/* Fast Read settings. */
for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];

[1]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L278
[2]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L3758

T***@microchip.com

2018-11-15 10:54:39 UTC

Permalink

Hi, Liu, Boris, Cyrille,

Post by Boris Brezillon
On Wed, 14 Nov 2018 20:56:05 +0800

Post by Liu Xiang
In is25lp256, the DWORD1 of JEDEC Basic Flash Parameter Header
is 0xfff920e5. So the DWORD1[18:17] Address Bytes bits are 0b00,

Liu, can you point us to a datasheet that has the JEDEC BFPT tables described? I
couldn't find one ...

Post by Boris Brezillon

Post by Liu Xiang
means that 3-Byte only addressing.

According to your other patch this NOR supports 4B opcode, which means
the SFDP table is wrong.

Post by Liu Xiang
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 not match

^does not

/* If the device exceeds 16MiB, ->addr_width must be 4. */

Post by Liu Xiang
+ if ((params->size > 0x1000000) && (nor->addr_width == 3))

if (params->size > 0x1000000 && nor->addr_width == 3)

Post by Liu Xiang
+ return -EINVAL;
+

Boris, this is in close relation with your second patch: [PATCH v3 2/2] mtd:
spi-nor: Use 4B opcodes when the NOR advertises both 3B and 4B.

When looking again at this, I would say that for the flashes that have a "4-byte
addressing" mode, but just 3B opcodes, I would expect the DWORD1[18:17] to be of
value BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 (enters 4-Byte mode on command - uses 3B
opcodes).

If BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 and 4B opcodes, then we can query BFPT
DWORD16[31:24]: it should have value xx1x_xxxxb to indicate that 4B opcodes are
supported. But which 4B opcodes are supported? Do all 3B opcodes have a 4B
opcode correspondent if SFDP 4-byte table is not available? This might be a good
assumption, but I can't see it anywhere in jesd216c.

Cyrille, when looking at ba3ae6a1d4c ("mtd: spi-nor: add a stateless method to
support memory size above 128Mib") I see that all flashes that declare
SPI_NOR_4B_OPCODES will have the 3B opcodes converted to 4B opcodes, assuming
that all 3B opcodes have a 4B correspondent. Is this assumption a general rule,
or it's just for the Spansion flashes?

Cheers,
ta

Post by Boris Brezillon
Anyway, I think this check should be moved here [2] to cover the
non-SFDP case.

Post by Liu Xiang
/* Fast Read settings. */
for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];

[1]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L278
[2]https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/mtd/spi-nor/spi-nor.c#L3758

Boris Brezillon

2018-11-15 11:02:56 UTC

Permalink

On Thu, 15 Nov 2018 10:54:39 +0000

Post by T***@microchip.com
Hi, Liu, Boris, Cyrille,

Post by Boris Brezillon
On Wed, 14 Nov 2018 20:56:05 +0800

Post by Liu Xiang
In is25lp256, the DWORD1 of JEDEC Basic Flash Parameter Header
is 0xfff920e5. So the DWORD1[18:17] Address Bytes bits are 0b00,

Liu, can you point us to a datasheet that has the JEDEC BFPT tables described? I
couldn't find one ...

Post by Boris Brezillon

Post by Liu Xiang
means that 3-Byte only addressing.

According to your other patch this NOR supports 4B opcode, which means
the SFDP table is wrong.

Post by Liu Xiang
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 not match

^does not

/* If the device exceeds 16MiB, ->addr_width must be 4. */

Post by Liu Xiang
+ if ((params->size > 0x1000000) && (nor->addr_width == 3))

if (params->size > 0x1000000 && nor->addr_width == 3)

Post by Liu Xiang
+ return -EINVAL;
+

spi-nor: Use 4B opcodes when the NOR advertises both 3B and 4B.
When looking again at this, I would say that for the flashes that have a "4-byte
addressing" mode, but just 3B opcodes, I would expect the DWORD1[18:17] to be of
value BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 (enters 4-Byte mode on command - uses 3B
opcodes).

The NOR we have and which is exposing BFPT_DWORD1_ADDRESS_BYTES_3_OR_4
actually supports both 3B and 4B commands, so, in this particular case,
BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 does not mean "3B opcode+4-byte
addressing mode"

Post by T***@microchip.com
If BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 and 4B opcodes, then we can query BFPT
DWORD16[31:24]: it should have value xx1x_xxxxb to indicate that 4B opcodes are
supported. But which 4B opcodes are supported?

I hope all of them. Wouldn't make sense to have only some of them
supported.

Post by T***@microchip.com
Do all 3B opcodes have a 4B
opcode correspondent if SFDP 4-byte table is not available? This might be a good
assumption, but I can't see it anywhere in jesd216c.

I hope so...

Liu Xiang

2018-11-16 13:24:10 UTC

Permalink

Hi Tudor, Boris, Cyrille,
There is no JEDEC BFPT tables in the datasheet.
In my test platform, I sent RDSFDP command to the flash and got the
parameters back.
My device type is IS25LP256D-JMLA, which is not in H/E/G/F series of flash
that is described in chapter 11 of the datasheet. The reply from ISSI suggests
only these certain devices can support SFDP table.
I am confused why my device can support RDSFDP command and give
parameters back. I will ask ISSI for more details.

Post by Boris Brezillon
On Thu, 15 Nov 2018 10:54:39 +0000

Post by T***@microchip.com
Hi, Liu, Boris, Cyrille,

Post by Boris Brezillon
On Wed, 14 Nov 2018 20:56:05 +0800

Post by Liu Xiang
In is25lp256, the DWORD1 of JEDEC Basic Flash Parameter Header
is 0xfff920e5. So the DWORD1[18:17] Address Bytes bits are 0b00,

Liu, can you point us to a datasheet that has the JEDEC BFPT tables described? I
couldn't find one ...

Post by Boris Brezillon

Post by Liu Xiang
means that 3-Byte only addressing.

According to your other patch this NOR supports 4B opcode, which means
the SFDP table is wrong.

Post by Liu Xiang
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 not match

^does not

/* If the device exceeds 16MiB, ->addr_width must be 4. */

Post by Liu Xiang
+ if ((params->size > 0x1000000) && (nor->addr_width == 3))

if (params->size > 0x1000000 && nor->addr_width == 3)

Post by Liu Xiang
+ return -EINVAL;
+

I hope all of them. Wouldn't make sense to have only some of them
supported.

Post by T***@microchip.com
Do all 3B opcodes have a 4B
opcode correspondent if SFDP 4-byte table is not available? This might be a good
assumption, but I can't see it anywhere in jesd216c.

I hope so...