| Message ID | 1359410300-26113-3-git-send-email-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 28, 2013 at 09:58:17PM +0000, Arnd Bergmann wrote: > With the new OF DMA binding, it is possible to completely avoid the > need for platform_data for configuring a DMA channel. In cases where the > platform has already been converted, calling dma_request_slave_channel > should get all the necessary information from the device tree. > > Like the patch that converts the dw_dma controller, this is completely > untested and is looking for someone to try it out. Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> I've no ability to test this but it looks good from a code point of view.
On Mon, 2013-01-28 at 21:58 +0000, Arnd Bergmann wrote: > With the new OF DMA binding, it is possible to completely avoid the > need for platform_data for configuring a DMA channel. In cases where the > platform has already been converted, calling dma_request_slave_channel > should get all the necessary information from the device tree. > > Like the patch that converts the dw_dma controller, this is completely > untested and is looking for someone to try it out. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> > Cc: spi-devel-general@lists.sourceforge.net > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Vinod Koul <vinod.koul@linux.intel.com> > Cc: devicetree-discuss@lists.ozlabs.org > Cc: linux-arm-kernel@lists.infradead.org > --- > .../devicetree/bindings/spi/spi_pl022.txt | 36 ++++++++++++++++++ > drivers/spi/spi-pl022.c | 43 +++++++++++++++++++++- > 2 files changed, 77 insertions(+), 2 deletions(-) > > --- a/drivers/spi/spi-pl022.c > +++ b/drivers/spi/spi-pl022.c > @@ -1139,6 +1139,35 @@ err_no_rxchan: > return -ENODEV; > } > > +static int pl022_dma_autoprobe(struct pl022 *pl022) > +{ > + struct device *dev = &pl022->adev->dev; > + > + /* automatically configure DMA channels from platform, normally using DT */ > + pl022->dma_rx_channel = dma_request_slave_channel(dev, "rx"); > + if (!pl022->dma_rx_channel) > + goto err_no_rxchan; > + > + pl022->dma_tx_channel = dma_request_slave_channel(dev, "tx"); > + if (!pl022->dma_tx_channel) > + goto err_no_txchan; > + > + pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); Where this memory will be freed? In dependence of the answer could you consider to use devm_kmalloc or __get_free_page? > + if (!pl022->dummypage) > + goto err_no_dummypage; > + > + return 0; > + > +err_no_dummypage: > + dma_release_channel(pl022->dma_tx_channel); > + pl022->dma_tx_channel = NULL; > +err_no_txchan: > + dma_release_channel(pl022->dma_rx_channel); > + pl022->dma_rx_channel = NULL; > +err_no_rxchan: > + return -ENODEV; > +} > + > static void terminate_dma(struct pl022 *pl022) > { > struct dma_chan *rxchan = pl022->dma_rx_channel; > @@ -1167,6 +1196,11 @@ static inline int configure_dma(struct pl022 *pl022) > return -ENODEV; > } > > +static inline int pl022_dma_autoprobe(struct pl022 *pl022) > +{ > + return 0; > +} > + > static inline int pl022_dma_probe(struct pl022 *pl022) > { > return 0; > @@ -2226,8 +2260,13 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id) > goto err_no_irq; > } > > - /* Get DMA channels */ > - if (platform_info->enable_dma) { > + /* Get DMA channels, try autoconfiguration first */ > + status = pl022_dma_autoprobe(pl022); > + > + /* If that failed, use channels from platform_info */ > + if (status == 0) > + platform_info->enable_dma = 1; > + else if (platform_info->enable_dma) { > status = pl022_dma_probe(pl022); > if (status != 0) > platform_info->enable_dma = 0;
On Tuesday 29 January 2013, Andy Shevchenko wrote: > > +static int pl022_dma_autoprobe(struct pl022 *pl022) > > +{ > > + struct device *dev = &pl022->adev->dev; > > + > > + /* automatically configure DMA channels from platform, normally using DT */ > > + pl022->dma_rx_channel = dma_request_slave_channel(dev, "rx"); > > + if (!pl022->dma_rx_channel) > > + goto err_no_rxchan; > > + > > + pl022->dma_tx_channel = dma_request_slave_channel(dev, "tx"); > > + if (!pl022->dma_tx_channel) > > + goto err_no_txchan; > > + > > + pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); > > Where this memory will be freed? > In dependence of the answer could you consider to use > devm_kmalloc or __get_free_page? There is another function like this called pl022_dma_probe() that has the same allocation, and it gets freed in the same place. It's probably worth changing this into something different, but I felt that it didn't belong into this patch. I was also not sure if the best option would be dmam_alloc_coherent, dev_kzalloc, or __get_free_page. Arnd
On Tue, Jan 29, 2013 at 2:13 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 29 January 2013, Andy Shevchenko wrote: >> > + pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); >> >> Where this memory will be freed? >> In dependence of the answer could you consider to use >> devm_kmalloc or __get_free_page? > > There is another function like this called pl022_dma_probe() > that has the same allocation, and it gets freed in the same place. > > It's probably worth changing this into something different, but > I felt that it didn't belong into this patch. I was also not > sure if the best option would be dmam_alloc_coherent, dev_kzalloc, > or __get_free_page. Actually I once read about a feature where the kernel provides a static page full of zeroes or something like this, that would be ideal to use in cases like this, then all of this dummy page allocation and freeing can be deleted. Yours, Linus Walleij
On Thursday 07 February 2013, Linus Walleij wrote: > On Tue, Jan 29, 2013 at 2:13 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 29 January 2013, Andy Shevchenko wrote: > > >> > + pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); > >> > >> Where this memory will be freed? > >> In dependence of the answer could you consider to use > >> devm_kmalloc or __get_free_page? > > > > There is another function like this called pl022_dma_probe() > > that has the same allocation, and it gets freed in the same place. > > > > It's probably worth changing this into something different, but > > I felt that it didn't belong into this patch. I was also not > > sure if the best option would be dmam_alloc_coherent, dev_kzalloc, > > or __get_free_page. > > Actually I once read about a feature where the kernel provides > a static page full of zeroes or something like this, that would be > ideal to use in cases like this, then all of this dummy page > allocation and freeing can be deleted. You mean empty_zero_page? That only works if this page is read-only from the perspective of the DMA controller, but then it would be a good fit, yes. Arnd
On Thu, Feb 7, 2013 at 8:42 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 07 February 2013, Linus Walleij wrote: >> Actually I once read about a feature where the kernel provides >> a static page full of zeroes or something like this, that would be >> ideal to use in cases like this, then all of this dummy page >> allocation and freeing can be deleted. > > You mean empty_zero_page? That only works if this page is > read-only from the perspective of the DMA controller, but > then it would be a good fit, yes. That's actually how it's used. SPI is symmetric, and in the DMA case we're not poking data into the buffers from the CPU so the controller need something - anything - to stream to the block. If we can use that page we'll even save a few remaps. Yours, Linus Walleij
On Thursday 07 February 2013 21:19:04 Linus Walleij wrote: > On Thu, Feb 7, 2013 at 8:42 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Thursday 07 February 2013, Linus Walleij wrote: > > >> Actually I once read about a feature where the kernel provides > >> a static page full of zeroes or something like this, that would be > >> ideal to use in cases like this, then all of this dummy page > >> allocation and freeing can be deleted. > > > > You mean empty_zero_page? That only works if this page is > > read-only from the perspective of the DMA controller, but > > then it would be a good fit, yes. > > That's actually how it's used. > > SPI is symmetric, and in the DMA case we're not poking > data into the buffers from the CPU so the controller need > something - anything - to stream to the block. > > If we can use that page we'll even save a few remaps. I'm slightly worried about the caching effects though. The idea of the empty-zero page is that all user processes get it when they read a page before they write to it, so the data in it can essentially always be cache-hot. If we do DMA from that page to a device what would be the overhead of flushing the (clean) cache lines? Arnd
On Thu, Feb 07, 2013 at 07:29:17PM +0100, Linus Walleij wrote: > Actually I once read about a feature where the kernel provides > a static page full of zeroes or something like this, that would be > ideal to use in cases like this, then all of this dummy page > allocation and freeing can be deleted. I think you're thinking of empty_zero_page which is used to provide the initial BSS pages for user apps.
On Thu, Feb 07, 2013 at 10:15:48PM +0100, Arnd Bergmann wrote: > On Thursday 07 February 2013 21:19:04 Linus Walleij wrote: > > On Thu, Feb 7, 2013 at 8:42 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > > On Thursday 07 February 2013, Linus Walleij wrote: > > > > >> Actually I once read about a feature where the kernel provides > > >> a static page full of zeroes or something like this, that would be > > >> ideal to use in cases like this, then all of this dummy page > > >> allocation and freeing can be deleted. > > > > > > You mean empty_zero_page? That only works if this page is > > > read-only from the perspective of the DMA controller, but > > > then it would be a good fit, yes. > > > > That's actually how it's used. > > > > SPI is symmetric, and in the DMA case we're not poking > > data into the buffers from the CPU so the controller need > > something - anything - to stream to the block. > > > > If we can use that page we'll even save a few remaps. > > I'm slightly worried about the caching effects though. The > idea of the empty-zero page is that all user processes get > it when they read a page before they write to it, so the > data in it can essentially always be cache-hot. > > If we do DMA from that page to a device what would be the > overhead of flushing the (clean) cache lines? If it's DMA _to_ a device, then we will only ever clean the lines prior to a transfer, never invalidate them. So that's not really a concern. (There better not be any dirty cache lines associated with the empty zero page either.)
On Friday 08 February 2013 16:22:48 Russell King - ARM Linux wrote: > If it's DMA _to_ a device, then we will only ever clean the lines prior to > a transfer, never invalidate them. So that's not really a concern. (There > better not be any dirty cache lines associated with the empty zero page > either.) Right, makes sense. I thought I had read about a CPU that could not flush a cache line without also invalidating it, but that must have been something other than ARM, or maybe I'm misremembering it. Arnd
On Fri, Feb 8, 2013 at 5:28 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 08 February 2013 16:22:48 Russell King - ARM Linux wrote: >> If it's DMA _to_ a device, then we will only ever clean the lines prior to >> a transfer, never invalidate them. So that's not really a concern. (There >> better not be any dirty cache lines associated with the empty zero page >> either.) > > Right, makes sense. I thought I had read about a CPU that > could not flush a cache line without also invalidating > it, but that must have been something other than ARM, > or maybe I'm misremembering it. I don't think it matters one bit. The page can contain a bitmap of Donald Duck or zero FWIW. It's just that the DMA controller just neeeds to read *something* that does not cause a bus stall. It's due to the syncronous nature of the SPI protocol, to get something out you need to put something in. So when reading, this is a way to feed in some junk. So this goes on my TODO... Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt index f158fd3..22ed679 100644 --- a/Documentation/devicetree/bindings/spi/spi_pl022.txt +++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt @@ -16,6 +16,11 @@ Optional properties: device will be suspended immediately - pl022,rt : indicates the controller should run the message pump with realtime priority to minimise the transfer latency on the bus (boolean) +- dmas : Two or more DMA channel specifiers following the convention outlined + in bindings/dma/dma.txt +- dma-names: Names for the dma channels, if present. There must be at + least one channel named "tx" for transmit and named "rx" for + receive. SPI slave nodes must be children of the SPI master node and can @@ -32,3 +37,34 @@ contain the following properties. - pl022,wait-state : Microwire interface: Wait state - pl022,duplex : Microwire interface: Full/Half duplex + +Example: + + spi@e0100000 { + compatible = "arm,pl022", "arm,primecell"; + reg = <0xe0100000 0x1000>; + #address-cells = <1>; + #size-cells = <0>; + interrupts = <0 31 0x4>; + dmas = <&dma-controller 23 1>, + <&dma-controller 24 0>; + dma-names = "rx", "tx"; + + m25p80@1 { + compatible = "st,m25p80"; + reg = <1>; + spi-max-frequency = <12000000>; + spi-cpol; + spi-cpha; + pl022,hierarchy = <0>; + pl022,interface = <0>; + pl022,slave-tx-disable; + pl022,com-mode = <0x2>; + pl022,rx-level-trig = <0>; + pl022,tx-level-trig = <0>; + pl022,ctrl-len = <0x11>; + pl022,wait-state = <0>; + pl022,duplex = <0>; + }; + }; + diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index b0fe393..371cc66f 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -1139,6 +1139,35 @@ err_no_rxchan: return -ENODEV; } +static int pl022_dma_autoprobe(struct pl022 *pl022) +{ + struct device *dev = &pl022->adev->dev; + + /* automatically configure DMA channels from platform, normally using DT */ + pl022->dma_rx_channel = dma_request_slave_channel(dev, "rx"); + if (!pl022->dma_rx_channel) + goto err_no_rxchan; + + pl022->dma_tx_channel = dma_request_slave_channel(dev, "tx"); + if (!pl022->dma_tx_channel) + goto err_no_txchan; + + pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!pl022->dummypage) + goto err_no_dummypage; + + return 0; + +err_no_dummypage: + dma_release_channel(pl022->dma_tx_channel); + pl022->dma_tx_channel = NULL; +err_no_txchan: + dma_release_channel(pl022->dma_rx_channel); + pl022->dma_rx_channel = NULL; +err_no_rxchan: + return -ENODEV; +} + static void terminate_dma(struct pl022 *pl022) { struct dma_chan *rxchan = pl022->dma_rx_channel; @@ -1167,6 +1196,11 @@ static inline int configure_dma(struct pl022 *pl022) return -ENODEV; } +static inline int pl022_dma_autoprobe(struct pl022 *pl022) +{ + return 0; +} + static inline int pl022_dma_probe(struct pl022 *pl022) { return 0; @@ -2226,8 +2260,13 @@ static int pl022_probe(struct amba_device *adev, const struct amba_id *id) goto err_no_irq; } - /* Get DMA channels */ - if (platform_info->enable_dma) { + /* Get DMA channels, try autoconfiguration first */ + status = pl022_dma_autoprobe(pl022); + + /* If that failed, use channels from platform_info */ + if (status == 0) + platform_info->enable_dma = 1; + else if (platform_info->enable_dma) { status = pl022_dma_probe(pl022); if (status != 0) platform_info->enable_dma = 0;
With the new OF DMA binding, it is possible to completely avoid the need for platform_data for configuring a DMA channel. In cases where the platform has already been converted, calling dma_request_slave_channel should get all the necessary information from the device tree. Like the patch that converts the dw_dma controller, this is completely untested and is looking for someone to try it out. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: Grant Likely <grant.likely@secretlab.ca> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com> Cc: spi-devel-general@lists.sourceforge.net Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Vinod Koul <vinod.koul@linux.intel.com> Cc: devicetree-discuss@lists.ozlabs.org Cc: linux-arm-kernel@lists.infradead.org --- .../devicetree/bindings/spi/spi_pl022.txt | 36 ++++++++++++++++++ drivers/spi/spi-pl022.c | 43 +++++++++++++++++++++- 2 files changed, 77 insertions(+), 2 deletions(-)