[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex at denx.de>:
>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>> Hi Marek
>>>
>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>> Hi Marek,
>>>>>
>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>> passed in,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>
>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>
>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>
>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>
>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>
>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>
>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>
>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>
>>>>>>>> Bump on this one ?
>>>>>>>>
>>>>>>>
>>>>>>> Sorry for delay.
>>>>>>>
>>>>>>>
>>>>>>> UBI is working for me without your patch.
>>>>>>>
>>>>>>> Not sure what is the difference.
>>>>>>>
>>>>>>> I will dig into it a little more, though.
>>>>>>
>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>
>>>>>
>>>>> Yeah, I am testing it now,
>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>
>>>>> However, I found a simple way to trigger the warning
>>>>> in raw device access.
>>>>>
>>>>>
>>>>>
>>>>> => nand  read  81000010   0  1000
>>>>>
>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>  4096 bytes read: OK
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I can fix it with one liner.
>>>>>
>>>>>
>>>>>
>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>> index 6266c8a..f391727 100644
>>>>> --- a/drivers/mtd/nand/denali.c
>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>                 denali->dma_avail = 1;
>>>>>
>>>>>         if (denali->dma_avail) {
>>>>> -               chip->buf_align = 16;
>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>                 else
>>>>>
>>>>>
>>>>> I guess this will work for you too.
>>>>
>>>> Doesn't that only apply if DMA is available ?
>>>
>>> Of course.
>>> If you use PIO instead of DMA,
>>> you do not need to perform cache operation, right?
>>>
>>>
>>>
>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>> implementation in a driver which we need to maintain and fix separately.
>>>
>>>
>>> bounce_buffer_{start,stop} does
>>> malloc() and free() every time.
>>> This is not efficient.
>>>
>>>
>>> struct nand_chip already contains page buffers,
>>> which guarantee alignment for DMA.
>>>
>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>
>>>
>>> We can rely on the NAND framework
>>> for handling DMA-capable alignment.
>>
>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>
> 
> 
> 
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 6266c8a..f391727 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>                 denali->dma_avail = 1;
> 
>         if (denali->dma_avail) {
> -               chip->buf_align = 16;
> +               chip->buf_align = ARCH_DMA_MINALIGN;
>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>                         denali->setup_dma = denali_setup_dma64;
>                 else
> 
> 
> Did you try this?
> I do not see unaligned cache operation.

Nope, I'll have to assemble the hardware.
But this only works if dma_avail, right ?

-- 
Best regards,
Marek Vasut