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. But I am still not very happy about keeping two copies of code doing the same. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
