[U-Boot] [PATCH v2 08/15] sf: Respect maximum SPI write size
Simon Glass
sjg at chromium.org
Fri Apr 26 20:45:50 CEST 2013
Hi,
On Fri, Apr 26, 2013 at 11:34 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Fri, Apr 26, 2013 at 5:51 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagan,
>>
>> On Thu, Apr 25, 2013 at 11:16 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Fri, Apr 26, 2013 at 7:13 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi,
>>>>
>>>> On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>> 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.
>>>>
>>>> Which SPI driver please? Does your SPI driver call spi_alloc_slave()?
>>>
>>> I am using xilinx zynq qspi controller driver, not mainline yet, planning to do.
>>>
>>> Yes we have a spi_alloc_slave func where I am initializing slave and
>>> spi_dev members.
>>> in that I have initialized slave.max_write_size = 0.
>>
>> OK, but this should be done by spi_do_alloc_slave() for you, if you
>> are calling the mainline spi_alloc_slave() macro. Please see how other
>> SPI drivers set themselves up.
>
> Thanks for your help, got the point.
> Could you please any reference driver, I am planning to make this qspi
> driver to push on mainline.
drivers/spi/tegra_spi.c is a reasonable one to copy I think.
Regards,
Simon
>
> Thanks,
> Jagan.
>
>>
>> Regards,
>> Simon
>>
>>>
>>> Thanks,
>>> Jagan.
>>>
>>>>
>>>> Regards,
>>>> Simon
More information about the U-Boot
mailing list