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

Fri Jul 13 18:15:16 UTC 2018

On 07/13/2018 05:09 PM, Masahiro Yamada wrote:
> 2018-07-13 23:51 GMT+09:00 Marek Vasut <marex at denx.de>:
>> On 07/13/2018 01:05 PM, Masahiro Yamada wrote:
>>> 2018-07-13 19:58 GMT+09:00 Marek Vasut <marex at denx.de>:
>>>> On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
>>>>> 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?
>>>>
>>>> That the denali NAND is broken in mainline on socfpga.
>>>
>>> I suggested more efficient fix.
>>>
>>>
>>> I am asking about your
>>> "But this only works if dma_avail, right ?"
>>>
>>> Do you see any problem in 'dma_avail == false' case?
>>
>> I cannot test this now.
> 
> 
> denali_dma_xfer() is only called when dma_avail == true.
> 
> You do not need to worry about the cache-op
> if dma_avail == false.  Believe me.
> 
> https://github.com/u-boot/u-boot/blob/v2018.07/drivers/mtd/nand/denali.c#L621
> 
> 
> 
> 
>> But I am still not very happy about keeping two copies of code doing the
>> same.
> 
> 
> What do you mean by 'two copies of code' ?

We now have two copies of bounce buffer code, one ad-hoc variant in the
denali driver and one generic variant.

-- 
Best regards,
Marek Vasut