On 07/13/2018 05:09 PM, Masahiro Yamada wrote: > 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' ?
We now have two copies of bounce buffer code, one ad-hoc variant in the denali driver and one generic variant. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot