2018-07-14 3:15 GMT+09:00 Marek Vasut marex@denx.de:
On 07/13/2018 05:09 PM, Masahiro Yamada wrote:
2018-07-13 23:51 GMT+09:00 Marek Vasut marex@denx.de:
On 07/13/2018 01:05 PM, Masahiro Yamada wrote:
2018-07-13 19:58 GMT+09:00 Marek Vasut marex@denx.de:
On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
2018-07-13 19:35 GMT+09:00 Marek Vasut marex@denx.de: > On 07/13/2018 12:22 PM, Masahiro Yamada wrote: >> 2018-07-13 19:18 GMT+09:00 Marek Vasut marex@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@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@denx.de: >>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote: >>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut marex@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@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@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#L13... >>>>>>>>>>>> >>>>>>>>>>>> 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#L12... >>>>>>>>>>>> >>>>>>>>>>>> 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#L... >>>> >>>> >>>> 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#L62...
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.