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