Hi Simon, On Fri, Apr 26, 2013 at 7:13 AM, Simon Glass <s...@chromium.org> wrote: > Hi, > > On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki <jagannadh.t...@gmail.com> wrote: >> Hi Simon, >> >> On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass <s...@chromium.org> wrote: >>> Hi Jagan, >>> >>> On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki <jagannadh.t...@gmail.com> >>> wrote: >>>> Hi Simon, >>>> >>>> On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass <s...@chromium.org> wrote: >>>>> Hi, >>>>> >>>>> On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki <jagannadh.t...@gmail.com> >>>>> wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki <jagannadh.t...@gmail.com> >>>>>> wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass <s...@chromium.org> wrote: >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>> On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki <jagannadh.t...@gmail.com> >>>>>>>> wrote: >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass <s...@chromium.org> >>>>>>>>> wrote: >>>>>>>>>> Hi Jagan, >>>>>>>>>> >>>>>>>>>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki >>>>>>>>>> <jagannadh.t...@gmail.com> wrote: >>>>>>>>>>> Hi Simon, >>>>>>>>>>> >>>>>>>>>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass <s...@chromium.org> >>>>>>>>>>> wrote: >>>>>>>>>>>> Hi Jagan, >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki >>>>>>>>>>>> <jagannadh.t...@gmail.com> wrote: >>>>>>>>>>>>> Hi Simon, >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass <s...@chromium.org> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> Hi Jagan, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki >>>>>>>>>>>>>> <jagannadh.t...@gmail.com> wrote: >>>>>>>>>>>>>>> Hi Simon, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass >>>>>>>>>>>>>>> <s...@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 <s...@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. Thanks, Jagan. > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot