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

[Boris Brezillon <boris.brezillon@bootlin.com>]

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