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

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

[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. 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 <boris.brezillon@bootlin.com>
Signed-off-by: Liu Xiang <liu.xiang6@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

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

[Boris Brezillon]

On Wed, 14 Nov 2018 20:56:05 +0800
Liu Xiang <liu.xiang6@zte.com.cn> wrote:

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

> 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

> the device size in spi_nor_parse_sfdp().
> 
> 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 | 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. */

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

Parens are not needed around sub-conditions:

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

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

>  	/* 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

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

[Liu Xiang]

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?



At 2018-11-14 21:51:29, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
>On Wed, 14 Nov 2018 20:56:05 +0800
>Liu Xiang <liu.xiang6@zte.com.cn> wrote:
>
>> 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.
>
>> 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
>
>> the device size in spi_nor_parse_sfdp().
>> 
>> 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 | 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. */
>
>> +	if ((params->size > 0x1000000) && (nor->addr_width == 3))
>
>Parens are not needed around sub-conditions:
>
>	if (params->size > 0x1000000 && nor->addr_width == 3)
>
>> +		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.
>
>>  	/* 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

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

[Tudor.Ambarus]

Hi, Liu, Boris, Cyrille,

On 11/14/2018 03:51 PM, Boris Brezillon wrote:
> On Wed, 14 Nov 2018 20:56:05 +0800
> Liu Xiang <liu.xiang6@zte.com.cn> wrote:
> 
>> 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 ...

>> means that 3-Byte only addressing.
> 
> According to your other patch this NOR supports 4B opcode, which means
> the SFDP table is wrong.
> 
>> 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
> 
>> the device size in spi_nor_parse_sfdp().
>>
>> 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 | 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. */
> 
>> +	if ((params->size > 0x1000000) && (nor->addr_width == 3))
> 
> Parens are not needed around sub-conditions:
> 
> 	if (params->size > 0x1000000 && nor->addr_width == 3)
> 
>> +		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).

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

> 
> Anyway, I think this check should be moved here [2] to cover the
> non-SFDP case.
> 
>>  	/* 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
> 

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

[Boris Brezillon]

On Thu, 15 Nov 2018 10:54:39 +0000
<Tudor.Ambarus@microchip.com> wrote:

> Hi, Liu, Boris, Cyrille,
> 
> On 11/14/2018 03:51 PM, Boris Brezillon wrote:
> > On Wed, 14 Nov 2018 20:56:05 +0800
> > Liu Xiang <liu.xiang6@zte.com.cn> wrote:
> >   
> >> 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 ...
> 
> >> means that 3-Byte only addressing.  
> > 
> > According to your other patch this NOR supports 4B opcode, which means
> > the SFDP table is wrong.
> >   
> >> 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
> >   
> >> the device size in spi_nor_parse_sfdp().
> >>
> >> 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 | 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. */
> >   
> >> +	if ((params->size > 0x1000000) && (nor->addr_width == 3))  
> > 
> > Parens are not needed around sub-conditions:
> > 
> > 	if (params->size > 0x1000000 && nor->addr_width == 3)
> >   
> >> +		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).  
> 
> 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).

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"

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

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

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

[Liu Xiang]

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.



At 2018-11-15 19:02:56, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
>On Thu, 15 Nov 2018 10:54:39 +0000
><Tudor.Ambarus@microchip.com> wrote:
>
>> Hi, Liu, Boris, Cyrille,
>> 
>> On 11/14/2018 03:51 PM, Boris Brezillon wrote:
>> > On Wed, 14 Nov 2018 20:56:05 +0800
>> > Liu Xiang <liu.xiang6@zte.com.cn> wrote:
>> >   
>> >> 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 ...
>> 
>> >> means that 3-Byte only addressing.  
>> > 
>> > According to your other patch this NOR supports 4B opcode, which means
>> > the SFDP table is wrong.
>> >   
>> >> 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
>> >   
>> >> the device size in spi_nor_parse_sfdp().
>> >>
>> >> 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 | 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. */
>> >   
>> >> +	if ((params->size > 0x1000000) && (nor->addr_width == 3))  
>> > 
>> > Parens are not needed around sub-conditions:
>> > 
>> > 	if (params->size > 0x1000000 && nor->addr_width == 3)
>> >   
>> >> +		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).  
>> 
>> 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).
>
>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"
>
>> 
>> 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.
>
>> 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...

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

[Liu Xiang]

Hi, Boris
I am sorry I have not got any further information from ISSI since last reply.
As your suggest, when the Address Bytes we are reading from BFPT is wrong, an error is returned
from spi_nor_parse_bfpt(). Then it can go back to use spi_nor_ids[] for setting addr_width. If
we make sure the spi_nor_ids[] is right, it can work well.
I will send a v2 patch.


At 2018-11-16 21:24:10, "Liu Xiang" <liuxiang_1999@126.com> wrote:
>
>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.
>
>
>
>At 2018-11-15 19:02:56, "Boris Brezillon" <boris.brezillon@bootlin.com> wrote:
>>On Thu, 15 Nov 2018 10:54:39 +0000
>><Tudor.Ambarus@microchip.com> wrote:
>>
>>> Hi, Liu, Boris, Cyrille,
>>> 
>>> On 11/14/2018 03:51 PM, Boris Brezillon wrote:
>>> > On Wed, 14 Nov 2018 20:56:05 +0800
>>> > Liu Xiang <liu.xiang6@zte.com.cn> wrote:
>>> >   
>>> >> 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 ...
>>> 
>>> >> means that 3-Byte only addressing.  
>>> > 
>>> > According to your other patch this NOR supports 4B opcode, which means
>>> > the SFDP table is wrong.
>>> >   
>>> >> 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
>>> >   
>>> >> the device size in spi_nor_parse_sfdp().
>>> >>
>>> >> 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 | 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. */
>>> >   
>>> >> +	if ((params->size > 0x1000000) && (nor->addr_width == 3))  
>>> > 
>>> > Parens are not needed around sub-conditions:
>>> > 
>>> > 	if (params->size > 0x1000000 && nor->addr_width == 3)
>>> >   
>>> >> +		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).  
>>> 
>>> 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).
>>
>>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"
>>
>>> 
>>> 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.
>>
>>> 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...