| Message ID | 1363018093-28979-9-git-send-email-sjg@chromium.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | Tom Rini |
| Headers | show |
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? Thanks, Jagan.
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? Regards, Simon > > Thanks, > Jagan.
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? Thanks, Jagan. > > Regards, > Simon > >> >> Thanks, >> Jagan.
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. Regards, Simon > > Thanks, > Jagan. > >> >> Regards, >> Simon >> >>> >>> Thanks, >>> Jagan.
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? Thanks, Jagan. > > Regards, > Simon > >> >> Thanks, >> Jagan. >> >>> >>> Regards, >>> Simon >>> >>>> >>>> Thanks, >>>> Jagan.
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. Regards, Simon > > Thanks, > Jagan. > >> >> Regards, >> Simon >> >>> >>> Thanks, >>> Jagan. >>> >>>> >>>> Regards, >>>> Simon >>>> >>>>> >>>>> Thanks, >>>>> Jagan.
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
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; + } } debug("SF: program %s %zu bytes @ %#x\n",
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(-)