[PATCH] serial: sh-sci: submit rx dma the way the DMA driver can handle it

[PATCH] serial: sh-sci: submit rx dma the way the DMA driver can handle it

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

The shdmac driver has a problem when one wants to submit two RX
descriptors and then use issue_pending on both (like this driver does):

Submit first desc
-> channel state after: SHDMA_PM_ESTABLISHED

Submit second desc
-> channel state after: SHDMA_PM_PENDING

PENDING here means: "Oh, we will wait until the first desc is finished
and then the second desc will be grabbed from the list anyhow"

However, because issue_pending does nothing for states !SHDMA_PM_ESTABLISHED, nothing ever gets active :(

Since the DMA driver is probably replaced soon anyhow, I decided for the
least intrusive workaround which is to use issue_pending on every RX
desc. (Yes, setting s->active_rx twice is ugly).

Probably not for upstream! However, this patch restores DMA capability
for SCIFA on my Lager board. Sadly, not for SCIF due to interrupt
problems still to be investigated.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Some more words about the not working SCIF:

Once I submit the RX descriptor, I don't get any irq anymore (which the driver
needs to detect timeouts). Before that, I got them.

SCIFA has a bit (SCSCR_RDRQE) to select if the interrupt should go to the CPU
or DMAC and the driver handles this correctly, so DMA works again.

Plain SCIF does not have such a bit. The documentation has a chapter "SCIF
Interrupt Sources and the DMAC" (chapter 51.4 here in v0.9) which is a little
vague to me. But from what I understand, I should get an irq along with the
DMAC transferring data. The implementation of this driver shows that earlier SH
hardware worked this way. However, I don't get interrupts, so the timeout
mechanism fails. DMA works partly, you can receive in 32 bytes chunks only. If
you are happy with that, you can also use SCIF. Good practice for your typing
skills ;)

So, my SCIF->SCIFA patch becomes more useful, too.

 drivers/tty/serial/sh-sci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index b11819bc95c4..f23b4d2ab51f 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1365,16 +1365,18 @@ static void sci_submit_rx(struct sci_port *s)
 			}
 			dev_warn(s->port.dev,
 				 "failed to re-start DMA, using PIO\n");
+
+			chan->device->device_control(chan, DMA_TERMINATE_ALL, 0);
 			sci_rx_dma_release(s, true);
 			return;
 		}
+
+		s->active_rx = s->cookie_rx[0];
+
+		dma_async_issue_pending(chan);
 		dev_dbg(s->port.dev, "%s(): cookie %d to #%d\n",
 			__func__, s->cookie_rx[i], i);
 	}
-
-	s->active_rx = s->cookie_rx[0];
-
-	dma_async_issue_pending(chan);
 }
 
 static void work_fn_rx(struct work_struct *work)
-- 
2.0.0

Re: [PATCH] serial: sh-sci: submit rx dma the way the DMA driver can handle it

[Simon Horman]

On Thu, Oct 02, 2014 at 06:14:08PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> The shdmac driver has a problem when one wants to submit two RX
> descriptors and then use issue_pending on both (like this driver does):
> 
> Submit first desc
> -> channel state after: SHDMA_PM_ESTABLISHED
> 
> Submit second desc
> -> channel state after: SHDMA_PM_PENDING
> 
> PENDING here means: "Oh, we will wait until the first desc is finished
> and then the second desc will be grabbed from the list anyhow"
> 
> However, because issue_pending does nothing for states !> SHDMA_PM_ESTABLISHED, nothing ever gets active :(
> 
> Since the DMA driver is probably replaced soon anyhow, I decided for the
> least intrusive workaround which is to use issue_pending on every RX
> desc. (Yes, setting s->active_rx twice is ugly).
> 
> Probably not for upstream! However, this patch restores DMA capability
> for SCIFA on my Lager board. Sadly, not for SCIF due to interrupt
> problems still to be investigated.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Simon Horman <horms+renesas@verge.net.au>

sh-sci patches usually go via linux-serial@vger.kernel.org and
Greg Kroah-Hartman <gregkh@linuxfoundation.org>. Could you repost this patch
with those parties included?

> ---
> 
> Some more words about the not working SCIF:
> 
> Once I submit the RX descriptor, I don't get any irq anymore (which the driver
> needs to detect timeouts). Before that, I got them.
> 
> SCIFA has a bit (SCSCR_RDRQE) to select if the interrupt should go to the CPU
> or DMAC and the driver handles this correctly, so DMA works again.
> 
> Plain SCIF does not have such a bit. The documentation has a chapter "SCIF
> Interrupt Sources and the DMAC" (chapter 51.4 here in v0.9) which is a little
> vague to me. But from what I understand, I should get an irq along with the
> DMAC transferring data. The implementation of this driver shows that earlier SH
> hardware worked this way. However, I don't get interrupts, so the timeout
> mechanism fails. DMA works partly, you can receive in 32 bytes chunks only. If
> you are happy with that, you can also use SCIF. Good practice for your typing
> skills ;)
> 
> So, my SCIF->SCIFA patch becomes more useful, too.
> 
>  drivers/tty/serial/sh-sci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index b11819bc95c4..f23b4d2ab51f 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1365,16 +1365,18 @@ static void sci_submit_rx(struct sci_port *s)
>  			}
>  			dev_warn(s->port.dev,
>  				 "failed to re-start DMA, using PIO\n");
> +
> +			chan->device->device_control(chan, DMA_TERMINATE_ALL, 0);
>  			sci_rx_dma_release(s, true);
>  			return;
>  		}
> +
> +		s->active_rx = s->cookie_rx[0];
> +
> +		dma_async_issue_pending(chan);
>  		dev_dbg(s->port.dev, "%s(): cookie %d to #%d\n",
>  			__func__, s->cookie_rx[i], i);
>  	}
> -
> -	s->active_rx = s->cookie_rx[0];
> -
> -	dma_async_issue_pending(chan);
>  }
>  
>  static void work_fn_rx(struct work_struct *work)
> -- 
> 2.0.0
> 

Re: [PATCH] serial: sh-sci: submit rx dma the way the DMA driver can handle it

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On Mon, Oct 06, 2014 at 10:55:40AM +0900, Simon Horman wrote:
> On Thu, Oct 02, 2014 at 06:14:08PM +0200, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > The shdmac driver has a problem when one wants to submit two RX
> > descriptors and then use issue_pending on both (like this driver does):
> > 
> > Submit first desc
> > -> channel state after: SHDMA_PM_ESTABLISHED
> > 
> > Submit second desc
> > -> channel state after: SHDMA_PM_PENDING
> > 
> > PENDING here means: "Oh, we will wait until the first desc is finished
> > and then the second desc will be grabbed from the list anyhow"
> > 
> > However, because issue_pending does nothing for states !=
> > SHDMA_PM_ESTABLISHED, nothing ever gets active :(
> > 
> > Since the DMA driver is probably replaced soon anyhow, I decided for the
> > least intrusive workaround which is to use issue_pending on every RX
> > desc. (Yes, setting s->active_rx twice is ugly).
> > 
> > Probably not for upstream! However, this patch restores DMA capability
> > for SCIFA on my Lager board. Sadly, not for SCIF due to interrupt
> > problems still to be investigated.
> > 
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Acked-by: Simon Horman <horms+renesas@verge.net.au>
> 
> sh-sci patches usually go via linux-serial@vger.kernel.org and
> Greg Kroah-Hartman <gregkh@linuxfoundation.org>. Could you repost this patch
> with those parties included?

I wanted to send this patch as RFC, but forgot to rename it :) I'd
really like some discussion about it, probably in Düsseldorf even. I am
still not sure this patch should go upstream, but we should talk about
it.

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] serial: sh-sci: submit rx dma the way the DMA driver can handle it

[Simon Horman]

On Mon, Oct 06, 2014 at 07:59:15AM +0200, Wolfram Sang wrote:
> On Mon, Oct 06, 2014 at 10:55:40AM +0900, Simon Horman wrote:
> > On Thu, Oct 02, 2014 at 06:14:08PM +0200, Wolfram Sang wrote:
> > > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > 
> > > The shdmac driver has a problem when one wants to submit two RX
> > > descriptors and then use issue_pending on both (like this driver does):
> > > 
> > > Submit first desc
> > > -> channel state after: SHDMA_PM_ESTABLISHED
> > > 
> > > Submit second desc
> > > -> channel state after: SHDMA_PM_PENDING
> > > 
> > > PENDING here means: "Oh, we will wait until the first desc is finished
> > > and then the second desc will be grabbed from the list anyhow"
> > > 
> > > However, because issue_pending does nothing for states !> > > SHDMA_PM_ESTABLISHED, nothing ever gets active :(
> > > 
> > > Since the DMA driver is probably replaced soon anyhow, I decided for the
> > > least intrusive workaround which is to use issue_pending on every RX
> > > desc. (Yes, setting s->active_rx twice is ugly).
> > > 
> > > Probably not for upstream! However, this patch restores DMA capability
> > > for SCIFA on my Lager board. Sadly, not for SCIF due to interrupt
> > > problems still to be investigated.
> > > 
> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > Acked-by: Simon Horman <horms+renesas@verge.net.au>
> > 
> > sh-sci patches usually go via linux-serial@vger.kernel.org and
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>. Could you repost this patch
> > with those parties included?
> 
> I wanted to send this patch as RFC, but forgot to rename it :) I'd
> really like some discussion about it, probably in Düsseldorf even. I am
> still not sure this patch should go upstream, but we should talk about
> it.

Thanks, I understand. Lets put this on the agenda for Düsseldorf.