dmaengine: add ACPI DMA helpers and use them in dw_dmac
Subsystems (4)
cover dmaengine: add ACPI DMA helpers and use them in dw_dmac
There is a patch series which introduces ACPI DMA helpers in similar way like we have for DeviceTree.
In addition it applies this to the first user, namely dw_dmac driver.
Andy Shevchenko (6):
dma: acpi-dma: introduce ACPI DMA helpers
dmaengine: call acpi_dma_request_slave_channel as well
dma: acpi-dma: parse CSRT to extract additional resources
dw_dmac: add ACPI support
ACPI / LPSS: return no error from register_device_clock in special
cases
ACPI / LPSS: add Lynxpoint DMA controller to the list
Documentation/acpi/enumeration.txt | 77 +++++++ drivers/acpi/Makefile | 1 - drivers/acpi/acpi_lpss.c | 11 + drivers/acpi/csrt.c | 159 ------------- drivers/acpi/internal.h | 1 - drivers/acpi/scan.c | 1 - drivers/clk/x86/clk-lpt.c | 2 +- drivers/dma/Kconfig | 4 + drivers/dma/Makefile | 1 + drivers/dma/acpi-dma.c | 447 +++++++++++++++++++++++++++++++++++++ drivers/dma/dmaengine.c | 6 + drivers/dma/dw_dmac.c | 68 ++++-- drivers/dma/dw_dmac_regs.h | 1 - include/linux/acpi_dma.h | 120 ++++++++++ 14 files changed, 717 insertions(+), 182 deletions(-) delete mode 100644 drivers/acpi/csrt.c create mode 100644 drivers/dma/acpi-dma.c create mode 100644 include/linux/acpi_dma.h
Signature · 1 line
1.8.2.rc0.22.gb3600c3
PATCH 1/6 dma: acpi-dma: introduce ACPI DMA helpers
There is a new generic API to get a DMA channel for a slave device (commit 9a6cecc8 "dmaengine: add helper function to request a slave DMA channel"). In similar fashion to the DT case (commit aa3da644 "of: Add generic device tree DMA helpers") we introduce helpers to the DMAC drivers which are enumerated by ACPI.
The proposed extension provides the following API calls:
acpi_dma_controller_register(), devm_acpi_dma_controller_register() acpi_dma_controller_free(), devm_acpi_dma_controller_free() acpi_dma_simple_xlate() acpi_dma_request_slave_chan_by_index() acpi_dma_request_slave_chan_by_name()
The first two should be used, for example, at probe() and remove() of the corresponding DMAC driver. At the register stage the DMAC driver supplies a custom xlate() function to translate a struct dma_spec into struct dma_chan.
Accordingly to the ACPI Fixed DMA resource specification the only two pieces of information the slave device has are the channel id and the request line (slave id). Those two are represented by struct dma_spec. The acpi_dma_request_slave_chan_by_index() provides access to the specifix FixedDMA resource by its index. Whereas dma_request_slave_channel() takes a string parameter to identify the DMA resources required by the slave device. To make a slave device driver work with both DeviceTree and ACPI enumeration a simple convention is established: "tx" corresponds to the index 0 and "rx" to the index 1. In case of robust configuration the slave device driver unfortunately needs to call acpi_dma_request_slave_chan_by_index() directly.
Additionally the patch provides "managed" version of the register/free pair i.e. devm_acpi_dma_controller_register() and devm_acpi_dma_controller_free(). Usually, the driver uses only devm_acpi_dma_controller_register().
--- Documentation/acpi/enumeration.txt | 77 ++++++++++ drivers/dma/Kconfig | 4 + drivers/dma/Makefile | 1 + drivers/dma/acpi-dma.c | 281 +++++++++++++++++++++++++++++++++++++ include/linux/acpi_dma.h | 116 +++++++++++++++ 5 files changed, 479 insertions(+) create mode 100644 drivers/dma/acpi-dma.c create mode 100644 include/linux/acpi_dma.h
Documentation/acpi/enumeration.txt +77 -0
@@ -66,6 +66,83 @@ the ACPI device explicitly to acpi_platform_device_ids list defined in drivers/acpi/acpi_platform.c. This limitation is only for the platform devices, SPI and I2C devices are created automatically as described below. +DMA support +~~~~~~~~~~~ +DMA controllers enumerated via ACPI should be registered in the system to +provide generic access to their resources. For example, a driver that would +like to be accessible to slave devices via generic API call +dma_request_slave_channel() must register itself at the end of the probe +function like this: + + err = devm_acpi_dma_controller_register(dev, xlate_func, dw); + /* Handle the error if it's not a case of !CONFIG_ACPI */ + +and implement custom xlate function if needed (usually acpi_dma_simple_xlate() +is enough) which converts the FixedDMA resource provided by struct +acpi_dma_spec into the corresponding DMA channel. A piece of code for that case +could look like: + + #ifdef CONFIG_ACPI + struct filter_args { + /* Provide necessary information for the filter_func */ + ... + }; + + static bool filter_func(struct dma_chan *chan, void *param) + { + /* Choose the proper channel */ + ... + } + + static struct dma_chan *xlate_func(struct acpi_dma_spec *dma_spec, + struct acpi_dma *adma) + { + dma_cap_mask_t cap; + struct filter_args args; + + /* Prepare arguments for filter_func */ + ... + return dma_request_channel(cap, filter_func, &args); + } + #else + static struct dma_chan *xlate_func(struct acpi_dma_spec *dma_spec, + struct acpi_dma *adma) + { + return NULL; + } + #endif + +dma_request_slave_channel() will call xlate_func() for each registered DMA +controller. In the xlate function the proper channel must be chosen based on +information in struct acpi_dma_spec and the properties of the controller +provided by struct acpi_dma. + +Clients must call dma_request_slave_channel() with the string parameter that +corresponds to a specific FixedDMA resource. By default "tx" means the first +entry of the FixedDMA resource array, "rx" means the second entry. The table +below shows a layout: + + Device (I2C0) + { + ... + Method (_CRS, 0, NotSerialized) + { + Name (DBUF, ResourceTemplate () + { + FixedDMA (0x0018, 0x0004, Width32bit, _Y48) + FixedDMA (0x0019, 0x0005, Width32bit, ) + }) + ... + } + } + +So, the FixedDMA with request line 0x0018 is "tx" and next one is "rx" in +this example. + +In robust cases the client unfortunately needs to call +acpi_dma_request_slave_chan_by_index() directly and therefore choose the +specific FixedDMA resource by its index. + SPI serial bus support ~~~~~~~~~~~~~~~~~~~~~~ Slave devices behind SPI bus have SpiSerialBus resource attached to them.
drivers/dma/Kconfig +4 -0
@@ -328,6 +328,10 @@ config DMA_ENGINE config DMA_VIRTUAL_CHANNELS tristate +config DMA_ACPI + def_bool y + depends on ACPI + config DMA_OF def_bool y depends on OF
drivers/dma/Makefile +1 -0
@@ -3,6 +3,7 @@ ccflags-$(CONFIG_DMADEVICES_VDEBUG) += -DVERBOSE_DEBUG obj-$(CONFIG_DMA_ENGINE) += dmaengine.o obj-$(CONFIG_DMA_VIRTUAL_CHANNELS) += virt-dma.o +obj-$(CONFIG_DMA_ACPI) += acpi-dma.o obj-$(CONFIG_DMA_OF) += of-dma.o obj-$(CONFIG_NET_DMA) += iovlock.o
drivers/dma/acpi-dma.c new file +281 -0
281 lines not on this page · raw message
include/linux/acpi_dma.h new file +116 -0
116 lines not on this page · raw message
Signature · 1 line
1.8.2.rc0.22.gb3600c3
Re: [PATCH 1/6] dma: acpi-dma: introduce ACPI DMA helpers
drivers/dma/acpi-dma.c line 189
Quoted 8 lines · Andy Shevchenko
+ * @dev: struct device to get DMA request from
+ * @index: index of FixedDMA descriptor for @dev
+ *
+ * Returns pointer to appropriate dma channel on success or NULL on error.
+ */drivers/dma/acpi-dma.c new file +3
-0+struct dma_chan *acpi_dma_request_slave_chan_by_index (struct device *dev, +size_t index) +{
So i think this will be called by client driver to request a channel right?
If so how does client find the device pointer for dma controller.
And index is a global one, right? What is it use ?
Quoted 32 lines
drivers/dma/acpi-dma.c new file +32
-0+struct acpi_dma_parser_data pdata; +struct acpi_dma_spec *dma_spec = &pdata.dma_spec; +struct list_head resource_list; +struct acpi_device *adev; +struct acpi_dma *adma; +struct dma_chan *chan; + +/* Check if the device was enumerated by ACPI */ +if (!dev || !ACPI_HANDLE (dev)) +return NULL ; + +if (acpi_bus_get_device (ACPI_HANDLE (dev), &adev)) +return NULL ; + +memset (&pdata,0 ,sizeof (pdata)); + pdata.index = index; + +/* Initial values for the request line and channel */ + dma_spec->chan_id =-1 ; + dma_spec->slave_id =-1 ; + +INIT_LIST_HEAD (&resource_list); +acpi_dev_get_resources (adev, &resource_list, + acpi_dma_parse_fixed_dma, &pdata); +acpi_dev_free_resource_list (&resource_list); + +if (dma_spec->slave_id <0 || dma_spec->chan_id <0 ) +return NULL ; + +mutex_lock (&acpi_dma_lock); + +list_for_each_entry (adma, &acpi_dma_list, dma_controllers) {
drivers/dma/acpi-dma.c line 231
Quoted 10 lines
drivers/dma/acpi-dma.c new file +10
-0+ dma_spec->dev = adma->dev; + chan = adma->acpi_dma_xlate (dma_spec, adma); +if (chan) { +mutex_unlock (&acpi_dma_lock); +return chan; + } + } + +mutex_unlock (&acpi_dma_lock); +return NULL ;
in this and error handling you are not doing anything different so why code duplication?
Quoted 17 lines
drivers/dma/acpi-dma.c new file +17
-0+} +EXPORT_SYMBOL_GPL (acpi_dma_request_slave_chan_by_index); + +/** +* acpi_dma_request_slave_chan_by_name - Get the DMA slave channel +* @dev: struct device to get DMA request from +* @name: represents corresponding FixedDMA descriptor for @dev +* +* In order to support both Device Tree and ACPI in a single driver we +* translate the names "tx" and "rx" here based on the most common case where +* the first FixedDMA descriptor is TX and second is RX. +* +* Returns pointer to appropriate dma channel on success or NULL on error. +*/ +struct dma_chan *acpi_dma_request_slave_chan_by_name (struct device *dev, +const char *name) +{
drivers/dma/acpi-dma.c line 258
Quoted 10 lines
drivers/dma/acpi-dma.c new file +10
-0+size_t index; + +if (!strcmp (name,"tx" )) + index =0 ; +else if (!strcmp (name,"rx" )) + index =1 ; +else +return NULL ; + +return acpi_dma_request_slave_chan_by_index (dev, index);
are you going to a have a different descriptor for tx and rx? Is index only for tx = 0 and rx =1 always. How would a controller be represented which has 8 channels, each channel can do DMA in either direction
Quoted 6 lines
drivers/dma/acpi-dma.c new file +6
-0+} +EXPORT_SYMBOL_GPL (acpi_dma_request_slave_chan_by_name); + +/** +* acpi_dma_simple_xlate - Simple ACPI DMA engine translation helper +* @dma_spec: pointer to ACPI DMA specifier
drivers/dma/acpi-dma.c line 274
Quoted 10 lines
drivers/dma/acpi-dma.c new file +10
-0+* @adma: pointer to ACPI DMA controller data +* +* A simple translation function for ACPI based devices. Passes &struct +* dma_spec to the DMA controller driver provided filter function. Returns +* pointer to the channel if found or %NULL otherwise. +*/ +struct dma_chan *acpi_dma_simple_xlate (struct acpi_dma_spec *dma_spec, +struct acpi_dma *adma) +{ +struct acpi_dma_filter_info *info = adma->data;
what is the purpose of filter here?
Quoted 35 lines
drivers/dma/acpi-dma.c new file +7
-0+ +if (!info || !info->filter_fn) +return NULL ; + +return dma_request_channel (info->dma_cap, info->filter_fn, dma_spec); +} +EXPORT_SYMBOL_GPL (acpi_dma_simple_xlate);include/linux/acpi_dma.h new file +22
-0@@ -0,0 +1,116 @@ +/* +* ACPI helpers for DMA request / controller +* +* Based on of_dma.h +* +* Copyright (C) 2013, Intel Corporation +* Author: Andy Shevchenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#ifndef __LINUX_ACPI_DMA_H +#define __LINUX_ACPI_DMA_H + +#include <linux/list.h> +#include <linux/device.h> +#include <linux/dmaengine.h> + +/** +* struct acpi_dma_spec - slave device DMA resources
include/linux/acpi_dma.h line 32
Quoted 10 lines
include/linux/acpi_dma.h new file +10
-0+* @chan_id: channel unique id +* @slave_id: request line unique id +* @dev: struct device of the DMA controller to be used in the filter +* function +*/ +struct acpi_dma_spec { +int chan_id; +int slave_id; +struct device *dev; +};
is this the representation of Fixed DMA, if so why have you omitted transfer widths? I can see obvious benefits of using that
--
~Vinod
Re: [PATCH 1/6] dma: acpi-dma: introduce ACPI DMA helpers
Quoted 11 lines · Vinod Koul
drivers/dma/acpi-dma.c line 189
Quoted 8 lines · Andy Shevchenko
+ * @dev: struct device to get DMA request from
+ * @index: index of FixedDMA descriptor for @dev
+ *
+ * Returns pointer to appropriate dma channel on success or NULL on error.
+ */drivers/dma/acpi-dma.c new file +3
-0+struct dma_chan *acpi_dma_request_slave_chan_by_index (struct device *dev, +size_t index) +{So i think this will be called by client driver to request a channel right?
Quoted 1 line
If so how does client find the device pointer for dma controller.
It doesn't have to. the client passes pointer to it's own device to this function.
Quoted 1 line
And index is a global one, right? What is it use ?
It is the index in the client device's ACPI DMA resources. It is explained in the documentation but it refers to something like:
Device (SPI0)
{
Method (_CRS, 0, NotSerialized)
{
Name (RBUF, ResourceTemplate ()
{
// Bunch of resources...
FixedDMA(...) // This will be index 0
FixedDMA(...) // and this is 1
// More resources..
}
Return (RBUF)
}
}So we use the index to refer the correct DMA resource in the resource list.
Quoted 36 lines
Quoted 36 lines
drivers/dma/acpi-dma.c new file +36
-0+struct acpi_dma_parser_data pdata; +struct acpi_dma_spec *dma_spec = &pdata.dma_spec; +struct list_head resource_list; +struct acpi_device *adev; +struct acpi_dma *adma; +struct dma_chan *chan; + +/* Check if the device was enumerated by ACPI */ +if (!dev || !ACPI_HANDLE (dev)) +return NULL ; + +if (acpi_bus_get_device (ACPI_HANDLE (dev), &adev)) +return NULL ; + +memset (&pdata,0 ,sizeof (pdata)); + pdata.index = index; + +/* Initial values for the request line and channel */ + dma_spec->chan_id =-1 ; + dma_spec->slave_id =-1 ; + +INIT_LIST_HEAD (&resource_list); +acpi_dev_get_resources (adev, &resource_list, + acpi_dma_parse_fixed_dma, &pdata); +acpi_dev_free_resource_list (&resource_list); + +if (dma_spec->slave_id <0 || dma_spec->chan_id <0 ) +return NULL ; + +mutex_lock (&acpi_dma_lock); + +list_for_each_entry (adma, &acpi_dma_list, dma_controllers) { + dma_spec->dev = adma->dev; + chan = adma->acpi_dma_xlate (dma_spec, adma); +if (chan) { +mutex_unlock (&acpi_dma_lock);
Quoted 8 lines
drivers/dma/acpi-dma.c line 231
Quoted 6 lines
drivers/dma/acpi-dma.c new file +6
-0+return chan; + } + } + +mutex_unlock (&acpi_dma_lock); +return NULL ;in this and error handling you are not doing anything different so why code duplication?
There is no specific reason for that. We can change it to goto + return chan in the next revision.
Quoted 23 lines
Quoted 23 lines
drivers/dma/acpi-dma.c new file +22
-0+} +EXPORT_SYMBOL_GPL (acpi_dma_request_slave_chan_by_index); + +/** +* acpi_dma_request_slave_chan_by_name - Get the DMA slave channel +* @dev: struct device to get DMA request from +* @name: represents corresponding FixedDMA descriptor for @dev +* +* In order to support both Device Tree and ACPI in a single driver we +* translate the names "tx" and "rx" here based on the most common case where +* the first FixedDMA descriptor is TX and second is RX. +* +* Returns pointer to appropriate dma channel on success or NULL on error. +*/ +struct dma_chan *acpi_dma_request_slave_chan_by_name (struct device *dev, +const char *name) +{ +size_t index; + +if (!strcmp (name,"tx" )) + index =0 ; +else if (!strcmp (name,"rx" ))
Quoted 8 lines
drivers/dma/acpi-dma.c line 258
Quoted 5 lines
drivers/dma/acpi-dma.c new file +5
-0+ index =1 ; + else + return NULL; + +return acpi_dma_request_slave_chan_by_index (dev, index);are you going to a have a different descriptor for tx and rx? Is index only for tx = 0 and rx =1 always. How would a controller be represented which has 8 channels, each channel can do DMA in either direction
This is a special case so that we can cope withdma_request_slave_channel().
But this has nothing to do with the controller. This is a client API and the FixedDMA descriptors are only associated with clients.
For example on Lynxpoint we have a DMA controller you describe above and we can use dma_request_slave_channel() with that for each client device (HSUART, SPI and I2C) without problems.
Quoted 9 lines
Quoted 9 lines
drivers/dma/acpi-dma.c new file +9
-0+} +EXPORT_SYMBOL_GPL (acpi_dma_request_slave_chan_by_name); + +/** +* acpi_dma_simple_xlate - Simple ACPI DMA engine translation helper +* @dma_spec: pointer to ACPI DMA specifier +* @adma: pointer to ACPI DMA controller data +* +* A simple translation function for ACPI based devices. Passes &struct
Quoted 8 lines
drivers/dma/acpi-dma.c line 274
Quoted 7 lines
+ *
dma_specto the DMA controller driver provided filter function. Returns
+ * pointer to the channel if found or %NULL otherwise.
+ */drivers/dma/acpi-dma.c new file +4
-0+struct dma_chan *acpi_dma_simple_xlate (struct acpi_dma_spec *dma_spec, +struct acpi_dma *adma) +{ +struct acpi_dma_filter_info *info = adma->data;what is the purpose of filter here?
It is pretty much the same as with DeviceTree version. There is an example of that in the dw_dmac ACPI support patch include in this series.
Basically the controller driver passes the filter (well the acpi_dma_filter_info) structure to acpi_dma_controller_register(). It can then do the requred setup for the channel once it is found.
Quoted 39 lines
Quoted 39 lines
drivers/dma/acpi-dma.c new file +7
-0+ +if (!info || !info->filter_fn) +return NULL ; + +return dma_request_channel (info->dma_cap, info->filter_fn, dma_spec); +} +EXPORT_SYMBOL_GPL (acpi_dma_simple_xlate);include/linux/acpi_dma.h new file +26
-0@@ -0,0 +1,116 @@ +/* +* ACPI helpers for DMA request / controller +* +* Based on of_dma.h +* +* Copyright (C) 2013, Intel Corporation +* Author: Andy Shevchenko <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 as +* published by the Free Software Foundation. +*/ + +#ifndef __LINUX_ACPI_DMA_H +#define __LINUX_ACPI_DMA_H + +#include <linux/list.h> +#include <linux/device.h> +#include <linux/dmaengine.h> + +/** +* struct acpi_dma_spec - slave device DMA resources +* @chan_id: channel unique id +* @slave_id: request line unique id +* @dev: struct device of the DMA controller to be used in the filter +* function
Quoted 8 lines
include/linux/acpi_dma.h line 32
Quoted 6 lines
include/linux/acpi_dma.h new file +6
-0+*/ +struct acpi_dma_spec { +int chan_id; +int slave_id; +struct device *dev; +};is this the representation of Fixed DMA, if so why have you omitted transfer widths? I can see obvious benefits of using that
It is, yes. But we haven't been using the widths with our hardware. BIOS seems to set it always 32-bit even though we should access the peripheral FIFO using 8-bit transfers for example.
The client never sees these when it uses dma_request_slave_channel() API.
PATCH 2/6 dmaengine: call acpi_dma_request_slave_channel as well
In reply to Andy ShevchenkoThe slave device could be enumerated by ACPI. In that case the dma_request_slave_channel should use the acpi_dma_request_slave_channel() helper.
--- drivers/dma/dmaengine.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/dma/dmaengine.c +6 -0
@@ -62,6 +62,8 @@#include <linux/rculist.h> #include <linux/idr.h> #include <linux/slab.h> +#include <linux/acpi.h> +#include <linux/acpi_dma.h> #include <linux/of_dma.h> static DEFINE_MUTEX (dma_list_mutex);
@@ -561,6 +563,10 @@ struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name)if (dev->of_node)return of_dma_request_slave_channel (dev->of_node, name); +/* If device was enumerated by ACPI get slave info from here */ +if (ACPI_HANDLE (dev)) +return acpi_dma_request_slave_chan_by_name (dev, name); +return NULL ; }EXPORT_SYMBOL_GPL (dma_request_slave_channel);
Signature · 1 line
1.8.2.rc0.22.gb3600c3
PATCH 3/6 dma: acpi-dma: parse CSRT to extract additional resources
In reply to Andy ShevchenkoSince we have CSRT only to get additional DMA controller resources, let's get rid of drivers/acpi/csrt.c and move its logic inside ACPI DMA helpers code.
--- drivers/acpi/Makefile | 1 - drivers/acpi/csrt.c | 159 ------------------------------------------- drivers/acpi/internal.h | 1 - drivers/acpi/scan.c | 1 - drivers/dma/acpi-dma.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++- include/linux/acpi_dma.h | 4 ++ 6 files changed, 173 insertions(+), 165 deletions(-) delete mode 100644 drivers/acpi/csrt.c
drivers/acpi/Makefile +0 -1
@@ -38,7 +38,6 @@ acpi-y += processor_core.o acpi-y += ec.o acpi-$(CONFIG_ACPI_DOCK) += dock.o acpi-y += pci_root.o pci_link.o pci_irq.o -acpi-y += csrt.o acpi-$(CONFIG_X86_INTEL_LPSS) += acpi_lpss.o acpi-y += acpi_platform.o acpi-y += power.o
drivers/acpi/csrt.c deleted +0 -159
159 lines not on this page · raw message
drivers/acpi/internal.h +0 -1
@@ -35,7 +35,6 @@ void acpi_pci_link_init(void);void acpi_pci_root_hp_init (void );void acpi_platform_init (void );int acpi_sysfs_init (void ); -void acpi_csrt_init (void );#ifdef CONFIG_ACPI_CONTAINER void acpi_container_init (void );#else
drivers/acpi/scan.c +0 -1
@@ -2042,7 +2042,6 @@ int __init acpi_scan_init(void)acpi_pci_link_init ();acpi_platform_init ();acpi_lpss_init (); -acpi_csrt_init ();acpi_container_init ();acpi_pci_slot_init ();acpi_memory_hotplug_init ();
drivers/dma/acpi-dma.c +169 -3
215 lines not on this page · raw message
include/linux/acpi_dma.h +4 -0
@@ -37,6 +37,8 @@ struct acpi_dma_spec {* @dev: struct device of this controller * @acpi_dma_xlate: callback function to find a suitable channel * @data: private data used by a callback function +* @base_request_line: first supported request line (CSRT) +* @end_request_line: last supported request line (CSRT) */ struct acpi_dma {struct list_head dma_controllers;
@@ -44,6 +46,8 @@ struct acpi_dma {struct dma_chan *(*acpi_dma_xlate) (struct acpi_dma_spec *,struct acpi_dma *);void *data; +unsigned short base_request_line; +unsigned short end_request_line; };/* Used with acpi_dma_simple_xlate() */
Signature · 1 line
1.8.2.rc0.22.gb3600c3
Re: [PATCH 3/6] dma: acpi-dma: parse CSRT to extract additional resources
Quoted 6 lines · Andy Shevchenko
Since we have CSRT only to get additional DMA controller resources, let's get rid of
drivers/acpi/csrt.cand move its logic inside ACPI DMA helpers code.Signed-off-by Andy Shevchenko <[email protected]>Signed-off-by Mika Westerberg <[email protected]>
for such a patch git format-patch -M is your friend. It generates patch to show file movement. It helps review greatly if you just move the file and then do additions on second patch, as diffstat tells me some changes have been done.
Quoted 41 lines
(quoted diff) +36
-0@@ -23,6 +25,117 @@ static LIST_HEAD(acpi_dma_list); static DEFINE_MUTEX(acpi_dma_lock); /** + * acpi_dma_parse_resource_group - match device and parse resource group + * @grp: CSRT resource group + * @adev: ACPI device to match with + * @adma: struct acpi_dma of the given DMA controller + * + * Returns 1 on success, 0 when no information is available, or appropriate + * errno value on error. + * + * In order to match a device from DSDT table to the corresponding CSRT device + * we use MMIO address and IRQ. + */ +/** + * acpi_dma_update_dma_spec - prepare dma specifier to pass to translation function + * @adma: struct acpi_dma of DMA controller + * @dma_spec: dma specifier to update + * + * Returns 0, if no information is avaiable, -1 on mismatch, and 1 otherwise. + * + * Accordingly to ACPI 5.0 Specification Table 6-170 "Fixed DMA Resource + * Descriptor": + * DMA Request Line bits is a platform-relative number uniquely + * identifying the request line assigned. Request line-to-Controller + * mapping is done in a controller-specific OS driver. + * That's why we can safely adjust slave_id when the appropriate controller is + * found. + */ +static int acpi_dma_update_dma_spec(struct acpi_dma *adma, + struct acpi_dma_spec *dma_spec) +{ + /* Set link to the DMA controller device */ + dma_spec->dev = adma->dev; + + /* Check if the request line range is available */ + if (adma->base_request_line == 0 && adma->end_request_line == 0) + return 0; +
Review static LIST_HEAD(acpi_dma_list); · line 74
Quoted 10 lines
(quoted diff) +10
-0+ /* Check if slave_id falls to the range */ + if (dma_spec->slave_id < adma->base_request_line || + dma_spec->slave_id > adma->end_request_line) + return -1; + + /* + * Here we adjust slave_id. It should be a relative number to the base + * request line. + */ + dma_spec->slave_id -= adma->base_request_line;
where are you getting the base_request_line, i didnt see anything for this in ACPI spec?
Quoted 43 lines
drivers/dma/acpi-dma.c +18
-2+ +return 1 ; +} +struct acpi_dma_parser_data {struct acpi_dma_spec dma_spec;size_t index;@@ -193,6 +347,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev,struct acpi_device *adev;struct acpi_dma *adma;struct dma_chan *chan; +int found;/* Check if the device was enumerated by ACPI */ if (!dev || !ACPI_HANDLE (dev))@@ -219,9 +374,20 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev,mutex_lock (&acpi_dma_lock);list_for_each_entry (adma, &acpi_dma_list, dma_controllers) { - dma_spec->dev = adma->dev; +/* +* We are not going to call translation function if slave_id +* doesn't fall to the request range. +*/ + found =acpi_dma_update_dma_spec (adma, dma_spec); +if (found <0 ) +continue ; chan = adma->acpi_dma_xlate (dma_spec, adma); -if (chan) { +/* +* Try to get a channel only from the DMA controller that +* matches the slave_id. See acpi_dma_update_dma_spec() +* description for the details. +*/ +if (found >0 || chan) {mutex_unlock (&acpi_dma_lock);return chan; }include/linux/acpi_dma.h +0
-0
include/linux/acpi_dma.h struct acpi_dma_spec { · line 41
Quoted 5 lines
include/linux/acpi_dma.h +2
-0@@ -37,6 +37,8 @@ struct acpi_dma_spec {* @dev: struct device of this controller * @acpi_dma_xlate: callback function to find a suitable channel * @data: private data used by a callback function +* @base_request_line: first supported request line (CSRT) +* @end_request_line: last supported request line (CSRT)
okay here it is, can you add the width here as well
include/linux/acpi_dma.h struct acpi_dma { · line 50
Quoted 9 lines
include/linux/acpi_dma.h +2
-0*/ struct acpi_dma {struct list_head dma_controllers;@@ -44,6 +46,8 @@ struct acpi_dma {struct dma_chan *(*acpi_dma_xlate) (struct acpi_dma_spec *,struct acpi_dma *);void *data; +unsigned short base_request_line; +unsigned short end_request_line;
how do you define these two?
Quoted 6 lines
include/linux/acpi_dma.h +0
-0};/* Used with acpi_dma_simple_xlate() */ Signature · 1 line
1.8.2.rc0.22.gb3600c3
Re: [PATCH 3/6] dma: acpi-dma: parse CSRT to extract additional resources
Quoted 10 lines · Vinod Koul
Quoted 6 lines · Andy Shevchenko
Since we have CSRT only to get additional DMA controller resources, let's get rid of
drivers/acpi/csrt.cand move its logic inside ACPI DMA helpers code.Signed-off-by Andy Shevchenko <[email protected]>Signed-off-by Mika Westerberg <[email protected]>for such a patch git format-patch -M is your friend. It generates patch to show file movement. It helps review greatly if you just move the file and then do additions on second patch, as diffstat tells me some changes have been done.
OK, thanks for the tip. We will use -M with the next revision.
Quoted 45 lines
Quoted 45 lines
(quoted diff) +40
-0@@ -23,6 +25,117 @@ static LIST_HEAD(acpi_dma_list); static DEFINE_MUTEX(acpi_dma_lock); /** + * acpi_dma_parse_resource_group - match device and parse resource group + * @grp: CSRT resource group + * @adev: ACPI device to match with + * @adma: struct acpi_dma of the given DMA controller + * + * Returns 1 on success, 0 when no information is available, or appropriate + * errno value on error. + * + * In order to match a device from DSDT table to the corresponding CSRT device + * we use MMIO address and IRQ. + */ +/** + * acpi_dma_update_dma_spec - prepare dma specifier to pass to translation function + * @adma: struct acpi_dma of DMA controller + * @dma_spec: dma specifier to update + * + * Returns 0, if no information is avaiable, -1 on mismatch, and 1 otherwise. + * + * Accordingly to ACPI 5.0 Specification Table 6-170 "Fixed DMA Resource + * Descriptor": + * DMA Request Line bits is a platform-relative number uniquely + * identifying the request line assigned. Request line-to-Controller + * mapping is done in a controller-specific OS driver. + * That's why we can safely adjust slave_id when the appropriate controller is + * found. + */ +static int acpi_dma_update_dma_spec(struct acpi_dma *adma, + struct acpi_dma_spec *dma_spec) +{ + /* Set link to the DMA controller device */ + dma_spec->dev = adma->dev; + + /* Check if the request line range is available */ + if (adma->base_request_line == 0 && adma->end_request_line == 0) + return 0; + + /* Check if slave_id falls to the range */ + if (dma_spec->slave_id < adma->base_request_line || + dma_spec->slave_id > adma->end_request_line) + return -1;
Quoted 8 lines
drivers/dma/acpi-dma.c void devm_acpi_dma_controller_free(struct device *dev) · line 301
Quoted 6 lines
drivers/dma/acpi-dma.c +6
-0+ +/* +* Here we adjust slave_id. It should be a relative number to the base +* request line. +*/ + dma_spec->slave_id -= adma->base_request_line;where are you getting the
base_request_line, i didnt see anything for this in ACPI spec?
It comes from the CSRT table. In this same patch there is a function acpi_dma_parse_resource_group() that extracts it.
If you check the kerneldoc of this function (acpi_dma_update_dma_spec()) it says this:
Accordingly to ACPI 5.0 Specification Table 6-170 "Fixed DMA Resource Descriptor":
DMA Request Line bits is a platform-relative number uniquely identifying the request line assigned. Request line-to-Controller mapping is done in a controller-specific OS driver.
So this patch series is actually the "controller-specific OS driver". And we use CSRT table to extract the request line range for a given controller.
Quoted 40 lines
Quoted 40 lines
drivers/dma/acpi-dma.c +17
-2+return 1 ; +} +struct acpi_dma_parser_data {struct acpi_dma_spec dma_spec;size_t index;@@ -193,6 +347,7 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev,struct acpi_device *adev;struct acpi_dma *adma;struct dma_chan *chan; +int found;/* Check if the device was enumerated by ACPI */ if (!dev || !ACPI_HANDLE (dev))@@ -219,9 +374,20 @@ struct dma_chan *acpi_dma_request_slave_chan_by_index(struct device *dev,mutex_lock (&acpi_dma_lock);list_for_each_entry (adma, &acpi_dma_list, dma_controllers) { - dma_spec->dev = adma->dev; +/* +* We are not going to call translation function if slave_id +* doesn't fall to the request range. +*/ + found =acpi_dma_update_dma_spec (adma, dma_spec); +if (found <0 ) +continue ; chan = adma->acpi_dma_xlate (dma_spec, adma); -if (chan) { +/* +* Try to get a channel only from the DMA controller that +* matches the slave_id. See acpi_dma_update_dma_spec() +* description for the details. +*/ +if (found >0 || chan) {mutex_unlock (&acpi_dma_lock);return chan; }include/linux/acpi_dma.h +0
-1--- a/include/linux/acpi_dma.h
Quoted 8 lines
include/linux/acpi_dma.h struct acpi_dma_spec { · line 41
Quoted 7 lines
+++
b/include/linux/acpi_dma.hinclude/linux/acpi_dma.h +2
-0@@ -37,6 +37,8 @@ struct acpi_dma_spec {* @dev: struct device of this controller * @acpi_dma_xlate: callback function to find a suitable channel * @data: private data used by a callback function +* @base_request_line: first supported request line (CSRT) +* @end_request_line: last supported request line (CSRT) okay here it is, can you add the width here as well
You mean the width from FixedDMA descriptor?
If yes, then I think it does not belong here. This stuff is for controllers and FixedDMA belongs to the clients. It can be differrent for each FixedDMA descriptor for each client (although we have only seen 32-bit widhts currently).
Quoted 10 lines
include/linux/acpi_dma.h struct acpi_dma { · line 50
Quoted 9 lines
include/linux/acpi_dma.h +2
-0*/ struct acpi_dma {struct list_head dma_controllers;@@ -44,6 +46,8 @@ struct acpi_dma {struct dma_chan *(*acpi_dma_xlate) (struct acpi_dma_spec *,struct acpi_dma *);void *data; +unsigned short base_request_line; +unsigned short end_request_line;how do you define these two?
It is the range of request lines assigned for each controller, if that is what you asked. In ACPI 5 it is the request line in FixedDMA descriptor that is used to locate the right controller. The request line should be unique accross the platform.
Quoted 6 lines
Quoted 6 lines
include/linux/acpi_dma.h +0
-0};/* Used with acpi_dma_simple_xlate() */ Signature · 1 line
1.8.2.rc0.22.gb3600c3
Re: [PATCH 3/6] dma: acpi-dma: parse CSRT to extract additional resources
In reply to Vinod KoulQuoted 10 lines · Vinod Koul
Quoted 6 lines · Andy Shevchenko
Since we have CSRT only to get additional DMA controller resources, let's get rid of
drivers/acpi/csrt.cand move its logic inside ACPI DMA helpers code.Signed-off-by Andy Shevchenko <[email protected]>Signed-off-by Mika Westerberg <[email protected]>for such a patch git format-patch -M is your friend. It generates patch to show file movement. It helps review greatly if you just move the file and then do additions on second patch, as diffstat tells me some changes have been done.
It obviously will not help. We are not creating new file, but moving (quite partially) contents from one file to the _existing_ one.
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
PATCH 4/6 dw_dmac: add ACPI support
In reply to Andy ShevchenkoSince we have proper ACPI DMA helpers implemented, the driver may use it. This patch introduces custom filter function together with acpi_device_id table.
--- drivers/dma/dw_dmac.c | 68 ++++++++++++++++++++++++++++++++++------------ drivers/dma/dw_dmac_regs.h | 1 - 2 files changed, 50 insertions(+), 19 deletions(-)
drivers/dma/dw_dmac.c +50 -18
@@ -25,6 +25,8 @@#include <linux/module.h> #include <linux/platform_device.h> #include <linux/slab.h> +#include <linux/acpi.h> +#include <linux/acpi_dma.h> #include "dw_dmac_regs.h" #include "dmaengine.h"
@@ -983,13 +985,6 @@ static inline void convert_burst(u32 *maxburst) *maxburst =0 ; } -static inline void convert_slave_id (struct dw_dma_chan *dwc) -{ -struct dw_dma *dw =to_dw_dma (dwc->chan.device); - - dwc->dma_sconfig.slave_id -= dw->request_line_base; -} -static int set_runtime_config (struct dma_chan *chan,struct dma_slave_config *sconfig) {
@@ -1008,7 +1003,6 @@ set_runtime_config(struct dma_chan *chan, struct dma_slave_config *sconfig)convert_burst (&dwc->dma_sconfig.src_maxburst);convert_burst (&dwc->dma_sconfig.dst_maxburst); -convert_slave_id (dwc);return 0 ; }
@@ -1284,6 +1278,46 @@ static struct dma_chan *dw_dma_of_xlate(struct of_phandle_args *dma_spec,return dma_request_channel (cap, dw_dma_of_filter, &fargs); } +#ifdef CONFIG_ACPI +static bool dw_dma_acpi_filter (struct dma_chan *chan,void *param) +{ +struct dw_dma_chan *dwc =to_dw_dma_chan (chan); +struct acpi_dma_spec *dma_spec = param; + +if (chan->device->dev != dma_spec->dev || + chan->chan_id != dma_spec->chan_id) +return false; + + dwc->request_line = dma_spec->slave_id; + dwc->src_master =dwc_get_sms (NULL ); + dwc->dst_master =dwc_get_dms (NULL ); + +return true; +} + +static void dw_dma_acpi_controller_register (struct dw_dma *dw) +{ +struct device *dev = dw->dma.dev; +struct acpi_dma_filter_info *info; +int ret; + + info =devm_kzalloc (dev,sizeof (*info),GFP_KERNEL ); +if (!info) +return ; + +dma_cap_zero (info->dma_cap); +dma_cap_set (DMA_SLAVE , info->dma_cap); + info->filter_fn = dw_dma_acpi_filter; + + ret =devm_acpi_dma_controller_register (dev, acpi_dma_simple_xlate, + info); +if (ret) +dev_err (dev,"could not register acpi_dma_controller\n" ); +} +#else /* !CONFIG_ACPI */ +static inline void dw_dma_acpi_controller_register (struct dw_dma *dw) {} +#endif /* !CONFIG_ACPI */ +/* --------------------- Cyclic DMA API extensions -------------------- */ /**
@@ -1620,7 +1654,6 @@ dw_dma_parse_dt(struct platform_device *pdev)static int dw_probe (struct platform_device *pdev) { -const struct platform_device_id *match;struct dw_dma_platform_data *pdata;struct resource *io;struct dw_dma *dw;
@@ -1704,11 +1737,6 @@ static int dw_probe(struct platform_device *pdev)memcpy (dw->data_width, pdata->data_width,4 ); } -/* Get the base request line if set */ - match =platform_get_device_id (pdev); -if (match) - dw->request_line_base = (unsigned int )match->driver_data; -/* Calculate all channel mask before DMA setup */ dw->all_chan_mask = (1 << nr_channels) -1 ;
@@ -1833,6 +1861,9 @@ static int dw_probe(struct platform_device *pdev) "could not register of_dma_controller\n"); } +if (ACPI_HANDLE (&pdev->dev)) +dw_dma_acpi_controller_register (dw); +return 0 ; }
@@ -1904,11 +1935,12 @@ static const struct of_device_id dw_dma_of_id_table[] = {MODULE_DEVICE_TABLE (of, dw_dma_of_id_table);#endif -static const struct platform_device_id dw_dma_ids[] = { -/* Name, Request Line Base */ - {"INTL9C60" , (kernel_ulong_t )16 }, +#ifdef CONFIG_ACPI +static const struct acpi_device_id dw_dma_acpi_id_table[] = { + {"INTL9C60" ,0 }, { } }; +#endif static struct platform_driver dw_driver = { .probe = dw_probe,
@@ -1918,8 +1950,8 @@ static struct platform_driver dw_driver = { .name = "dw_dmac", .pm = &dw_dev_pm_ops, .of_match_table = of_match_ptr(dw_dma_of_id_table), + .acpi_match_table = ACPI_PTR(dw_dma_acpi_id_table), }, - .id_table = dw_dma_ids, };static int __init dw_init (void )
drivers/dma/dw_dmac_regs.h +0 -1
@@ -250,7 +250,6 @@ struct dw_dma {/* hardware configuration */ unsigned char nr_masters;unsigned char data_width[4 ]; -unsigned int request_line_base;struct dw_dma_chan chan[0 ]; };
Signature · 1 line
1.8.2.rc0.22.gb3600c3
PATCH 5/6 ACPI / LPSS: return no error from register_device_clock in special cases
In reply to Andy ShevchenkoWhen device uses the fixed clock and has no private space of LTR, we have to create main LPSS clock and register platform device. This is normally a case for LPSS DMA controller.
--- drivers/acpi/acpi_lpss.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/acpi/acpi_lpss.c +4 -0
@@ -95,6 +95,10 @@ static int register_device_clock(struct acpi_device *adev,if (!lpss_clk_dev)lpt_register_clock_device (); +if (!dev_desc->prv_offset && dev_desc->clk_required) +/* There is no error when device uses fixed clock */ +return 0 ; +if (!dev_desc->clk_parent || !pdata->mmio_base || pdata->mmio_size < dev_desc->prv_offset +LPSS_CLK_SIZE )return -ENODATA ;
Signature · 1 line
1.8.2.rc0.22.gb3600c3
Re: [PATCH 5/6] ACPI / LPSS: return no error from register_device_clock in special cases
Quoted 14 lines · Andy Shevchenko
When device uses the fixed clock and has no private space of LTR, we have to create main LPSS clock and register platform device. This is normally a case for LPSS DMA controller.
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/acpi/acpi_lpss.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/acpi/acpi_lpss.c +0
-0
drivers/acpi/acpi_lpss.c static int register_device_clock(struct acpi_device *adev, · line 98
Quoted 4 lines
drivers/acpi/acpi_lpss.c +1
-0@@ -95,6 +95,10 @@ static int register_device_clock(struct acpi_device *adev,if (!lpss_clk_dev)lpt_register_clock_device (); +if (!dev_desc->prv_offset && dev_desc->clk_required)
If clk_required is not set, register_device_clock() won't be called at all for the device. Moreover, prv_offset may be zero in principle.
You'd need to add support for shared clocks for that to really work. I have code for that, please ping me off-list.
drivers/acpi/acpi_lpss.c static int register_device_clock(struct acpi_device *adev, · line 104
Quoted 6 lines
drivers/acpi/acpi_lpss.c +3
-0+/* There is no error when device uses fixed clock */ +return 0 ; +if (!dev_desc->clk_parent || !pdata->mmio_base || pdata->mmio_size < dev_desc->prv_offset +LPSS_CLK_SIZE )return -ENODATA ;
Thanks,
Rafael
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.
PATCH 6/6 ACPI / LPSS: add Lynxpoint DMA controller to the list
In reply to Andy ShevchenkoIn Lynxpoint we have to enable clock per each LPSS device. That's why we have to enumerate them from drivers/acpi/acpi_lpss.c. The DMA controller is one of such devices.
--- drivers/acpi/acpi_lpss.c | 7 +++++++ drivers/clk/x86/clk-lpt.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-)
drivers/acpi/acpi_lpss.c +7 -0
@@ -47,6 +47,10 @@ struct lpss_private_data {const struct lpss_device_desc *dev_desc; }; +static struct lpss_device_desc lpss_dev_desc = { + .clk_required = true, +}; +static struct lpss_device_desc lpt_dev_desc = { .clk_required = true, .clk_parent ="lpss_clk" ,
@@ -60,6 +64,9 @@ static struct lpss_device_desc lpt_sdio_dev_desc = { };static const struct acpi_device_id acpi_lpss_device_ids[] = { +/* Generic LPSS devices */ + {"INTL9C60" , (unsigned long )&lpss_dev_desc }, +/* Lynxpoint LPSS devices */ {"INT33C0" , (unsigned long )&lpt_dev_desc }, {"INT33C1" , (unsigned long )&lpt_dev_desc },
drivers/clk/x86/clk-lpt.c +1 -1
@@ -30,7 +30,7 @@ static int lpt_clk_probe(struct platform_device *pdev)return PTR_ERR (clk);/* Shared DMA clock */ -clk_register_clkdev (clk,"hclk" ,"INTL9C60.0.auto" ); +clk_register_clkdev (clk,"hclk" ,"INTL9C60:00" );return 0 ; }
Signature · 1 line
1.8.2.rc0.22.gb3600c3
Re: [PATCH 6/6] ACPI / LPSS: add Lynxpoint DMA controller to the list
Quoted 3 lines · Andy Shevchenko
In Lynxpoint we have to enable clock per each LPSS device. That's why we have to enumerate them from
drivers/acpi/acpi_lpss.c. The DMA controller is one of such devices.
We'll do that after we've sorted out [5/6].
Thanks,
Rafael
Quoted 45 lines
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/acpi/acpi_lpss.c | 7 +++++++ drivers/clk/x86/clk-lpt.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-)drivers/acpi/acpi_lpss.c +7
-0@@ -47,6 +47,10 @@ struct lpss_private_data {const struct lpss_device_desc *dev_desc; }; +static struct lpss_device_desc lpss_dev_desc = { + .clk_required = true, +}; +static struct lpss_device_desc lpt_dev_desc = { .clk_required = true, .clk_parent ="lpss_clk" ,@@ -60,6 +64,9 @@ static struct lpss_device_desc lpt_sdio_dev_desc = { };static const struct acpi_device_id acpi_lpss_device_ids[] = { +/* Generic LPSS devices */ + {"INTL9C60" , (unsigned long )&lpss_dev_desc }, +/* Lynxpoint LPSS devices */ {"INT33C0" , (unsigned long )&lpt_dev_desc }, {"INT33C1" , (unsigned long )&lpt_dev_desc },drivers/clk/x86/clk-lpt.c +1
-1@@ -30,7 +30,7 @@ static int lpt_clk_probe(struct platform_device *pdev)return PTR_ERR (clk);/* Shared DMA clock */ -clk_register_clkdev (clk,"hclk" ,"INTL9C60.0.auto" ); +clk_register_clkdev (clk,"hclk" ,"INTL9C60:00" );return 0 ; }
Signature · 2 lines
I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center.