| Message ID | 1406196111-22861-1-git-send-email-hock.leong.kweh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2014-07-24 at 18:01 +0800, 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. 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@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;
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;