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