[PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On some sh-mobile systems there are more than one DMA controllers, that
can be used for serial ports. Specifying a DMA device in sh-sci platform
data unnecessarily restricts the driver to only use one DMA controller.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/tty/serial/sh-sci.c |   25 ++++++++-----------------
 include/linux/serial_sci.h  |    2 --
 2 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index ebd8629..8711f4e 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -1295,12 +1295,8 @@ static bool filter(struct dma_chan *chan, void *slave)
 	dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
 		param->slave_id);
 
-	if (param->dma_dev == chan->device->dev) {
-		chan->private = param;
-		return true;
-	} else {
-		return false;
-	}
+	chan->private = param;
+	return true;
 }
 
 static void rx_timer_fn(unsigned long arg)
@@ -1326,10 +1322,10 @@ static void sci_request_dma(struct uart_port *port)
 	dma_cap_mask_t mask;
 	int nent;
 
-	dev_dbg(port->dev, "%s: port %d DMA %p\n", __func__,
-		port->line, s->cfg->dma_dev);
+	dev_dbg(port->dev, "%s: port %d\n", __func__,
+		port->line);
 
-	if (!s->cfg->dma_dev)
+	if (s->cfg->dma_slave_tx <= 0 || s->cfg->dma_slave_rx <= 0)
 		return;
 
 	dma_cap_zero(mask);
@@ -1339,7 +1335,6 @@ static void sci_request_dma(struct uart_port *port)
 
 	/* Slave ID, e.g., SHDMA_SLAVE_SCIF0_TX */
 	param->slave_id = s->cfg->dma_slave_tx;
-	param->dma_dev = s->cfg->dma_dev;
 
 	s->cookie_tx = -EINVAL;
 	chan = dma_request_channel(mask, filter, param);
@@ -1368,7 +1363,6 @@ static void sci_request_dma(struct uart_port *port)
 
 	/* Slave ID, e.g., SHDMA_SLAVE_SCIF0_RX */
 	param->slave_id = s->cfg->dma_slave_rx;
-	param->dma_dev = s->cfg->dma_dev;
 
 	chan = dma_request_channel(mask, filter, param);
 	dev_dbg(port->dev, "%s: RX: got channel %p\n", __func__, chan);
@@ -1413,9 +1407,6 @@ static void sci_free_dma(struct uart_port *port)
 {
 	struct sci_port *s = to_sci_port(port);
 
-	if (!s->cfg->dma_dev)
-		return;
-
 	if (s->chan_tx)
 		sci_tx_dma_release(s, false);
 	if (s->chan_rx)
@@ -1790,9 +1781,9 @@ static int __devinit sci_init_single(struct platform_device *dev,
 	 */
 	port->irq		= p->irqs[SCIx_RXI_IRQ];
 
-	if (p->dma_dev)
-		dev_dbg(port->dev, "DMA device %p, tx %d, rx %d\n",
-			p->dma_dev, p->dma_slave_tx, p->dma_slave_rx);
+	if (p->dma_slave_tx > 0 && p->dma_slave_rx > 0)
+		dev_dbg(port->dev, "DMA tx %d, rx %d\n",
+			p->dma_slave_tx, p->dma_slave_rx);
 
 	return 0;
 }
diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
index a2afc9f..9d01ffa 100644
--- a/include/linux/serial_sci.h
+++ b/include/linux/serial_sci.h
@@ -56,8 +56,6 @@ struct plat_sci_port {
 	unsigned int	scbrr_algo_id;		/* SCBRR calculation algo */
 	unsigned int	scscr;			/* SCSCR initialization */
 
-	struct device	*dma_dev;
-
 	unsigned int	dma_slave_tx;
 	unsigned int	dma_slave_rx;
 };
-- 
1.7.2.5

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Koul, Vinod]

On Fri, 2011-06-24 at 13:56 +0200, Guennadi Liakhovetski wrote:
> On some sh-mobile systems there are more than one DMA controllers, that
> can be used for serial ports. Specifying a DMA device in sh-sci platform
> data unnecessarily restricts the driver to only use one DMA controller.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/tty/serial/sh-sci.c |   25 ++++++++-----------------
>  include/linux/serial_sci.h  |    2 --
>  2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index ebd8629..8711f4e 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -1295,12 +1295,8 @@ static bool filter(struct dma_chan *chan, void *slave)
>  	dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
>  		param->slave_id);
>  
> -	if (param->dma_dev == chan->device->dev) {
> -		chan->private = param;
> -		return true;
> -	} else {
> -		return false;
> -	}
> +	chan->private = param;
> +	return true;
You should not assign chan->private. 
Please move this  to dma_slave_control API
>  }
>  
>  static void rx_timer_fn(unsigned long arg)
> @@ -1326,10 +1322,10 @@ static void sci_request_dma(struct uart_port *port)
>  	dma_cap_mask_t mask;
>  	int nent;
>  
> -	dev_dbg(port->dev, "%s: port %d DMA %p\n", __func__,
> -		port->line, s->cfg->dma_dev);
> +	dev_dbg(port->dev, "%s: port %d\n", __func__,
> +		port->line);
>  
> -	if (!s->cfg->dma_dev)
> +	if (s->cfg->dma_slave_tx <= 0 || s->cfg->dma_slave_rx <= 0)
>  		return;
>  
>  	dma_cap_zero(mask);
> @@ -1339,7 +1335,6 @@ static void sci_request_dma(struct uart_port *port)
>  
>  	/* Slave ID, e.g., SHDMA_SLAVE_SCIF0_TX */
>  	param->slave_id = s->cfg->dma_slave_tx;
> -	param->dma_dev = s->cfg->dma_dev;
>  
>  	s->cookie_tx = -EINVAL;
>  	chan = dma_request_channel(mask, filter, param);
> @@ -1368,7 +1363,6 @@ static void sci_request_dma(struct uart_port *port)
>  
>  	/* Slave ID, e.g., SHDMA_SLAVE_SCIF0_RX */
>  	param->slave_id = s->cfg->dma_slave_rx;
> -	param->dma_dev = s->cfg->dma_dev;
>  
>  	chan = dma_request_channel(mask, filter, param);
>  	dev_dbg(port->dev, "%s: RX: got channel %p\n", __func__, chan);
> @@ -1413,9 +1407,6 @@ static void sci_free_dma(struct uart_port *port)
>  {
>  	struct sci_port *s = to_sci_port(port);
>  
> -	if (!s->cfg->dma_dev)
> -		return;
> -
>  	if (s->chan_tx)
>  		sci_tx_dma_release(s, false);
>  	if (s->chan_rx)
> @@ -1790,9 +1781,9 @@ static int __devinit sci_init_single(struct platform_device *dev,
>  	 */
>  	port->irq		= p->irqs[SCIx_RXI_IRQ];
>  
> -	if (p->dma_dev)
> -		dev_dbg(port->dev, "DMA device %p, tx %d, rx %d\n",
> -			p->dma_dev, p->dma_slave_tx, p->dma_slave_rx);
> +	if (p->dma_slave_tx > 0 && p->dma_slave_rx > 0)
> +		dev_dbg(port->dev, "DMA tx %d, rx %d\n",
> +			p->dma_slave_tx, p->dma_slave_rx);
>  
>  	return 0;
>  }
> diff --git a/include/linux/serial_sci.h b/include/linux/serial_sci.h
> index a2afc9f..9d01ffa 100644
> --- a/include/linux/serial_sci.h
> +++ b/include/linux/serial_sci.h
> @@ -56,8 +56,6 @@ struct plat_sci_port {
>  	unsigned int	scbrr_algo_id;		/* SCBRR calculation algo */
>  	unsigned int	scscr;			/* SCSCR initialization */
>  
> -	struct device	*dma_dev;
> -
>  	unsigned int	dma_slave_tx;
>  	unsigned int	dma_slave_rx;
>  };


-- 
~Vinod

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Paul Mundt]

On Fri, Jun 24, 2011 at 05:17:51PM +0530, Koul, Vinod wrote:
> On Fri, 2011-06-24 at 13:56 +0200, Guennadi Liakhovetski wrote:
> > On some sh-mobile systems there are more than one DMA controllers, that
> > can be used for serial ports. Specifying a DMA device in sh-sci platform
> > data unnecessarily restricts the driver to only use one DMA controller.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/tty/serial/sh-sci.c |   25 ++++++++-----------------
> >  include/linux/serial_sci.h  |    2 --
> >  2 files changed, 8 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index ebd8629..8711f4e 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -1295,12 +1295,8 @@ static bool filter(struct dma_chan *chan, void *slave)
> >  	dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
> >  		param->slave_id);
> >  
> > -	if (param->dma_dev == chan->device->dev) {
> > -		chan->private = param;
> > -		return true;
> > -	} else {
> > -		return false;
> > -	}
> > +	chan->private = param;
> > +	return true;
> You should not assign chan->private. 
> Please move this  to dma_slave_control API

I haven't seen any reply to this comment and this patch seems to still be
outstanding, is there an updated version of this patch that I've missed?

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Magnus Damm]

On Mon, Aug 29, 2011 at 5:00 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Fri, Jun 24, 2011 at 05:17:51PM +0530, Koul, Vinod wrote:
>> On Fri, 2011-06-24 at 13:56 +0200, Guennadi Liakhovetski wrote:
>> > On some sh-mobile systems there are more than one DMA controllers, that
>> > can be used for serial ports. Specifying a DMA device in sh-sci platform
>> > data unnecessarily restricts the driver to only use one DMA controller.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > ---
>> >  drivers/tty/serial/sh-sci.c |   25 ++++++++-----------------
>> >  include/linux/serial_sci.h  |    2 --
>> >  2 files changed, 8 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> > index ebd8629..8711f4e 100644
>> > --- a/drivers/tty/serial/sh-sci.c
>> > +++ b/drivers/tty/serial/sh-sci.c
>> > @@ -1295,12 +1295,8 @@ static bool filter(struct dma_chan *chan, void *slave)
>> >     dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
>> >             param->slave_id);
>> >
>> > -   if (param->dma_dev == chan->device->dev) {
>> > -           chan->private = param;
>> > -           return true;
>> > -   } else {
>> > -           return false;
>> > -   }
>> > +   chan->private = param;
>> > +   return true;
>> You should not assign chan->private.
>> Please move this  to dma_slave_control API
>
> I haven't seen any reply to this comment and this patch seems to still be
> outstanding, is there an updated version of this patch that I've missed?

This patch is simply making the SCIF driver behave as other drivers
that support DMA Engine like for instance:
MMCIF - drivers/mmc/host/sh_mmcif.c
SDHI - drivers/mmc/host/tmio_mmc_dma.c
USBHS - drivers/usb/renesas_usbhs/fifo.c

The SIU driver is still acting like the SCIF driver though:
SIU - sound/soc/sh/siu_pcm.c

If possible I would prefer if the drivers for the SCIF and the SIU
first could be cleaned up to behave like MMCIF/SDHI/USBHS and then we
can move to make use of the dma_slave_control API after that.

Any thoughs?

Thanks,

/ magnus

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Mon, 2011-08-29 at 17:16 +0900, Magnus Damm wrote:
> On Mon, Aug 29, 2011 at 5:00 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Fri, Jun 24, 2011 at 05:17:51PM +0530, Koul, Vinod wrote:
> >> On Fri, 2011-06-24 at 13:56 +0200, Guennadi Liakhovetski wrote:
> >> > On some sh-mobile systems there are more than one DMA controllers, that
> >> > can be used for serial ports. Specifying a DMA device in sh-sci platform
> >> > data unnecessarily restricts the driver to only use one DMA controller.
> >> >
> >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> > ---
> >> >  drivers/tty/serial/sh-sci.c |   25 ++++++++-----------------
> >> >  include/linux/serial_sci.h  |    2 --
> >> >  2 files changed, 8 insertions(+), 19 deletions(-)
> >> >
> >> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> >> > index ebd8629..8711f4e 100644
> >> > --- a/drivers/tty/serial/sh-sci.c
> >> > +++ b/drivers/tty/serial/sh-sci.c
> >> > @@ -1295,12 +1295,8 @@ static bool filter(struct dma_chan *chan, void *slave)
> >> >     dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
> >> >             param->slave_id);
> >> >
> >> > -   if (param->dma_dev == chan->device->dev) {
> >> > -           chan->private = param;
> >> > -           return true;
> >> > -   } else {
> >> > -           return false;
> >> > -   }
> >> > +   chan->private = param;
> >> > +   return true;
> >> You should not assign chan->private.
> >> Please move this  to dma_slave_control API
> >
> > I haven't seen any reply to this comment and this patch seems to still be
> > outstanding, is there an updated version of this patch that I've missed?
I don't recall seeing any updated version fixing this 

--
~Vinod
> This patch is simply making the SCIF driver behave as other drivers
> that support DMA Engine like for instance:
> MMCIF - drivers/mmc/host/sh_mmcif.c
> SDHI - drivers/mmc/host/tmio_mmc_dma.c
> USBHS - drivers/usb/renesas_usbhs/fifo.c
> 
> The SIU driver is still acting like the SCIF driver though:
> SIU - sound/soc/sh/siu_pcm.c
> 
> If possible I would prefer if the drivers for the SCIF and the SIU
> first could be cleaned up to behave like MMCIF/SDHI/USBHS and then we
> can move to make use of the dma_slave_control API after that.
> 
> Any thoughs?
> 
> Thanks,
> 
> / magnus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Mon, 29 Aug 2011, Vinod Koul wrote:

> On Mon, 2011-08-29 at 17:16 +0900, Magnus Damm wrote:
> > On Mon, Aug 29, 2011 at 5:00 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > > On Fri, Jun 24, 2011 at 05:17:51PM +0530, Koul, Vinod wrote:
> > >> On Fri, 2011-06-24 at 13:56 +0200, Guennadi Liakhovetski wrote:
> > >> > On some sh-mobile systems there are more than one DMA controllers, that
> > >> > can be used for serial ports. Specifying a DMA device in sh-sci platform
> > >> > data unnecessarily restricts the driver to only use one DMA controller.
> > >> >
> > >> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > >> > ---
> > >> >  drivers/tty/serial/sh-sci.c |   25 ++++++++-----------------
> > >> >  include/linux/serial_sci.h  |    2 --
> > >> >  2 files changed, 8 insertions(+), 19 deletions(-)
> > >> >
> > >> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > >> > index ebd8629..8711f4e 100644
> > >> > --- a/drivers/tty/serial/sh-sci.c
> > >> > +++ b/drivers/tty/serial/sh-sci.c
> > >> > @@ -1295,12 +1295,8 @@ static bool filter(struct dma_chan *chan, void *slave)
> > >> >     dev_dbg(chan->device->dev, "%s: slave ID %d\n", __func__,
> > >> >             param->slave_id);
> > >> >
> > >> > -   if (param->dma_dev == chan->device->dev) {
> > >> > -           chan->private = param;
> > >> > -           return true;
> > >> > -   } else {
> > >> > -           return false;
> > >> > -   }
> > >> > +   chan->private = param;
> > >> > +   return true;
> > >> You should not assign chan->private.
> > >> Please move this  to dma_slave_control API
> > >
> > > I haven't seen any reply to this comment and this patch seems to still be
> > > outstanding, is there an updated version of this patch that I've missed?
> I don't recall seeing any updated version fixing this 

As a matter of fact, when you say "use dma_slave_control API," you 
actually mean the dma_slave_config, right? Then, how is one supposed to 
use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
the filter and check the return code? The problem is, that not all DMA 
controllers on sh-mobile SoCs can service the same slave devices. So, if 
we don't check in the filter we might well get an unsuitable DMA channel.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > >> You should not assign chan->private.
> > > >> Please move this  to dma_slave_control API
> > > >
> > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > outstanding, is there an updated version of this patch that I've missed?
> > I don't recall seeing any updated version fixing this 
> 
> As a matter of fact, when you say "use dma_slave_control API," you 
> actually mean the dma_slave_config, right? Then, how is one supposed to 
> use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> the filter and check the return code? The problem is, that not all DMA 
> controllers on sh-mobile SoCs can service the same slave devices. So, if 
> we don't check in the filter we might well get an unsuitable DMA channel.
Here you are assigning to chan->private your specific values, which
should be moved to the dma_slave_config. 
But here you are removing the checks, and accepting the first channel
you got, so how do you find channel is suitable or not.

-- 
~Vinod

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Tue, 30 Aug 2011, Vinod Koul wrote:

> On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > >> You should not assign chan->private.
> > > > >> Please move this  to dma_slave_control API
> > > > >
> > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > outstanding, is there an updated version of this patch that I've missed?
> > > I don't recall seeing any updated version fixing this 
> > 
> > As a matter of fact, when you say "use dma_slave_control API," you 
> > actually mean the dma_slave_config, right? Then, how is one supposed to 
> > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> > the filter and check the return code? The problem is, that not all DMA 
> > controllers on sh-mobile SoCs can service the same slave devices. So, if 
> > we don't check in the filter we might well get an unsuitable DMA channel.
> Here you are assigning to chan->private your specific values, which
> should be moved to the dma_slave_config. 
> But here you are removing the checks, and accepting the first channel
> you got, so how do you find channel is suitable or not.

That's done in the driver's .device_alloc_chan_resources() method. It 
checkx the .private pointer, tries to find a suitable channel, if it fails 
- it returns -EINVAL. See 
drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():

	if (param) {
		const struct sh_dmae_slave_config *cfg;

		cfg = sh_dmae_find_slave(sh_chan, param);
		if (!cfg) {
			ret = -EINVAL;
			goto efindslave;
		}

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> On Tue, 30 Aug 2011, Vinod Koul wrote:
> 
> > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > >> You should not assign chan->private.
> > > > > >> Please move this  to dma_slave_control API
> > > > > >
> > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > I don't recall seeing any updated version fixing this 
> > > 
> > > As a matter of fact, when you say "use dma_slave_control API," you 
> > > actually mean the dma_slave_config, right? Then, how is one supposed to 
> > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> > > the filter and check the return code? The problem is, that not all DMA 
> > > controllers on sh-mobile SoCs can service the same slave devices. So, if 
> > > we don't check in the filter we might well get an unsuitable DMA channel.
> > Here you are assigning to chan->private your specific values, which
> > should be moved to the dma_slave_config. 
> > But here you are removing the checks, and accepting the first channel
> > you got, so how do you find channel is suitable or not.
> 
> That's done in the driver's .device_alloc_chan_resources() method. It 
> checkx the .private pointer, tries to find a suitable channel, if it fails 
> - it returns -EINVAL. See 
> drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> 
> 	if (param) {
> 		const struct sh_dmae_slave_config *cfg;
> 
> 		cfg = sh_dmae_find_slave(sh_chan, param);
> 		if (!cfg) {
> 			ret = -EINVAL;
> 			goto efindslave;
> 		}
Now am doubly confused. Are you saying that you are using
alloc_chan_resources for doing filtering??

-- 
~Vinod

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Tue, 30 Aug 2011, Vinod Koul wrote:

> On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > 
> > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > >> You should not assign chan->private.
> > > > > > >> Please move this  to dma_slave_control API
> > > > > > >
> > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > I don't recall seeing any updated version fixing this 
> > > > 
> > > > As a matter of fact, when you say "use dma_slave_control API," you 
> > > > actually mean the dma_slave_config, right? Then, how is one supposed to 
> > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> > > > the filter and check the return code? The problem is, that not all DMA 
> > > > controllers on sh-mobile SoCs can service the same slave devices. So, if 
> > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > Here you are assigning to chan->private your specific values, which
> > > should be moved to the dma_slave_config. 
> > > But here you are removing the checks, and accepting the first channel
> > > you got, so how do you find channel is suitable or not.
> > 
> > That's done in the driver's .device_alloc_chan_resources() method. It 
> > checkx the .private pointer, tries to find a suitable channel, if it fails 
> > - it returns -EINVAL. See 
> > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > 
> > 	if (param) {
> > 		const struct sh_dmae_slave_config *cfg;
> > 
> > 		cfg = sh_dmae_find_slave(sh_chan, param);
> > 		if (!cfg) {
> > 			ret = -EINVAL;
> > 			goto efindslave;
> > 		}
> Now am doubly confused. Are you saying that you are using
> alloc_chan_resources for doing filtering??

Let's look at __dma_request_channel(). It iterates over DMA devices and 
calls private_candidate() for each of them, which in turn calls the filter 
function for each free channel on the current device. As soon as the 
filter returns "true" for one of channels, a private candidate is found. 
And in my filter I do exactly this - assign the .private pointer and 
return "true." Next dma_chan_get() is called, which in turn calls driver's 
.device_alloc_chan_resources() method. There we check the previously set 
.private pointer to see, if this slave can be served by this DMA device. 
On sh-mobile you can freely configure single DMA channels for different 
slaves, as long as this slave is at all supported by the current DMA 
controller device. If this slave can be supported - all is good, we use 
the private data to configure the channel. If not - we return an error and 
__dma_request_channel() iterates to the next DMA controller device, which 
is exactly what we need.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

Hi Vinod

On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:

> On Tue, 30 Aug 2011, Vinod Koul wrote:
> 
> > On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > > 
> > > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > > >> You should not assign chan->private.
> > > > > > > >> Please move this  to dma_slave_control API
> > > > > > > >
> > > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > > I don't recall seeing any updated version fixing this 
> > > > > 
> > > > > As a matter of fact, when you say "use dma_slave_control API," you 
> > > > > actually mean the dma_slave_config, right? Then, how is one supposed to 
> > > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> > > > > the filter and check the return code? The problem is, that not all DMA 
> > > > > controllers on sh-mobile SoCs can service the same slave devices. So, if 
> > > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > > Here you are assigning to chan->private your specific values, which
> > > > should be moved to the dma_slave_config. 
> > > > But here you are removing the checks, and accepting the first channel
> > > > you got, so how do you find channel is suitable or not.
> > > 
> > > That's done in the driver's .device_alloc_chan_resources() method. It 
> > > checkx the .private pointer, tries to find a suitable channel, if it fails 
> > > - it returns -EINVAL. See 
> > > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > > 
> > > 	if (param) {
> > > 		const struct sh_dmae_slave_config *cfg;
> > > 
> > > 		cfg = sh_dmae_find_slave(sh_chan, param);
> > > 		if (!cfg) {
> > > 			ret = -EINVAL;
> > > 			goto efindslave;
> > > 		}
> > Now am doubly confused. Are you saying that you are using
> > alloc_chan_resources for doing filtering??
> 
> Let's look at __dma_request_channel(). It iterates over DMA devices and 
> calls private_candidate() for each of them, which in turn calls the filter 
> function for each free channel on the current device. As soon as the 
> filter returns "true" for one of channels, a private candidate is found. 
> And in my filter I do exactly this - assign the .private pointer and 
> return "true." Next dma_chan_get() is called, which in turn calls driver's 
> .device_alloc_chan_resources() method. There we check the previously set 
> .private pointer to see, if this slave can be served by this DMA device. 
> On sh-mobile you can freely configure single DMA channels for different 
> slaves, as long as this slave is at all supported by the current DMA 
> controller device. If this slave can be supported - all is good, we use 
> the private data to configure the channel. If not - we return an error and 
> __dma_request_channel() iterates to the next DMA controller device, which 
> is exactly what we need.

Haven't heard back from you after this my reply. Does this mean, that 
you're now satisfied with my explanation and are going to apply my patch?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Mon, 2011-09-05 at 10:04 +0200, Guennadi Liakhovetski wrote:
> Hi Vinod
> 
> On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> 
> > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > 
> > > On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > > > 
> > > > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > > > >> You should not assign chan->private.
> > > > > > > > >> Please move this  to dma_slave_control API
> > > > > > > > >
> > > > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > > > I don't recall seeing any updated version fixing this 
> > > > > > 
> > > > > > As a matter of fact, when you say "use dma_slave_control API," you 
> > > > > > actually mean the dma_slave_config, right? Then, how is one supposed to 
> > > > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> > > > > > the filter and check the return code? The problem is, that not all DMA 
> > > > > > controllers on sh-mobile SoCs can service the same slave devices. So, if 
> > > > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > > > Here you are assigning to chan->private your specific values, which
> > > > > should be moved to the dma_slave_config. 
> > > > > But here you are removing the checks, and accepting the first channel
> > > > > you got, so how do you find channel is suitable or not.
> > > > 
> > > > That's done in the driver's .device_alloc_chan_resources() method. It 
> > > > checkx the .private pointer, tries to find a suitable channel, if it fails 
> > > > - it returns -EINVAL. See 
> > > > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > > > 
> > > > 	if (param) {
> > > > 		const struct sh_dmae_slave_config *cfg;
> > > > 
> > > > 		cfg = sh_dmae_find_slave(sh_chan, param);
> > > > 		if (!cfg) {
> > > > 			ret = -EINVAL;
> > > > 			goto efindslave;
> > > > 		}
> > > Now am doubly confused. Are you saying that you are using
> > > alloc_chan_resources for doing filtering??
> > 
> > Let's look at __dma_request_channel(). It iterates over DMA devices and 
> > calls private_candidate() for each of them, which in turn calls the filter 
> > function for each free channel on the current device. As soon as the 
> > filter returns "true" for one of channels, a private candidate is found. 
> > And in my filter I do exactly this - assign the .private pointer and 
> > return "true." Next dma_chan_get() is called, which in turn calls driver's 
> > .device_alloc_chan_resources() method. There we check the previously set 
> > .private pointer to see, if this slave can be served by this DMA device. 
> > On sh-mobile you can freely configure single DMA channels for different 
> > slaves, as long as this slave is at all supported by the current DMA 
> > controller device. If this slave can be supported - all is good, we use 
> > the private data to configure the channel. If not - we return an error and 
> > __dma_request_channel() iterates to the next DMA controller device, which 
> > is exactly what we need.
> 
> Haven't heard back from you after this my reply. Does this mean, that 
> you're now satisfied with my explanation and are going to apply my patch?
> 
Sorry for the delay, we had a v long weekend here in India :)
I am still not fully convinced yet. Why do you need the private pointer
to be assigned in filter function? Once you return true for a channel in
filter function, you should be able to use the channel properly.
Assuming that you are calling for correct channel and you have set your
private pointer (i don't understand for why/what), then above check
seems to be bogus, why should you check again
in .device_alloc_chan_resources?

> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/


-- 
~Vinod

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Mon, 5 Sep 2011, Vinod Koul wrote:

> On Mon, 2011-09-05 at 10:04 +0200, Guennadi Liakhovetski wrote:
> > Hi Vinod
> > 
> > On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> > 
> > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > > 
> > > > On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > > > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > > > > 
> > > > > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > > > > >> You should not assign chan->private.
> > > > > > > > > >> Please move this  to dma_slave_control API
> > > > > > > > > >
> > > > > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > > > > I don't recall seeing any updated version fixing this 
> > > > > > > 
> > > > > > > As a matter of fact, when you say "use dma_slave_control API," you 
> > > > > > > actually mean the dma_slave_config, right? Then, how is one supposed to 
> > > > > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of 
> > > > > > > the filter and check the return code? The problem is, that not all DMA 
> > > > > > > controllers on sh-mobile SoCs can service the same slave devices. So, if 
> > > > > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > > > > Here you are assigning to chan->private your specific values, which
> > > > > > should be moved to the dma_slave_config. 
> > > > > > But here you are removing the checks, and accepting the first channel
> > > > > > you got, so how do you find channel is suitable or not.
> > > > > 
> > > > > That's done in the driver's .device_alloc_chan_resources() method. It 
> > > > > checkx the .private pointer, tries to find a suitable channel, if it fails 
> > > > > - it returns -EINVAL. See 
> > > > > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > > > > 
> > > > > 	if (param) {
> > > > > 		const struct sh_dmae_slave_config *cfg;
> > > > > 
> > > > > 		cfg = sh_dmae_find_slave(sh_chan, param);
> > > > > 		if (!cfg) {
> > > > > 			ret = -EINVAL;
> > > > > 			goto efindslave;
> > > > > 		}
> > > > Now am doubly confused. Are you saying that you are using
> > > > alloc_chan_resources for doing filtering??
> > > 
> > > Let's look at __dma_request_channel(). It iterates over DMA devices and 
> > > calls private_candidate() for each of them, which in turn calls the filter 
> > > function for each free channel on the current device. As soon as the 
> > > filter returns "true" for one of channels, a private candidate is found. 
> > > And in my filter I do exactly this - assign the .private pointer and 
> > > return "true." Next dma_chan_get() is called, which in turn calls driver's 
> > > .device_alloc_chan_resources() method. There we check the previously set 
> > > .private pointer to see, if this slave can be served by this DMA device. 
> > > On sh-mobile you can freely configure single DMA channels for different 
> > > slaves, as long as this slave is at all supported by the current DMA 
> > > controller device. If this slave can be supported - all is good, we use 
> > > the private data to configure the channel. If not - we return an error and 
> > > __dma_request_channel() iterates to the next DMA controller device, which 
> > > is exactly what we need.
> > 
> > Haven't heard back from you after this my reply. Does this mean, that 
> > you're now satisfied with my explanation and are going to apply my patch?
> > 
> Sorry for the delay, we had a v long weekend here in India :)
> I am still not fully convinced yet. Why do you need the private pointer
> to be assigned in filter function? Once you return true for a channel in
> filter function, you should be able to use the channel properly.
> Assuming that you are calling for correct channel and you have set your
> private pointer (i don't understand for why/what), then above check
> seems to be bogus, why should you check again
> in .device_alloc_chan_resources?

That's exactly what I tried to explain above:

> > > On sh-mobile you can freely configure single DMA channels for different
> > > slaves, as long as this slave is at all supported by the current DMA
> > > controller device. If this slave can be supported - all is good, we use
> > > the private data to configure the channel. If not - we return an error and
> > > __dma_request_channel() iterates to the next DMA controller device, which
> > > is exactly what we need.

Let me try again. DMA channels on these DMA controllers are not dedicated. 
On one such SoC there can be several such DMA controllers of different 
kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
freely configured for one of onboard peripherals: serial, mmc, etc. Some 
of them can also serve external DMA-capable devices. Another kind of DMA 
controllers, served by the same driver, can only be used with USB 
controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
dmaengine core first finds the USB DMA controller. The MMC driver cannot 
know this. It assigns its MMC DMA configuration to the.private pointer and 
returns true. Next the DMA driver is entered, it checks the private 
pointer, sees an MMC channel request, looks at the DMA controller and 
sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
fails. When the same is attempted with a suitable DMA controller, the 
shdma driver recognises, that the controller can service MMC and uses the 
data, provided the MMC driver, to configure the DMA channel for MMC.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> 
> Let me try again. DMA channels on these DMA controllers are not dedicated. 
> On one such SoC there can be several such DMA controllers of different 
> kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
> freely configured for one of onboard peripherals: serial, mmc, etc. Some 
> of them can also serve external DMA-capable devices. Another kind of DMA 
> controllers, served by the same driver, can only be used with USB 
> controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
> dmaengine core first finds the USB DMA controller. The MMC driver cannot 
> know this. It assigns its MMC DMA configuration to the.private pointer and 
> returns true. Next the DMA driver is entered, it checks the private 
> pointer, sees an MMC channel request, looks at the DMA controller and 
> sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
> fails. When the same is attempted with a suitable DMA controller, the 
> shdma driver recognises, that the controller can service MMC and uses the 
> data, provided the MMC driver, to configure the DMA channel for MMC.
Hmmm, Can't you know in filter function if the respective channel can do
the dma for you or not? Maybe export a dma function or use platform data
for this (wont you soc have these caps fixed), i prefer latter.
That maybe a better approach. 

Once you have been allocated it should normally work, additional
filtering confuses :(

-- 
~Vinod

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Mon, 5 Sep 2011, Vinod Koul wrote:

> On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > 
> > Let me try again. DMA channels on these DMA controllers are not dedicated. 
> > On one such SoC there can be several such DMA controllers of different 
> > kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
> > freely configured for one of onboard peripherals: serial, mmc, etc. Some 
> > of them can also serve external DMA-capable devices. Another kind of DMA 
> > controllers, served by the same driver, can only be used with USB 
> > controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
> > dmaengine core first finds the USB DMA controller. The MMC driver cannot 
> > know this. It assigns its MMC DMA configuration to the.private pointer and 
> > returns true. Next the DMA driver is entered, it checks the private 
> > pointer, sees an MMC channel request, looks at the DMA controller and 
> > sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
> > fails. When the same is attempted with a suitable DMA controller, the 
> > shdma driver recognises, that the controller can service MMC and uses the 
> > data, provided the MMC driver, to configure the DMA channel for MMC.
> Hmmm, Can't you know in filter function if the respective channel can do
> the dma for you or not? Maybe export a dma function or use platform data
> for this (wont you soc have these caps fixed), i prefer latter.
> That maybe a better approach. 

How? On a system you can have 3 suitable DMA controllers and 2 unsuitable. 
Do you want to pass a list of 3 suitable DMA controllers to each 
peripheral driver?...

> Once you have been allocated it should normally work, additional
> filtering confuses :(

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Mon, 2011-09-05 at 17:01 +0200, Guennadi Liakhovetski wrote:
> On Mon, 5 Sep 2011, Vinod Koul wrote:
> 
> > On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > > 
> > > Let me try again. DMA channels on these DMA controllers are not dedicated. 
> > > On one such SoC there can be several such DMA controllers of different 
> > > kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
> > > freely configured for one of onboard peripherals: serial, mmc, etc. Some 
> > > of them can also serve external DMA-capable devices. Another kind of DMA 
> > > controllers, served by the same driver, can only be used with USB 
> > > controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
> > > dmaengine core first finds the USB DMA controller. The MMC driver cannot 
> > > know this. It assigns its MMC DMA configuration to the.private pointer and 
> > > returns true. Next the DMA driver is entered, it checks the private 
> > > pointer, sees an MMC channel request, looks at the DMA controller and 
> > > sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
> > > fails. When the same is attempted with a suitable DMA controller, the 
> > > shdma driver recognises, that the controller can service MMC and uses the 
> > > data, provided the MMC driver, to configure the DMA channel for MMC.
> > Hmmm, Can't you know in filter function if the respective channel can do
> > the dma for you or not? Maybe export a dma function or use platform data
> > for this (wont you soc have these caps fixed), i prefer latter.
> > That maybe a better approach. 
> 
> How? On a system you can have 3 suitable DMA controllers and 2 unsuitable. 
> Do you want to pass a list of 3 suitable DMA controllers to each 
> peripheral driver?...
The peripheral driver (client driver in slave-dma terminology) should
already know which dmac it wants. (base on information in platform data
etc) That is why the filter function is provided. Please use it properly
Other soc have similar capabilities and they can filter properly so why
cant you..?

-- 
~Vinod

PS: This might well be my last post before Tue EOD PST, traveling to
LPC, if you are there feel free to chat with me on this.

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Mon, 5 Sep 2011, Vinod Koul wrote:

> On Mon, 2011-09-05 at 17:01 +0200, Guennadi Liakhovetski wrote:
> > On Mon, 5 Sep 2011, Vinod Koul wrote:
> > 
> > > On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > > > 
> > > > Let me try again. DMA channels on these DMA controllers are not dedicated. 
> > > > On one such SoC there can be several such DMA controllers of different 
> > > > kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
> > > > freely configured for one of onboard peripherals: serial, mmc, etc. Some 
> > > > of them can also serve external DMA-capable devices. Another kind of DMA 
> > > > controllers, served by the same driver, can only be used with USB 
> > > > controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
> > > > dmaengine core first finds the USB DMA controller. The MMC driver cannot 
> > > > know this. It assigns its MMC DMA configuration to the.private pointer and 
> > > > returns true. Next the DMA driver is entered, it checks the private 
> > > > pointer, sees an MMC channel request, looks at the DMA controller and 
> > > > sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
> > > > fails. When the same is attempted with a suitable DMA controller, the 
> > > > shdma driver recognises, that the controller can service MMC and uses the 
> > > > data, provided the MMC driver, to configure the DMA channel for MMC.
> > > Hmmm, Can't you know in filter function if the respective channel can do
> > > the dma for you or not? Maybe export a dma function or use platform data
> > > for this (wont you soc have these caps fixed), i prefer latter.
> > > That maybe a better approach. 
> > 
> > How? On a system you can have 3 suitable DMA controllers and 2 unsuitable. 
> > Do you want to pass a list of 3 suitable DMA controllers to each 
> > peripheral driver?...
> The peripheral driver (client driver in slave-dma terminology) should
> already know which dmac it wants. (base on information in platform data
> etc) That is why the filter function is provided. Please use it properly
> Other soc have similar capabilities and they can filter properly so why
> cant you..?

Sorry, I have been thinking about these possibilities, but I really didn't 
find any similar case in existing drivers. Normally either channels are 
fixed - only one channel can be used for a specific peripheral, or any at 
all, or there is only one suitable controller. I only see two 
possibilities here, and they both look ugly to me:

(1) pass a list of suitable DMA controllers to slave-dma drivers, there in 
the filter you'd have to scan that list.
(2) select only one out of several suitable DMA controllers in the 
platform configuration - that needlessly reduces flexibility.

Whereas on the contrary, the DMA controller itself can perfectly look 
through the list of supported peripherals on the current controller and 
decide, whether the requested one is among them or not.

> PS: This might well be my last post before Tue EOD PST, traveling to
> LPC, if you are there feel free to chat with me on this.

No, unfortunately, I won#t be there. Are you coming to the KS in Prague in 
October?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Koul, Vinod]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3785 bytes --]

On Mon, 2011-09-05 at 17:21 +0200, Guennadi Liakhovetski wrote:
> On Mon, 5 Sep 2011, Vinod Koul wrote:
> 
> > On Mon, 2011-09-05 at 17:01 +0200, Guennadi Liakhovetski wrote:
> > > On Mon, 5 Sep 2011, Vinod Koul wrote:
> > > 
> > > > On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > > > > 
> > > > > Let me try again. DMA channels on these DMA controllers are not dedicated. 
> > > > > On one such SoC there can be several such DMA controllers of different 
> > > > > kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
> > > > > freely configured for one of onboard peripherals: serial, mmc, etc. Some 
> > > > > of them can also serve external DMA-capable devices. Another kind of DMA 
> > > > > controllers, served by the same driver, can only be used with USB 
> > > > > controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
> > > > > dmaengine core first finds the USB DMA controller. The MMC driver cannot 
> > > > > know this. It assigns its MMC DMA configuration to the.private pointer and 
> > > > > returns true. Next the DMA driver is entered, it checks the private 
> > > > > pointer, sees an MMC channel request, looks at the DMA controller and 
> > > > > sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
> > > > > fails. When the same is attempted with a suitable DMA controller, the 
> > > > > shdma driver recognises, that the controller can service MMC and uses the 
> > > > > data, provided the MMC driver, to configure the DMA channel for MMC.
> > > > Hmmm, Can't you know in filter function if the respective channel can do
> > > > the dma for you or not? Maybe export a dma function or use platform data
> > > > for this (wont you soc have these caps fixed), i prefer latter.
> > > > That maybe a better approach. 
> > > 
> > > How? On a system you can have 3 suitable DMA controllers and 2 unsuitable. 
> > > Do you want to pass a list of 3 suitable DMA controllers to each 
> > > peripheral driver?...
> > The peripheral driver (client driver in slave-dma terminology) should
> > already know which dmac it wants. (base on information in platform data
> > etc) That is why the filter function is provided. Please use it properly
> > Other soc have similar capabilities and they can filter properly so why
> > cant you..?
> 
> Sorry, I have been thinking about these possibilities, but I really didn't 
> find any similar case in existing drivers. Normally either channels are 
> fixed - only one channel can be used for a specific peripheral, or any at 
> all, or there is only one suitable controller. I only see two 
> possibilities here, and they both look ugly to me:
> 
> (1) pass a list of suitable DMA controllers to slave-dma drivers, there in 
> the filter you'd have to scan that list.
> (2) select only one out of several suitable DMA controllers in the 
> platform configuration - that needlessly reduces flexibility.
> 
> Whereas on the contrary, the DMA controller itself can perfectly look 
> through the list of supported peripherals on the current controller and 
> decide, whether the requested one is among them or not.
Then why not have shdma_filter_func() exported and then used by all your
clients for proper filtering.

I cant agree to the whole point of filtering in alloc

One channel is allocated, we should _not_ do any further filtering

> 
> > PS: This might well be my last post before Tue EOD PST, traveling to
> > LPC, if you are there feel free to chat with me on this.
> 
> No, unfortunately, I won#t be there. Are you coming to the KS in Prague in 
> October?
Not decided yet...

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Guennadi Liakhovetski]

On Thu, 8 Sep 2011, Koul, Vinod wrote:

> On Mon, 2011-09-05 at 17:21 +0200, Guennadi Liakhovetski wrote:
> > On Mon, 5 Sep 2011, Vinod Koul wrote:
> > 
> > > On Mon, 2011-09-05 at 17:01 +0200, Guennadi Liakhovetski wrote:
> > > > On Mon, 5 Sep 2011, Vinod Koul wrote:
> > > > 
> > > > > On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > > > > > 
> > > > > > Let me try again. DMA channels on these DMA controllers are not dedicated. 
> > > > > > On one such SoC there can be several such DMA controllers of different 
> > > > > > kinds. One kind is "generic" - it can do memcpy(), besides channels can be 
> > > > > > freely configured for one of onboard peripherals: serial, mmc, etc. Some 
> > > > > > of them can also serve external DMA-capable devices. Another kind of DMA 
> > > > > > controllers, served by the same driver, can only be used with USB 
> > > > > > controllers. Now, if the MMC driver requests a DMA channel, let's say, the 
> > > > > > dmaengine core first finds the USB DMA controller. The MMC driver cannot 
> > > > > > know this. It assigns its MMC DMA configuration to the.private pointer and 
> > > > > > returns true. Next the DMA driver is entered, it checks the private 
> > > > > > pointer, sees an MMC channel request, looks at the DMA controller and 
> > > > > > sees, that it doesn't support MMC. So, .device_alloc_chan_resources() 
> > > > > > fails. When the same is attempted with a suitable DMA controller, the 
> > > > > > shdma driver recognises, that the controller can service MMC and uses the 
> > > > > > data, provided the MMC driver, to configure the DMA channel for MMC.
> > > > > Hmmm, Can't you know in filter function if the respective channel can do
> > > > > the dma for you or not? Maybe export a dma function or use platform data
> > > > > for this (wont you soc have these caps fixed), i prefer latter.
> > > > > That maybe a better approach. 
> > > > 
> > > > How? On a system you can have 3 suitable DMA controllers and 2 unsuitable. 
> > > > Do you want to pass a list of 3 suitable DMA controllers to each 
> > > > peripheral driver?...
> > > The peripheral driver (client driver in slave-dma terminology) should
> > > already know which dmac it wants. (base on information in platform data
> > > etc) That is why the filter function is provided. Please use it properly
> > > Other soc have similar capabilities and they can filter properly so why
> > > cant you..?
> > 
> > Sorry, I have been thinking about these possibilities, but I really didn't 
> > find any similar case in existing drivers. Normally either channels are 
> > fixed - only one channel can be used for a specific peripheral, or any at 
> > all, or there is only one suitable controller. I only see two 
> > possibilities here, and they both look ugly to me:
> > 
> > (1) pass a list of suitable DMA controllers to slave-dma drivers, there in 
> > the filter you'd have to scan that list.
> > (2) select only one out of several suitable DMA controllers in the 
> > platform configuration - that needlessly reduces flexibility.
> > 
> > Whereas on the contrary, the DMA controller itself can perfectly look 
> > through the list of supported peripherals on the current controller and 
> > decide, whether the requested one is among them or not.
> Then why not have shdma_filter_func() exported and then used by all your
> clients for proper filtering.

You're seriously suggesting to export and use an additional shdma private 
function, bypassing the dmaengine API?... That really doesn't sound like a 
good idea to me, sorry. How about using .device_control(DMA_SLAVE_CONFIG) 
from the filter function directly to verify channel suitability?

> I cant agree to the whole point of filtering in alloc
> 
> One channel is allocated, we should _not_ do any further filtering

Sorry, I do not understand this argumentation. Filtering and resource 
allocation are parts of the channel-acquisition process, either of them 
can fail, in which case the channel remains free for future requests.

Thanks
Guennadi

> > > PS: This might well be my last post before Tue EOD PST, traveling to
> > > LPC, if you are there feel free to chat with me on this.
> > 
> > No, unfortunately, I won#t be there. Are you coming to the KS in Prague in 
> > October?
> Not decided yet...

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Koul, Vinod]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1388 bytes --]

On Wed, 2011-09-07 at 22:01 +0200, Guennadi Liakhovetski wrote:
> On Thu, 8 Sep 2011, Koul, Vinod wrote:
> You're seriously suggesting to export and use an additional shdma private 
> function, bypassing the dmaengine API?... That really doesn't sound like a 
> good idea to me, sorry. How about using .device_control(DMA_SLAVE_CONFIG) 
> from the filter function directly to verify channel suitability?
Yes see stedma40, coh90138 drivers
.device_control is not right place as channel is already allocated.

You are basically moving filter fn from all clients to dmac driver.

> 
> > I cant agree to the whole point of filtering in alloc
> > 
> > One channel is allocated, we should _not_ do any further filtering
> 
> Sorry, I do not understand this argumentation. Filtering and resource 
> allocation are parts of the channel-acquisition process, either of them 
> can fail, in which case the channel remains free for future requests.
DMA drivers set caps for each channel
Client requests a channel and dmaengine matches channel based on caps
and filer function is called so that additional filtering can take
place.
And .alloc_chan_resources is supposed to allocate the channel resource
and not do anything else.

--
~Vinod

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Paul Mundt]

On Thu, Sep 08, 2011 at 03:07:53AM +0530, Koul, Vinod wrote:
> On Wed, 2011-09-07 at 22:01 +0200, Guennadi Liakhovetski wrote:
> > On Thu, 8 Sep 2011, Koul, Vinod wrote:
> > You're seriously suggesting to export and use an additional shdma private 
> > function, bypassing the dmaengine API?... That really doesn't sound like a 
> > good idea to me, sorry. How about using .device_control(DMA_SLAVE_CONFIG) 
> > from the filter function directly to verify channel suitability?
> Yes see stedma40, coh90138 drivers
> .device_control is not right place as channel is already allocated.
> 

No, that's not going to happen either. Many of these drivers are used in
different CPUs with different DMACs. The dmaengine driver in question
only applies to a subset, so the driver bits need to be wholly generic.
In short, if the dmaengine API can't handle this sort of stuff then it's
the dmaengine API that needs to be extended, we won't be working around
dmaengine shortcomings in drivers that simply want a sensible DMA API to
plug in to.

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Thu, 2011-09-08 at 10:21 +0900, Paul Mundt wrote:
> On Thu, Sep 08, 2011 at 03:07:53AM +0530, Koul, Vinod wrote:
> > On Wed, 2011-09-07 at 22:01 +0200, Guennadi Liakhovetski wrote:
> > > On Thu, 8 Sep 2011, Koul, Vinod wrote:
> > > You're seriously suggesting to export and use an additional shdma private 
> > > function, bypassing the dmaengine API?... That really doesn't sound like a 
> > > good idea to me, sorry. How about using .device_control(DMA_SLAVE_CONFIG) 
> > > from the filter function directly to verify channel suitability?
> > Yes see stedma40, coh90138 drivers
> > .device_control is not right place as channel is already allocated.
> > 
> 
> No, that's not going to happen either. Many of these drivers are used in
> different CPUs with different DMACs. The dmaengine driver in question
> only applies to a subset, so the driver bits need to be wholly generic.
> In short, if the dmaengine API can't handle this sort of stuff then it's
> the dmaengine API that needs to be extended, we won't be working around
> dmaengine shortcomings in drivers that simply want a sensible DMA API to
> plug in to.The only thing lacking in dmaengine API is proper filtering for slave
usages where channel/controller is matches to specfic dmac slave.

There was a proposal by Linux W, and Jassi Brar for solving this
problem, and I think that should solve above problem.

I relooked at the patch, since the filtering is already done in
your .alloc_chan_resources, (which should be fixed when we fix
filtering), I am going to apply this patch

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Fri, 2011-09-09 at 01:01 +0530, Vinod Koul wrote:
> On Thu, 2011-09-08 at 10:21 +0900, Paul Mundt wrote:
> > On Thu, Sep 08, 2011 at 03:07:53AM +0530, Koul, Vinod wrote:
> > > On Wed, 2011-09-07 at 22:01 +0200, Guennadi Liakhovetski wrote:
> 
> I relooked at the patch, since the filtering is already done in
> your .alloc_chan_resources, (which should be fixed when we fix
> filtering), I am going to apply this patch
Since this is for serial driver, I can carry this patch with ACK from
Alan or Alan can take this patch with Ack from me


-- 
~Vinod

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Alan Cox]

On Wed, 14 Sep 2011 10:46:50 +0530
Vinod Koul <vinod.koul@intel.com> wrote:

> On Fri, 2011-09-09 at 01:01 +0530, Vinod Koul wrote:
> > On Thu, 2011-09-08 at 10:21 +0900, Paul Mundt wrote:
> > > On Thu, Sep 08, 2011 at 03:07:53AM +0530, Koul, Vinod wrote:
> > > > On Wed, 2011-09-07 at 22:01 +0200, Guennadi Liakhovetski wrote:
> > 
> > I relooked at the patch, since the filtering is already done in
> > your .alloc_chan_resources, (which should be fixed when we fix
> > filtering), I am going to apply this patch
> Since this is for serial driver, I can carry this patch with ACK from
> Alan or Alan can take this patch with Ack from me

Greg normally carries them but I'm happy with this going via your tree.

Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only channel ID

[Vinod Koul]

On Wed, 2011-09-14 at 19:52 +0100, Alan Cox wrote:
> On Wed, 14 Sep 2011 10:46:50 +0530
> Vinod Koul <vinod.koul@intel.com> wrote:
> 
> > On Fri, 2011-09-09 at 01:01 +0530, Vinod Koul wrote:
> > > On Thu, 2011-09-08 at 10:21 +0900, Paul Mundt wrote:
> > > > On Thu, Sep 08, 2011 at 03:07:53AM +0530, Koul, Vinod wrote:
> > > > > On Wed, 2011-09-07 at 22:01 +0200, Guennadi Liakhovetski wrote:
> > > 
> > > I relooked at the patch, since the filtering is already done in
> > > your .alloc_chan_resources, (which should be fixed when we fix
> > > filtering), I am going to apply this patch
> > Since this is for serial driver, I can carry this patch with ACK from
> > Alan or Alan can take this patch with Ack from me
> 
> Greg normally carries them but I'm happy with this going via your tree.
Okay thanks I have applied it after fixing a trivial conflict

-- 
~Vinod