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