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

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