* Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-01-23 11:51 [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window Rafał Miłecki
@ 2017-02-04 22:01 ` Marek Vasut
2017-02-04 22:27 ` Rafał Miłecki
2017-02-08 19:35 ` Brian Norris
2017-02-08 22:53 ` [PATCH V4] " Rafał Miłecki
2 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2017-02-04 22:01 UTC (permalink / raw)
To: Rafał Miłecki, David Woodhouse, Brian Norris,
Boris Brezillon, Richard Weinberger, Cyrille Pitchen
Cc: linux-mtd, Hauke Mehrtens, Rafał Miłecki
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-02-04 22:01 ` Marek Vasut
@ 2017-02-04 22:27 ` Rafał Miłecki
2017-02-04 22:31 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Rafał Miłecki @ 2017-02-04 22:27 UTC (permalink / raw)
To: Marek Vasut
Cc: Rafał Miłecki, David Woodhouse, Brian Norris,
Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-mtd,
Hauke Mehrtens
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-02-04 22:27 ` Rafał Miłecki
@ 2017-02-04 22:31 ` Marek Vasut
0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2017-02-04 22:31 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Rafał Miłecki, David Woodhouse, Brian Norris,
Boris Brezillon, Richard Weinberger, Cyrille Pitchen, linux-mtd,
Hauke Mehrtens
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-01-23 11:51 [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window Rafał Miłecki
2017-02-04 22:01 ` Marek Vasut
@ 2017-02-08 19:35 ` Brian Norris
2017-02-08 22:53 ` Rafał Miłecki
2017-02-08 22:53 ` [PATCH V4] " Rafał Miłecki
2 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2017-02-08 19:35 UTC (permalink / raw)
To: Rafał Miłecki
Cc: David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd, Hauke Mehrtens,
Rafał Miłecki
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-02-08 19:35 ` Brian Norris
@ 2017-02-08 22:53 ` Rafał Miłecki
0 siblings, 0 replies; 9+ messages in thread
From: Rafał Miłecki @ 2017-02-08 22:53 UTC (permalink / raw)
To: Brian Norris
Cc: David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd@lists.infradead.org, Hauke Mehrtens,
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ł
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V4] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-01-23 11:51 [PATCH V3] mtd: bcm47xxsflash: support reading flash out of mapping window Rafał Miłecki
2017-02-04 22:01 ` Marek Vasut
2017-02-08 19:35 ` Brian Norris
@ 2017-02-08 22:53 ` Rafał Miłecki
2017-02-08 23:09 ` Brian Norris
2 siblings, 1 reply; 9+ messages in thread
From: Rafał Miłecki @ 2017-02-08 22:53 UTC (permalink / raw)
To: David Woodhouse, Brian Norris, Boris Brezillon, Marek Vasut,
Richard Weinberger, Cyrille Pitchen
Cc: linux-mtd, Hauke Mehrtens, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH V4] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-02-08 22:53 ` [PATCH V4] " Rafał Miłecki
@ 2017-02-08 23:09 ` Brian Norris
2017-02-08 23:12 ` Brian Norris
0 siblings, 1 reply; 9+ messages in thread
From: Brian Norris @ 2017-02-08 23:09 UTC (permalink / raw)
To: Rafał Miłecki
Cc: David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd, Hauke Mehrtens,
Rafał Miłecki
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 :)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH V4] mtd: bcm47xxsflash: support reading flash out of mapping window
2017-02-08 23:09 ` Brian Norris
@ 2017-02-08 23:12 ` Brian Norris
0 siblings, 0 replies; 9+ messages in thread
From: Brian Norris @ 2017-02-08 23:12 UTC (permalink / raw)
To: Rafał Miłecki
Cc: David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger,
Cyrille Pitchen, linux-mtd, Hauke Mehrtens,
Rafał Miłecki
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.
^ permalink raw reply [flat|nested] 9+ messages in thread