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

[Marek Vasut <marek.vasut@gmail.com>]

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