RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

[Bean Huo 霍斌斌 (beanhuo)]

Hi, Adam
This is true, but this only exist in Micron 65nm spi nor N25Q.
For our 45nm spi nor MT25Q , MT25TL and MT25Q, does exist this question.
So I think, can you differentiate between these in your patch?

> From: Adam Somerville <adamsomerville@gmail.com>
> 
> So as far as I can see there is a bug in the spi-nor driver when issuing die
> boundary crossing reads on Micron multi-die devices.
> Micron N25Q512A, N25Q00AA and probably any other Micron multi-die
> devices do not support a single read request that crosses a die boundary.
> 
> The current behaviour is that the address on the device wraps back to the
> start of the first die, with any data returned beyond the boundary being from
> the start of the first die.
> 
> To reproduce run:
> (assuming mtd0 is at offset 0 and spans across die boundary)
> 
> mtd_debug read /dev/mtd0 0x1FFFFF0 32 bad
> 
> and compare to:
> 
> mtd_debug read /dev/mtd0 0x1FFFFF0 16 good1 mtd_debug read /dev/mtd0
> 0x2000000 16 good2
> 
> Instead we should split the read request in to 1 read per die.
> 
> Tested on Cyclone 5 SOC with cadence-qspi connected to Micron n25q512ax3
> 
> Signed-off-by: Adam Somerville <adamsomerville@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c |   21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index ed0c19c..68dc20e 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -903,10 +903,19 @@ static const struct flash_info
> *spi_nor_read_id(struct spi_nor *nor)
>  	return ERR_PTR(-ENODEV);
>  }
> 
> +/*
> + * Micron multi-die devices do not support cross die boundary
> + * reads. Split the read request in to 1 read per die  */ #define
> +MIN_READ_BOUNDARY 0x2000000
> +#define DIST_TO_BOUNDARY(x)	\
> +		(MIN_READ_BOUNDARY - ((u32)x & (MIN_READ_BOUNDARY - 1)))
> +
>  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>  			size_t *retlen, u_char *buf)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	size_t read_len;
>  	int ret;
> 
>  	dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); @@
> -915,7 +924,17 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
>  	if (ret)
>  		return ret;
> 
> -	ret = nor->read(nor, from, len, retlen, buf);
> +	do {
> +		read_len = min(len, DIST_TO_BOUNDARY(from));
> +
> +		ret = nor->read(nor, from, read_len, retlen, buf);
> +		if (ret)
> +			break;
> +
> +		from += read_len;
> +		buf += read_len;
> +		len -= read_len;
> +	} while (len > 0);
> 
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
>  	return ret;
> --
> 1.7.9.5

Re: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

[Adam Somerville]

Hi,

Thanks for looking at the this.

I can rewrite so it only affects N25Q devices, I hoped a simpler
implementation would outweigh the small overhead incurred on non
affected devices.

Do you know if there are any plans for MT25* based multi-die devices
and if they would have the same issue?

Thanks,

Adam

On Wed, Jan 20, 2016 at 2:45 AM, Bean Huo 霍斌斌 (beanhuo)
<beanhuo@micron.com> wrote:
> Hi, Adam
> This is true, but this only exist in Micron 65nm spi nor N25Q.
> For our 45nm spi nor MT25Q , MT25TL and MT25Q, does exist this question.
> So I think, can you differentiate between these in your patch?
>
>> From: Adam Somerville <adamsomerville@gmail.com>
>>
>> So as far as I can see there is a bug in the spi-nor driver when issuing die
>> boundary crossing reads on Micron multi-die devices.
>> Micron N25Q512A, N25Q00AA and probably any other Micron multi-die
>> devices do not support a single read request that crosses a die boundary.
>>
>> The current behaviour is that the address on the device wraps back to the
>> start of the first die, with any data returned beyond the boundary being from
>> the start of the first die.
>>

Re: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

[Boris Brezillon]

Adam, Bean,

On Wed, 20 Jan 2016 12:01:06 +0000
Adam Somerville <adamsomerville@gmail.com> wrote:

> Hi,
> 
> Thanks for looking at the this.
> 
> I can rewrite so it only affects N25Q devices, I hoped a simpler
> implementation would outweigh the small overhead incurred on non
> affected devices.

I tend to think the same way: not sure the performance penalty is worth
the pain.

Best Regards,

Boris

> 
> Do you know if there are any plans for MT25* based multi-die devices
> and if they would have the same issue?
> 
> Thanks,
> 
> Adam
> 
> On Wed, Jan 20, 2016 at 2:45 AM, Bean Huo 霍斌斌 (beanhuo)
> <beanhuo@micron.com> wrote:
> > Hi, Adam
> > This is true, but this only exist in Micron 65nm spi nor N25Q.
> > For our 45nm spi nor MT25Q , MT25TL and MT25Q, does exist this question.
> > So I think, can you differentiate between these in your patch?
> >
> >> From: Adam Somerville <adamsomerville@gmail.com>
> >>
> >> So as far as I can see there is a bug in the spi-nor driver when issuing die
> >> boundary crossing reads on Micron multi-die devices.
> >> Micron N25Q512A, N25Q00AA and probably any other Micron multi-die
> >> devices do not support a single read request that crosses a die boundary.
> >>
> >> The current behaviour is that the address on the device wraps back to the
> >> start of the first die, with any data returned beyond the boundary being from
> >> the start of the first die.
> >>



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

[Bean Huo 霍斌斌 (beanhuo)]

 Hi, Adam and Boris

For Micron MT25Q ,MT25T and MT35Q, they does not exist this action even they are
Multi-die devices. So when the last byte of the die selected is read, the next byte output
is the first byte of next die(not the same die). 
You can check this by extended address register chapter in our datasheet, there are detail
Information.

> Hi,
> 
> Thanks for looking at the this.
> 
> I can rewrite so it only affects N25Q devices, I hoped a simpler
> implementation would outweigh the small overhead incurred on non affected
> devices.
> 
> Do you know if there are any plans for MT25* based multi-die devices and if
> they would have the same issue?
> 
> Thanks,
> 
> Adam
> 
> On Wed, Jan 20, 2016 at 2:45 AM, Bean Huo 霍斌斌 (beanhuo)
> <beanhuo@micron.com> wrote:
> > Hi, Adam
> > This is true, but this only exist in Micron 65nm spi nor N25Q.
> > For our 45nm spi nor MT25Q , MT25TL and MT25Q, does exist this
> question.
> > So I think, can you differentiate between these in your patch?
> >
> >> From: Adam Somerville <adamsomerville@gmail.com>
> >>
> >> So as far as I can see there is a bug in the spi-nor driver when
> >> issuing die boundary crossing reads on Micron multi-die devices.
> >> Micron N25Q512A, N25Q00AA and probably any other Micron multi-die
> >> devices do not support a single read request that crosses a die boundary.
> >>
> >> The current behaviour is that the address on the device wraps back to
> >> the start of the first die, with any data returned beyond the
> >> boundary being from the start of the first die.
> >>

Re: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

[Boris Brezillon]

Hi Bean,

On Thu, 21 Jan 2016 01:06:48 +0000
Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote:

>  Hi, Adam and Boris
> 
> For Micron MT25Q ,MT25T and MT35Q, they does not exist this action even they are
> Multi-die devices. So when the last byte of the die selected is read, the next byte output
> is the first byte of next die(not the same die). 
> You can check this by extended address register chapter in our datasheet, there are detail
> Information.

I never said you were wrong ;), I just asked if it was relevant to
differentiate the two cases. IOW, would the implementation proposed by
Adam work correctly on all chips? And what is the real performance
penalty for MT25Q ,MT25T and MT35Q if we decide to split the read
command in several reads to handle this cross die case?

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices

[Bean Huo 霍斌斌 (beanhuo)]

> Hi Bean,
> 
> On Thu, 21 Jan 2016 01:06:48 +0000
> Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote:
> 
> >  Hi, Adam and Boris
> >
> > For Micron MT25Q ,MT25T and MT35Q, they does not exist this action
> > even they are Multi-die devices. So when the last byte of the die
> > selected is read, the next byte output is the first byte of next die(not the
> same die).
> > You can check this by extended address register chapter in our
> > datasheet, there are detail Information.
> 
> I never said you were wrong ;), I just asked if it was relevant to differentiate
> the two cases. IOW, would the implementation proposed by Adam work
> correctly on all chips? And what is the real performance penalty for
> MT25Q ,MT25T and MT35Q if we decide to split the read command in several
> reads to handle this cross die case?
For this , performance penalty is tiny, can ignore. 
SPI NOR read performance only depends on SPI I/O clock. Not the same as NAND.
> Best Regards,
> 
> Boris
> 
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[RFC] spi-nor: fix cross die reads on Micron multi-die devices

[adamsomerville]

From: Adam Somerville <adamsomerville@gmail.com>

So as far as I can see there is a bug in the spi-nor driver when
issuing die boundary crossing reads on Micron multi-die devices.
Micron N25Q512A, N25Q00AA and probably any other Micron multi-die
devices do not support a single read request that crosses a die
boundary.

The current behaviour is that the address on the device wraps back
to the start of the first die, with any data returned beyond the
boundary being from the start of the first die.

To reproduce run:
(assuming mtd0 is at offset 0 and spans across die boundary)

mtd_debug read /dev/mtd0 0x1FFFFF0 32 bad

and compare to:

mtd_debug read /dev/mtd0 0x1FFFFF0 16 good1
mtd_debug read /dev/mtd0 0x2000000 16 good2

Instead we should split the read request in to 1 read per die.

Tested on Cyclone 5 SOC with cadence-qspi connected to Micron n25q512ax3

Signed-off-by: Adam Somerville <adamsomerville@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ed0c19c..68dc20e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -903,10 +903,19 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 	return ERR_PTR(-ENODEV);
 }
 
+/*
+ * Micron multi-die devices do not support cross die boundary
+ * reads. Split the read request in to 1 read per die
+ */
+#define MIN_READ_BOUNDARY 0x2000000
+#define DIST_TO_BOUNDARY(x)	\
+		(MIN_READ_BOUNDARY - ((u32)x & (MIN_READ_BOUNDARY - 1)))
+
 static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 			size_t *retlen, u_char *buf)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	size_t read_len;
 	int ret;
 
 	dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
@@ -915,7 +924,17 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
-	ret = nor->read(nor, from, len, retlen, buf);
+	do {
+		read_len = min(len, DIST_TO_BOUNDARY(from));
+
+		ret = nor->read(nor, from, read_len, retlen, buf);
+		if (ret)
+			break;
+
+		from += read_len;
+		buf += read_len;
+		len -= read_len;
+	} while (len > 0);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
 	return ret;
-- 
1.7.9.5