2018-07-13 19:58 GMT+09:00 Marek Vasut <ma...@denx.de>: > On 07/13/2018 12:52 PM, Masahiro Yamada wrote: >> 2018-07-13 19:35 GMT+09:00 Marek Vasut <ma...@denx.de>: >>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote: >>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <ma...@denx.de>: >>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote: >>>>>> Hi Marek >>>>>> >>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <ma...@denx.de>: >>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote: >>>>>>>> Hi Marek, >>>>>>>> >>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <ma...@denx.de>: >>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote: >>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <ma...@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 <ma...@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 <ma...@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? -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot