Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Stefan Roese]

On 03.05.2016 19:18, Marek Vasut wrote:

On 05/03/2016 07:00 PM, Stefan Roese wrote:

On 03.05.2016 18:53, Marek Vasut wrote:

On 05/02/2016 05:20 PM, Stefan Roese wrote:

On 29.04.2016 12:13, Marek Vasut wrote:

On 28.04.2016 00:36, Marek Vasut wrote:

The indirect write code is buggy pile of nastiness which fails
horribly
when the system runs fast enough to saturate the controller. The
failure
results in some pages (256B) not being written to the flash. This
can be
observed on systems which run with Dcache enabled and L2 cache
enabled,
like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write
implementation
with the one from upcoming Linux CQSPI driver, which went through
multiple
rounds of thorough review and testing. While this makes the patch
look
terrifying and violates all best-practices of software
development, all
the patch does is it plucks out duplicate ad-hoc code distributed
across
the driver and replaces it with more compact code doing exactly
the same
thing.

Signed-off-by: Marek Vasut 
Cc: Anatolij Gustschin 
Cc: Chin Liang See 
Cc: Dinh Nguyen 
Cc: Jagan Teki 
Cc: Pavel Machek 
Cc: Stefan Roese 
Cc: Vignesh R 


I've applied both patches and tested them on SR1500 (SPI-NOR used
for booting and redundant environment). This is what I get upon
"saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB,
total 16 MiB
Erasing SPI flash...Writing to SPI flash...data abort
pc : [<3ff8368a>]  lr : [<3ff8301b>]
reloc pc : [<010216ca>]lr : [<0102105b>]
sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
r10:   r9 : 3bf5dee8 r8 : 
r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)


Any idea, what might be going wrong here?


Does it work without the patch ?


Yes, of course. I wouldn't have posted as a reply to this patch
if this is not the root cause.


*grumble*


?


I just sensed slight USB subtext here ;-)


The board is using SPI NOR for env storage from the beginning.


It only happens if you use redundant env in SPI NOR.


   Where does your PC point to at the time

of the crash ,which function is it ?


Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem
in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte
aligned txbuf. This causes the data-abort here. Here my small
patch that fixes the problem:


Thanks, see below.


diff --git a/drivers/spi/cadence_qspi_apb.c
b/drivers/spi/cadence_qspi_apb.c
index ac47c6f..021a3e8 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -745,7 +745,15 @@ int
cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata
*plat,

  while (remaining > 0) {
  write_bytes = remaining > page_size ? page_size :
remaining;
-   writesl(plat->ahbbase, txbuf,
DIV_ROUND_UP(write_bytes, 4));
+
+   /*
+* Handle non 4-byte aligned access differently to avoid
+* data-aborts
+*/
+   if (((u32)txbuf % 4) || (write_bytes % 4))
+   writesb(plat->ahbbase, txbuf, write_bytes);
+   else
+   writesl(plat->ahbbase, txbuf, write_bytes >> 2);

  ret = wait_for_bit("QSPI", plat->regbase +
CQSPI_REG_SDRAMLEVEL,
 CQSPI_REG_SDRAMLEVEL_WR_MASK <<


Please fell free to use this patch as-is and squash it into
your patches or enhance it while doing this. The read function
is also missing this unaligned handling.


Im afraid of the performance hit that we can suffer if we use byte-level
access for every unaligned buffer.


This is why is wrote: "or enhance it while doing this". You might want
to change the code to first write the (optionally) unaligned bytes,
then the aligned bytes via writesl() and last the (optionally) unaligned
bytes.


What do you think
about using a bounce-buffer instead ?


I would prefer the simple solution I've drafted above.


OK, I checked how many aligned and unaligned transfers happens and I
guess it's really not worth going through the bounce buffer for 1 byte
which only happens once during saveenv.

I'll squash it and pick via socfpga tree, so we can get this in 2016.05
and have working QSPI NOR support there.


Good, thanks!


And of course the Linux driver version as well.


Does linux use unaligned buffers internally ?


Frankly, I don't know for sure. But I suspect, that you can also
see unaligned buffers (and sizes!!!) in Linux as 

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Marek Vasut]

On 05/03/2016 07:00 PM, Stefan Roese wrote:
> On 03.05.2016 18:53, Marek Vasut wrote:
>> On 05/02/2016 05:20 PM, Stefan Roese wrote:
>>> On 29.04.2016 12:13, Marek Vasut wrote:
> On 28.04.2016 00:36, Marek Vasut wrote:
>> The indirect write code is buggy pile of nastiness which fails
>> horribly
>> when the system runs fast enough to saturate the controller. The
>> failure
>> results in some pages (256B) not being written to the flash. This
>> can be
>> observed on systems which run with Dcache enabled and L2 cache
>> enabled,
>> like the Altera SoCFPGA.
>>
>> This patch replaces the whole unmaintainable indirect write
>> implementation
>> with the one from upcoming Linux CQSPI driver, which went through
>> multiple
>> rounds of thorough review and testing. While this makes the patch
>> look
>> terrifying and violates all best-practices of software
>> development, all
>> the patch does is it plucks out duplicate ad-hoc code distributed
>> across
>> the driver and replaces it with more compact code doing exactly
>> the same
>> thing.
>>
>> Signed-off-by: Marek Vasut 
>> Cc: Anatolij Gustschin 
>> Cc: Chin Liang See 
>> Cc: Dinh Nguyen 
>> Cc: Jagan Teki 
>> Cc: Pavel Machek 
>> Cc: Stefan Roese 
>> Cc: Vignesh R 
>
> I've applied both patches and tested them on SR1500 (SPI-NOR used
> for booting and redundant environment). This is what I get upon
> "saveeenv":
>
> => saveenv
> Saving Environment to SPI Flash...
> SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB,
> total 16 MiB
> Erasing SPI flash...Writing to SPI flash...data abort
> pc : [<3ff8368a>]  lr : [<3ff8301b>]
> reloc pc : [<010216ca>]lr : [<0102105b>]
> sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
> r10:   r9 : 3bf5dee8 r8 : 
> r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
> r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
> Resetting CPU ...
>
> resetting ...
>
> U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)
>
>
> Any idea, what might be going wrong here?

 Does it work without the patch ?
>>>
>>> Yes, of course. I wouldn't have posted as a reply to this patch
>>> if this is not the root cause.
>>
>> *grumble*
> 
> ?

I just sensed slight USB subtext here ;-)

>>> The board is using SPI NOR for env storage from the beginning.
>>
>> It only happens if you use redundant env in SPI NOR.
>>
>>>   Where does your PC point to at the time
 of the crash ,which function is it ?
>>>
>>> Its in cadence_qspi_apb_indirect_write_execute().
>>>
>>> I debugged this issue a bit and found the following problem
>>> in cadence_qspi_apb_indirect_write_execute():
>>>
>>> saveenv issues a 1-byte SPI write transfer with a non 4-byte
>>> aligned txbuf. This causes the data-abort here. Here my small
>>> patch that fixes the problem:
>>
>> Thanks, see below.
>>
>>> diff --git a/drivers/spi/cadence_qspi_apb.c
>>> b/drivers/spi/cadence_qspi_apb.c
>>> index ac47c6f..021a3e8 100644
>>> --- a/drivers/spi/cadence_qspi_apb.c
>>> +++ b/drivers/spi/cadence_qspi_apb.c
>>> @@ -745,7 +745,15 @@ int
>>> cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata
>>> *plat,
>>>
>>>  while (remaining > 0) {
>>>  write_bytes = remaining > page_size ? page_size :
>>> remaining;
>>> -   writesl(plat->ahbbase, txbuf,
>>> DIV_ROUND_UP(write_bytes, 4));
>>> +
>>> +   /*
>>> +* Handle non 4-byte aligned access differently to avoid
>>> +* data-aborts
>>> +*/
>>> +   if (((u32)txbuf % 4) || (write_bytes % 4))
>>> +   writesb(plat->ahbbase, txbuf, write_bytes);
>>> +   else
>>> +   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>>>
>>>  ret = wait_for_bit("QSPI", plat->regbase +
>>> CQSPI_REG_SDRAMLEVEL,
>>> CQSPI_REG_SDRAMLEVEL_WR_MASK <<
>>>
>>>
>>> Please fell free to use this patch as-is and squash it into
>>> your patches or enhance it while doing this. The read function
>>> is also missing this unaligned handling.
>>
>> Im afraid of the performance hit that we can suffer if we use byte-level
>> access for every unaligned buffer.
> 
> This is why is wrote: "or enhance it while doing this". You might want
> to change the code to first write the (optionally) unaligned bytes,
> then the aligned bytes via writesl() and last the (optionally) unaligned
> bytes.
> 
>> What do you think
>> about using a bounce-buffer instead ?
> 
> I would prefer the simple 

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Stefan Roese]

On 03.05.2016 18:53, Marek Vasut wrote:

On 05/02/2016 05:20 PM, Stefan Roese wrote:

On 29.04.2016 12:13, Marek Vasut wrote:

On 28.04.2016 00:36, Marek Vasut wrote:

The indirect write code is buggy pile of nastiness which fails horribly
when the system runs fast enough to saturate the controller. The failure
results in some pages (256B) not being written to the flash. This can be
observed on systems which run with Dcache enabled and L2 cache enabled,
like the Altera SoCFPGA.

This patch replaces the whole unmaintainable indirect write implementation
with the one from upcoming Linux CQSPI driver, which went through multiple
rounds of thorough review and testing. While this makes the patch look
terrifying and violates all best-practices of software development, all
the patch does is it plucks out duplicate ad-hoc code distributed across
the driver and replaces it with more compact code doing exactly the same
thing.

Signed-off-by: Marek Vasut 
Cc: Anatolij Gustschin 
Cc: Chin Liang See 
Cc: Dinh Nguyen 
Cc: Jagan Teki 
Cc: Pavel Machek 
Cc: Stefan Roese 
Cc: Vignesh R 


I've applied both patches and tested them on SR1500 (SPI-NOR used
for booting and redundant environment). This is what I get upon
"saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB
Erasing SPI flash...Writing to SPI flash...data abort
pc : [<3ff8368a>]  lr : [<3ff8301b>]
reloc pc : [<010216ca>]lr : [<0102105b>]
sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
r10:   r9 : 3bf5dee8 r8 : 
r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)


Any idea, what might be going wrong here?


Does it work without the patch ?


Yes, of course. I wouldn't have posted as a reply to this patch
if this is not the root cause.


*grumble*


?


The board is using SPI NOR for env storage from the beginning.


It only happens if you use redundant env in SPI NOR.


  Where does your PC point to at the time

of the crash ,which function is it ?


Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem
in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte
aligned txbuf. This causes the data-abort here. Here my small
patch that fixes the problem:


Thanks, see below.


diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index ac47c6f..021a3e8 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -745,7 +745,15 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,

 while (remaining > 0) {
 write_bytes = remaining > page_size ? page_size : remaining;
-   writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+   /*
+* Handle non 4-byte aligned access differently to avoid
+* data-aborts
+*/
+   if (((u32)txbuf % 4) || (write_bytes % 4))
+   writesb(plat->ahbbase, txbuf, write_bytes);
+   else
+   writesl(plat->ahbbase, txbuf, write_bytes >> 2);

 ret = wait_for_bit("QSPI", plat->regbase + 
CQSPI_REG_SDRAMLEVEL,
CQSPI_REG_SDRAMLEVEL_WR_MASK <<


Please fell free to use this patch as-is and squash it into
your patches or enhance it while doing this. The read function
is also missing this unaligned handling.


Im afraid of the performance hit that we can suffer if we use byte-level
access for every unaligned buffer.


This is why is wrote: "or enhance it while doing this". You might want
to change the code to first write the (optionally) unaligned bytes,
then the aligned bytes via writesl() and last the (optionally) unaligned
bytes.


What do you think
about using a bounce-buffer instead ?


I would prefer the simple solution I've drafted above.


And of course the Linux driver version as well.


Does linux use unaligned buffers internally ?


Frankly, I don't know for sure. But I suspect, that you can also
see unaligned buffers (and sizes!!!) in Linux as well. And you
can't just write a different amount of data to the SPI NOR, which
happens when you use DIV_ROUND_UP on an unaligned size.

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Marek Vasut]

On 05/02/2016 05:20 PM, Stefan Roese wrote:
> On 29.04.2016 12:13, Marek Vasut wrote:
>>> On 28.04.2016 00:36, Marek Vasut wrote:
 The indirect write code is buggy pile of nastiness which fails horribly
 when the system runs fast enough to saturate the controller. The failure
 results in some pages (256B) not being written to the flash. This can be
 observed on systems which run with Dcache enabled and L2 cache enabled,
 like the Altera SoCFPGA.

 This patch replaces the whole unmaintainable indirect write implementation
 with the one from upcoming Linux CQSPI driver, which went through multiple
 rounds of thorough review and testing. While this makes the patch look
 terrifying and violates all best-practices of software development, all
 the patch does is it plucks out duplicate ad-hoc code distributed across
 the driver and replaces it with more compact code doing exactly the same
 thing.

 Signed-off-by: Marek Vasut 
 Cc: Anatolij Gustschin 
 Cc: Chin Liang See 
 Cc: Dinh Nguyen 
 Cc: Jagan Teki 
 Cc: Pavel Machek 
 Cc: Stefan Roese 
 Cc: Vignesh R 
>>>
>>> I've applied both patches and tested them on SR1500 (SPI-NOR used
>>> for booting and redundant environment). This is what I get upon
>>> "saveeenv":
>>>
>>> => saveenv
>>> Saving Environment to SPI Flash...
>>> SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 
>>> MiB
>>> Erasing SPI flash...Writing to SPI flash...data abort
>>> pc : [<3ff8368a>]  lr : [<3ff8301b>]
>>> reloc pc : [<010216ca>]lr : [<0102105b>]
>>> sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
>>> r10:   r9 : 3bf5dee8 r8 : 
>>> r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
>>> r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
>>> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
>>> Resetting CPU ...
>>>
>>> resetting ...
>>>
>>> U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)
>>>
>>>
>>> Any idea, what might be going wrong here?
>>
>> Does it work without the patch ?
> 
> Yes, of course. I wouldn't have posted as a reply to this patch
> if this is not the root cause.

*grumble*

> The board is using SPI NOR for env storage from the beginning.

It only happens if you use redundant env in SPI NOR.

>  Where does your PC point to at the time
>> of the crash ,which function is it ?
> 
> Its in cadence_qspi_apb_indirect_write_execute().
> 
> I debugged this issue a bit and found the following problem
> in cadence_qspi_apb_indirect_write_execute():
> 
> saveenv issues a 1-byte SPI write transfer with a non 4-byte
> aligned txbuf. This causes the data-abort here. Here my small
> patch that fixes the problem:

Thanks, see below.

> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index ac47c6f..021a3e8 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -745,7 +745,15 @@ int cadence_qspi_apb_indirect_write_execute(struct 
> cadence_spi_platdata *plat,
>  
> while (remaining > 0) {
> write_bytes = remaining > page_size ? page_size : remaining;
> -   writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
> +
> +   /*
> +* Handle non 4-byte aligned access differently to avoid
> +* data-aborts
> +*/
> +   if (((u32)txbuf % 4) || (write_bytes % 4))
> +   writesb(plat->ahbbase, txbuf, write_bytes);
> +   else
> +   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
>  
> ret = wait_for_bit("QSPI", plat->regbase + 
> CQSPI_REG_SDRAMLEVEL,
>CQSPI_REG_SDRAMLEVEL_WR_MASK <<
> 
> 
> Please fell free to use this patch as-is and squash it into
> your patches or enhance it while doing this. The read function
> is also missing this unaligned handling.

Im afraid of the performance hit that we can suffer if we use byte-level
access for every unaligned buffer. What do you think
about using a bounce-buffer instead ?

> And of course the Linux driver version as well.

Does linux use unaligned buffers internally ?

> Thanks,
> Stefan
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Marek Vasut]

On 05/03/2016 12:42 PM, Pavel Machek wrote:
> Hi!
> 
>> This patch replaces the whole unmaintainable indirect write implementation
>> with the one from upcoming Linux CQSPI driver, which went through multiple
>> rounds of thorough review and testing. While this makes the patch look
>> terrifying and violates all best-practices of software development,
>> all
> 
> Could we get something less terifying and less violating? :-).
> 
>> the patch does is it plucks out duplicate ad-hoc code distributed across
>> the driver and replaces it with more compact code doing exactly the same
>> thing.
> 
> So this one is just a cleanup, and no behaviour change yet?

Apparently Stefan discovered one behavior change, which I need to look
into. Apparently, u-boot can pass unaligned buffer to the driver, which
can cause problems.

>   Pavel
> 
>> ---
>>  drivers/spi/cadence_qspi_apb.c | 122 
>> +
>>  1 file changed, 26 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
>> index 7786dd6..00a50cb 100644
>> --- a/drivers/spi/cadence_qspi_apb.c
>> +++ b/drivers/spi/cadence_qspi_apb.c
>> @@ -28,6 +28,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include "cadence_qspi.h"
>>  
>>  #define CQSPI_REG_POLL_US   (1) /* 1us */
>> @@ -214,32 +215,6 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
>>  return;
>>  }
>>  
>> -static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
>> -const void *src, unsigned int bytes)
>> -{
>> -unsigned int temp = 0;
>> -int i;
>> -int remaining = bytes;
>> -unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
>> -unsigned int *src_ptr = (unsigned int *)src;
>> -
>> -while (remaining >= CQSPI_FIFO_WIDTH) {
>> -for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
>> -writel(*(src_ptr+i), dest_ptr+i);
>> -src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
>> -remaining -= CQSPI_FIFO_WIDTH;
>> -}
>> -if (remaining) {
>> -/* dangling bytes */
>> -i = remaining/sizeof(dest_ptr);
>> -memcpy(, src_ptr+i, remaining % sizeof(dest_ptr));
>> -writel(temp, dest_ptr+i);
>> -for (--i; i >= 0; i--)
>> -writel(*(src_ptr+i), dest_ptr+i);
>> -}
>> -return;
>> -}
>> -
>>  /* Read from SRAM FIFO with polling SRAM fill level. */
>>  static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
>>  const void *src_addr,  unsigned int num_bytes)
>> @@ -276,44 +251,6 @@ static int qspi_read_sram_fifo_poll(const void 
>> *reg_base, void *dest_addr,
>>  return 0;
>>  }
>>  
>> -/* Write to SRAM FIFO with polling SRAM fill level. */
>> -static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
>> -const void *src_addr, unsigned int num_bytes)
>> -{
>> -const void *reg_base = plat->regbase;
>> -void *dest_addr = plat->ahbbase;
>> -unsigned int retry = CQSPI_REG_RETRY;
>> -unsigned int sram_level;
>> -unsigned int wr_bytes;
>> -unsigned char *src = (unsigned char *)src_addr;
>> -int remaining = num_bytes;
>> -unsigned int page_size = plat->page_size;
>> -unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
>> -
>> -while (remaining > 0) {
>> -retry = CQSPI_REG_RETRY;
>> -while (retry--) {
>> -sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
>> -if (sram_level <= sram_threshold_words)
>> -break;
>> -}
>> -if (!retry) {
>> -printf("QSPI: SRAM fill level (0x%08x) not hit lower 
>> expected level (0x%08x)",
>> -   sram_level, sram_threshold_words);
>> -return -1;
>> -}
>> -/* Write a page or remaining bytes. */
>> -wr_bytes = (remaining > page_size) ?
>> -page_size : remaining;
>> -
>> -cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
>> -src += wr_bytes;
>> -remaining -= wr_bytes;
>> -}
>> -
>> -return 0;
>> -}
>> -
>>  void cadence_qspi_apb_controller_enable(void *reg_base)
>>  {
>>  unsigned int reg;
>> @@ -810,48 +747,41 @@ int cadence_qspi_apb_indirect_write_setup(struct 
>> cadence_spi_platdata *plat,
>>  }
>>  
>>  int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata 
>> *plat,
>> -unsigned int txlen, const u8 *txbuf)
>> +unsigned int n_tx, const u8 *txbuf)
>>  {
>> -unsigned int reg = 0;
>> -unsigned int retry;
>> +unsigned int page_size = plat->page_size;
>> +unsigned int remaining = n_tx;
>> +unsigned int write_bytes;
>> +int ret;
>>  

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Pavel Machek]

Hi!

> This patch replaces the whole unmaintainable indirect write implementation
> with the one from upcoming Linux CQSPI driver, which went through multiple
> rounds of thorough review and testing. While this makes the patch look
> terrifying and violates all best-practices of software development,
> all

Could we get something less terifying and less violating? :-).

> the patch does is it plucks out duplicate ad-hoc code distributed across
> the driver and replaces it with more compact code doing exactly the same
> thing.

So this one is just a cleanup, and no behaviour change yet?

Pavel

> ---
>  drivers/spi/cadence_qspi_apb.c | 122 
> +
>  1 file changed, 26 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
> index 7786dd6..00a50cb 100644
> --- a/drivers/spi/cadence_qspi_apb.c
> +++ b/drivers/spi/cadence_qspi_apb.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "cadence_qspi.h"
>  
>  #define CQSPI_REG_POLL_US(1) /* 1us */
> @@ -214,32 +215,6 @@ static void cadence_qspi_apb_read_fifo_data(void *dest,
>   return;
>  }
>  
> -static void cadence_qspi_apb_write_fifo_data(const void *dest_ahb_addr,
> - const void *src, unsigned int bytes)
> -{
> - unsigned int temp = 0;
> - int i;
> - int remaining = bytes;
> - unsigned int *dest_ptr = (unsigned int *)dest_ahb_addr;
> - unsigned int *src_ptr = (unsigned int *)src;
> -
> - while (remaining >= CQSPI_FIFO_WIDTH) {
> - for (i = CQSPI_FIFO_WIDTH/sizeof(src_ptr) - 1; i >= 0; i--)
> - writel(*(src_ptr+i), dest_ptr+i);
> - src_ptr += CQSPI_FIFO_WIDTH/sizeof(src_ptr);
> - remaining -= CQSPI_FIFO_WIDTH;
> - }
> - if (remaining) {
> - /* dangling bytes */
> - i = remaining/sizeof(dest_ptr);
> - memcpy(, src_ptr+i, remaining % sizeof(dest_ptr));
> - writel(temp, dest_ptr+i);
> - for (--i; i >= 0; i--)
> - writel(*(src_ptr+i), dest_ptr+i);
> - }
> - return;
> -}
> -
>  /* Read from SRAM FIFO with polling SRAM fill level. */
>  static int qspi_read_sram_fifo_poll(const void *reg_base, void *dest_addr,
>   const void *src_addr,  unsigned int num_bytes)
> @@ -276,44 +251,6 @@ static int qspi_read_sram_fifo_poll(const void 
> *reg_base, void *dest_addr,
>   return 0;
>  }
>  
> -/* Write to SRAM FIFO with polling SRAM fill level. */
> -static int qpsi_write_sram_fifo_push(struct cadence_spi_platdata *plat,
> - const void *src_addr, unsigned int num_bytes)
> -{
> - const void *reg_base = plat->regbase;
> - void *dest_addr = plat->ahbbase;
> - unsigned int retry = CQSPI_REG_RETRY;
> - unsigned int sram_level;
> - unsigned int wr_bytes;
> - unsigned char *src = (unsigned char *)src_addr;
> - int remaining = num_bytes;
> - unsigned int page_size = plat->page_size;
> - unsigned int sram_threshold_words = CQSPI_REG_SRAM_THRESHOLD_WORDS;
> -
> - while (remaining > 0) {
> - retry = CQSPI_REG_RETRY;
> - while (retry--) {
> - sram_level = CQSPI_GET_WR_SRAM_LEVEL(reg_base);
> - if (sram_level <= sram_threshold_words)
> - break;
> - }
> - if (!retry) {
> - printf("QSPI: SRAM fill level (0x%08x) not hit lower 
> expected level (0x%08x)",
> -sram_level, sram_threshold_words);
> - return -1;
> - }
> - /* Write a page or remaining bytes. */
> - wr_bytes = (remaining > page_size) ?
> - page_size : remaining;
> -
> - cadence_qspi_apb_write_fifo_data(dest_addr, src, wr_bytes);
> - src += wr_bytes;
> - remaining -= wr_bytes;
> - }
> -
> - return 0;
> -}
> -
>  void cadence_qspi_apb_controller_enable(void *reg_base)
>  {
>   unsigned int reg;
> @@ -810,48 +747,41 @@ int cadence_qspi_apb_indirect_write_setup(struct 
> cadence_spi_platdata *plat,
>  }
>  
>  int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata 
> *plat,
> - unsigned int txlen, const u8 *txbuf)
> + unsigned int n_tx, const u8 *txbuf)
>  {
> - unsigned int reg = 0;
> - unsigned int retry;
> + unsigned int page_size = plat->page_size;
> + unsigned int remaining = n_tx;
> + unsigned int write_bytes;
> + int ret;
>  
>   /* Configure the indirect read transfer bytes */
> - writel(txlen, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
> + writel(n_tx, plat->regbase + CQSPI_REG_INDIRECTWRBYTES);
>  
>   /* Start the indirect write transfer */
>   

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Stefan Roese]

On 29.04.2016 12:13, Marek Vasut wrote:
>> On 28.04.2016 00:36, Marek Vasut wrote:
>>> The indirect write code is buggy pile of nastiness which fails horribly
>>> when the system runs fast enough to saturate the controller. The failure
>>> results in some pages (256B) not being written to the flash. This can be
>>> observed on systems which run with Dcache enabled and L2 cache enabled,
>>> like the Altera SoCFPGA.
>>>
>>> This patch replaces the whole unmaintainable indirect write implementation
>>> with the one from upcoming Linux CQSPI driver, which went through multiple
>>> rounds of thorough review and testing. While this makes the patch look
>>> terrifying and violates all best-practices of software development, all
>>> the patch does is it plucks out duplicate ad-hoc code distributed across
>>> the driver and replaces it with more compact code doing exactly the same
>>> thing.
>>>
>>> Signed-off-by: Marek Vasut 
>>> Cc: Anatolij Gustschin 
>>> Cc: Chin Liang See 
>>> Cc: Dinh Nguyen 
>>> Cc: Jagan Teki 
>>> Cc: Pavel Machek 
>>> Cc: Stefan Roese 
>>> Cc: Vignesh R 
>>
>> I've applied both patches and tested them on SR1500 (SPI-NOR used
>> for booting and redundant environment). This is what I get upon
>> "saveeenv":
>>
>> => saveenv
>> Saving Environment to SPI Flash...
>> SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 
>> MiB
>> Erasing SPI flash...Writing to SPI flash...data abort
>> pc : [<3ff8368a>]  lr : [<3ff8301b>]
>> reloc pc : [<010216ca>]lr : [<0102105b>]
>> sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
>> r10:   r9 : 3bf5dee8 r8 : 
>> r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
>> r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
>> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
>> Resetting CPU ...
>>
>> resetting ...
>>
>> U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)
>>
>>
>> Any idea, what might be going wrong here?
> 
> Does it work without the patch ?

Yes, of course. I wouldn't have posted as a reply to this patch
if this is not the root cause. The board is using SPI NOR
for env storage from the beginning.

 Where does your PC point to at the time
> of the crash ,which function is it ?

Its in cadence_qspi_apb_indirect_write_execute().

I debugged this issue a bit and found the following problem
in cadence_qspi_apb_indirect_write_execute():

saveenv issues a 1-byte SPI write transfer with a non 4-byte
aligned txbuf. This causes the data-abort here. Here my small
patch that fixes the problem:

diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c
index ac47c6f..021a3e8 100644
--- a/drivers/spi/cadence_qspi_apb.c
+++ b/drivers/spi/cadence_qspi_apb.c
@@ -745,7 +745,15 @@ int cadence_qspi_apb_indirect_write_execute(struct 
cadence_spi_platdata *plat,
 
while (remaining > 0) {
write_bytes = remaining > page_size ? page_size : remaining;
-   writesl(plat->ahbbase, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+   /*
+* Handle non 4-byte aligned access differently to avoid
+* data-aborts
+*/
+   if (((u32)txbuf % 4) || (write_bytes % 4))
+   writesb(plat->ahbbase, txbuf, write_bytes);
+   else
+   writesl(plat->ahbbase, txbuf, write_bytes >> 2);
 
ret = wait_for_bit("QSPI", plat->regbase + CQSPI_REG_SDRAMLEVEL,
   CQSPI_REG_SDRAMLEVEL_WR_MASK <<


Please fell free to use this patch as-is and squash it into
your patches or enhance it while doing this. The read function
is also missing this unaligned handling. And of course the
Linux driver version as well.

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Marek Vasut]

On 04/29/2016 11:35 AM, Stefan Roese wrote:
> Hi Marek,
> 
> On 28.04.2016 00:36, Marek Vasut wrote:
>> The indirect write code is buggy pile of nastiness which fails horribly
>> when the system runs fast enough to saturate the controller. The failure
>> results in some pages (256B) not being written to the flash. This can be
>> observed on systems which run with Dcache enabled and L2 cache enabled,
>> like the Altera SoCFPGA.
>>
>> This patch replaces the whole unmaintainable indirect write implementation
>> with the one from upcoming Linux CQSPI driver, which went through multiple
>> rounds of thorough review and testing. While this makes the patch look
>> terrifying and violates all best-practices of software development, all
>> the patch does is it plucks out duplicate ad-hoc code distributed across
>> the driver and replaces it with more compact code doing exactly the same
>> thing.
>>
>> Signed-off-by: Marek Vasut 
>> Cc: Anatolij Gustschin 
>> Cc: Chin Liang See 
>> Cc: Dinh Nguyen 
>> Cc: Jagan Teki 
>> Cc: Pavel Machek 
>> Cc: Stefan Roese 
>> Cc: Vignesh R 
> 
> I've applied both patches and tested them on SR1500 (SPI-NOR used
> for booting and redundant environment). This is what I get upon
> "saveeenv":
> 
> => saveenv
> Saving Environment to SPI Flash...
> SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB
> Erasing SPI flash...Writing to SPI flash...data abort
> pc : [<3ff8368a>]  lr : [<3ff8301b>]
> reloc pc : [<010216ca>]lr : [<0102105b>]
> sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
> r10:   r9 : 3bf5dee8 r8 : 
> r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
> r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
> Resetting CPU ...
> 
> resetting ...
> 
> U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)
> 
> 
> Any idea, what might be going wrong here?

Does it work without the patch ? Where does your PC point to at the time
of the crash ,which function is it ?

> Thanks,
> Stefan
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Stefan Roese]

Hi Marek,

On 28.04.2016 00:36, Marek Vasut wrote:
> The indirect write code is buggy pile of nastiness which fails horribly
> when the system runs fast enough to saturate the controller. The failure
> results in some pages (256B) not being written to the flash. This can be
> observed on systems which run with Dcache enabled and L2 cache enabled,
> like the Altera SoCFPGA.
> 
> This patch replaces the whole unmaintainable indirect write implementation
> with the one from upcoming Linux CQSPI driver, which went through multiple
> rounds of thorough review and testing. While this makes the patch look
> terrifying and violates all best-practices of software development, all
> the patch does is it plucks out duplicate ad-hoc code distributed across
> the driver and replaces it with more compact code doing exactly the same
> thing.
> 
> Signed-off-by: Marek Vasut 
> Cc: Anatolij Gustschin 
> Cc: Chin Liang See 
> Cc: Dinh Nguyen 
> Cc: Jagan Teki 
> Cc: Pavel Machek 
> Cc: Stefan Roese 
> Cc: Vignesh R 

I've applied both patches and tested them on SR1500 (SPI-NOR used
for booting and redundant environment). This is what I get upon
"saveeenv":

=> saveenv
Saving Environment to SPI Flash...
SF: Detected N25Q128 with page size 256 Bytes, erase size 64 KiB, total 16 MiB
Erasing SPI flash...Writing to SPI flash...data abort
pc : [<3ff8368a>]  lr : [<3ff8301b>]
reloc pc : [<010216ca>]lr : [<0102105b>]
sp : 3bf54eb8  ip : 3ff82f69 fp : 0002
r10:   r9 : 3bf5dee8 r8 : 
r7 : 0001  r6 : 3bf54f9b r5 : 0001  r4 : 3bf5e520
r3 :   r2 : 3bf54f9b r1 : 0001  r0 : ffa0
Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
Resetting CPU ...

resetting ...

U-Boot SPL 2016.05-rc3-9-ge1bf9b8 (Apr 29 2016 - 11:25:46)


Any idea, what might be going wrong here?

Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/2] mtd: cqspi: Simplify indirect write code

[Vignesh R]

On 04/28/2016 04:06 AM, Marek Vasut wrote:
> The indirect write code is buggy pile of nastiness which fails horribly
> when the system runs fast enough to saturate the controller. The failure
> results in some pages (256B) not being written to the flash. This can be
> observed on systems which run with Dcache enabled and L2 cache enabled,
> like the Altera SoCFPGA.
> 
> This patch replaces the whole unmaintainable indirect write implementation
> with the one from upcoming Linux CQSPI driver, which went through multiple
> rounds of thorough review and testing. While this makes the patch look
> terrifying and violates all best-practices of software development, all
> the patch does is it plucks out duplicate ad-hoc code distributed across
> the driver and replaces it with more compact code doing exactly the same
> thing.
> 
> Signed-off-by: Marek Vasut 
> Cc: Anatolij Gustschin 
> Cc: Chin Liang See 
> Cc: Dinh Nguyen 
> Cc: Jagan Teki 
> Cc: Pavel Machek 
> Cc: Stefan Roese 
> Cc: Vignesh R 
> ---

Tested on K2G EVM that has Cadence QSPI controller and s25fl512s
Spansion flash
For both the patches:

Tested-by: Vignesh R 



-- 
Regards
Vignesh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot