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

[PATCH] 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>
---
 drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
 drivers/mtd/devices/bcm47xxsflash.h |  3 +++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
index 4decd8c..5d57119 100644
--- a/drivers/mtd/devices/bcm47xxsflash.c
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if ((from + len) > mtd->size)
 		return -EINVAL;
 
-	memcpy_fromio(buf, b47s->window + from, len);
+	if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
+		memcpy_fromio(buf, b47s->window + from, len);
+	} else {
+		size_t i;
+
+		for (i = 0; i < len; i++) {
+			b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
+			bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
+			*(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
+		}
+	}
 	*retlen = len;
 
 	return len;
diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
index 1564b62..d8d1093 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_SIZE		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
 
 /* Used for Atmel flashes only. */
 #define OPCODE_AT_READ				0x07e8
-- 
2.10.1

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

[Marek Vasut]

On 01/16/2017 11:09 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>
> ---
>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> index 4decd8c..5d57119 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.c
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	if ((from + len) > mtd->size)
>  		return -EINVAL;
>  
> -	memcpy_fromio(buf, b47s->window + from, len);
> +	if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
> +		memcpy_fromio(buf, b47s->window + from, len);
> +	} else {
> +		size_t i;
> +
> +		for (i = 0; i < len; i++) {
> +			b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
> +			bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
> +			*(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;

Do you really need to send command on every write into the FLASHADDR
register ? Doesn't this hardware support some sort of seqeuntial reading?

Are you sure this has no side-effects when you mix this with reading
from the flash window ?

Also, the parenthesis around buf++ are not needed and the & 0xff should
not be needed either.

> +		}
> +	}
>  	*retlen = len;
>  
>  	return len;
> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
> index 1564b62..d8d1093 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_SIZE		SZ_16M
> +
>  /* Used for ST flashes only. */
>  #define OPCODE_ST_WREN		0x0006		/* Write Enable */
>  #define OPCODE_ST_WRDIS		0x0004		/* Write Disable */

These look like standard opcodes, why don't you use standard opcodes in
this driver and instead redefine them here ?

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

Needs a comment to keep this consistent I think.

>  /* Used for Atmel flashes only. */
>  #define OPCODE_AT_READ				0x07e8
> 


-- 
Best regards,
Marek Vasut

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

[Rafał Miłecki]

On 17 January 2017 at 04:18, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/16/2017 11:09 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>
>> ---
>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>> index 4decd8c..5d57119 100644
>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>       if ((from + len) > mtd->size)
>>               return -EINVAL;
>>
>> -     memcpy_fromio(buf, b47s->window + from, len);
>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>> +             memcpy_fromio(buf, b47s->window + from, len);
>> +     } else {
>> +             size_t i;
>> +
>> +             for (i = 0; i < len; i++) {
>> +                     b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
>> +                     bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
>> +                     *(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
>
> Do you really need to send command on every write into the FLASHADDR
> register ? Doesn't this hardware support some sort of seqeuntial reading?

Yes, I need to. If there is some other way it's undocumented and not
used by Broadcom. As a quick check I tried not writing to FLASHADDR
every time, but it didn't work.


> Are you sure this has no side-effects when you mix this with reading
> from the flash window ?

No side effects. I also tried reading whole flash content using this
indirect access and it worked as well.


> Also, the parenthesis around buf++ are not needed and the & 0xff should
> not be needed either.

You're right about buf. I'm not sure about & 0xff though. In theory
you're correct. This is 32b register and we use 32b read function to
access it but I never saw anything set in 0xffffff00. On the other
hand Broadcom /advises/ (does) this & 0xff so I'm wondering if there
can be some hardware with unexpected behavior in the world? What would
happen if we got 32b value with some bits in ~0xff?


>> +             }
>> +     }
>>       *retlen = len;
>>
>>       return len;
>> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
>> index 1564b62..d8d1093 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_SIZE            SZ_16M
>> +
>>  /* Used for ST flashes only. */
>>  #define OPCODE_ST_WREN               0x0006          /* Write Enable */
>>  #define OPCODE_ST_WRDIS              0x0004          /* Write Disable */
>
> These look like standard opcodes, why don't you use standard opcodes in
> this driver and instead redefine them here ?

I guess it's because these are the only 2 opcodes we could share.
Other than them Broadcom uses their magic 16b opcodes so I guess it
doesn't change that much since we need most of them defined locally
anyway.


>> @@ -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
>
> Needs a comment to keep this consistent I think.

OK

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

[Marek Vasut]

On 01/17/2017 07:58 AM, Rafał Miłecki wrote:
> On 17 January 2017 at 04:18, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/16/2017 11:09 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>
>>> ---
>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>> index 4decd8c..5d57119 100644
>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>       if ((from + len) > mtd->size)
>>>               return -EINVAL;
>>>
>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>> +     } else {
>>> +             size_t i;
>>> +
>>> +             for (i = 0; i < len; i++) {
>>> +                     b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
>>> +                     bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
>>> +                     *(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
>>
>> Do you really need to send command on every write into the FLASHADDR
>> register ? Doesn't this hardware support some sort of seqeuntial reading?
> 
> Yes, I need to. If there is some other way it's undocumented and not
> used by Broadcom. As a quick check I tried not writing to FLASHADDR
> every time, but it didn't work.

Oh well, that's a bit sad. Thanks for checking.

>> Are you sure this has no side-effects when you mix this with reading
>> from the flash window ?
> 
> No side effects. I also tried reading whole flash content using this
> indirect access and it worked as well.

Well, if you read the whole flash, you will trigger the first windowed
read and then the new code, in sequence. Try doing some read pattern
where you read from the beginning and mix it with reads from the end.
That will alternate between these two bits of code.

>> Also, the parenthesis around buf++ are not needed and the & 0xff should
>> not be needed either.
> 
> You're right about buf. I'm not sure about & 0xff though. In theory
> you're correct. This is 32b register and we use 32b read function to
> access it but I never saw anything set in 0xffffff00. On the other
> hand Broadcom /advises/ (does) this & 0xff so I'm wondering if there
> can be some hardware with unexpected behavior in the world? What would
> happen if we got 32b value with some bits in ~0xff?

Well you're storing unsigned 32bit to unsigned 8bit, right ?

>>> +             }
>>> +     }
>>>       *retlen = len;
>>>
>>>       return len;
>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
>>> index 1564b62..d8d1093 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_SIZE            SZ_16M
>>> +
>>>  /* Used for ST flashes only. */
>>>  #define OPCODE_ST_WREN               0x0006          /* Write Enable */
>>>  #define OPCODE_ST_WRDIS              0x0004          /* Write Disable */
>>
>> These look like standard opcodes, why don't you use standard opcodes in
>> this driver and instead redefine them here ?
> 
> I guess it's because these are the only 2 opcodes we could share.
> Other than them Broadcom uses their magic 16b opcodes so I guess it
> doesn't change that much since we need most of them defined locally
> anyway.

I see.

>>> @@ -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
>>
>> Needs a comment to keep this consistent I think.
> 
> OK
> 


-- 
Best regards,
Marek Vasut

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

[Rafał Miłecki]

On 17 January 2017 at 10:49, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/17/2017 07:58 AM, Rafał Miłecki wrote:
>> On 17 January 2017 at 04:18, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/16/2017 11:09 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>
>>>> ---
>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>> index 4decd8c..5d57119 100644
>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>       if ((from + len) > mtd->size)
>>>>               return -EINVAL;
>>>>
>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>> +     } else {
>>>> +             size_t i;
>>>> +
>>>> +             for (i = 0; i < len; i++) {
>>>> +                     b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
>>>> +                     bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
>>>> +                     *(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
>>>
>>> Do you really need to send command on every write into the FLASHADDR
>>> register ? Doesn't this hardware support some sort of seqeuntial reading?
>>
>> Yes, I need to. If there is some other way it's undocumented and not
>> used by Broadcom. As a quick check I tried not writing to FLASHADDR
>> every time, but it didn't work.
>
> Oh well, that's a bit sad. Thanks for checking.
>
>>> Are you sure this has no side-effects when you mix this with reading
>>> from the flash window ?
>>
>> No side effects. I also tried reading whole flash content using this
>> indirect access and it worked as well.
>
> Well, if you read the whole flash, you will trigger the first windowed
> read and then the new code, in sequence. Try doing some read pattern
> where you read from the beginning and mix it with reads from the end.
> That will alternate between these two bits of code.

I really can't follow your possible-fail scenario. With my patch first
16 MiB are read using windowed access, another 16 MiB using indirect
access. I scanned whole flash this way, it worked. I got a working
system. It just works...

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

[Marek Vasut]

On 01/17/2017 11:39 AM, Rafał Miłecki wrote:
> On 17 January 2017 at 10:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/17/2017 07:58 AM, Rafał Miłecki wrote:
>>> On 17 January 2017 at 04:18, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 01/16/2017 11:09 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>
>>>>> ---
>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>> index 4decd8c..5d57119 100644
>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>       if ((from + len) > mtd->size)
>>>>>               return -EINVAL;
>>>>>
>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>> +     } else {
>>>>> +             size_t i;
>>>>> +
>>>>> +             for (i = 0; i < len; i++) {
>>>>> +                     b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
>>>>> +                     bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
>>>>> +                     *(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
>>>>
>>>> Do you really need to send command on every write into the FLASHADDR
>>>> register ? Doesn't this hardware support some sort of seqeuntial reading?
>>>
>>> Yes, I need to. If there is some other way it's undocumented and not
>>> used by Broadcom. As a quick check I tried not writing to FLASHADDR
>>> every time, but it didn't work.
>>
>> Oh well, that's a bit sad. Thanks for checking.
>>
>>>> Are you sure this has no side-effects when you mix this with reading
>>>> from the flash window ?
>>>
>>> No side effects. I also tried reading whole flash content using this
>>> indirect access and it worked as well.
>>
>> Well, if you read the whole flash, you will trigger the first windowed
>> read and then the new code, in sequence. Try doing some read pattern
>> where you read from the beginning and mix it with reads from the end.
>> That will alternate between these two bits of code.
> 
> I really can't follow your possible-fail scenario. With my patch first
> 16 MiB are read using windowed access, another 16 MiB using indirect
> access. I scanned whole flash this way, it worked. I got a working
> system. It just works...
> 
Well if you scanned the whole flash, you triggered the memcpy_fromio()
first and then the b47s->cc_write(..) stuff , in that order. So does it
work if you read the first 16 MiB after you read the region past 16 MiB?

-- 
Best regards,
Marek Vasut

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

[Rafał Miłecki]

On 17 January 2017 at 11:49, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/17/2017 11:39 AM, Rafał Miłecki wrote:
>> On 17 January 2017 at 10:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/17/2017 07:58 AM, Rafał Miłecki wrote:
>>>> On 17 January 2017 at 04:18, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On 01/16/2017 11:09 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>
>>>>>> ---
>>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> index 4decd8c..5d57119 100644
>>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>>       if ((from + len) > mtd->size)
>>>>>>               return -EINVAL;
>>>>>>
>>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>>> +     } else {
>>>>>> +             size_t i;
>>>>>> +
>>>>>> +             for (i = 0; i < len; i++) {
>>>>>> +                     b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
>>>>>> +                     bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
>>>>>> +                     *(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
>>>>>
>>>>> Do you really need to send command on every write into the FLASHADDR
>>>>> register ? Doesn't this hardware support some sort of seqeuntial reading?
>>>>
>>>> Yes, I need to. If there is some other way it's undocumented and not
>>>> used by Broadcom. As a quick check I tried not writing to FLASHADDR
>>>> every time, but it didn't work.
>>>
>>> Oh well, that's a bit sad. Thanks for checking.
>>>
>>>>> Are you sure this has no side-effects when you mix this with reading
>>>>> from the flash window ?
>>>>
>>>> No side effects. I also tried reading whole flash content using this
>>>> indirect access and it worked as well.
>>>
>>> Well, if you read the whole flash, you will trigger the first windowed
>>> read and then the new code, in sequence. Try doing some read pattern
>>> where you read from the beginning and mix it with reads from the end.
>>> That will alternate between these two bits of code.
>>
>> I really can't follow your possible-fail scenario. With my patch first
>> 16 MiB are read using windowed access, another 16 MiB using indirect
>> access. I scanned whole flash this way, it worked. I got a working
>> system. It just works...
>>
> Well if you scanned the whole flash, you triggered the memcpy_fromio()
> first and then the b47s->cc_write(..) stuff , in that order. So does it
> work if you read the first 16 MiB after you read the region past 16 MiB?

Yes, of course it does.

-- 
Rafał

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

[Marek Vasut]

On 01/17/2017 11:50 AM, Rafał Miłecki wrote:
> On 17 January 2017 at 11:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/17/2017 11:39 AM, Rafał Miłecki wrote:
>>> On 17 January 2017 at 10:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 01/17/2017 07:58 AM, Rafał Miłecki wrote:
>>>>> On 17 January 2017 at 04:18, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 01/16/2017 11:09 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>
>>>>>>> ---
>>>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>>> index 4decd8c..5d57119 100644
>>>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>>>       if ((from + len) > mtd->size)
>>>>>>>               return -EINVAL;
>>>>>>>
>>>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>>>> +     } else {
>>>>>>> +             size_t i;
>>>>>>> +
>>>>>>> +             for (i = 0; i < len; i++) {
>>>>>>> +                     b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
>>>>>>> +                     bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
>>>>>>> +                     *(buf++) = b47s->cc_read(b47s, BCMA_CC_FLASHDATA) & 0xff;
>>>>>>
>>>>>> Do you really need to send command on every write into the FLASHADDR
>>>>>> register ? Doesn't this hardware support some sort of seqeuntial reading?
>>>>>
>>>>> Yes, I need to. If there is some other way it's undocumented and not
>>>>> used by Broadcom. As a quick check I tried not writing to FLASHADDR
>>>>> every time, but it didn't work.
>>>>
>>>> Oh well, that's a bit sad. Thanks for checking.
>>>>
>>>>>> Are you sure this has no side-effects when you mix this with reading
>>>>>> from the flash window ?
>>>>>
>>>>> No side effects. I also tried reading whole flash content using this
>>>>> indirect access and it worked as well.
>>>>
>>>> Well, if you read the whole flash, you will trigger the first windowed
>>>> read and then the new code, in sequence. Try doing some read pattern
>>>> where you read from the beginning and mix it with reads from the end.
>>>> That will alternate between these two bits of code.
>>>
>>> I really can't follow your possible-fail scenario. With my patch first
>>> 16 MiB are read using windowed access, another 16 MiB using indirect
>>> access. I scanned whole flash this way, it worked. I got a working
>>> system. It just works...
>>>
>> Well if you scanned the whole flash, you triggered the memcpy_fromio()
>> first and then the b47s->cc_write(..) stuff , in that order. So does it
>> work if you read the first 16 MiB after you read the region past 16 MiB?
> 
> Yes, of course it does.

Ah cool, thanks for checking.

-- 
Best regards,
Marek Vasut

[PATCH V2] 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
---
 drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
 drivers/mtd/devices/bcm47xxsflash.h |  3 +++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
index 4decd8c..8d4c1db 100644
--- a/drivers/mtd/devices/bcm47xxsflash.c
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if ((from + len) > mtd->size)
 		return -EINVAL;
 
-	memcpy_fromio(buf, b47s->window + from, len);
+	if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
+		memcpy_fromio(buf, b47s->window + from, len);
+	} else {
+		size_t i;
+
+		for (i = 0; i < len; i++) {
+			b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
+			bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
+			*buf++ = b47s->cc_read(b47s, BCMA_CC_FLASHDATA);
+		}
+	}
 	*retlen = len;
 
 	return len;
diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
index 1564b62..6b478af 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_SIZE		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 address */
 
 /* Used for Atmel flashes only. */
 #define OPCODE_AT_READ				0x07e8
-- 
2.10.1

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

[Marek Vasut]

On 01/17/2017 11:51 AM, 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
> ---
>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> index 4decd8c..8d4c1db 100644
> --- a/drivers/mtd/devices/bcm47xxsflash.c
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	if ((from + len) > mtd->size)
>  		return -EINVAL;
>  
> -	memcpy_fromio(buf, b47s->window + from, len);
> +	if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
> +		memcpy_fromio(buf, b47s->window + from, len);

One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
Shouldn't that use partly the windowed mode and partly the other mode?

> +	} else {
> +		size_t i;
> +
> +		for (i = 0; i < len; i++) {
> +			b47s->cc_write(b47s, BCMA_CC_FLASHADDR, from++);
> +			bcm47xxsflash_cmd(b47s, OPCODE_ST_READ4B);
> +			*buf++ = b47s->cc_read(b47s, BCMA_CC_FLASHDATA);
> +		}
> +	}
>  	*retlen = len;
>  
>  	return len;
> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
> index 1564b62..6b478af 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_SIZE		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 address */
>  
>  /* Used for Atmel flashes only. */
>  #define OPCODE_AT_READ				0x07e8
> 


-- 
Best regards,
Marek Vasut

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

[Rafał Miłecki]

On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/17/2017 11:51 AM, 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
>> ---
>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>> index 4decd8c..8d4c1db 100644
>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>       if ((from + len) > mtd->size)
>>               return -EINVAL;
>>
>> -     memcpy_fromio(buf, b47s->window + from, len);
>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>> +             memcpy_fromio(buf, b47s->window + from, len);
>
> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
> Shouldn't that use partly the windowed mode and partly the other mode?

You'll lost 10ns*... or not as splitting it into 2 code paths could
take longer, who knows. Most access are block aligned anyway. I don't
think such corner case with doubtful gain is much worth considering &
optimizing.

* Statistic from my mind

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

[Marek Vasut]

On 01/17/2017 12:08 PM, Rafał Miłecki wrote:
> On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/17/2017 11:51 AM, 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
>>> ---
>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>> index 4decd8c..8d4c1db 100644
>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>       if ((from + len) > mtd->size)
>>>               return -EINVAL;
>>>
>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>
>> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
>> Shouldn't that use partly the windowed mode and partly the other mode?
> 
> You'll lost 10ns*... or not as splitting it into 2 code paths could
> take longer, who knows. Most access are block aligned anyway. I don't
> think such corner case with doubtful gain is much worth considering &
> optimizing.

So you can also read the first 16 MiB using the new method , right ?

> * Statistic from my mind
> 


-- 
Best regards,
Marek Vasut

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

[Rafał Miłecki]

On 17 January 2017 at 12:49, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/17/2017 12:08 PM, Rafał Miłecki wrote:
>> On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/17/2017 11:51 AM, 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
>>>> ---
>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>> index 4decd8c..8d4c1db 100644
>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>       if ((from + len) > mtd->size)
>>>>               return -EINVAL;
>>>>
>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>
>>> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
>>> Shouldn't that use partly the windowed mode and partly the other mode?
>>
>> You'll lost 10ns*... or not as splitting it into 2 code paths could
>> take longer, who knows. Most access are block aligned anyway. I don't
>> think such corner case with doubtful gain is much worth considering &
>> optimizing.
>
> So you can also read the first 16 MiB using the new method , right ?

I could, but this could be noticeable in performance. Reading 16 MiB
using slower method is different from reading what... a few KiB? Are
you actually sure mtd does unaligned reads at all?

Please let's argue about real problems and not few % performance in
some corner case on some obsolete hardware with poor design.

-- 
Rafał

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

[Rafał Miłecki]

On 17 January 2017 at 13:04, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 17 January 2017 at 12:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/17/2017 12:08 PM, Rafał Miłecki wrote:
>>> On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 01/17/2017 11:51 AM, 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
>>>>> ---
>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>> index 4decd8c..8d4c1db 100644
>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>       if ((from + len) > mtd->size)
>>>>>               return -EINVAL;
>>>>>
>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>
>>>> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
>>>> Shouldn't that use partly the windowed mode and partly the other mode?
>>>
>>> You'll lost 10ns*... or not as splitting it into 2 code paths could
>>> take longer, who knows. Most access are block aligned anyway. I don't
>>> think such corner case with doubtful gain is much worth considering &
>>> optimizing.
>>
>> So you can also read the first 16 MiB using the new method , right ?
>
> I could, but this could be noticeable in performance. Reading 16 MiB
> using slower method is different from reading what... a few KiB? Are
> you actually sure mtd does unaligned reads at all?

I did some quick test:
if ((from & (b47s->blocksize - 1)) + len > b47s->blocksize)
        pr_warn("[%s] Block unaligned read from 0x%llx len:0x%zx\n",
                __func__, from, len);

And it seems unaligned reads can happen:
[  147.338850] [bcm47xxsflash_read] Block unaligned read from 0x4fe1c len:0x200
[  147.663053] [bcm47xxsflash_read] Block unaligned read from 0x5fe1c len:0x200
[  147.983868] [bcm47xxsflash_read] Block unaligned read from 0x6fe1c len:0x200
[  148.304766] [bcm47xxsflash_read] Block unaligned read from 0x7fe1c len:0x200
[  148.625637] [bcm47xxsflash_read] Block unaligned read from 0x8fe1c len:0x200
[  148.955133] [bcm47xxsflash_read] Block unaligned read from 0x9fe1c len:0x200
[  149.275948] [bcm47xxsflash_read] Block unaligned read from 0xafe1c len:0x200
[  149.596790] [bcm47xxsflash_read] Block unaligned read from 0xbfe1c len:0x200
[  149.917604] [bcm47xxsflash_read] Block unaligned read from 0xcfe1c len:0x200
[  150.248641] [bcm47xxsflash_read] Block unaligned read from 0xdfe1c len:0x200
[  150.569484] [bcm47xxsflash_read] Block unaligned read from 0xefe1c len:0x200
[  150.890298] [bcm47xxsflash_read] Block unaligned read from 0xffe1c len:0x200
[  151.211140] [bcm47xxsflash_read] Block unaligned read from 0x10fe1c len:0x200
[  151.541393] [bcm47xxsflash_read] Block unaligned read from 0x11fe1c len:0x200
[  151.862292] [bcm47xxsflash_read] Block unaligned read from 0x12fe1c len:0x200
[  152.183246] [bcm47xxsflash_read] Block unaligned read from 0x13fe1c len:0x200
[  152.504200] [bcm47xxsflash_read] Block unaligned read from 0x14fe1c len:0x200
[  152.834957] [bcm47xxsflash_read] Block unaligned read from 0x15fe1c len:0x200
[  153.155856] [bcm47xxsflash_read] Block unaligned read from 0x16fe1c len:0x200
[  153.476782] [bcm47xxsflash_read] Block unaligned read from 0x17fe1c len:0x200
[  153.797681] [bcm47xxsflash_read] Block unaligned read from 0x18fe1c len:0x200
[  154.126925] [bcm47xxsflash_read] Block unaligned read from 0x19fe1c len:0x200
[  154.447823] [bcm47xxsflash_read] Block unaligned read from 0x1afe1c len:0x200

I got these reads of only 0x200 so that doesn't worry me much. Should
I ever expect bigger reads in my driver callback?

-- 
Rafał

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

[Marek Vasut]

On 01/17/2017 01:37 PM, Rafał Miłecki wrote:
> On 17 January 2017 at 13:04, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On 17 January 2017 at 12:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/17/2017 12:08 PM, Rafał Miłecki wrote:
>>>> On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On 01/17/2017 11:51 AM, 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
>>>>>> ---
>>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> index 4decd8c..8d4c1db 100644
>>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>>       if ((from + len) > mtd->size)
>>>>>>               return -EINVAL;
>>>>>>
>>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>>
>>>>> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
>>>>> Shouldn't that use partly the windowed mode and partly the other mode?
>>>>
>>>> You'll lost 10ns*... or not as splitting it into 2 code paths could
>>>> take longer, who knows. Most access are block aligned anyway. I don't
>>>> think such corner case with doubtful gain is much worth considering &
>>>> optimizing.
>>>
>>> So you can also read the first 16 MiB using the new method , right ?
>>
>> I could, but this could be noticeable in performance. Reading 16 MiB
>> using slower method is different from reading what... a few KiB? Are
>> you actually sure mtd does unaligned reads at all?
> 
> I did some quick test:
> if ((from & (b47s->blocksize - 1)) + len > b47s->blocksize)
>         pr_warn("[%s] Block unaligned read from 0x%llx len:0x%zx\n",
>                 __func__, from, len);
> 
> And it seems unaligned reads can happen:
> [  147.338850] [bcm47xxsflash_read] Block unaligned read from 0x4fe1c len:0x200
> [  147.663053] [bcm47xxsflash_read] Block unaligned read from 0x5fe1c len:0x200
> [  147.983868] [bcm47xxsflash_read] Block unaligned read from 0x6fe1c len:0x200
> [  148.304766] [bcm47xxsflash_read] Block unaligned read from 0x7fe1c len:0x200
> [  148.625637] [bcm47xxsflash_read] Block unaligned read from 0x8fe1c len:0x200
> [  148.955133] [bcm47xxsflash_read] Block unaligned read from 0x9fe1c len:0x200
> [  149.275948] [bcm47xxsflash_read] Block unaligned read from 0xafe1c len:0x200
> [  149.596790] [bcm47xxsflash_read] Block unaligned read from 0xbfe1c len:0x200
> [  149.917604] [bcm47xxsflash_read] Block unaligned read from 0xcfe1c len:0x200
> [  150.248641] [bcm47xxsflash_read] Block unaligned read from 0xdfe1c len:0x200
> [  150.569484] [bcm47xxsflash_read] Block unaligned read from 0xefe1c len:0x200
> [  150.890298] [bcm47xxsflash_read] Block unaligned read from 0xffe1c len:0x200
> [  151.211140] [bcm47xxsflash_read] Block unaligned read from 0x10fe1c len:0x200
> [  151.541393] [bcm47xxsflash_read] Block unaligned read from 0x11fe1c len:0x200
> [  151.862292] [bcm47xxsflash_read] Block unaligned read from 0x12fe1c len:0x200
> [  152.183246] [bcm47xxsflash_read] Block unaligned read from 0x13fe1c len:0x200
> [  152.504200] [bcm47xxsflash_read] Block unaligned read from 0x14fe1c len:0x200
> [  152.834957] [bcm47xxsflash_read] Block unaligned read from 0x15fe1c len:0x200
> [  153.155856] [bcm47xxsflash_read] Block unaligned read from 0x16fe1c len:0x200
> [  153.476782] [bcm47xxsflash_read] Block unaligned read from 0x17fe1c len:0x200
> [  153.797681] [bcm47xxsflash_read] Block unaligned read from 0x18fe1c len:0x200
> [  154.126925] [bcm47xxsflash_read] Block unaligned read from 0x19fe1c len:0x200
> [  154.447823] [bcm47xxsflash_read] Block unaligned read from 0x1afe1c len:0x200
> 
> I got these reads of only 0x200 so that doesn't worry me much. Should
> I ever expect bigger reads in my driver callback?

Try using /dev/mtdX for read directly (do not use for write), see my
previous email.

-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

On 01/17/2017 01:04 PM, Rafał Miłecki wrote:
> On 17 January 2017 at 12:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 01/17/2017 12:08 PM, Rafał Miłecki wrote:
>>> On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 01/17/2017 11:51 AM, 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
>>>>> ---
>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>> index 4decd8c..8d4c1db 100644
>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>       if ((from + len) > mtd->size)
>>>>>               return -EINVAL;
>>>>>
>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>
>>>> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
>>>> Shouldn't that use partly the windowed mode and partly the other mode?
>>>
>>> You'll lost 10ns*... or not as splitting it into 2 code paths could
>>> take longer, who knows. Most access are block aligned anyway. I don't
>>> think such corner case with doubtful gain is much worth considering &
>>> optimizing.
>>
>> So you can also read the first 16 MiB using the new method , right ?
> 
> I could, but this could be noticeable in performance. Reading 16 MiB
> using slower method is different from reading what... a few KiB? Are
> you actually sure mtd does unaligned reads at all?

No, but I'm quite sure the code above can be pushed to misbehave and I'm
trying to confirm/refute that.

> Please let's argue about real problems and not few % performance in
> some corner case on some obsolete hardware with poor design.

OK, real issue:
$ dd if=/dev/mtdX of=/dev/null bs=32M

How will this driver handle it ?

-- 
Best regards,
Marek Vasut

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

[Rafał Miłecki]

On 19 January 2017 at 22:30, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 01/17/2017 01:04 PM, Rafał Miłecki wrote:
>> On 17 January 2017 at 12:49, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 01/17/2017 12:08 PM, Rafał Miłecki wrote:
>>>> On 17 January 2017 at 12:00, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On 01/17/2017 11:51 AM, 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
>>>>>> ---
>>>>>>  drivers/mtd/devices/bcm47xxsflash.c | 12 +++++++++++-
>>>>>>  drivers/mtd/devices/bcm47xxsflash.h |  3 +++
>>>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> index 4decd8c..8d4c1db 100644
>>>>>> --- a/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>>>>>> @@ -110,7 +110,17 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>>>       if ((from + len) > mtd->size)
>>>>>>               return -EINVAL;
>>>>>>
>>>>>> -     memcpy_fromio(buf, b47s->window + from, len);
>>>>>> +     if (from + len <= BCM47XXSFLASH_WINDOW_SIZE) {
>>>>>> +             memcpy_fromio(buf, b47s->window + from, len);
>>>>>
>>>>> One last nit, what if the read starts < 16 MiB and ends > 16 MiB ?
>>>>> Shouldn't that use partly the windowed mode and partly the other mode?
>>>>
>>>> You'll lost 10ns*... or not as splitting it into 2 code paths could
>>>> take longer, who knows. Most access are block aligned anyway. I don't
>>>> think such corner case with doubtful gain is much worth considering &
>>>> optimizing.
>>>
>>> So you can also read the first 16 MiB using the new method , right ?
>>
>> I could, but this could be noticeable in performance. Reading 16 MiB
>> using slower method is different from reading what... a few KiB? Are
>> you actually sure mtd does unaligned reads at all?
>
> No, but I'm quite sure the code above can be pushed to misbehave and I'm
> trying to confirm/refute that.
>
>> Please let's argue about real problems and not few % performance in
>> some corner case on some obsolete hardware with poor design.
>
> OK, real issue:
> $ dd if=/dev/mtdX of=/dev/null bs=32M
>
> How will this driver handle it ?

It results in reading using 0x400000 chunks. Now this is a good
argument to optimize bcm47xxsflash_read for reads crossing flash
window boundary.
[  401.927715] [bcm47xxsflash_read] len:0x400000
[  403.476166] [bcm47xxsflash_read] len:0x400000
(...)

-- 
Rafał