| Message ID | 20220218181226.431098-6-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | RZN1 DMA support | expand |
On Fri, Feb 18, 2022 at 07:12:23PM +0100, Miquel Raynal wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > Pausing a partial transfer only causes data to be written to mem that is > a multiple of the memory width setting. > > However, when a DMA client driver finishes DMA early, e.g. due to UART > char timeout interrupt, all data read from the DEV must be written to MEM. > > Therefore, allow the slave to limit the memory width to ensure all data > read from the DEV is written to MEM when DMA is paused. Is this a fix? What happens to the data if you don't do this? As far as I understood the Synopsys DesignWare specification the DMA controller is capable of flushing FIFO in that case on byte-by-byte basis. Do you have an HW integration bug? TL;DR: tell us more about this. ... > + if (sconfig->dst_addr_width && sconfig->dst_addr_width < data_width) > + data_width = sconfig->dst_addr_width; But here no check that you do it for explicitly peripheral to memory, so this will affect memory to peripheral transfers as well.
Hi Andy, I wrote the patch a few years ago, but didn't get the time to upstream it. I am not aware of a HW integration bug on the RZ/N1 device but can't rule it out. I am struggling to see what kind of HW issue this could be as, iirc, word accesses work fine when the size of the transfer is a multiple of the MEM width. I found the issue when testing DMA with the UART transferring different amounts of data. > > + if (sconfig->dst_addr_width && sconfig->dst_addr_width < > data_width) > > + data_width = sconfig->dst_addr_width; > > But here no check that you do it for explicitly peripheral to memory, so > this > will affect memory to peripheral transfers as well. No, this should be ok as this change is within: case DMA_DEV_TO_MEM: BR Phil > -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: 20 February 2022 10:50 > To: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Viresh Kumar <vireshk@kernel.org>; Vinod Koul <vkoul@kernel.org>; > Geert Uytterhoeven <geert+renesas@glider.be>; Magnus Damm > <magnus.damm@gmail.com>; Michael Turquette <mturquette@baylibre.com>; > Stephen Boyd <sboyd@kernel.org>; Rob Herring <robh+dt@kernel.org>; > devicetree@vger.kernel.org; dmaengine@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; linux-clk@vger.kernel.org; Thomas Petazzoni > <thomas.petazzoni@bootlin.com>; Milan Stevanovic > <milan.stevanovic@se.com>; Jimmy Lalande <jimmy.lalande@se.com>; Laetitia > MARIOTTINI <laetitia.mariottini@se.com>; Phil Edworthy > <phil.edworthy@renesas.com> > Subject: Re: [PATCH 5/8] dma: dw: Avoid partial transfers > > On Fri, Feb 18, 2022 at 07:12:23PM +0100, Miquel Raynal wrote: > > From: Phil Edworthy <phil.edworthy@renesas.com> > > > > Pausing a partial transfer only causes data to be written to mem that is > > a multiple of the memory width setting. > > > > However, when a DMA client driver finishes DMA early, e.g. due to UART > > char timeout interrupt, all data read from the DEV must be written to > MEM. > > > > Therefore, allow the slave to limit the memory width to ensure all data > > read from the DEV is written to MEM when DMA is paused. > > Is this a fix? > What happens to the data if you don't do this? > As far as I understood the Synopsys DesignWare specification the DMA > controller > is capable of flushing FIFO in that case on byte-by-byte basis. Do you > have an > HW integration bug? > > TL;DR: tell us more about this. > > ... > > > + if (sconfig->dst_addr_width && sconfig->dst_addr_width < > data_width) > > + data_width = sconfig->dst_addr_width; > > But here no check that you do it for explicitly peripheral to memory, so > this > will affect memory to peripheral transfers as well. > > > -- > With Best Regards, > Andy Shevchenko >
Hi Andy, phil.edworthy@renesas.com wrote on Mon, 21 Feb 2022 08:14:47 +0000: > Hi Andy, > > I wrote the patch a few years ago, but didn't get the time to upstream it. > > I am not aware of a HW integration bug on the RZ/N1 device but can't rule it out. I am struggling to see what kind of HW issue this could be as, iirc, word accesses work fine when the size of the transfer is a multiple of the MEM width. > > I found the issue when testing DMA with the UART transferring different amounts of data. > > > > + if (sconfig->dst_addr_width && sconfig->dst_addr_width < > > data_width) > > > + data_width = sconfig->dst_addr_width; > > > > But here no check that you do it for explicitly peripheral to memory, so > > this > > will affect memory to peripheral transfers as well. > No, this should be ok as this change is within: > case DMA_DEV_TO_MEM: I will add this to the commit log to clarify. Thanks, Miquèl
On Mon, Feb 21, 2022 at 08:14:47AM +0000, Phil Edworthy wrote: > Hi Andy, > > I wrote the patch a few years ago, but didn't get the time to upstream it. > > I am not aware of a HW integration bug on the RZ/N1 device but can't rule it > out. I am struggling to see what kind of HW issue this could be as, iirc, > word accesses work fine when the size of the transfer is a multiple of the > MEM width. > > I found the issue when testing DMA with the UART transferring different amounts of data. Can you tell more about the setup and test cases? Also, which version of the DW DMAC IP is being used in this SoC? ... > > > + if (sconfig->dst_addr_width && sconfig->dst_addr_width < > > data_width) > > > + data_width = sconfig->dst_addr_width; > > > > But here no check that you do it for explicitly peripheral to memory, so > > this > > will affect memory to peripheral transfers as well. > No, this should be ok as this change is within: > case DMA_DEV_TO_MEM: Ah, it's better. But still unclear to me why we need this. P.S. Please avoid top-postings. > > -----Original Message----- > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: 20 February 2022 10:50 > > On Fri, Feb 18, 2022 at 07:12:23PM +0100, Miquel Raynal wrote: > > > Pausing a partial transfer only causes data to be written to mem that is > > > a multiple of the memory width setting. > > > > > > However, when a DMA client driver finishes DMA early, e.g. due to UART > > > char timeout interrupt, all data read from the DEV must be written to > > MEM. > > > > > > Therefore, allow the slave to limit the memory width to ensure all data > > > read from the DEV is written to MEM when DMA is paused. > > > > Is this a fix? > > What happens to the data if you don't do this? > > As far as I understood the Synopsys DesignWare specification the DMA > > controller > > is capable of flushing FIFO in that case on byte-by-byte basis. Do you > > have an > > HW integration bug? > > > > TL;DR: tell us more about this. > > > > ... > > > > > + if (sconfig->dst_addr_width && sconfig->dst_addr_width < > > data_width) > > > + data_width = sconfig->dst_addr_width; > > > > But here no check that you do it for explicitly peripheral to memory, so > > this > > will affect memory to peripheral transfers as well.
Hi Andy, > > I found the issue when testing DMA with the UART transferring different > amounts of data. > > Can you tell more about the setup and test cases? We had a loopback test from one uart to another. The test checks the following transfer lengths (bytes): 1 to 33, 2043 to 2053, 4091 to 4101, 8187 to 8297, 16379 to 16389 I think 1 to 33 and 4095 to 4097 were enough to find the problem. > Also, which version of the DW DMAC IP is being used in this SoC? I'm still checking, but it looks to be 2.18b Thanks Phil
> > Also, which version of the DW DMAC IP is being used in this SoC? > I'm still checking, but it looks to be 2.18b Our HW people have told me it's v2.19a Thanks Phil
On Wed, Feb 23, 2022 at 08:01:25AM +0000, Phil Edworthy wrote: > > > Also, which version of the DW DMAC IP is being used in this SoC? > > I'm still checking, but it looks to be 2.18b > Our HW people have told me it's v2.19a Thanks for both answers, I have commented the same patch in v2 with my interpretation of what's going on. Let's continue there.
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 7ab83fe601ed..48cdefe997f1 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -705,6 +705,9 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) : DWC_CTLL_FC(DW_DMA_FC_D_P2M); + if (sconfig->dst_addr_width && sconfig->dst_addr_width < data_width) + data_width = sconfig->dst_addr_width; + for_each_sg(sgl, sg, sg_len, i) { struct dw_desc *desc; u32 len, mem;