All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices
@ 2016-01-20  2:45 Bean Huo 霍斌斌 (beanhuo)
  2016-01-20 12:01 ` Adam Somerville
  0 siblings, 1 reply; 7+ messages in thread
From: Bean Huo 霍斌斌 (beanhuo) @ 2016-01-20  2:45 UTC (permalink / raw)
  To: adamsomerville, David Woodhouse, Brian Norris
  Cc: linux-mtd, zajec5, Boris Brezillon, jteki, mika.westerberg,
	furquan, linux-mtd, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] spi-nor: fix cross die reads on Micron multi-die devices
  2016-01-20  2:45 [RFC] spi-nor: fix cross die reads on Micron multi-die devices Bean Huo 霍斌斌 (beanhuo)
@ 2016-01-20 12:01 ` Adam Somerville
  2016-01-20 12:24   ` Boris Brezillon
  2016-01-21  1:06   ` Bean Huo 霍斌斌 (beanhuo)
  0 siblings, 2 replies; 7+ messages in thread
From: Adam Somerville @ 2016-01-20 12:01 UTC (permalink / raw)
  To: Bean Huo 霍斌斌 (beanhuo)
  Cc: David Woodhouse, Brian Norris, linux-mtd, zajec5,
	Boris Brezillon, jteki, mika.westerberg, furquan, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] spi-nor: fix cross die reads on Micron multi-die devices
  2016-01-20 12:01 ` Adam Somerville
@ 2016-01-20 12:24   ` Boris Brezillon
  2016-01-21  1:06   ` Bean Huo 霍斌斌 (beanhuo)
  1 sibling, 0 replies; 7+ messages in thread
From: Boris Brezillon @ 2016-01-20 12:24 UTC (permalink / raw)
  To: Adam Somerville
  Cc: Bean Huo 霍斌斌 (beanhuo),
	David Woodhouse, Brian Norris, linux-mtd, zajec5, jteki,
	mika.westerberg, furquan, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices
  2016-01-20 12:01 ` Adam Somerville
  2016-01-20 12:24   ` Boris Brezillon
@ 2016-01-21  1:06   ` Bean Huo 霍斌斌 (beanhuo)
  2016-01-21  8:56     ` Boris Brezillon
  1 sibling, 1 reply; 7+ messages in thread
From: Bean Huo 霍斌斌 (beanhuo) @ 2016-01-21  1:06 UTC (permalink / raw)
  To: Adam Somerville
  Cc: David Woodhouse, Brian Norris, linux-mtd, zajec5,
	Boris Brezillon, jteki, mika.westerberg, furquan, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC] spi-nor: fix cross die reads on Micron multi-die devices
  2016-01-21  1:06   ` Bean Huo 霍斌斌 (beanhuo)
@ 2016-01-21  8:56     ` Boris Brezillon
  2016-01-22  7:00       ` Bean Huo 霍斌斌 (beanhuo)
  0 siblings, 1 reply; 7+ messages in thread
From: Boris Brezillon @ 2016-01-21  8:56 UTC (permalink / raw)
  To: Bean Huo 霍斌斌 (beanhuo)
  Cc: Adam Somerville, David Woodhouse, Brian Norris, linux-mtd,
	zajec5, jteki, mika.westerberg, furquan, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [RFC] spi-nor: fix cross die reads on Micron multi-die devices
  2016-01-21  8:56     ` Boris Brezillon
@ 2016-01-22  7:00       ` Bean Huo 霍斌斌 (beanhuo)
  0 siblings, 0 replies; 7+ messages in thread
From: Bean Huo 霍斌斌 (beanhuo) @ 2016-01-22  7:00 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Adam Somerville, David Woodhouse, Brian Norris, linux-mtd,
	zajec5, jteki, mika.westerberg, furquan, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC] spi-nor: fix cross die reads on Micron multi-die devices
@ 2016-01-19 15:01 adamsomerville
  0 siblings, 0 replies; 7+ messages in thread
From: adamsomerville @ 2016-01-19 15:01 UTC (permalink / raw)
  To: David Woodhouse, Brian Norris
  Cc: Rafał Miłecki, Gabor Juhos, Boris Brezillon,
	Jagan Teki, Mika Westerberg, Furquan Shaikh, linux-mtd,
	linux-kernel, Adam Somerville

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-01-22  7:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-20  2:45 [RFC] spi-nor: fix cross die reads on Micron multi-die devices Bean Huo 霍斌斌 (beanhuo)
2016-01-20 12:01 ` Adam Somerville
2016-01-20 12:24   ` Boris Brezillon
2016-01-21  1:06   ` Bean Huo 霍斌斌 (beanhuo)
2016-01-21  8:56     ` Boris Brezillon
2016-01-22  7:00       ` Bean Huo 霍斌斌 (beanhuo)
  -- strict thread matches above, loose matches on Subject: below --
2016-01-19 15:01 adamsomerville

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.