[PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Kweh Hock Leong]

From: "Chew, Chiau Ee" <chiau.ee.chew@intel.com>

Intel LPSS Baytrail supports two DMA controllers and SPI is only
using one of the DMA controller. During DMA channel request,
we need to ensure the requested Tx and Rx channels are from the correct
DMA controller. Thus, we add extra checking in filter callback funtion
by matching against the DMA controller device name.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/spi/spi-pxa2xx-dma.c   | 5 +++++
 drivers/spi/spi-pxa2xx-pci.c   | 3 +++
 include/linux/spi/pxa2xx_spi.h | 1 +
 3 files changed, 9 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index c41ff14..4c4e918 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -214,6 +214,11 @@ static bool pxa2xx_spi_dma_filter(struct dma_chan *chan, void *param)
 {
 	const struct pxa2xx_spi_master *pdata = param;
 
+	if (pdata->dma_devname) {
+		if (strcmp(dev_name(chan->device->dev), pdata->dma_devname))
+			return false;
+	}
+
 	return chan->chan_id == pdata->tx_chan_id ||
 	       chan->chan_id == pdata->rx_chan_id;
 }
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index c1865c9..7a21bce 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -21,6 +21,7 @@ struct pxa_spi_info {
 	int tx_chan_id;
 	int rx_slave_id;
 	int rx_chan_id;
+	char *dma_devname;
 };
 
 static struct pxa_spi_info spi_info_configs[] = {
@@ -41,6 +42,7 @@ static struct pxa_spi_info spi_info_configs[] = {
 		.tx_chan_id = 0,
 		.rx_slave_id = 1,
 		.rx_chan_id = 1,
+		.dma_devname = "0000:00:1e.0"
 	},
 };
 
@@ -72,6 +74,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 	spi_pdata.rx_slave_id = c->rx_slave_id;
 	spi_pdata.rx_chan_id = c->rx_chan_id;
 	spi_pdata.enable_dma = c->rx_slave_id >= 0 && c->tx_slave_id >= 0;
+	spi_pdata.dma_devname = c->dma_devname;
 
 	ssp = &spi_pdata.ssp;
 	ssp->phys_base = pci_resource_start(dev, 0);
diff --git a/include/linux/spi/pxa2xx_spi.h b/include/linux/spi/pxa2xx_spi.h
index 82d5111..264c3cb 100644
--- a/include/linux/spi/pxa2xx_spi.h
+++ b/include/linux/spi/pxa2xx_spi.h
@@ -34,6 +34,7 @@ struct pxa2xx_spi_master {
 	int tx_chan_id;
 	int rx_slave_id;
 	int tx_slave_id;
+	char *dma_devname;
 
 	/* For non-PXA arches */
 	struct ssp_device ssp;
-- 
1.9.1

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Andy Shevchenko]

On Thu, 2014-07-24 at 18:01 +0800, Kweh Hock Leong wrote:
> From: "Chew, Chiau Ee" <chiau.ee.chew-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Intel LPSS Baytrail supports two DMA controllers and SPI is only
> using one of the DMA controller. During DMA channel request,
> we need to ensure the requested Tx and Rx channels are from the correct
> DMA controller. Thus, we add extra checking in filter callback funtion
> by matching against the DMA controller device name.

I think this is bot good approach.

I already discussed with Mika how we could do this better for devices in
PCI mode. (Seems you have the problem only in PCI, right?)

So, for PCI case you have to get the device with BDF = BD0, where BDF is
Bus:Device:Function triplet for an SPI controller itself.

I don't know if it's good to enable CONFIG_PCI_QUIRKS and tweak
pci_dev_dma_source. In that case you just call pci_get_dma_source() and
get a PCI device of the DMA.

> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/spi/spi-pxa2xx-dma.c   | 5 +++++
>  drivers/spi/spi-pxa2xx-pci.c   | 3 +++
>  include/linux/spi/pxa2xx_spi.h | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
> index c41ff14..4c4e918 100644
> --- a/drivers/spi/spi-pxa2xx-dma.c
> +++ b/drivers/spi/spi-pxa2xx-dma.c
> @@ -214,6 +214,11 @@ static bool pxa2xx_spi_dma_filter(struct dma_chan *chan, void *param)
>  {
>  	const struct pxa2xx_spi_master *pdata = param;
>  
> +	if (pdata->dma_devname) {
> +		if (strcmp(dev_name(chan->device->dev), pdata->dma_devname))
> +			return false;
> +	}
> +
>  	return chan->chan_id == pdata->tx_chan_id ||
>  	       chan->chan_id == pdata->rx_chan_id;
>  }
> diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
> index c1865c9..7a21bce 100644
> --- a/drivers/spi/spi-pxa2xx-pci.c
> +++ b/drivers/spi/spi-pxa2xx-pci.c
> @@ -21,6 +21,7 @@ struct pxa_spi_info {
>  	int tx_chan_id;
>  	int rx_slave_id;
>  	int rx_chan_id;
> +	char *dma_devname;
>  };
>  
>  static struct pxa_spi_info spi_info_configs[] = {
> @@ -41,6 +42,7 @@ static struct pxa_spi_info spi_info_configs[] = {
>  		.tx_chan_id = 0,
>  		.rx_slave_id = 1,
>  		.rx_chan_id = 1,
> +		.dma_devname = "0000:00:1e.0"
>  	},
>  };
>  
> @@ -72,6 +74,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
>  	spi_pdata.rx_slave_id = c->rx_slave_id;
>  	spi_pdata.rx_chan_id = c->rx_chan_id;
>  	spi_pdata.enable_dma = c->rx_slave_id >= 0 && c->tx_slave_id >= 0;
> +	spi_pdata.dma_devname = c->dma_devname;
>  
>  	ssp = &spi_pdata.ssp;
>  	ssp->phys_base = pci_resource_start(dev, 0);
> diff --git a/include/linux/spi/pxa2xx_spi.h b/include/linux/spi/pxa2xx_spi.h
> index 82d5111..264c3cb 100644
> --- a/include/linux/spi/pxa2xx_spi.h
> +++ b/include/linux/spi/pxa2xx_spi.h
> @@ -34,6 +34,7 @@ struct pxa2xx_spi_master {
>  	int tx_chan_id;
>  	int rx_slave_id;
>  	int tx_slave_id;
> +	char *dma_devname;
>  
>  	/* For non-PXA arches */
>  	struct ssp_device ssp;


-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Thursday 24 July 2014 18:01:51 Kweh Hock Leong wrote:
> From: "Chew, Chiau Ee" <chiau.ee.chew@intel.com>
> 
> Intel LPSS Baytrail supports two DMA controllers and SPI is only
> using one of the DMA controller. During DMA channel request,
> we need to ensure the requested Tx and Rx channels are from the correct
> DMA controller. Thus, we add extra checking in filter callback funtion
> by matching against the DMA controller device name.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>

I'm confused. Doesn't  Bay Trail use ACPI to do the DMA
engine configuration? That should set find the right device/chan_id/slave_id
combination without any interaction, through the use of dma_request_slave_channel.

On a related note, there seems to be a bug in this driver, which
attempts to set the slave_id through dmaengine_slave_config(), which
is wrong in both cases, ACPI and filter functions.

	Arnd

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Mika Westerberg]

On Thu, Jul 24, 2014 at 01:42:10PM +0200, Arnd Bergmann wrote:
> On Thursday 24 July 2014 18:01:51 Kweh Hock Leong wrote:
> > From: "Chew, Chiau Ee" <chiau.ee.chew-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > Intel LPSS Baytrail supports two DMA controllers and SPI is only
> > using one of the DMA controller. During DMA channel request,
> > we need to ensure the requested Tx and Rx channels are from the correct
> > DMA controller. Thus, we add extra checking in filter callback funtion
> > by matching against the DMA controller device name.
> > 
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> I'm confused. Doesn't  Bay Trail use ACPI to do the DMA
> engine configuration? That should set find the right device/chan_id/slave_id
> combination without any interaction, through the use of dma_request_slave_channel.

It is also possible that the corresponding Baytrail system doesn't have
ACPI enabled BIOS in which case it dma_request_slave_channel() doesn't
help here.

> On a related note, there seems to be a bug in this driver, which
> attempts to set the slave_id through dmaengine_slave_config(), which
> is wrong in both cases, ACPI and filter functions.

Good point. We will fix this, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Mika Westerberg]

On Thu, Jul 24, 2014 at 05:06:20PM +0300, Mika Westerberg wrote:
> > On a related note, there seems to be a bug in this driver, which
> > attempts to set the slave_id through dmaengine_slave_config(), which
> > is wrong in both cases, ACPI and filter functions.
> 
> Good point. We will fix this, thanks.

I take that back. How we are supposed to set the slave_id if we don't
have request line information available (from ACPI or DT)?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Friday 25 July 2014 10:11:38 Mika Westerberg wrote:
> On Thu, Jul 24, 2014 at 05:06:20PM +0300, Mika Westerberg wrote:
> > > On a related note, there seems to be a bug in this driver, which
> > > attempts to set the slave_id through dmaengine_slave_config(), which
> > > is wrong in both cases, ACPI and filter functions.
> > 
> > Good point. We will fix this, thanks.
> 
> I take that back. How we are supposed to set the slave_id if we don't
> have request line information available (from ACPI or DT)?

If you don't have the request line information, you are screwed and you
can't set it from either the filter function or slave_config.

However, it looks like you do have it, at least this is what the
code tells me:

static struct pxa_spi_info spi_info_configs[] = {
        [PORT_CE4100] = {
		...
        },
        [PORT_BYT] = {
                .type = LPSS_SSP,
                .port_id = 0,
                .num_chipselect = 1,
                .tx_slave_id = 0,
                .tx_chan_id = 0,
                .rx_slave_id = 1,
                .rx_chan_id = 1,
        },
};

All you need to do is change your filter function to take the
slave id from pxa_spi_info and stick it in there, e.g.

static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
{       
        const struct pxa2xx_spi_master *pdata = param;
        struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
        
        dwc->request_line = fargs->req;
        dwc->src_master = 0;
        dwc->dst_master = 0;
        
        return 1;
}           

Note that the filter function by definition is specific to the dma
controller, not the dma slave (that's why most people define it in
the dmaengine driver), and the pxa2xx_spi_dma_filter() function used
in spi-pxa2xx-dma.c looks like it was written for another dma engine:

The dw-dma driver doesn't care at all about the channel number, so
matching it with the platform data is pointless, but it does need
the master and request numbers to be set in the filter function,
as dw_dma_of_filter() and dw_dma_acpi_filter() do.

It also really needs a check to see if the dma engine is the right
one for the device, as the current filter function will just take
a channel (with the right number) from any dma engine, and that
breaks as soon as you register a second dma controller in the system.

I agree that this is very ugly, but that's exactly why we came up
with the dma_request_slave_channel interface to replace the need
for filter functions that everybody gets wrong.

	Arnd

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Mika Westerberg]

On Fri, Jul 25, 2014 at 09:58:42AM +0200, Arnd Bergmann wrote:
> On Friday 25 July 2014 10:11:38 Mika Westerberg wrote:
> > On Thu, Jul 24, 2014 at 05:06:20PM +0300, Mika Westerberg wrote:
> > > > On a related note, there seems to be a bug in this driver, which
> > > > attempts to set the slave_id through dmaengine_slave_config(), which
> > > > is wrong in both cases, ACPI and filter functions.
> > > 
> > > Good point. We will fix this, thanks.
> > 
> > I take that back. How we are supposed to set the slave_id if we don't
> > have request line information available (from ACPI or DT)?
> 
> If you don't have the request line information, you are screwed and you
> can't set it from either the filter function or slave_config.
> 
> However, it looks like you do have it, at least this is what the
> code tells me:
> 
> static struct pxa_spi_info spi_info_configs[] = {
>         [PORT_CE4100] = {
> 		...
>         },
>         [PORT_BYT] = {
>                 .type = LPSS_SSP,
>                 .port_id = 0,
>                 .num_chipselect = 1,
>                 .tx_slave_id = 0,
>                 .tx_chan_id = 0,
>                 .rx_slave_id = 1,
>                 .rx_chan_id = 1,
>         },
> };

That's right we do have it.

> All you need to do is change your filter function to take the
> slave id from pxa_spi_info and stick it in there, e.g.
> 
> static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
> {       
>         const struct pxa2xx_spi_master *pdata = param;
>         struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
>         
>         dwc->request_line = fargs->req;
>         dwc->src_master = 0;
>         dwc->dst_master = 0;
>         
>         return 1;
> }           

Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine
driver.

> Note that the filter function by definition is specific to the dma
> controller, not the dma slave (that's why most people define it in
> the dmaengine driver), and the pxa2xx_spi_dma_filter() function used
> in spi-pxa2xx-dma.c looks like it was written for another dma engine:

I wonder what's the rationale that passing slave_id with
dma_slave_config is wrong? The current code works fine with that and is
is independent of the DMA engine driver (even though we know that it is
going to be dw-dma).

The dw-dma handles slave_id in its implementation of
dmaengine_slave_config().

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > All you need to do is change your filter function to take the
> > slave id from pxa_spi_info and stick it in there, e.g.
> > 
> > static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
> > {       
> >         const struct pxa2xx_spi_master *pdata = param;
> >         struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> >         
> >         dwc->request_line = fargs->req;
> >         dwc->src_master = 0;
> >         dwc->dst_master = 0;
> >         
> >         return 1;
> > }           
> 
> Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine
> driver.

I think you can improve this by putting the filter function (and a pointer
to it) into the pxa2xx_spi_master data provided by the PCI driver.

On ARM, we usually provide those pointers through platform_data from
the board file.

> > Note that the filter function by definition is specific to the dma
> > controller, not the dma slave (that's why most people define it in
> > the dmaengine driver), and the pxa2xx_spi_dma_filter() function used
> > in spi-pxa2xx-dma.c looks like it was written for another dma engine:
> 
> I wonder what's the rationale that passing slave_id with
> dma_slave_config is wrong? The current code works fine with that and is
> is independent of the DMA engine driver (even though we know that it is
> going to be dw-dma).
> 
> The dw-dma handles slave_id in its implementation of
> dmaengine_slave_config().

The main point is that a single 'slave_id' is not actually enough to
identify what a slave needs. In case of dw_dma, the dma engine actually
requires three numbers (request line, source master, destination master)
to identify it, and there is no good way to put all that information into
a single integer. Other dma engines require a different set of data.

There are only two drivers left that actually use slave_id in this way,
and only for the legacy (no DT or ACPI) case, every other driver uses
either DT or a filter function. I believe the shmobile part will soon
be done with, after shmobile has been converted to DT, and after that
we should remove the slave_id field from the dma_slave_config interface.

	Arnd

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Mika Westerberg]

On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > > All you need to do is change your filter function to take the
> > > slave id from pxa_spi_info and stick it in there, e.g.
> > > 
> > > static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
> > > {       
> > >         const struct pxa2xx_spi_master *pdata = param;
> > >         struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> > >         
> > >         dwc->request_line = fargs->req;
> > >         dwc->src_master = 0;
> > >         dwc->dst_master = 0;
> > >         
> > >         return 1;
> > > }           
> > 
> > Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine
> > driver.
> 
> I think you can improve this by putting the filter function (and a pointer
> to it) into the pxa2xx_spi_master data provided by the PCI driver.

Indeed, that looks better. It still makes the PCI part of the driver
dependent on a particular DMA engine driver but is certainly better than
the core pxa2xx_spi driver.

> On ARM, we usually provide those pointers through platform_data from
> the board file.
> 
> > > Note that the filter function by definition is specific to the dma
> > > controller, not the dma slave (that's why most people define it in
> > > the dmaengine driver), and the pxa2xx_spi_dma_filter() function used
> > > in spi-pxa2xx-dma.c looks like it was written for another dma engine:
> > 
> > I wonder what's the rationale that passing slave_id with
> > dma_slave_config is wrong? The current code works fine with that and is
> > is independent of the DMA engine driver (even though we know that it is
> > going to be dw-dma).
> > 
> > The dw-dma handles slave_id in its implementation of
> > dmaengine_slave_config().
> 
> The main point is that a single 'slave_id' is not actually enough to
> identify what a slave needs. In case of dw_dma, the dma engine actually
> requires three numbers (request line, source master, destination master)
> to identify it, and there is no good way to put all that information into
> a single integer. Other dma engines require a different set of data.
> 
> There are only two drivers left that actually use slave_id in this way,
> and only for the legacy (no DT or ACPI) case, every other driver uses
> either DT or a filter function. I believe the shmobile part will soon
> be done with, after shmobile has been converted to DT, and after that
> we should remove the slave_id field from the dma_slave_config interface.

OK, thanks for the explanation.

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Mika Westerberg]

On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > > > All you need to do is change your filter function to take the
> > > > slave id from pxa_spi_info and stick it in there, e.g.
> > > > 
> > > > static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
> > > > {       
> > > >         const struct pxa2xx_spi_master *pdata = param;
> > > >         struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> > > >         
> > > >         dwc->request_line = fargs->req;
> > > >         dwc->src_master = 0;
> > > >         dwc->dst_master = 0;
> > > >         
> > > >         return 1;
> > > > }           
> > > 
> > > Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine
> > > driver.
> > 
> > I think you can improve this by putting the filter function (and a pointer
> > to it) into the pxa2xx_spi_master data provided by the PCI driver.
> 
> Indeed, that looks better. It still makes the PCI part of the driver
> dependent on a particular DMA engine driver but is certainly better than
> the core pxa2xx_spi driver.

Something like this?

Hock Leong / Chiaue Ee, are you able to check if this works on your BYT
machines?

diff --git a/drivers/spi/spi-pxa2xx-dma.c b/drivers/spi/spi-pxa2xx-dma.c
index c41ff148a2b4..5d46972ffc40 100644
--- a/drivers/spi/spi-pxa2xx-dma.c
+++ b/drivers/spi/spi-pxa2xx-dma.c
@@ -157,7 +157,6 @@ static struct dma_async_tx_descriptor *
 pxa2xx_spi_dma_prepare_one(struct driver_data *drv_data,
 			   enum dma_transfer_direction dir)
 {
-	struct pxa2xx_spi_master *pdata = drv_data->master_info;
 	struct chip_data *chip = drv_data->cur_chip;
 	enum dma_slave_buswidth width;
 	struct dma_slave_config cfg;
@@ -184,7 +183,6 @@ pxa2xx_spi_dma_prepare_one(struct driver_data *drv_data,
 		cfg.dst_addr = drv_data->ssdr_physical;
 		cfg.dst_addr_width = width;
 		cfg.dst_maxburst = chip->dma_burst_size;
-		cfg.slave_id = pdata->tx_slave_id;
 
 		sgt = &drv_data->tx_sgt;
 		nents = drv_data->tx_nents;
@@ -193,7 +191,6 @@ pxa2xx_spi_dma_prepare_one(struct driver_data *drv_data,
 		cfg.src_addr = drv_data->ssdr_physical;
 		cfg.src_addr_width = width;
 		cfg.src_maxburst = chip->dma_burst_size;
-		cfg.slave_id = pdata->rx_slave_id;
 
 		sgt = &drv_data->rx_sgt;
 		nents = drv_data->rx_nents;
@@ -210,14 +207,6 @@ pxa2xx_spi_dma_prepare_one(struct driver_data *drv_data,
 				       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
 }
 
-static bool pxa2xx_spi_dma_filter(struct dma_chan *chan, void *param)
-{
-	const struct pxa2xx_spi_master *pdata = param;
-
-	return chan->chan_id == pdata->tx_chan_id ||
-	       chan->chan_id == pdata->rx_chan_id;
-}
-
 bool pxa2xx_spi_dma_is_possible(size_t len)
 {
 	return len <= MAX_DMA_LEN;
@@ -321,12 +310,14 @@ int pxa2xx_spi_dma_setup(struct driver_data *drv_data)
 		return -ENOMEM;
 
 	drv_data->tx_chan = dma_request_slave_channel_compat(mask,
-				pxa2xx_spi_dma_filter, pdata, dev, "tx");
+				pdata->dma_filter, pdata->dma_filter_param,
+				dev, "tx");
 	if (!drv_data->tx_chan)
 		return -ENODEV;
 
 	drv_data->rx_chan = dma_request_slave_channel_compat(mask,
-				pxa2xx_spi_dma_filter, pdata, dev, "rx");
+				pdata->dma_filter, pdata->dma_filter_param,
+				dev, "rx");
 	if (!drv_data->rx_chan) {
 		dma_release_channel(drv_data->tx_chan);
 		drv_data->tx_chan = NULL;
diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index c1865c92ccb9..aee3aa9365cf 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -8,6 +8,9 @@
 #include <linux/module.h>
 #include <linux/spi/pxa2xx_spi.h>
 
+#include <linux/dmaengine.h>
+#include "../../drivers/dma/dw/regs.h"
+
 enum {
 	PORT_CE4100,
 	PORT_BYT,
@@ -23,6 +26,24 @@ struct pxa_spi_info {
 	int rx_chan_id;
 };
 
+static bool pxa2xx_spi_pci_dma_filter(struct dma_chan *chan, void *param)
+{
+	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
+	const struct pxa_spi_info *info = param;
+
+	if (chan->chan_id == info->tx_chan_id)
+		dwc->request_line = info->tx_slave_id;
+	else if (chan->chan_id == info->rx_chan_id)
+		dwc->request_line = info->rx_slave_id;
+	else
+		return false;
+
+	dwc->src_master = 0;
+	dwc->dst_master = 0;
+
+	return true;
+}
+
 static struct pxa_spi_info spi_info_configs[] = {
 	[PORT_CE4100] = {
 		.type = PXA25x_SSP,
@@ -67,10 +88,9 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 	memset(&spi_pdata, 0, sizeof(spi_pdata));
 	spi_pdata.num_chipselect = (c->num_chipselect > 0) ?
 					c->num_chipselect : dev->devfn;
-	spi_pdata.tx_slave_id = c->tx_slave_id;
-	spi_pdata.tx_chan_id = c->tx_chan_id;
-	spi_pdata.rx_slave_id = c->rx_slave_id;
-	spi_pdata.rx_chan_id = c->rx_chan_id;
+
+	spi_pdata.dma_filter = pxa2xx_spi_pci_dma_filter;
+	spi_pdata.dma_filter_param = c;
 	spi_pdata.enable_dma = c->rx_slave_id >= 0 && c->tx_slave_id >= 0;
 
 	ssp = &spi_pdata.ssp;
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index fe792106bdc5..256c0abbddd2 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -1062,8 +1062,6 @@ pxa2xx_spi_acpi_get_pdata(struct platform_device *pdev)
 
 	pdata->num_chipselect = 1;
 	pdata->enable_dma = true;
-	pdata->tx_chan_id = -1;
-	pdata->rx_chan_id = -1;
 
 	return pdata;
 }
diff --git a/include/linux/spi/pxa2xx_spi.h b/include/linux/spi/pxa2xx_spi.h
index 82d5111cd0c2..2eb00920c169 100644
--- a/include/linux/spi/pxa2xx_spi.h
+++ b/include/linux/spi/pxa2xx_spi.h
@@ -23,6 +23,8 @@
 #define PXA2XX_CS_ASSERT (0x01)
 #define PXA2XX_CS_DEASSERT (0x02)
 
+struct dma_chan;
+
 /* device.platform_data for SSP controller devices */
 struct pxa2xx_spi_master {
 	u32 clock_enable;
@@ -30,10 +32,8 @@ struct pxa2xx_spi_master {
 	u8 enable_dma;
 
 	/* DMA engine specific config */
-	int rx_chan_id;
-	int tx_chan_id;
-	int rx_slave_id;
-	int tx_slave_id;
+	bool (*dma_filter)(struct dma_chan *chan, void *param);
+	void *dma_filter_param;
 
 	/* For non-PXA arches */
 	struct ssp_device ssp;
-- 
2.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:
> On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > > > > All you need to do is change your filter function to take the
> > > > > slave id from pxa_spi_info and stick it in there, e.g.
> > > > > 
> > > > > static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
> > > > > {       
> > > > >         const struct pxa2xx_spi_master *pdata = param;
> > > > >         struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> > > > >         
> > > > >         dwc->request_line = fargs->req;
> > > > >         dwc->src_master = 0;
> > > > >         dwc->dst_master = 0;
> > > > >         
> > > > >         return 1;
> > > > > }           
> > > > 
> > > > Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine
> > > > driver.
> > > 
> > > I think you can improve this by putting the filter function (and a pointer
> > > to it) into the pxa2xx_spi_master data provided by the PCI driver.
> > 
> > Indeed, that looks better. It still makes the PCI part of the driver
> > dependent on a particular DMA engine driver but is certainly better than
> > the core pxa2xx_spi driver.
> 
> Something like this?
> 
> Hock Leong / Chiaue Ee, are you able to check if this works on your BYT
> machines?

I think I found a small bug, and one detail that could be improved.

> @@ -321,12 +310,14 @@ int pxa2xx_spi_dma_setup(struct driver_data *drv_data)
>  		return -ENOMEM;
>  
>  	drv_data->tx_chan = dma_request_slave_channel_compat(mask,
> -				pxa2xx_spi_dma_filter, pdata, dev, "tx");
> +				pdata->dma_filter, pdata->dma_filter_param,
> +				dev, "tx");
>  	if (!drv_data->tx_chan)
>  		return -ENODEV;

If you change this part to pass '(void *)info->tx_slave_id' rather than
pdata->dma_filter_param, you can simplify the next code:

> +static bool pxa2xx_spi_pci_dma_filter(struct dma_chan *chan, void *param)
> +{
> +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> +	const struct pxa_spi_info *info = param;
> +
> +	if (chan->chan_id == info->tx_chan_id)
> +		dwc->request_line = info->tx_slave_id;
> +	else if (chan->chan_id == info->rx_chan_id)
> +		dwc->request_line = info->rx_slave_id;
> +	else
> +		return false;
> +
> +	dwc->src_master = 0;
> +	dwc->dst_master = 0;
> +
> +	return true;
> +}

to disregard the tx_chan_id, which is really arbitrary as far as I can tell.

What I think you got wrong here (by following my bad advice) is the master
number. Looking at the code for dw_dma, I think src_master needs to be '1'
for your driver.

If we do the same change in the drivers/tty/serial/8250/8250_pci.c driver,
the dw_dma driver can be simplified a little by removing the special
case for the request line setting.

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 1af731b83b3f..674f69fe6662 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -37,24 +37,6 @@
  * support descriptor writeback.
  */
 
-static inline bool is_request_line_unset(struct dw_dma_chan *dwc)
-{
-	return dwc->request_line == (typeof(dwc->request_line))~0;
-}
-
-static inline void dwc_set_masters(struct dw_dma_chan *dwc)
-{
-	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
-	struct dw_dma_slave *dws = dwc->chan.private;
-	unsigned char mmax = dw->nr_masters - 1;
-
-	if (!is_request_line_unset(dwc))
-		return;
-
-	dwc->src_master = min_t(unsigned char, mmax, dwc_get_sms(dws));
-	dwc->dst_master = min_t(unsigned char, mmax, dwc_get_dms(dws));
-}
-
 #define DWC_DEFAULT_CTLLO(_chan) ({				\
 		struct dw_dma_chan *_dwc = to_dw_dma_chan(_chan);	\
 		struct dma_slave_config	*_sconfig = &_dwc->dma_sconfig;	\
@@ -967,10 +949,6 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
 	memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
 	dwc->direction = sconfig->direction;
 
-	/* Take the request line from slave_id member */
-	if (is_request_line_unset(dwc))
-		dwc->request_line = sconfig->slave_id;
-
 	convert_burst(&dwc->dma_sconfig.src_maxburst);
 	convert_burst(&dwc->dma_sconfig.dst_maxburst);
 
@@ -1123,8 +1101,6 @@ static int dwc_alloc_chan_resources(struct dma_chan *chan)
 	 * doesn't mean what you think it means), and status writeback.
 	 */
 
-	dwc_set_masters(dwc);
-
 	spin_lock_irqsave(&dwc->lock, flags);
 	i = dwc->descs_allocated;
 	while (dwc->descs_allocated < NR_DESCS_PER_CHANNEL) {

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Andy Shevchenko]

On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote:
> On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:
> > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:

[]

> > Something like this?

Arnd, this dependency to certain DMA driver looks really bad.

If we go that way, can we split that part to [another] module and make
it dependent to DW_DMAC?

Or shall we introduce a dmaengine type field in the platform data and
dynamically choose proper filter-whatever-function to get the channel?

> > Hock Leong / Chiaue Ee, are you able to check if this works on your BYT
> > machines?

> What I think you got wrong here (by following my bad advice) is the master
> number. Looking at the code for dw_dma, I think src_master needs to be '1'
> for your driver.

On some SoCs we have up to 4 masters. It's blurry for me how the SPI
should choose those masters. Currently it works fine, but I suspect
there are [might be] performance issues.

What about AVR32 case? We have to fix drivers as well there.

> the dw_dma driver can be simplified a little by removing the special
> case for the request line setting.

Yes, this part I like.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote:
> On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote:
> > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:
> > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> 
> []
> 
> > > Something like this?
> 
> Arnd, this dependency to certain DMA driver looks really bad.
> 
> If we go that way, can we split that part to [another] module and make
> it dependent to DW_DMAC?

I don't see what you gain from that. The PCI ID will tell you which DMA
engine is being used. The driver already hardcodes a slave_id based on
the PCI ID today, and the 

> Or shall we introduce a dmaengine type field in the platform data and
> dynamically choose proper filter-whatever-function to get the channel?

We already have an interface for this, in the form of
dma_request_slave_channel(), which takes a string identifier that
is used to look up all that information in either device tree or
ACPI. It wouldn't be unreasonable to add a third path in there
to handle hardcoded platform devices, but that's a lot of work.
Note that you still need to encode a reference to the dma engine
in some way to do this right. The current code (with or without Mika's
patch) will break as soon as you have multiple DMA engine devices.

The current plan I think is to convert all platforms to use DT
or ACPI so they get the right data from tables passed by the
platform. I'm a bit puzzled about why Intel wants to support the
non-ACPI non-DT case again. If we have to support this case anyway,
what good will ACPI do us on those platforms?

> > > Hock Leong / Chiaue Ee, are you able to check if this works on your BYT
> > > machines?
> 
> > What I think you got wrong here (by following my bad advice) is the master
> > number. Looking at the code for dw_dma, I think src_master needs to be '1'
> > for your driver.
> 
> On some SoCs we have up to 4 masters. It's blurry for me how the SPI
> should choose those masters. Currently it works fine, but I suspect
> there are [might be] performance issues.

I think it works because the dw-dma defaults to the values used by
the specific implementation in your hardware.

> What about AVR32 case? We have to fix drivers as well there.

which ones?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[One Thousand Gnomes]

> The current plan I think is to convert all platforms to use DT
> or ACPI so they get the right data from tables passed by the
> platform. I'm a bit puzzled about why Intel wants to support the
> non-ACPI non-DT case again. If we have to support this case anyway,
> what good will ACPI do us on those platforms?

Because some industries move very very slowly so still don't believe in
requiring ACPI or ACPI capable operating systems.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Andy Shevchenko]

On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote:
> On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote:
> > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote:
> > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:
> > > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> > > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > 
> > []
> > 
> > > > Something like this?
> > 
> > Arnd, this dependency to certain DMA driver looks really bad.
> > 
> > If we go that way, can we split that part to [another] module and make
> > it dependent to DW_DMAC?
> 
> I don't see what you gain from that. The PCI ID will tell you which DMA
> engine is being used. The driver already hardcodes a slave_id based on
> the PCI ID today, and the 

"...and the..."?

> 
> > Or shall we introduce a dmaengine type field in the platform data and
> > dynamically choose proper filter-whatever-function to get the channel?
> 
> We already have an interface for this, in the form of
> dma_request_slave_channel(), which takes a string identifier that
> is used to look up all that information in either device tree or
> ACPI. It wouldn't be unreasonable to add a third path in there
> to handle hardcoded platform devices, but that's a lot of work.
> Note that you still need to encode a reference to the dma engine
> in some way to do this right. The current code (with or without Mika's
> patch) will break as soon as you have multiple DMA engine devices.

What about to keep PCI case still valid? We can pass struct pci_dev (or
actual struct device) of DMA controller to filter proper device.

> The current plan I think is to convert all platforms to use DT
> or ACPI so they get the right data from tables passed by the
> platform.

Good to know the road map.

[]

> > > What I think you got wrong here (by following my bad advice) is the master
> > > number. Looking at the code for dw_dma, I think src_master needs to be '1'
> > > for your driver.
> > 
> > On some SoCs we have up to 4 masters. It's blurry for me how the SPI
> > should choose those masters. Currently it works fine, but I suspect
> > there are [might be] performance issues.
> 
> I think it works because the dw-dma defaults to the values used by
> the specific implementation in your hardware.



> > What about AVR32 case? We have to fix drivers as well there.

> which ones?

arch/avr32/mach-at32ap/at32ap700x.c:1332:at32_add_device_mci

It seems opaque for me if it's used anywhere.


-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Shevchenko, Andriy]

On Mon, 2014-07-28 at 14:06 +0300, Andy Shevchenko wrote:
> On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote:
> > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote:

[]

> > > > What I think you got wrong here (by following my bad advice) is the master
> > > > number. Looking at the code for dw_dma, I think src_master needs to be '1'
> > > > for your driver.
> > > 
> > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI
> > > should choose those masters. Currently it works fine, but I suspect
> > > there are [might be] performance issues.
> > 
> > I think it works because the dw-dma defaults to the values used by
> > the specific implementation in your hardware.

[Missed in previous email]

Actually the defaults came from original driver for AVR32 case.

Regarding to DW DMA databook the AHB masters could be used each by any
channel, though it depends on what AHB layer is bound to (and
corresponding peripheral device).

Thus, like I said we might have the [minor] performance issues if we
use, for example, two out of four masters.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Monday 28 July 2014 14:06:20 Andy Shevchenko wrote:
> On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote:
> > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote:
> > > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote:
> > > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:
> > > > > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> > > > > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > > > > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > > 
> > > []
> > > 
> > > > > Something like this?
> > > 
> > > Arnd, this dependency to certain DMA driver looks really bad.
> > > 
> > > If we go that way, can we split that part to [another] module and make
> > > it dependent to DW_DMAC?
> > 
> > I don't see what you gain from that. The PC ID will tell you which DMA
> > engine is being used. The driver already hardcodes a slave_id based on
> > the PCI ID today, and the 
> 
> "...and the..."?

Sorry for missing that.

the slave ID only makes sense in combination with the master device
it is used in, so the dependency exists already.

> > > Or shall we introduce a dmaengine type field in the platform data and
> > > dynamically choose proper filter-whatever-function to get the channel?
> > 
> > We already have an interface for this, in the form of
> > dma_request_slave_channel(), which takes a string identifier that
> > is used to look up all that information in either device tree or
> > ACPI. It wouldn't be unreasonable to add a third path in there
> > to handle hardcoded platform devices, but that's a lot of work.
> > Note that you still need to encode a reference to the dma engine
> > in some way to do this right. The current code (with or without Mika's
> > patch) will break as soon as you have multiple DMA engine devices.
> 
> What about to keep PCI case still valid? We can pass struct pci_dev (or
> actual struct device) of DMA controller to filter proper device.

Yes, that would be best. IIRC you have a single PCI device that instantiates
both the SPI and the DMA engine as subdevices (I haven't looked at the
code again), so that information would be readily available.
 
> > > > What I think you got wrong here (by following my bad advice) is the master
> > > > number. Looking at the code for dw_dma, I think src_master needs to be '1'
> > > > for your driver.
> > > 
> > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI
> > > should choose those masters. Currently it works fine, but I suspect
> > > there are [might be] performance issues.
> > 
> > I think it works because the dw-dma defaults to the values used by
> > the specific implementation in your hardware.
> 
> 
> 
> > > What about AVR32 case? We have to fix drivers as well there.
> 
> > which ones?
> 
> arch/avr32/mach-at32ap/at32ap700x.c:1332:at32_add_device_mci
> 
> It seems opaque for me if it's used anywhere.

It seems correct to me. This is the filter function used by atmel_mci:

static bool atmci_filter(struct dma_chan *chan, void *pdata)
{
        struct mci_platform_data *sl_pdata = pdata;
        struct mci_dma_data *sl;

        if (!sl_pdata)
                return false;

        sl = sl_pdata->dma_slave;
        if (sl && find_slave_dev(sl) == chan->device->dev) {
                chan->private = slave_data_ptr(sl);
                return true;
        } else {
                return false;
        }
}

It sets the dw_dma_slave information from  slave_data_ptr(sl) here,
and does not attempt to set a slave_id at all, the slave_config
call only sets the required fields.

Do you still see a problem here?

> Actually the defaults came from original driver for AVR32 case.
> 
> Regarding to DW DMA databook the AHB masters could be used each by any
> channel, though it depends on what AHB layer is bound to (and
> corresponding peripheral device).
> 
> Thus, like I said we might have the [minor] performance issues if we
> use, for example, two out of four masters.

I don't see how: We have four cases that I am aware of, and all seem
to handle this right:

a) request line and masters are all configured from device-tree.
b) request line is configured from ACPI, masters are autoconfigured
c) request line and masters are all configured from avr32/arm32
   platform data.
d) request line and masters are all configured from Bay Trail
   PCI ID (Implemented by Mika's patch)

The ACPI case could be extended if we ever get an ACPI based system
that has more than two masters.
In the other cases, the same source of information that configures
the request line also configures the masters, so it's up to the
system integration to pick the best setting.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Andy Shevchenko]

On Mon, 2014-07-28 at 14:47 +0200, Arnd Bergmann wrote:
> On Monday 28 July 2014 14:06:20 Andy Shevchenko wrote:
> > On Fri, 2014-07-25 at 17:55 +0200, Arnd Bergmann wrote:
> > > On Friday 25 July 2014 13:45:47 Andy Shevchenko wrote:
> > > > On Fri, 2014-07-25 at 12:19 +0200, Arnd Bergmann wrote:
> > > > > On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:

> > > > Arnd, this dependency to certain DMA driver looks really bad.
> > > > 
> > > > If we go that way, can we split that part to [another] module and make
> > > > it dependent to DW_DMAC?
> > > 
> > > I don't see what you gain from that. The PC ID will tell you which DMA
> > > engine is being used. The driver already hardcodes a slave_id based on
> > > the PCI ID today, and the 
> > 
> > "...and the..."?
> 
> Sorry for missing that.
> 
> the slave ID only makes sense in combination with the master device
> it is used in, so the dependency exists already.

Yes, this is plain data that could be nicely fit in the pci driver.

> > > > > What I think you got wrong here (by following my bad advice) is the master
> > > > > number. Looking at the code for dw_dma, I think src_master needs to be '1'
> > > > > for your driver.
> > > > 
> > > > On some SoCs we have up to 4 masters. It's blurry for me how the SPI
> > > > should choose those masters. Currently it works fine, but I suspect
> > > > there are [might be] performance issues.
> > > 
> > > I think it works because the dw-dma defaults to the values used by
> > > the specific implementation in your hardware.
> > 
> > 
> > 
> > > > What about AVR32 case? We have to fix drivers as well there.
> > 
> > > which ones?
> > 
> > arch/avr32/mach-at32ap/at32ap700x.c:1332:at32_add_device_mci
> > 
> > It seems opaque for me if it's used anywhere.
> 
> It seems correct to me. This is the filter function used by atmel_mci:
> 
> static bool atmci_filter(struct dma_chan *chan, void *pdata)
> {
>         struct mci_platform_data *sl_pdata = pdata;
>         struct mci_dma_data *sl;
> 
>         if (!sl_pdata)
>                 return false;
> 
>         sl = sl_pdata->dma_slave;
>         if (sl && find_slave_dev(sl) == chan->device->dev) {
>                 chan->private = slave_data_ptr(sl);
>                 return true;
>         } else {
>                 return false;
>         }
> }
> 
> It sets the dw_dma_slave information from  slave_data_ptr(sl) here,
> and does not attempt to set a slave_id at all, the slave_config
> call only sets the required fields.
> 
> Do you still see a problem here?

Sorry, I was talking about master defaults regarding to your patch that
proposes to remove that part from dw_dmac. I mean it should be set in
function mentioned early.

> > Actually the defaults came from original driver for AVR32 case.
> > 
> > Regarding to DW DMA databook the AHB masters could be used each by any
> > channel, though it depends on what AHB layer is bound to (and
> > corresponding peripheral device).
> > 
> > Thus, like I said we might have the [minor] performance issues if we
> > use, for example, two out of four masters.
> 
> I don't see how: We have four cases that I am aware of, and all seem
> to handle this right:
> 
> a) request line and masters are all configured from device-tree.
> b) request line is configured from ACPI, masters are autoconfigured

They are just defaults. Say, hardcoded. We just get master bus width and
number of masters we have, but we don't use them.

> c) request line and masters are all configured from avr32/arm32
>    platform data.
> d) request line and masters are all configured from Bay Trail
>    PCI ID (Implemented by Mika's patch)

Actually here I just recall the case when we might have different number
of masters and use the same IP block [of SPI controller], how could you
distinguish that case? How to provide proper masters?

It's luckily not a problem right now, but still potential one in some
cases.

> The ACPI case could be extended if we ever get an ACPI based system
> that has more than two masters.
> In the other cases, the same source of information that configures
> the request line also configures the masters, so it's up to the
> system integration to pick the best setting.

Yeah, ACPI / DT cases work / will work fine, since we can get any
information we need from there.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Arnd Bergmann]

On Monday 28 July 2014 16:34:04 Andy Shevchenko wrote:
> On Mon, 2014-07-28 at 14:47 +0200, Arnd Bergmann wrote:
> > On Monday 28 July 2014 14:06:20 Andy Shevchenko wrote:
> > static bool atmci_filter(struct dma_chan *chan, void *pdata)
> > {
> >         struct mci_platform_data *sl_pdata = pdata;
> >         struct mci_dma_data *sl;
> > 
> >         if (!sl_pdata)
> >                 return false;
> > 
> >         sl = sl_pdata->dma_slave;
> >         if (sl && find_slave_dev(sl) == chan->device->dev) {
> >                 chan->private = slave_data_ptr(sl);
> >                 return true;
> >         } else {
> >                 return false;
> >         }
> > }
> > 
> > It sets the dw_dma_slave information from  slave_data_ptr(sl) here,
> > and does not attempt to set a slave_id at all, the slave_config
> > call only sets the required fields.
> > 
> > Do you still see a problem here?
> 
> Sorry, I was talking about master defaults regarding to your patch that
> proposes to remove that part from dw_dmac. I mean it should be set in
> function mentioned early.

Ok. So atmci_filter is still correct because chan->private contains
a pointer to a structure with both request and master settings, right?

> > c) request line and masters are all configured from avr32/arm32
> >    platform data.
> > d) request line and masters are all configured from Bay Trail
> >    PCI ID (Implemented by Mika's patch)
> 
> Actually here I just recall the case when we might have different number
> of masters and use the same IP block [of SPI controller], how could you
> distinguish that case? How to provide proper masters?
> 
> It's luckily not a problem right now, but still potential one in some
> cases.

Do you mean case d? I think for each PCI ID, you will have to add the
correct settings to some table that is used in the filter function.

Are there any cases you know where we can't hardcode them?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/pxa2xx-pci: Enable DMA binding through device name

[Mika Westerberg]

On Fri, Jul 25, 2014 at 12:19:02PM +0200, Arnd Bergmann wrote:
> On Friday 25 July 2014 12:55:59 Mika Westerberg wrote:
> > On Fri, Jul 25, 2014 at 12:07:14PM +0300, Mika Westerberg wrote:
> > > On Fri, Jul 25, 2014 at 10:38:36AM +0200, Arnd Bergmann wrote:
> > > > On Friday 25 July 2014 11:22:49 Mika Westerberg wrote:
> > > > > > All you need to do is change your filter function to take the
> > > > > > slave id from pxa_spi_info and stick it in there, e.g.
> > > > > > 
> > > > > > static bool pxa2xx_spi_dw_dma_filter(struct dma_chan *chan, void *param)
> > > > > > {       
> > > > > >         const struct pxa2xx_spi_master *pdata = param;
> > > > > >         struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> > > > > >         
> > > > > >         dwc->request_line = fargs->req;
> > > > > >         dwc->src_master = 0;
> > > > > >         dwc->dst_master = 0;
> > > > > >         
> > > > > >         return 1;
> > > > > > }           
> > > > > 
> > > > > Oh man. That makes pxa2xx_spi dependent on a certain specific DMA engine
> > > > > driver.
> > > > 
> > > > I think you can improve this by putting the filter function (and a pointer
> > > > to it) into the pxa2xx_spi_master data provided by the PCI driver.
> > > 
> > > Indeed, that looks better. It still makes the PCI part of the driver
> > > dependent on a particular DMA engine driver but is certainly better than
> > > the core pxa2xx_spi driver.
> > 
> > Something like this?
> > 
> > Hock Leong / Chiaue Ee, are you able to check if this works on your BYT
> > machines?
> 
> I think I found a small bug, and one detail that could be improved.
> 
> > @@ -321,12 +310,14 @@ int pxa2xx_spi_dma_setup(struct driver_data *drv_data)
> >  		return -ENOMEM;
> >  
> >  	drv_data->tx_chan = dma_request_slave_channel_compat(mask,
> > -				pxa2xx_spi_dma_filter, pdata, dev, "tx");
> > +				pdata->dma_filter, pdata->dma_filter_param,
> > +				dev, "tx");
> >  	if (!drv_data->tx_chan)
> >  		return -ENODEV;
> 
> If you change this part to pass '(void *)info->tx_slave_id' rather than
> pdata->dma_filter_param, you can simplify the next code:

OK

> > +static bool pxa2xx_spi_pci_dma_filter(struct dma_chan *chan, void *param)
> > +{
> > +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> > +	const struct pxa_spi_info *info = param;
> > +
> > +	if (chan->chan_id == info->tx_chan_id)
> > +		dwc->request_line = info->tx_slave_id;
> > +	else if (chan->chan_id == info->rx_chan_id)
> > +		dwc->request_line = info->rx_slave_id;
> > +	else
> > +		return false;
> > +
> > +	dwc->src_master = 0;
> > +	dwc->dst_master = 0;
> > +
> > +	return true;
> > +}
> 
> to disregard the tx_chan_id, which is really arbitrary as far as I can tell.
> 
> What I think you got wrong here (by following my bad advice) is the master
> number. Looking at the code for dw_dma, I think src_master needs to be '1'
> for your driver.

Right, I just copied your example. I'll change it to '1' in the next
revision.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html