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

Marek Vasut
2016-05-03T16:53:14Z

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*

> 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