Patchwork [U-Boot,v2,08/15] sf: Respect maximum SPI write size

login
register
mail settings
Submitter Simon Glass
Date March 11, 2013, 4:08 p.m.
Message ID <1363018093-28979-9-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/226585/
State Accepted, archived
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - March 11, 2013, 4:08 p.m.
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(-)
Jagannadha Sutradharudu Teki - April 23, 2013, 8:44 p.m.
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.
Simon Glass - April 23, 2013, 8:50 p.m.
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.
Jagannadha Sutradharudu Teki - April 23, 2013, 9:01 p.m.
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.
Simon Glass - April 23, 2013, 9:13 p.m.
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.
Jagannadha Sutradharudu Teki - April 23, 2013, 9:15 p.m.
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.
Simon Glass - April 23, 2013, 9:18 p.m.
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.
Simon Glass - April 24, 2013, 12:33 a.m.
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.?
Jagannadha Sutradharudu Teki - April 25, 2013, 1:55 p.m.
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.?
Simon Glass - April 25, 2013, 6:52 p.m.
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.?
Jagannadha Sutradharudu Teki - April 25, 2013, 7:06 p.m.
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.?
Simon Glass - April 26, 2013, 1:43 a.m.
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
Jagannadha Sutradharudu Teki - April 26, 2013, 6:16 a.m.
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
Simon Glass - April 26, 2013, 12:21 p.m.
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
Jagannadha Sutradharudu Teki - April 26, 2013, 6:34 p.m.
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
Simon Glass - April 26, 2013, 6:45 p.m.
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

Patch

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",