[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 <marex at denx.de>
>>>>> Cc: Anatolij Gustschin <agust at denx.de>
>>>>> Cc: Chin Liang See <clsee at altera.com>
>>>>> Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
>>>>> Cc: Jagan Teki <jteki at openedev.com>
>>>>> Cc: Pavel Machek <pavel at denx.de>
>>>>> Cc: Stefan Roese <sr at denx.de>
>>>>> Cc: Vignesh R <vigneshr at ti.com>
>>>>
>>>> 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 : 00000002
>>>> r10: 00000000  r9 : 3bf5dee8     r8 : ffff0000
>>>> r7 : 00000001  r6 : 3bf54f9b     r5 : 00000001  r4 : 3bf5e520
>>>> r3 : 00000000  r2 : 3bf54f9b     r1 : 00000001  r0 : ffa00000
>>>> Flags: nZCv  IRQs off  FIQs off  Mode SVC_32
>>>> Resetting CPU ...
>>>>
>>>> resetting ...
>>>>
>>>> U-Boot SPL 2016.05-rc3-00009-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