| Message ID | 20220310155755.287294-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | RZN1 DMA support | expand |
+Cc: Ilpo who is currently doing adjoining stuff. Ilpo, this one affects Intel Bay Trail and Cherry Trail platforms. Not sure if it's in scope of your interest right now, but it might be useful to see how DMA <--> 8250 UART functioning. On Thu, Mar 10, 2022 at 04:57:53PM +0100, Miquel Raynal wrote: > As investigated by Phil Edworthy <phil.edworthy@renesas.com> on RZN1 a Email can be dropped as you put it below, just (full) name is enough. I'm wondering if Phil or anybody else who possess the hardware can test / tested this. > while ago, pausing a partial transfer only causes data to be written to > memory that is a multiple of the memory width setting. Such a situation > can happen eg. because of a char timeout interrupt on a UART. In this > case, the current ->terminate_all() implementation does not always flush > the remaining data as it should. > > In order to workaround this, a solutions is to resume and then pause > again the transfer before termination. The resume call in practice > actually flushes the remaining data. Perhaps Fixes tag? > Reported-by: Phil Edworthy <phil.edworthy@renesas.com> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/dma/dw/core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c > index 7ab83fe601ed..2f6183177ba5 100644 > --- a/drivers/dma/dw/core.c > +++ b/drivers/dma/dw/core.c > @@ -862,6 +862,10 @@ static int dwc_terminate_all(struct dma_chan *chan) > > clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags); > > + /* Ensure the last byte(s) are drained before disabling the channel */ > + if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags)) > + dwc_chan_resume(dwc, true); > + > dwc_chan_pause(dwc, true); > > dwc_chan_disable(dw, dwc); > -- > 2.27.0 >
Hi Andy, andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 19:50:09 +0200: > +Cc: Ilpo who is currently doing adjoining stuff. > > Ilpo, this one affects Intel Bay Trail and Cherry Trail platforms. > Not sure if it's in scope of your interest right now, but it might > be useful to see how DMA <--> 8250 UART functioning. > > On Thu, Mar 10, 2022 at 04:57:53PM +0100, Miquel Raynal wrote: > > As investigated by Phil Edworthy <phil.edworthy@renesas.com> on RZN1 a > > Email can be dropped as you put it below, just (full) name is enough. Sure. > I'm wondering if Phil or anybody else who possess the hardware can > test / tested this. I have a board with an RZN1 SoC but I don't have exactly the same setup as Phil (I only have one port with DMA working, while he used two as a loopback device). I tried to reproduce the error with no luck so far. I however verified that there was apparently no performance hit whatsoever due to this change. IIRC Phil does not have the hardware anymore. > > while ago, pausing a partial transfer only causes data to be written to > > memory that is a multiple of the memory width setting. Such a situation > > can happen eg. because of a char timeout interrupt on a UART. In this > > case, the current ->terminate_all() implementation does not always flush > > the remaining data as it should. > > > > In order to workaround this, a solutions is to resume and then pause > > again the transfer before termination. The resume call in practice > > actually flushes the remaining data. > > Perhaps Fixes tag? I don't know exactly what hardware can suffer from this, hence I decided not to add a Fixes tag given the fact that it was only observed on RZN1 (which was until now not yet supported upstream). > > Reported-by: Phil Edworthy <phil.edworthy@renesas.com> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/dma/dw/core.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c > > index 7ab83fe601ed..2f6183177ba5 100644 > > --- a/drivers/dma/dw/core.c > > +++ b/drivers/dma/dw/core.c > > @@ -862,6 +862,10 @@ static int dwc_terminate_all(struct dma_chan *chan) > > > > clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags); > > > > + /* Ensure the last byte(s) are drained before disabling the channel */ > > + if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags)) > > + dwc_chan_resume(dwc, true); > > + > > dwc_chan_pause(dwc, true); > > > > dwc_chan_disable(dw, dwc); > > -- > > 2.27.0 > > > Thanks, Miquèl
On Thu, 10 Mar 2022, Miquel Raynal wrote: > Hi Andy, > > andriy.shevchenko@linux.intel.com wrote on Thu, 10 Mar 2022 19:50:09 > +0200: > > > +Cc: Ilpo who is currently doing adjoining stuff. Thanks for the head up. I was only aware of the ones on linux-serial. > > Ilpo, this one affects Intel Bay Trail and Cherry Trail platforms. > > Not sure if it's in scope of your interest right now, but it might > > be useful to see how DMA <--> 8250 UART functioning. > > > > On Thu, Mar 10, 2022 at 04:57:53PM +0100, Miquel Raynal wrote: > > > As investigated by Phil Edworthy <phil.edworthy@renesas.com> on RZN1 a > > > > Email can be dropped as you put it below, just (full) name is enough. > > Sure. > > > I'm wondering if Phil or anybody else who possess the hardware can > > test / tested this. > > I have a board with an RZN1 SoC but I don't have exactly the same setup > as Phil (I only have one port with DMA working, while he used two as a > loopback device). I tried to reproduce the error with no luck so far. I > however verified that there was apparently no performance hit > whatsoever due to this change. IIRC Phil does not have the hardware > anymore. > > > > while ago, > > > pausing a partial transfer only causes data to be written to > > > memory that is a multiple of the memory width setting. This should be rephrased. As is it doesn't make sense.
diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c index 7ab83fe601ed..2f6183177ba5 100644 --- a/drivers/dma/dw/core.c +++ b/drivers/dma/dw/core.c @@ -862,6 +862,10 @@ static int dwc_terminate_all(struct dma_chan *chan) clear_bit(DW_DMA_IS_SOFT_LLP, &dwc->flags); + /* Ensure the last byte(s) are drained before disabling the channel */ + if (test_bit(DW_DMA_IS_PAUSED, &dwc->flags)) + dwc_chan_resume(dwc, true); + dwc_chan_pause(dwc, true); dwc_chan_disable(dw, dwc);
As investigated by Phil Edworthy <phil.edworthy@renesas.com> on RZN1 a while ago, pausing a partial transfer only causes data to be written to memory that is a multiple of the memory width setting. Such a situation can happen eg. because of a char timeout interrupt on a UART. In this case, the current ->terminate_all() implementation does not always flush the remaining data as it should. In order to workaround this, a solutions is to resume and then pause again the transfer before termination. The resume call in practice actually flushes the remaining data. Reported-by: Phil Edworthy <phil.edworthy@renesas.com> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/dma/dw/core.c | 4 ++++ 1 file changed, 4 insertions(+)