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@xxxxxxxxxxx> wrote:
>On Thu, 15 Nov 2018 10:54:39 +0000
><Tudor.Ambarus@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxx>
>> >> Signed-off-by: Liu Xiang <liu.xiang6@xxxxxxxxxx>
>> >> ---
>> >>  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...