[PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

For reading flash content we use MMIO but it's possible to read only
first 16 MiB this way. It's simply an arch design/limitation.
To support flash sizes bigger than 16 MiB implement indirect access
using ChipCommon registers.
This has been tested using MX25L25635F.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Simplify line writing to buf
    Add some trivial comment for OPCODE_ST_READ4B
    Both requested by Marek
V3: Use as much as possible MMIO access for read crossing boundary (performance)
---
 drivers/mtd/devices/bcm47xxsflash.c | 23 ++++++++++++++++++++---
 drivers/mtd/devices/bcm47xxsflash.h |  3 +++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
index 4decd8c0360a..138689619ab4 100644
--- a/drivers/mtd/devices/bcm47xxsflash.c
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -105,15 +105,32 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 			      size_t *retlen, u_char *buf)
 {
 	struct bcm47xxsflash *b47s = mtd->priv;
+	size_t orig_len = len;
 
 	/* Check address range */
 	if ((from + len) > mtd->size)
 		return -EINVAL;
 
-	memcpy_fromio(buf, b47s->window + from, len);
-	*retlen = len;
+	/* Read as much as possible using fast MMIO window */
+	if (from < BCM47XXSFLASH_WINDOW_SZ) {
+		size_t memcpy_len;
 
-	return len;
+		memcpy_len = min(len, (size_t)(BCM47XXSFLASH_WINDOW_SZ - from));
+		memcpy_fromio(buf, b47s->window + from, memcpy_len);
+		from += memcpy_len;
+		len -= memcpy_len;
+	}
+
+	/* Use indirect access for content out of the window */
+	for (; len; len--) {
+		b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
+		bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
+		*buf++ = b47s->cc_read(b47s, BCMA_CC_FLASHDATA);
+	}
+
+	*retlen = orig_len;
+
+	return orig_len;
 }
 
 static int bcm47xxsflash_write_st(struct mtd_info *mtd, u32 offset, size_t len,
diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
index 1564b62b412e..b2d7b38f75fd 100644
--- a/drivers/mtd/devices/bcm47xxsflash.h
+++ b/drivers/mtd/devices/bcm47xxsflash.h
@@ -3,6 +3,8 @@
 
 #include <linux/mtd/mtd.h>
 
+#define BCM47XXSFLASH_WINDOW_SZ			SZ_16M
+
 /* Used for ST flashes only. */
 #define OPCODE_ST_WREN		0x0006		/* Write Enable */
 #define OPCODE_ST_WRDIS		0x0004		/* Write Disable */
@@ -16,6 +18,7 @@
 #define OPCODE_ST_RES		0x03ab		/* Read Electronic Signature */
 #define OPCODE_ST_CSA		0x1000		/* Keep chip select asserted */
 #define OPCODE_ST_SSE		0x0220		/* Sub-sector Erase */
+#define OPCODE_ST_READ4B	0x6313		/* Read Data Bytes in 4Byte addressing mode */
 
 /* Used for Atmel flashes only. */
 #define OPCODE_AT_READ				0x07e8
-- 
2.11.0

Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[Marek Vasut]

On 01/23/2017 12:51 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> For reading flash content we use MMIO but it's possible to read only
> first 16 MiB this way. It's simply an arch design/limitation.
> To support flash sizes bigger than 16 MiB implement indirect access
> using ChipCommon registers.
> This has been tested using MX25L25635F.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Simplify line writing to buf
>     Add some trivial comment for OPCODE_ST_READ4B
>     Both requested by Marek
> V3: Use as much as possible MMIO access for read crossing boundary (performance)

It's not because of performance, but because you can start reading at 15
MiB offset and read 2 MiB , at which point the previous patch would IMO
fail.

Acked-by: Marek Vasut <marek.vasut@gmail.com>

Thanks!

> ---
>  drivers/mtd/devices/bcm47xxsflash.c | 23 ++++++++++++++++++++---
>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> index 4decd8c0360a..138689619ab4 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.c
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -105,15 +105,32 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>  			      size_t *retlen, u_char *buf)
>  {
>  	struct bcm47xxsflash *b47s = mtd->priv;
> +	size_t orig_len = len;
>  
>  	/* Check address range */
>  	if ((from + len) > mtd->size)
>  		return -EINVAL;
>  
> -	memcpy_fromio(buf, b47s->window + from, len);
> -	*retlen = len;
> +	/* Read as much as possible using fast MMIO window */
> +	if (from < BCM47XXSFLASH_WINDOW_SZ) {
> +		size_t memcpy_len;
>  
> -	return len;
> +		memcpy_len = min(len, (size_t)(BCM47XXSFLASH_WINDOW_SZ - from));
> +		memcpy_fromio(buf, b47s->window + from, memcpy_len);
> +		from += memcpy_len;
> +		len -= memcpy_len;
> +	}
> +
> +	/* Use indirect access for content out of the window */
> +	for (; len; len--) {
> +		b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
> +		bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
> +		*buf++ = b47s->cc_read(b47s, BCMA_CC_FLASHDATA);
> +	}
> +
> +	*retlen = orig_len;
> +
> +	return orig_len;
>  }
>  
>  static int bcm47xxsflash_write_st(struct mtd_info *mtd, u32 offset, size_t len,
> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
> index 1564b62b412e..b2d7b38f75fd 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.h
> +++ b/drivers/mtd/devices/bcm47xxsflash.h
> @@ -3,6 +3,8 @@
>  
>  #include <linux/mtd/mtd.h>
>  
> +#define BCM47XXSFLASH_WINDOW_SZ			SZ_16M
> +
>  /* Used for ST flashes only. */
>  #define OPCODE_ST_WREN		0x0006		/* Write Enable */
>  #define OPCODE_ST_WRDIS		0x0004		/* Write Disable */
> @@ -16,6 +18,7 @@
>  #define OPCODE_ST_RES		0x03ab		/* Read Electronic Signature */
>  #define OPCODE_ST_CSA		0x1000		/* Keep chip select asserted */
>  #define OPCODE_ST_SSE		0x0220		/* Sub-sector Erase */
> +#define OPCODE_ST_READ4B	0x6313		/* Read Data Bytes in 4Byte addressing mode */
>  
>  /* Used for Atmel flashes only. */
>  #define OPCODE_AT_READ				0x07e8
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[Rafał Miłecki]

On 2017-02-04 23:01, Marek Vasut wrote:
> On 01/23/2017 12:51 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>> 
>> For reading flash content we use MMIO but it's possible to read only
>> first 16 MiB this way. It's simply an arch design/limitation.
>> To support flash sizes bigger than 16 MiB implement indirect access
>> using ChipCommon registers.
>> This has been tested using MX25L25635F.
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Simplify line writing to buf
>>     Add some trivial comment for OPCODE_ST_READ4B
>>     Both requested by Marek
>> V3: Use as much as possible MMIO access for read crossing boundary 
>> (performance)
> 
> It's not because of performance, but because you can start reading at 
> 15
> MiB offset and read 2 MiB , at which point the previous patch would IMO
> fail.

It never failed. It was slow but was working as expected.


> Acked-by: Marek Vasut <marek.vasut@gmail.com>
> 
> Thanks!

Thanks

Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[Marek Vasut]

On 02/04/2017 11:27 PM, Rafał Miłecki wrote:
> On 2017-02-04 23:01, Marek Vasut wrote:
>> On 01/23/2017 12:51 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> For reading flash content we use MMIO but it's possible to read only
>>> first 16 MiB this way. It's simply an arch design/limitation.
>>> To support flash sizes bigger than 16 MiB implement indirect access
>>> using ChipCommon registers.
>>> This has been tested using MX25L25635F.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> V2: Simplify line writing to buf
>>>     Add some trivial comment for OPCODE_ST_READ4B
>>>     Both requested by Marek
>>> V3: Use as much as possible MMIO access for read crossing boundary
>>> (performance)
>>
>> It's not because of performance, but because you can start reading at 15
>> MiB offset and read 2 MiB , at which point the previous patch would IMO
>> fail.
> 
> It never failed. It was slow but was working as expected.

Maybe you didn't test with dumb enough mis-usage ;) I think something
like this would've triggered it:
$ dd if=/dev/mtd0 of=/dev/null seek=15M bs=2M
Assuming mtd0 is the whole flash device, and the flash is larger than 17
MiB.

But then again, it should be OK now and we can move on to the next problem.

>> Acked-by: Marek Vasut <marek.vasut@gmail.com>
>>
>> Thanks!
> 
> Thanks


-- 
Best regards,
Marek Vasut

Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[Brian Norris]

Hi,

On Mon, Jan 23, 2017 at 12:51:54PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> For reading flash content we use MMIO but it's possible to read only
> first 16 MiB this way. It's simply an arch design/limitation.
> To support flash sizes bigger than 16 MiB implement indirect access
> using ChipCommon registers.
> This has been tested using MX25L25635F.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Simplify line writing to buf
>     Add some trivial comment for OPCODE_ST_READ4B
>     Both requested by Marek
> V3: Use as much as possible MMIO access for read crossing boundary (performance)
> ---
>  drivers/mtd/devices/bcm47xxsflash.c | 23 ++++++++++++++++++++---
>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> index 4decd8c0360a..138689619ab4 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.c
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -105,15 +105,32 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>  			      size_t *retlen, u_char *buf)
>  {
>  	struct bcm47xxsflash *b47s = mtd->priv;
> +	size_t orig_len = len;
>  
>  	/* Check address range */
>  	if ((from + len) > mtd->size)
>  		return -EINVAL;
>  
> -	memcpy_fromio(buf, b47s->window + from, len);
> -	*retlen = len;
> +	/* Read as much as possible using fast MMIO window */
> +	if (from < BCM47XXSFLASH_WINDOW_SZ) {
> +		size_t memcpy_len;
>  
> -	return len;
> +		memcpy_len = min(len, (size_t)(BCM47XXSFLASH_WINDOW_SZ - from));
> +		memcpy_fromio(buf, b47s->window + from, memcpy_len);
> +		from += memcpy_len;
> +		len -= memcpy_len;

Am I crazy, or should you either be doing 'buf += memcpy_len'?
Otherwise, you'll be copying over the first few elements of 'buf' below,
instead of finishing off the tail end.

Brian

> +	}
> +
> +	/* Use indirect access for content out of the window */
> +	for (; len; len--) {
> +		b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
> +		bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
> +		*buf++ = b47s->cc_read(b47s, BCMA_CC_FLASHDATA);
> +	}
> +
> +	*retlen = orig_len;
> +
> +	return orig_len;
>  }
>  
>  static int bcm47xxsflash_write_st(struct mtd_info *mtd, u32 offset, size_t len,
> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
> index 1564b62b412e..b2d7b38f75fd 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.h
> +++ b/drivers/mtd/devices/bcm47xxsflash.h
> @@ -3,6 +3,8 @@
>  
>  #include <linux/mtd/mtd.h>
>  
> +#define BCM47XXSFLASH_WINDOW_SZ			SZ_16M
> +
>  /* Used for ST flashes only. */
>  #define OPCODE_ST_WREN		0x0006		/* Write Enable */
>  #define OPCODE_ST_WRDIS		0x0004		/* Write Disable */
> @@ -16,6 +18,7 @@
>  #define OPCODE_ST_RES		0x03ab		/* Read Electronic Signature */
>  #define OPCODE_ST_CSA		0x1000		/* Keep chip select asserted */
>  #define OPCODE_ST_SSE		0x0220		/* Sub-sector Erase */
> +#define OPCODE_ST_READ4B	0x6313		/* Read Data Bytes in 4Byte addressing mode */
>  
>  /* Used for Atmel flashes only. */
>  #define OPCODE_AT_READ				0x07e8
> -- 
> 2.11.0
> 

Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window

[Rafał Miłecki]

On 8 February 2017 at 20:35, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Jan 23, 2017 at 12:51:54PM +0100, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> For reading flash content we use MMIO but it's possible to read only
>> first 16 MiB this way. It's simply an arch design/limitation.
>> To support flash sizes bigger than 16 MiB implement indirect access
>> using ChipCommon registers.
>> This has been tested using MX25L25635F.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> V2: Simplify line writing to buf
>>     Add some trivial comment for OPCODE_ST_READ4B
>>     Both requested by Marek
>> V3: Use as much as possible MMIO access for read crossing boundary (performance)
>> ---
>>  drivers/mtd/devices/bcm47xxsflash.c | 23 ++++++++++++++++++++---
>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>  2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>> index 4decd8c0360a..138689619ab4 100644
>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>> @@ -105,15 +105,32 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>                             size_t *retlen, u_char *buf)
>>  {
>>       struct bcm47xxsflash *b47s = mtd->priv;
>> +     size_t orig_len = len;
>>
>>       /* Check address range */
>>       if ((from + len) > mtd->size)
>>               return -EINVAL;
>>
>> -     memcpy_fromio(buf, b47s->window + from, len);
>> -     *retlen = len;
>> +     /* Read as much as possible using fast MMIO window */
>> +     if (from < BCM47XXSFLASH_WINDOW_SZ) {
>> +             size_t memcpy_len;
>>
>> -     return len;
>> +             memcpy_len = min(len, (size_t)(BCM47XXSFLASH_WINDOW_SZ - from));
>> +             memcpy_fromio(buf, b47s->window + from, memcpy_len);
>> +             from += memcpy_len;
>> +             len -= memcpy_len;
>
> Am I crazy, or should you either be doing 'buf += memcpy_len'?
> Otherwise, you'll be copying over the first few elements of 'buf' below,
> instead of finishing off the tail end.

Excellent catch, thank you!

-- 
Rafał

[PATCH V4] mtd: bcm47xxsflash: support reading flash out of mapping window

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

For reading flash content we use MMIO but it's possible to read only
first 16 MiB this way. It's simply an arch design/limitation.
To support flash sizes bigger than 16 MiB implement indirect access
using ChipCommon registers.
This has been tested using MX25L25635F.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Acked-by: Marek Vasut <marek.vasut@gmail.com>
---
V2: Simplify line writing to buf
    Add some trivial comment for OPCODE_ST_READ4B
    Both requested by Marek
V3: Use as much as possible MMIO access for reads crossing boundary
V4: Add missing buf increment (thanks Brian!)
---
 drivers/mtd/devices/bcm47xxsflash.c | 24 +++++++++++++++++++++---
 drivers/mtd/devices/bcm47xxsflash.h |  3 +++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
index 4decd8c0360a..e2bd81817df4 100644
--- a/drivers/mtd/devices/bcm47xxsflash.c
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -105,15 +105,33 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 			      size_t *retlen, u_char *buf)
 {
 	struct bcm47xxsflash *b47s = mtd->priv;
+	size_t orig_len = len;
 
 	/* Check address range */
 	if ((from + len) > mtd->size)
 		return -EINVAL;
 
-	memcpy_fromio(buf, b47s->window + from, len);
-	*retlen = len;
+	/* Read as much as possible using fast MMIO window */
+	if (from < BCM47XXSFLASH_WINDOW_SZ) {
+		size_t memcpy_len;
 
-	return len;
+		memcpy_len = min(len, (size_t)(BCM47XXSFLASH_WINDOW_SZ - from));
+		memcpy_fromio(buf, b47s->window + from, memcpy_len);
+		from += memcpy_len;
+		len -= memcpy_len;
+		buf += memcpy_len;
+	}
+
+	/* Use indirect access for content out of the window */
+	for (; len; len--) {
+		b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
+		bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
+		*buf++ = b47s->cc_read(b47s, BCMA_CC_FLASHDATA);
+	}
+
+	*retlen = orig_len;
+
+	return orig_len;
 }
 
 static int bcm47xxsflash_write_st(struct mtd_info *mtd, u32 offset, size_t len,
diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
index 1564b62b412e..b2d7b38f75fd 100644
--- a/drivers/mtd/devices/bcm47xxsflash.h
+++ b/drivers/mtd/devices/bcm47xxsflash.h
@@ -3,6 +3,8 @@
 
 #include <linux/mtd/mtd.h>
 
+#define BCM47XXSFLASH_WINDOW_SZ			SZ_16M
+
 /* Used for ST flashes only. */
 #define OPCODE_ST_WREN		0x0006		/* Write Enable */
 #define OPCODE_ST_WRDIS		0x0004		/* Write Disable */
@@ -16,6 +18,7 @@
 #define OPCODE_ST_RES		0x03ab		/* Read Electronic Signature */
 #define OPCODE_ST_CSA		0x1000		/* Keep chip select asserted */
 #define OPCODE_ST_SSE		0x0220		/* Sub-sector Erase */
+#define OPCODE_ST_READ4B	0x6313		/* Read Data Bytes in 4Byte addressing mode */
 
 /* Used for Atmel flashes only. */
 #define OPCODE_AT_READ				0x07e8
-- 
2.11.0

Re: [PATCH V4] mtd: bcm47xxsflash: support reading flash out of mapping window

[Brian Norris]

On Wed, Feb 08, 2017 at 11:53:44PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> For reading flash content we use MMIO but it's possible to read only
> first 16 MiB this way. It's simply an arch design/limitation.
> To support flash sizes bigger than 16 MiB implement indirect access
> using ChipCommon registers.
> This has been tested using MX25L25635F.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> Acked-by: Marek Vasut <marek.vasut@gmail.com>
> ---
> V2: Simplify line writing to buf
>     Add some trivial comment for OPCODE_ST_READ4B
>     Both requested by Marek
> V3: Use as much as possible MMIO access for reads crossing boundary
> V4: Add missing buf increment (thanks Brian!)

Presumably you tested for this case now? Seems like quick turnaround :)

Re: [PATCH V4] mtd: bcm47xxsflash: support reading flash out of mapping window

[Brian Norris]

On Wed, Feb 08, 2017 at 03:09:21PM -0800, Brian Norris wrote:
> On Wed, Feb 08, 2017 at 11:53:44PM +0100, Rafał Miłecki wrote:
> > From: Rafał Miłecki <rafal@milecki.pl>
> > 
> > For reading flash content we use MMIO but it's possible to read only
> > first 16 MiB this way. It's simply an arch design/limitation.
> > To support flash sizes bigger than 16 MiB implement indirect access
> > using ChipCommon registers.
> > This has been tested using MX25L25635F.
> > 
> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > Acked-by: Marek Vasut <marek.vasut@gmail.com>
> > ---
> > V2: Simplify line writing to buf
> >     Add some trivial comment for OPCODE_ST_READ4B
> >     Both requested by Marek
> > V3: Use as much as possible MMIO access for reads crossing boundary
> > V4: Add missing buf increment (thanks Brian!)
> 
> Presumably you tested for this case now? Seems like quick turnaround :)

Anyway, LGTM. Applied to l2-mtd.git.