Re: [PATCH] of: Add generic device tree DMA helpers
On Sat, 17 Mar 2012 16:03:02 +, Arnd Bergmann a...@arndb.de wrote: On Saturday 17 March 2012, Grant Likely wrote: On Thu, 15 Mar 2012 09:26:52 +, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Thu, Mar 15, 2012 at 09:22:06AM +, Arnd Bergmann wrote: On Thursday 15 March 2012, Nicolas Ferre wrote: Add some basic helpers to retrieve a DMA controller device_node and the DMA request specifications. By making DMA controllers register a generic translation function, we allow the management of any type of DMA requests specification. The void * output of an of_dma_xlate() function that will be implemented by the DMA controller can carry any type of dma-request argument. The DMA client will search its associated DMA controller in the list and call the registered of_dam_xlate() function to retrieve the request values. One simple xlate function is provided for the single number type of request binding. This implementation is independent from dmaengine so it can also be used by legacy drivers. For legacy reason another API will export the DMA request number into a Linux resource of type IORESOURCE_DMA. This looks very good. I missed the first version of this patch, but was thinking of very similar bindings. There's one issue which is concerning me - when we switch OMAP to use DMA engine, it won't use numeric stuff anymore but the DMA engine way of requesting a channel (match function + data). How does that fit into this scheme? Not well as the current patch set stands. The xlate function doesn't return any context for the dma channel number that it returns, so the driver cannot figure out which DMA controller to use if there are multiple. Shouldn't that be part of the data returned by xlate? I was under the impression that this would be the data that you would pass into dma_request_channel, together with a filter function that find the instance from the data. Yes, that's my point. The patch as it is currently written doesn't do what it needs to. g. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
* Grant Likely wrote: On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding thierry.red...@avionic-design.de wrote: So if we decide to explicitly allow specifying names, then we can always add a pwm-names property (or name-pwm-names respectively) to use as label and fallback to the user OF device node name if that property is not present. After implementing both schemes (ie. interrupts+interrupt-names [*-]gpios) I definitely prefer the fixed property name plus a separate names property. It is easier to use common code with that scheme, and easier to statically check for correctness. Okay. Would everyone be happy with pwms and pwm-names? Thierry pgplUvVS1Lb0u.pgp Description: PGP signature
Re: [PATCH] of: Add generic device tree DMA helpers
On Sunday 18 March 2012, Thierry Reding wrote: Not enough information to check signature validity. Show Details * Grant Likely wrote: On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding thierry.red...@avionic-design.de wrote: So if we decide to explicitly allow specifying names, then we can always add a pwm-names property (or name-pwm-names respectively) to use as label and fallback to the user OF device node name if that property is not present. After implementing both schemes (ie. interrupts+interrupt-names [*-]gpios) I definitely prefer the fixed property name plus a separate names property. It is easier to use common code with that scheme, and easier to statically check for correctness. Okay. Would everyone be happy with pwms and pwm-names? Sounds good. I would have suggested pwm, but the plural also works since that is used for interrupts, too. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sun, 18 Mar 2012 10:22:41 +0100, Thierry Reding thierry.red...@avionic-design.de wrote: * Grant Likely wrote: On Thu, 15 Mar 2012 11:27:36 +0100, Thierry Reding thierry.red...@avionic-design.de wrote: So if we decide to explicitly allow specifying names, then we can always add a pwm-names property (or name-pwm-names respectively) to use as label and fallback to the user OF device node name if that property is not present. After implementing both schemes (ie. interrupts+interrupt-names [*-]gpios) I definitely prefer the fixed property name plus a separate names property. It is easier to use common code with that scheme, and easier to statically check for correctness. Okay. Would everyone be happy with pwms and pwm-names? okay. g. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Saturday 17 March 2012, Grant Likely wrote:
+static LIST_HEAD(of_dma_list);
+
+struct of_dma {
+ struct list_head of_dma_controllers;
+ struct device_node *of_node;
+ int of_dma_n_cells;
+ int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
+};
This _xlate is nearly useless as a generic API. It solves the problem for
the specific case where the driver is hard-coded to know which DMA engine
to talk to, but since the returned data doesn't provide any context, it
isn't useful if there are multiple DMA controllers to choose from.
The void *data pointer must be replaced with a typed structure so that
context can be returned.
I've read up a bit more on how the existing drivers use the filter
functions, it seems there are multiple classes of them, the classes
that I've encountered are:
1. matches on chan-device pointer and/or chan_id
(8 drivers)
2. will match anything
(6 drivers)
3. requires specific dma engine driver, then behaves like 1 or 2
(8 drivers, almost all freescale)
4. one of a kind, matches resource name string or device-dev_id
(two drivers)
5. filter function and data both provided by platform code,
platform picks dmaengine driver.
(4 amba pl* drivers, used on ARM, ux500, ...)
The last category is interesting because here, the dmaengine
driver (pl330, coh901318, sirf-dma, ste_dma40) provides the filter
function while in the other cases that is provided by the device
driver! Out of these, the ste_dma40 is special because it's the
only one where the data is a complex data structure describing the
constraints on the driver, while all others just find the right
channel.
Some drivers also pass assign driver specific data to chan-private.
I would hope that we can all make them use something like
struct dma_channel *of_dma_request_channel(struct of_node*,
int index, void *driver_data);
with an appropriate common definition behind it. In the cases
where the driver can just match anything, I'd assume that all
channels are equal, so #dma-cells would be 0. For the ste_dma40,
#dma-cells needs to cover all of stedma40_chan_cfg. In most
other cases, #dma-cells can be 1 and just enumerate the channels,
unless we want to simplify the cases that Russell mentioned where
we want to keep a two stage mapping channel identifiers and physical
channel numbers.
How about an implementation like this:?
typedef bool dma_filter_simple(struct dma_chan *chan, void *filter_param)
{
/* zero #dma-cells, accept anything */
return true;
}
struct dma_channel *of_dma_request_channel(struct of_node*, int index,
dma_cap_mask_t *mask,
void *driver_data)
{
struct of_phandle_args dma_spec;
struct dma_device *device;
struct dma_chan *chan = NULL;
dma_filter_fn *filter;
ret = of_parse_phandle_with_args(np, dma-request, #dma-cells,
index, dma_spec);
device = dma_find_device(dma_spec-np);
if (!device)
goto out;
if (dma_spec-args_count == 0)
filter = dma_filter_simple;
else
filter = device-dma_dt_filter; /* new member */
chan = private_candidate(mask, device, filter, dma_spec-args);
if (chan !chan-private)
chan-private = driver_data;
out:
return chan;
}
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sun, 18 Mar 2012 20:13:22 +, Arnd Bergmann a...@arndb.de wrote:
On Saturday 17 March 2012, Grant Likely wrote:
+static LIST_HEAD(of_dma_list);
+
+struct of_dma {
+ struct list_head of_dma_controllers;
+ struct device_node *of_node;
+ int of_dma_n_cells;
+ int (*of_dma_xlate)(struct of_phandle_args *dma_spec, void *data);
+};
This _xlate is nearly useless as a generic API. It solves the problem for
the specific case where the driver is hard-coded to know which DMA engine
to talk to, but since the returned data doesn't provide any context, it
isn't useful if there are multiple DMA controllers to choose from.
The void *data pointer must be replaced with a typed structure so that
context can be returned.
I've read up a bit more on how the existing drivers use the filter
functions, it seems there are multiple classes of them, the classes
that I've encountered are:
1. matches on chan-device pointer and/or chan_id
(8 drivers)
2. will match anything
(6 drivers)
3. requires specific dma engine driver, then behaves like 1 or 2
(8 drivers, almost all freescale)
4. one of a kind, matches resource name string or device-dev_id
(two drivers)
5. filter function and data both provided by platform code,
platform picks dmaengine driver.
(4 amba pl* drivers, used on ARM, ux500, ...)
The last category is interesting because here, the dmaengine
driver (pl330, coh901318, sirf-dma, ste_dma40) provides the filter
function while in the other cases that is provided by the device
driver! Out of these, the ste_dma40 is special because it's the
only one where the data is a complex data structure describing the
constraints on the driver, while all others just find the right
channel.
Some drivers also pass assign driver specific data to chan-private.
I would hope that we can all make them use something like
struct dma_channel *of_dma_request_channel(struct of_node*,
int index, void *driver_data);
certainly my hope too!
with an appropriate common definition behind it. In the cases
where the driver can just match anything, I'd assume that all
channels are equal, so #dma-cells would be 0. For the ste_dma40,
#dma-cells needs to cover all of stedma40_chan_cfg. In most
other cases, #dma-cells can be 1 and just enumerate the channels,
unless we want to simplify the cases that Russell mentioned where
we want to keep a two stage mapping channel identifiers and physical
channel numbers.
How about an implementation like this:?
typedef bool dma_filter_simple(struct dma_chan *chan, void *filter_param)
{
/* zero #dma-cells, accept anything */
return true;
}
struct dma_channel *of_dma_request_channel(struct of_node*, int index,
dma_cap_mask_t *mask,
void *driver_data)
{
struct of_phandle_args dma_spec;
struct dma_device *device;
struct dma_chan *chan = NULL;
dma_filter_fn *filter;
ret = of_parse_phandle_with_args(np, dma-request, #dma-cells,
index, dma_spec);
device = dma_find_device(dma_spec-np);
Is dma_find_device() a new function? How does it look up the dma
device?
if (!device)
goto out;
Just return NULL here.
if (dma_spec-args_count == 0)
filter = dma_filter_simple;
else
filter = device-dma_dt_filter; /* new member */
I'm not thrilled with this if/else hunk; even the case of
#dma-cells=0 should provide a hook; even it if is the stock simple
filter. Leaving filter as NULL is the same as accepting everything
anyway.
chan = private_candidate(mask, device, filter, dma_spec-args);
if (chan !chan-private)
chan-private = driver_data;
out:
return chan;
I think this looks right, except for the comments above.
g.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] misc: emif: add device tree support to emif driver
On Fri, 16 Mar 2012 00:04:07 +0530, Aneesh V ane...@ti.com wrote: Cc: Rajendra Nayak rna...@ti.com Cc: Benoit Cousson b-cous...@ti.com Cc: Grant Likely grant.lik...@secretlab.ca Signed-off-by: Aneesh V ane...@ti.com All 4 look okay to me. Reviewed-by: Grant Likely grant.lik...@secretlab.ca -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] misc: emif: add device tree support to emif driver
On Fri, 09 Mar 2012 18:20:51 +0530, Aneesh V ane...@ti.com wrote:
Hi Grant,
On Friday 09 March 2012 11:07 AM, Grant Likely wrote:
On Thu, 8 Mar 2012 22:03:57 +0530, Aneesh Vane...@ti.com wrote:
Cc: Rajendra Nayakrna...@ti.com
Cc: Benoit Coussonb-cous...@ti.com
Signed-off-by: Aneesh Vane...@ti.com
---
Changes since RFC v4:
- Rebased to the latest version of EMIF series
- Replace kzalloc()/kfree() with devm_* variants
---
drivers/misc/emif.c | 289
++-
1 files changed, 288 insertions(+), 1 deletions(-)
diff --git a/drivers/misc/emif.c b/drivers/misc/emif.c
index 79fb161..0aaa61e 100644
--- a/drivers/misc/emif.c
+++ b/drivers/misc/emif.c
@@ -18,6 +18,7 @@
#includelinux/platform_device.h
#includelinux/interrupt.h
#includelinux/slab.h
+#includelinux/of.h
#includelinux/debugfs.h
#includelinux/seq_file.h
#includelinux/module.h
@@ -49,6 +50,7 @@
* frequency in effect at the moment)
* @plat_data: Pointer to saved platform data.
* @debugfs_root:dentry to the root folder for EMIF in
debugfs
+ * @np_ddr: Pointer to ddr device tree node
*/
struct emif_data {
u8 duplicate;
@@ -63,6 +65,9 @@ struct emif_data {
struct emif_regs*curr_regs;
struct emif_platform_data *plat_data;
struct dentry *debugfs_root;
+#if defined(CONFIG_OF)
+ struct device_node *np_ddr;
+#endif
Don't bother with the #if/#endif wrapper here.
Ok.
};
static struct emif_data *emif1;
@@ -1147,6 +1152,270 @@ static int is_custom_config_valid(struct
emif_custom_configs *cust_cfgs,
return valid;
}
+#if defined(CONFIG_OF)
+static void __init of_get_custom_configs(struct device_node *np_emif,
__devinit. Same through the rest of the file.
I am not sure if we need __devinit. I see that __devinit will not have
any effect if CONFIG_HOTPLUG is enabled and CONFIG_HOTPLUG is always
enabled in our configuration. EMIF devices are not hot-pluggable
devices and are always added at boot-time from the board file. They are
on-chip IP modules and dynamic discovery and addition doesn't make
sense for them. So, can't we save some memory by making them __init.
AFAIU, __init doesn't have any effect in a module. However, I can make
that more explicit by using '__init_or_module'. Is that ok?
[...]
+ return NULL;
+out:
+ return emif;
+}
+#endif
+
static struct emif_data * __init get_device_details(
This function also must be __devinit
struct platform_device *pdev)
{
@@ -1266,7 +1535,13 @@ static int __init emif_probe(struct platform_device
*pdev)
struct resource *res;
int irq;
- emif = get_device_details(pdev);
+#if defined(CONFIG_OF)
+ if (pdev-dev.of_node)
+ emif = of_get_device_details(pdev-dev.of_node,pdev-dev);
+ else
+#endif
+ emif = get_device_details(pdev);
+
if (!emif) {
pr_err(%s: error getting device data\n, __func__);
goto error;
@@ -1643,11 +1918,23 @@ static void __attribute__((unused))
freq_post_notify_handling(void)
spin_unlock_irqrestore(emif_lock, irq_state);
}
+#if defined(CONFIG_OF)
+static const struct of_device_id emif_of_match[] = {
+ { .compatible = ti,emif-4d },
+ { .compatible = ti,emif-4d5 },
+ {},
+};
+MODULE_DEVICE_TABLE(of, emif_of_match);
+#endif
+
static struct platform_driver emif_driver = {
.remove = __exit_p(emif_remove),
Not part of this patch I realize, but this is a bug. .remove must
be in the __devexit section and the __devexit_p() wrapper must
be used. Similarly, emif_probe must be in the __devinit section.
Again, in our case remove() is not needed unless the driver is built as
a module because the devices will never be un-registered. When it is
built as a module the effect is same for both:
It's just outside the pattern expected for device drivers. I like to
be careful about things like this. That's why I recommend __devinit
also, but I see now that the probe hook isn't added to the
platform_device so it isn't required.
#if defined(MODULE) || defined(CONFIG_HOTPLUG)
#define __devexit_p(x) x
#else
#define __devexit_p(x) NULL
#endif
#ifdef MODULE
#define __exit_p(x) x
#else
#define __exit_p(x) NULL
#endif
Don't go with what is written in the code here; go with what the
meaning of the sections are. The code can be changed, but the drivers
are expected to honor what the sections mean.
__init can be freed after module init
__devinit in reality never gets freed because devices can show up later
__devexit can be freed if hotplug is not enabled
__exit can be freed after module
Re: [PATCH] of: Add generic device tree DMA helpers
On Sunday 18 March 2012, Grant Likely wrote:
struct dma_channel *of_dma_request_channel(struct of_node*, int index,
dma_cap_mask_t *mask,
void *driver_data)
{
struct of_phandle_args dma_spec;
struct dma_device *device;
struct dma_chan *chan = NULL;
dma_filter_fn *filter;
ret = of_parse_phandle_with_args(np, dma-request, #dma-cells,
index, dma_spec);
device = dma_find_device(dma_spec-np);
Is dma_find_device() a new function? How does it look up the dma
device?
Yes, it would be similar to the proposed function in Benoit's patch
if (dma_spec-args_count == 0)
filter = dma_filter_simple;
else
filter = device-dma_dt_filter; /* new member */
I'm not thrilled with this if/else hunk; even the case of
#dma-cells=0 should provide a hook; even it if is the stock simple
filter. Leaving filter as NULL is the same as accepting everything
anyway.
Right, good point. So a dmaengine driver would either register a trivial
filter if it wants to do anything here, or it would just leave it as a NULL
pointer otherwise.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] of: Add generic device tree DMA helpers
On Sunday 18 March 2012, Arnd Bergmann wrote:
Is dma_find_device() a new function? How does it look up the dma
device?
Yes, it would be similar to the proposed function in Benoit's patch
Well, actually we would not even need a new list with all the devices,
it can simply become
/* must be called with dma_list_mutex held */
static struct dma_device *dma_find_device(struct of_node *node)
{
struct dma_device *device;
list_for_each_entry(device, dma_device_list, global_node) {
if (device-dev.of_node == node)
break;
}
return device;
}
Given that dma_device_list is most likely be a very short list, this will
be a fast operation.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH v4 0/7] Add TI EMIF SDRAM controller driver
Kyungmin Parkkmp...@infradead.org 2012-03-17 15:10 (GMT+09:00) Hi, On 3/17/12, Aneesh V wrote: Add a driver for the EMIF SDRAM controller used in TI SoCs EMIF is an SDRAM controller that supports, based on its revision, one or more of LPDDR2/DDR2/DDR3 protocols.This driver adds support for LPDDR2. The driver supports the following features: - Calculates the DDR AC timing parameters to be set in EMIF registers using data from the device data-sheets and based on the DDR frequency. If data from data-sheets is not available default timing values from the JEDEC spec are used. These will be safe, but not necessarily optimal - API for changing timings during DVFS or at boot-up This means that you alreeady have callbacks to create a devfreq device driver that supports DVFS on the device. This doesn't need to be a misc device driver then. - Temperature alert configuration and handling of temperature alerts, if any for LPDDR2 devices * temperature alert is based on periodic polling of MR4 mode register in DDR devices automatically performed by hardware * timings are de-rated and brought back to nominal when temperature raises and falls respectively This can be a feature overriding max_freq inside the Omap EMIF devfreq device driver though it maybe (or not.. I just don't sure) be better to use thermal framework as well. Cheers! MyungJoo - Cache of calculated register values to avoid re-calculating them The driver will need some minor updates when it is eventually integrated with Dynamic Voltage and Frequency Scaling (DVFS). This can not be done now as DVFS support is not available in the mainline yet. Do you see the devfreq? it's designed for non-cpu device frequency. It's role is similar with cpufreq. Now samsung exynos uses devfreq for DRAM bus frequency. Thank you, Kyungmin Park Discussions with Santosh Shilimkar were immensely helpful in shaping up the interfaces. Vibhore Vardhan did the initial code snippet for thermal handling. Testing: - The driver is tested on OMAP4430 SDP. - The driver in a slightly adapted form is also tested on OMAP5. - Since mainline kernel doesn't have DVFS support yet, testing was done using a test module. - Temperature alert handling was tested with simulated interrupts and faked temperature values as testing all cases in real-life scenarios is difficult. - Tested the driver as a module Cc: Greg KH v4: - Converted instances of EXPORT_SYMBOL to EXPORT_SYMBOL_GPL - Removed un-necessary #ifndef __ASSEMBLY__' - Minor formatting fix v2: - Fixed a bug found in the implementation of errata i728 workaround - Fixed the value of frequency printed in debugfs - Dropped the hwmod patch as Paul has already posted a a hwmod series [1] that adds hwmod for EMIF - Converted instances of __init to __init_or_module [1] http://thread.gmane.org/gmane.linux.ports.arm.omap/72855 Aneesh V (7): misc: ddr: add LPDDR2 data from JESD209-2 misc: emif: add register definitions for EMIF misc: emif: add basic infrastructure for EMIF driver misc: emif: handle frequency and voltage change events misc: emif: add interrupt and temperature handling misc: emif: add one-time settings misc: emif: add debugfs entries for emif Documentation/misc-devices/ti-emif.txt | 58 ++ drivers/misc/Kconfig| 12 + drivers/misc/Makefile |1 + drivers/misc/emif.c | 1670 +++ drivers/misc/emif.h | 589 +++ include/linux/platform_data/emif_plat.h | 128 +++ include/misc/jedec_ddr.h| 175 lib/Kconfig |8 + lib/Makefile|2 + lib/jedec_ddr_data.c| 135 +++ 10 files changed, 2778 insertions(+), 0 deletions(-) create mode 100644 Documentation/misc-devices/ti-emif.txt create mode 100644 drivers/misc/emif.c create mode 100644 drivers/misc/emif.h create mode 100644 include/linux/platform_data/emif_plat.h create mode 100644 include/misc/jedec_ddr.h create mode 100644 lib/jedec_ddr_data.c ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- MyungJoo Ham (함명주), PHD System S/W Lab, S/W Platform Team, Software Center Samsung Electronics Cell: +82-10-6714-2858
