2018-07-13 23:51 GMT+09:00 Marek Vasut <ma...@denx.de>: > On 07/13/2018 01:05 PM, Masahiro Yamada wrote: >> 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? > > 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@lists.denx.de > https://lists.denx.de/listinfo/u-boot -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
