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.