[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf
Masahiro Yamada
yamada.masahiro at socionext.com
Fri Jul 13 23:16:30 UTC 2018
2018-07-14 3:15 GMT+09:00 Marek Vasut <marex at denx.de>:
> 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.
>
The denali driver has no bounce buffer in it.
It is *you* who is trying to add a bounce buffer.
If you are referring the following code,
the purpose of denali->buf is _not_
to guarantee the DMA-safe alignment.
/*
* This buffer is DMA-mapped by denali_{read,write}_page_raw. Do not
* use devm_kmalloc() because the memory allocated by devm_ does not
* guarantee DMA-safe alignment.
*/
denali->buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
if (!denali->buf)
return -ENOMEM;
denali->buf is used to cope with the syndrome ECC layout.
If you do not know why this is needed,
check the commit log of
26d266e10e5eb59cfbcc47922655dc3149e1bd59
in Linux.
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list