[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf
Masahiro Yamada
yamada.masahiro at socionext.com
Fri Jul 13 15:09:25 UTC 2018
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' ?
> --
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list