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

[Marek Vasut <marex@denx.de>]

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 <marex@denx.de>
>>>>>> Cc: Anatolij Gustschin <agust@denx.de>
>>>>>> Cc: Chin Liang See <clsee@altera.com>
>>>>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>>>>>> Cc: Jagan Teki <jteki@openedev.com>
>>>>>> Cc: Pavel Machek <pavel@denx.de>
>>>>>> Cc: Stefan Roese <sr@denx.de>
>>>>>> Cc: Vignesh R <vigneshr@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*
> 
> ?

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.

>>> 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.

You can write different amount of data into the FIFO, the amount which
is transferred is controlled by INDIRECTRDBYTES / INDIRECTWRBYTES register.

> Thanks,
> Stefan


-- 
Best regards,
Marek Vasut