Re: [U-Boot] [PATCH v2 08/15] sf: Respect maximum SPI write size
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: > Hi Jagan, > > On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >> Hi Simon, >> >> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>> Hi Jagan, >>> >>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>> Hi Simon, >>>> >>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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.?
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: > Hi Simon, > > On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >> Hi Jagan, >> >> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>> Hi Simon, >>> >>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>> Hi Jagan, >>>> >>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>> Hi Simon, >>>>> >>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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.?
Thanks, Jagan.
Regards, Simon
Thanks, Jagan.
May be the new code handle this situation as earlier may not have.?
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: > Hi Jagan, > > On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >> Hi Simon, >> >> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>> Hi Jagan, >>> >>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>> Hi Simon, >>>> >>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>> Hi Jagan, >>>>> >>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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.
Regards, Simon
Thanks, Jagan.
Regards, Simon
Thanks, Jagan.
May be the new code handle this situation as earlier may not have.?
Hi Simon,
On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote: > Hi Simon, > > On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: >> Hi Jagan, >> >> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>> Hi Simon, >>> >>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>>> Hi Jagan, >>>> >>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>> Hi Simon, >>>>> >>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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.?
Hi,
On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote: > Hi Jagan, > > On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >> Hi Simon, >> >> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: >>> Hi Jagan, >>> >>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>> Hi Simon, >>>> >>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>>>> Hi Jagan, >>>>> >>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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()?
Regards, Simon
Hi Simon,
On Fri, Apr 26, 2013 at 7:13 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote: > Hi Simon, > > On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote: >> Hi Jagan, >> >> On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>> Hi Simon, >>> >>> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: >>>> Hi Jagan, >>>> >>>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>> Hi Simon, >>>>> >>>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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
Hi Jagan,
On Thu, Apr 25, 2013 at 11:16 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 7:13 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote: > Hi Simon, > > On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote: >> Hi Simon, >> >> On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote: >>> Hi Jagan, >>> >>> On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>> Hi Simon, >>>> >>>> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: >>>>> Hi Jagan, >>>>> >>>>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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.
Regards, Simon
Thanks, Jagan.
Regards, Simon
Hi Simon,
On Fri, Apr 26, 2013 at 5:51 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 11:16 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 7:13 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote: > Hi, > > On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >> Hi Simon, >> >> On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote: >>> Hi Simon, >>> >>> On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote: >>>> Hi Jagan, >>>> >>>> On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>> Hi Simon, >>>>> >>>>> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>>>>>>> Hi Jagan, >>>>>>>>>> >>>>>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>>>>> Hi Simon, >>>>>>>>>>> >>>>>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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.
Thanks, Jagan.
Regards, Simon
Thanks, Jagan.
Regards, Simon
Hi,
On Fri, Apr 26, 2013 at 11:34 AM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 5:51 PM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 11:16 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 7:13 AM, Simon Glass sjg@chromium.org wrote:
Hi,
On Thu, Apr 25, 2013 at 12:06 PM, Jagan Teki jagannadh.teki@gmail.com wrote:
Hi Simon,
On Fri, Apr 26, 2013 at 12:22 AM, Simon Glass sjg@chromium.org wrote:
Hi Jagan,
On Thu, Apr 25, 2013 at 6:55 AM, Jagan Teki jagannadh.teki@gmail.com wrote: > Hi Simon, > > On Wed, Apr 24, 2013 at 6:03 AM, Simon Glass sjg@chromium.org wrote: >> Hi, >> >> On Tue, Apr 23, 2013 at 3:11 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>> Hi Simon, >>> >>> On Wed, Apr 24, 2013 at 3:25 AM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>> Hi Simon, >>>> >>>> On Wed, Apr 24, 2013 at 3:10 AM, Simon Glass sjg@chromium.org wrote: >>>>> Hi Jagan, >>>>> >>>>> On Tue, Apr 23, 2013 at 2:31 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Wed, Apr 24, 2013 at 2:48 AM, Simon Glass sjg@chromium.org wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Tue, Apr 23, 2013 at 2:15 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Wed, Apr 24, 2013 at 2:43 AM, Simon Glass sjg@chromium.org wrote: >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On Tue, Apr 23, 2013 at 2:01 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On Wed, Apr 24, 2013 at 2:20 AM, Simon Glass sjg@chromium.org wrote: >>>>>>>>>>> Hi Jagan, >>>>>>>>>>> >>>>>>>>>>> On Tue, Apr 23, 2013 at 1:44 PM, Jagan Teki jagannadh.teki@gmail.com wrote: >>>>>>>>>>>> Hi Simon, >>>>>>>>>>>> >>>>>>>>>>>> On Mon, Mar 11, 2013 at 9:38 PM, Simon Glass sjg@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@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
participants (2)
-
Jagan Teki -
Simon Glass