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

Thu Apr 25 21:06:35 CEST 2013

Hi Simon,

On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Jagan,
>
> On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass <sjg at chromium.org> wrote:
>>> 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.
>>
>> So I need to initialize slave.max_write_size to 0 on my controller driver?
>> is that the good idea to make change in all drivers with this issue,
>> may be i am wrong.?
>
> If your driver is using spi_flash_alloc(), as it now should be, then
> it should work OK.

Sorry I couldn't understand.
As per as i know spi_flash_alloc is a generic call used for spi_flash
read/write/erase calls.

When i use the code as it is i got the garbage value for
slave.max_write_size and chunk_len on write call has 1
due to this flash write failed. I fixed when I explicitly assign 0 to
slave.max_write_size on my controller driver.

Request for your comments.

Thanks,
Jagan.

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