[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf
Marek Vasut
marex at denx.de
Fri Jul 13 14:51:15 UTC 2018
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.
But I am still not very happy about keeping two copies of code doing the
same.
--
Best regards,
Marek Vasut
More information about the U-Boot
mailing list