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

[Rafał Miłecki]

On 17 January 2017 at 04:18, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 01/16/2017 11:09 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal at 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 at 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