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

[Masahiro Yamada]

2018-07-13 19:35 GMT+09:00 Marek Vasut <marex at denx.de>:
> 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 ?
>


So, what are you worried about?



-- 
Best Regards
Masahiro Yamada