[U-Boot] [PATCH v2 08/15] sf: Respect maximum SPI write size

[Simon Glass]

Hi,

On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>> Hi Jagan,
>>>>>
>>>>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>>> Hi Jagan,
>>>>>>>
>>>>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>>>> Hi Simon,
>>>>>>>>
>>>>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>>>>> Hi Jagan,
>>>>>>>>>
>>>>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>>>>>>> Hi Simon,
>>>>>>>>>>
>>>>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass <sjg at chromium.org> wrote:
>>>>>>>>>>> Some SPI flash controllers (e.g. Intel ICH) have a limit on the number of
>>>>>>>>>>> bytes that can be in a write transaction. Support this by breaking the
>>>>>>>>>>> writes into multiple transactions.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>>>>>>>> ---
>>>>>>>>>>> Changes in v2: None
>>>>>>>>>>>
>>>>>>>>>>>  drivers/mtd/spi/spi_flash.c | 10 ++++++++--
>>>>>>>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>>>>>>>>> index 17f3d3c..b82011d 100644
>>>>>>>>>>> --- a/drivers/mtd/spi/spi_flash.c
>>>>>>>>>>> +++ b/drivers/mtd/spi/spi_flash.c
>>>>>>>>>>> @@ -87,6 +87,9 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
>>>>>>>>>>>         for (actual = 0; actual < len; actual += chunk_len) {
>>>>>>>>>>>                 chunk_len = min(len - actual, page_size - byte_addr);
>>>>>>>>>>>
>>>>>>>>>>> +               if (flash->spi->max_write_size)
>>>>>>>>>>> +                       chunk_len = min(chunk_len, flash->spi->max_write_size);
>>>>>>>>>>> +
>>>>>>>>>>>                 cmd[1] = page_addr >> 8;
>>>>>>>>>>>                 cmd[2] = page_addr;
>>>>>>>>>>>                 cmd[3] = byte_addr;
>>>>>>>>>>> @@ -111,8 +114,11 @@ int spi_flash_cmd_write_multi(struct spi_flash *flash, u32 offset,
>>>>>>>>>>>                 if (ret)
>>>>>>>>>>>                         break;
>>>>>>>>>>>
>>>>>>>>>>> -               page_addr++;
>>>>>>>>>>> -               byte_addr = 0;
>>>>>>>>>>> +               byte_addr += chunk_len;
>>>>>>>>>>> +               if (byte_addr == page_size) {
>>>>>>>>>>> +                       page_addr++;
>>>>>>>>>>> +                       byte_addr = 0;
>>>>>>>>>>> +               }
>>>>>>>>>>
>>>>>>>>>> Does this change required to handle < page_size writes, means if the
>>>>>>>>>> user is giving an offset other than
>>>>>>>>>> multiples of page_sizes?
>>>>>>>>>
>>>>>>>>> I'm not quite sure what you are saying, but let me try to response.
>>>>>>>>>
>>>>>>>>> I believe what should happen is that byte_addr should become aligned
>>>>>>>>> to the page_size after the first transfer, and from then on it should
>>>>>>>>> start at 0 for each page.
>>>>>>>>>
>>>>>>>>> Are you seeing a problem?
>>>>>>>>
>>>>>>>> My question,what if this change doesn't have.?
>>>>>>>> Can't I able to write data starts from unaligned page_sizes?
>>>>>>>
>>>>>>> It should work fine - I did in fact find a problem in the driver in
>>>>>>> this case, which I fixed. Let me know if you see any problem.
>>>>>>
>>>>>> Means this change is for proper handling of write data starts from
>>>>>> unaligned page_sizes, is it?
>>>>>
>>>>> Not really - it was designed to handle the case where the driver
>>>>> cannot write a whole page at once. The Intel ICH peripheral has this
>>>>> problem.
>>>>>
>>>>> I believe that writing starting from unaligned page sizes worked OK
>>>>> before this change.
>>>>
>>>> Thanks.
>>>>
>>>> But I am some how confusing instead of this change may be you may
>>>> place the existing code as it is
>>>>  page_addr++;
>>>>  byte_addr = 0;
>>>> prior to above may be you can place intel ICH per hack as other will
>>>> do whole page at once, i may be wrong.
>>>
>>> The old code assumed that it could skip to the start of the next page
>>> after each write. The new code skips forward by chunk_len, which
>>> generally takes as to the start of the next page, but not always (e.g.
>>> with Intel ICH).
>>>
>>> Yes, AFAIK every other SPI driver can still do a whole page at a time,
>>> with or without this patch.
>>
>> Thats what if the user is giving an unaligned page size suppose 0x80
>> with 512 bytes (if the page_size=256)
>> sf write 0x100 0x80 0x200
>> the loop will goes 2 non page_sizes and 1 pages_size like this
>> iteration 1--- 128
>> iteration 2--  256
>> iteration 3--  128
>
> I was tested the old and new code w.r.t this unaligned page_size as a start
> the result is same
> uboot> sf write 0x100 0x80 0x200
> actual = 0.....chunk_len = 128
> actual = 128.....chunk_len = 256
> actual = 384.....chunk_len = 128
> SF: program success 512 bytes @ 0x80
>
> Means the old and new code does the same thing, but still i couldn't understand.
> What exactly this change is for, if it is specific to intel flash what
> is state in above condition.

Yes it is for the Intel SPI controller which has a strange limitation
that it can only write 64 bytes at a time.

Regards,
Simon

>
> Thanks,
> Jagan.
>
>>
>> May be the new code handle this situation as earlier may not have.?