[PATCH RFC v11 0/6] MPC512x DMA slave s/g support, OF DMA lookup

[PATCH RFC v11 0/6] MPC512x DMA slave s/g support, OF DMA lookup

[Alexander Popov]

2013/7/14 Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>:
> this series
> - introduces slave s/g support (that's support for DMA transfers which
>    involve peripherals in contrast to mem-to-mem transfers)
> - adds device tree based lookup support for DMA channels
> - combines floating patches and related feedback which already covered
>    several aspects of what the suggested LPB driver needs, to demonstrate
>    how integration might be done
> - carries Q&D SD card support to enable another DMA client during test,
>    while this patch needs to get dropped upon pickup

Changes in v2:
> - re-order mpc8308 related code paths for improved readability, no
>    change in behaviour, introduction of symbolic channel names here
>    already
> - squash 'execute() start condition' and 'terminate all' into the
>    introduction of 'slave s/g prep' and 'device control' support; refuse
>    s/g lists with more than one item since slave support is operational
>    yet proper s/g support is missing (can get addressed later)
> - always start transfers from software on MPC8308 as there are no
>    external request lines for peripheral flow control
> - drop dt-bindings header file and symbolic channel names in OF nodes

Changes in v3 and v4:
 Part 1/5:
 - use #define instead of enum since individual channels don't require
    special handling.
 Part 2/5:
 - add a flag "will_access_peripheral" to DMA transfer descriptor
    according recommendations of Gerhard Sittig.
    This flag is set in mpc_dma_prep_memcpy() and mpc_dma_prep_slave_sg()
    and is evaluated in mpc_dma_execute() to choose a type of start for
    the transfer.
 - prevent descriptors of transfers which involve peripherals from
    being chained together;
    each of such transfers needs hardware initiated start.
 - add locking while working with struct mpc_dma_chan
    according recommendations of Lars-Peter Clausen.
 - remove default nbytes value. Client kernel modules must set
    src_maxburst and dst_maxburst fields of struct dma_slave_config (dmaengine.h).

Changes in v5:
 Part 2/5:
 - add and improve comments;
 - improve the code moving transfer descriptors from 'queued' to 'active' list
    in mpc_dma_execute();
 - allow mpc_dma_prep_slave_sg() to run with non-empty 'active' list;
 - take 'mdesc' back to 'free' list in case of error in mpc_dma_prep_slave_sg();
 - improve checks of the transfer parameters;
 - provide the default value for 'maxburst' in mpc_dma_device_control().

Changes in v6:
 Part 2/5:
 - remove doubtful comment;
 - fix coding style issues;
 - set default value for 'maxburst' to 1 which applies to most cases;
 Part 3/5:
 - use dma_get_slave_channel() instead of dma_request_channel()
    in new function of_dma_xlate_by_chan_id() according recommendations of
    Arnd Bergmann;
 Part 4/5:
 - set DMA_PRIVATE flag for MPC512x DMA controller since its driver relies on
    of_dma_xlate_by_chan_id() which doesn't use dma_request_channel()
    any more; (removed in v7)
 - resolve little patch conflict;
 Part 5/5:
 - resolve little patch conflict;

Changes in v7:
 Part 2:
 - improve comment;
 Part 4:
 - split in two separate patches. Part 4/6 contains device tree
    binding document and in part 5/6 MPC512x DMA controller is registered
    for device tree channel lookup;
 - remove setting DMA_PRIVATE flag for MPC512x DMA controller from part 5/6;

Changes in v8:
 Part 2:
 - improve comments;
 - fix style issues;
 Part 6:
 - remove since it has become obsolete;

Changes in v9:
 A new patch (part 3/6) is added to this series according the
   feedback of Andy Shevchenko.
 Part 2/6:
 - keep style of the comments;
 - use is_slave_direction() instead of manual checks;
 - remove redundant else branches of the conditions;
 - make mpc_dma_device_control() return -ENXIO for unknown command;
 Part 6/6:
 - change according the new part 3/6;
 - fix style issues;

Changes in v10:
 Part 2/6:
 - don't use direction field of dma_slave_config in mpc_dma_device_control()
    but store settings in mpc_dma_chan for both DMA_DEV_TO_MEM and
    DMA_MEM_TO_DEV cases; then retrieve the needed values in
    mpc_dma_prep_slave_sg();
 - fix style issue and put 2014 instead of 2013;
 Part 3/6:
 - fix mpc_dma_probe() error path and mpc_dma_remove(): manually free IRQs and
    dispose IRQ mappings before devm_* takes care of other resources;
 Part 6/6:
 - change according the new part 3/6;
 - fix style issue;

Changes in v11:
 Part 5/6:
 - remake device tree binding document according the recommendations of
    Gerhard Sittig, Mark Rutland and Arnd Bergmann;

> known issues:
> - it's yet to get confirmed whether MPC8308 can use slave support or
>    whether the DMA controller's driver shall actively reject it, the
>    information that's available so far suggests that peripheral transfers
>    to IP bus attached I/O is useful and shall not get blocked right away
 - adding support for transfers which don't increment the RAM address or
    do increment the peripheral "port's" address is easy with
    this implementation; but which options of the common API
    should be used for specifying such transfers?


Alexander Popov (6):
  dma: mpc512x: reorder mpc8308 specific instructions
  dma: mpc512x: add support for peripheral transfers
  dma: mpc512x: fix freeing resources in mpc_dma_probe() and
    mpc_dma_remove()
  dma: of: Add common xlate function for matching by channel id
  dma: mpc512x: add device tree binding document
  dma: mpc512x: register for device tree channel lookup

 .../devicetree/bindings/dma/mpc512x-dma.txt        |  51 +++
 arch/powerpc/boot/dts/mpc5121.dtsi                 |   1 +
 drivers/dma/mpc512x_dma.c                          | 345 ++++++++++++++++++---
 drivers/dma/of-dma.c                               |  35 +++
 include/linux/of_dma.h                             |   4 +
 5 files changed, 398 insertions(+), 38 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt

-- 
1.8.4.2

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

[PATCH RFC v11 4/6] dma: of: Add common xlate function for matching by channel id

[Alexander Popov]

This patch adds a new common OF dma xlate callback function which will match a
channel by it's id. The binding expects one integer argument which it will use to
lookup the channel by the id.

Unlike of_dma_simple_xlate this function is able to handle a system with
multiple DMA controllers. When registering the of dma provider with
of_dma_controller_register a pointer to the dma_device struct which is
associated with the dt node needs to passed as the data parameter.
New function will use this pointer to match only channels which belong to the
specified DMA controller.

Signed-off-by: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/of-dma.c   | 35 +++++++++++++++++++++++++++++++++++
 include/linux/of_dma.h |  4 ++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index e8fe9dc..d5fbeaa 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -218,3 +218,38 @@ struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 			&dma_spec->args[0]);
 }
 EXPORT_SYMBOL_GPL(of_dma_simple_xlate);
+
+/**
+ * of_dma_xlate_by_chan_id - Translate dt property to DMA channel by channel id
+ * @dma_spec:	pointer to DMA specifier as found in the device tree
+ * @of_dma:	pointer to DMA controller data
+ *
+ * This function can be used as the of xlate callback for DMA driver which wants
+ * to match the channel based on the channel id. When using this xlate function
+ * the #dma-cells propety of the DMA controller dt node needs to be set to 1.
+ * The data parameter of of_dma_controller_register must be a pointer to the
+ * dma_device struct the function should match upon.
+ *
+ * Returns pointer to appropriate dma channel on success or NULL on error.
+ */
+struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
+					 struct of_dma *ofdma)
+{
+	struct dma_device *dev = ofdma->of_dma_data;
+	struct dma_chan *chan, *candidate = NULL;
+
+	if (!dev || dma_spec->args_count != 1)
+		return NULL;
+
+	list_for_each_entry(chan, &dev->channels, device_node)
+		if (chan->chan_id == dma_spec->args[0]) {
+			candidate = chan;
+			break;
+		}
+
+	if (!candidate)
+		return NULL;
+
+	return dma_get_slave_channel(candidate);
+}
+EXPORT_SYMBOL_GPL(of_dma_xlate_by_chan_id);
diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
index ae36298..56bc026 100644
--- a/include/linux/of_dma.h
+++ b/include/linux/of_dma.h
@@ -41,6 +41,8 @@ extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
 						     const char *name);
 extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
 		struct of_dma *ofdma);
+extern struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
+		struct of_dma *ofdma);
 #else
 static inline int of_dma_controller_register(struct device_node *np,
 		struct dma_chan *(*of_dma_xlate)
@@ -66,6 +68,8 @@ static inline struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_s
 	return NULL;
 }
 
+#define of_dma_xlate_by_chan_id NULL
+
 #endif
 
 #endif /* __LINUX_OF_DMA_H */
-- 
1.8.4.2

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

[PATCH RFC v11 5/6] dma: mpc512x: add device tree binding document

[Alexander Popov]

Introduce a device tree binding document for the MPC512x DMA controller

Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
Signed-off-by: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/dma/mpc512x-dma.txt        | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/mpc512x-dma.txt

diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
new file mode 100644
index 0000000..92eb0d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
@@ -0,0 +1,51 @@
+* Freescale MPC512x and MPC8308 DMA Controller
+
+The DMA controller in the Freescale MPC512x and MPC8308 SoCs can move
+blocks of memory contents between memory and peripherals or
+from memory to memory.
+
+Refer to the "Generic DMA Controller and DMA request bindings" in
+the dma/dma.txt file for a more detailed description of binding.
+
+* DMA controller
+
+Required properties:
+- compatible: Should be one of
+	"fsl,mpc5121-dma"
+	"fsl,mpc8308-dma", "fsl,mpc5121-dma"
+- reg: Address and size of the DMA controller's register set
+- interrupts: Interrupt for the DMA controller. Generic interrupt client node
+	is described in interrupt-controller/interrupts.txt
+
+Optional properties:
+- #dma-cells: The length of the DMA specifier, must be <1> since
+	the DMA controller uses a fixed assignment of request lines
+	per channel. Refer to dma/dma.txt for the detailed description
+	of this property
+
+Example:
+
+	dma0: dma@14000 {
+		compatible = "fsl,mpc5121-dma";
+		reg = <0x14000 0x1800>;
+		interrupts = <65 0x8>;
+		#dma-cells = <1>;
+	};
+
+* DMA client
+
+Required properties:
+- dmas: List of one or more DMA request specifiers. One DMA request specifier
+	consists of a phandle to the DMA controller node followed by
+	the integer specifying the request line
+- dma-names: Contains an identifier string for each DMA request specifier.
+	Refer to dma/dma.txt for the description of this property
+
+Example:
+
+	sdhc@1500 {
+		compatible = "fsl,mpc5121-sdhc";
+		/* ... */
+		dmas = <&dma0 30>;
+		dma-names = "rx-tx";
+	};
-- 
1.8.4.2

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

[PATCH RFC v11 6/6] dma: mpc512x: register for device tree channel lookup

[Alexander Popov]

Register the controller for device tree based lookup of DMA channels
(non-fatal for backwards compatibility with older device trees) and
provide the '#dma-cells' property in the shared mpc5121.dtsi file

Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
Signed-off-by: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/powerpc/boot/dts/mpc5121.dtsi |  1 +
 drivers/dma/mpc512x_dma.c          | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi
index 2c0e155..7f9d14f 100644
--- a/arch/powerpc/boot/dts/mpc5121.dtsi
+++ b/arch/powerpc/boot/dts/mpc5121.dtsi
@@ -498,6 +498,7 @@
 			compatible = "fsl,mpc5121-dma";
 			reg = <0x14000 0x1800>;
 			interrupts = <65 0x8>;
+			#dma-cells = <1>;
 		};
 	};
 
diff --git a/drivers/dma/mpc512x_dma.c b/drivers/dma/mpc512x_dma.c
index 1b90b3b..ab70012 100644
--- a/drivers/dma/mpc512x_dma.c
+++ b/drivers/dma/mpc512x_dma.c
@@ -52,6 +52,7 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
+#include <linux/of_dma.h>
 #include <linux/of_platform.h>
 
 #include <linux/random.h>
@@ -1031,7 +1032,15 @@ static int mpc_dma_probe(struct platform_device *op)
 	if (retval)
 		goto err_free2;
 
-	return retval;
+	/* Register with OF helpers for DMA lookups (nonfatal) */
+	if (dev->of_node) {
+		retval = of_dma_controller_register(dev->of_node,
+						of_dma_xlate_by_chan_id, mdma);
+		if (retval)
+			dev_warn(dev, "Could not register for OF lookup\n");
+	}
+
+	return 0;
 
 err_free2:
 	if (mdma->is_mpc8308)
@@ -1052,6 +1061,8 @@ static int mpc_dma_remove(struct platform_device *op)
 	struct device *dev = &op->dev;
 	struct mpc_dma *mdma = dev_get_drvdata(dev);
 
+	if (dev->of_node)
+		of_dma_controller_free(dev->of_node);
 	dma_async_device_unregister(&mdma->dma);
 	if (mdma->is_mpc8308) {
 		free_irq(mdma->irq2, mdma);
-- 
1.8.4.2

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

Re: [PATCH RFC v11 5/6] dma: mpc512x: add device tree binding document

[Gerhard Sittig]

On Tue, 2014-04-15 at 14:54 +0400, Alexander Popov wrote:
> 
> Introduce a device tree binding document for the MPC512x DMA controller
> 
> Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
> Signed-off-by: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I'm not certain whether the attribution is right.  Is the S-o-b
appropriate when the patch is not "from" me?  As I've stated
before, it's OK if you pick up and extend what I provide, but
please don't pretend that I wrote what you did, and don't pretend
that I ACKed or passed along your submission when I didn't.


This binding certainly needs further improvement to become a good
one.  As I've communicated in the past, I was rather ignorant
"back then" when I wrote v1 and v2 of the RFC.  We have learned
something in the meantime.  Though I admit having gone silent
after several review iterations.  Assumed you would pick up
information that showed up several times on public lists.

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
> @@ -0,0 +1,51 @@
> +* Freescale MPC512x and MPC8308 DMA Controller
> +
> +The DMA controller in the Freescale MPC512x and MPC8308 SoCs can move
> +blocks of memory contents between memory and peripherals or
> +from memory to memory.
> +
> +Refer to the "Generic DMA Controller and DMA request bindings" in
> +the dma/dma.txt file for a more detailed description of binding.
> +
> +* DMA controller
> +
> +Required properties:
> +- compatible: Should be one of
> +	"fsl,mpc5121-dma"
> +	"fsl,mpc8308-dma", "fsl,mpc5121-dma"

is this a duplicate?  looks funny, needs a fix

or is it a requirement that for MPC8308 you need to provide both
compatible strings?  that would be wrong, as MPC8308 certainly is
not an MPC5121

a quick search reveals: the drivers/dma/mpc512x_dma.c Linux
driver implementation is wrong, it should match on both strings;
expecting the MPC8308 to disguise as an MPC5121 when it's not is
inappropriate (and only went unnoticed because of missing
bindings, I guess)

> +- reg: Address and size of the DMA controller's register set
> +- interrupts: Interrupt for the DMA controller. Generic interrupt client node
> +	is described in interrupt-controller/interrupts.txt

'interrupts' only works in combinations with 'interrupt-parent',
that actual .dts files don't have the latter in the nodes is an
implementation detail but not a binding's requirement

and an alternative method of specifying interrupts was introduced
recently, a reference to the common binding without naming one
specific property name could be most appropriate

> +
> +Optional properties:
> +- #dma-cells: The length of the DMA specifier, must be <1> since
> +	the DMA controller uses a fixed assignment of request lines
> +	per channel. Refer to dma/dma.txt for the detailed description
> +	of this property

I'm afraid that a generic/common document does not and cannot
describe the specific semantics of this provider's cells

this binding should explicitly mention that the number of cells
needs to be one, and that this one cell is the DMA channel (which
translates to "peripheral request line"), because these
assigments are fixed in hardware

> +
> +Example:
> +
> +	dma0: dma@14000 {
> +		compatible = "fsl,mpc5121-dma";
> +		reg = <0x14000 0x1800>;
> +		interrupts = <65 0x8>;
> +		#dma-cells = <1>;
> +	};
> +
> +* DMA client

the DMA provider's binding probably need not discuss client
specs, a reference to the common binding should suffice if it's
appropriate at all


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v11 5/6] dma: mpc512x: add device tree binding document

[Alexander Popov]

Thanks for your reply, Gerhard

2014-04-17 0:44 GMT+04:00 Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>:
> On Tue, 2014-04-15 at 14:54 +0400, Alexander Popov wrote:
>>
>> Introduce a device tree binding document for the MPC512x DMA controller
>>
>> Signed-off-by: Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>
>> Signed-off-by: Alexander Popov <a13xp0p0v88-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> I'm not certain whether the attribution is right.  Is the S-o-b
> appropriate when the patch is not "from" me? As I've stated
> before, it's OK if you pick up and extend what I provide, but
> please don't pretend that I wrote what you did,
Thanks. I've read the corresponding part of
Documentation/SubmittingPatches once again and now I see
my mistake.

> and don't pretend
> that I ACKed or passed along your submission when I didn't.
I didn't have any malicious intent.

> This binding certainly needs further improvement to become a good
> one.  As I've communicated in the past, I was rather ignorant
> "back then" when I wrote v1 and v2 of the RFC.  We have learned
> something in the meantime.  Though I admit having gone silent
> after several review iterations.  Assumed you would pick up
> information that showed up several times on public lists.
>
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt
>> @@ -0,0 +1,51 @@
>> +* Freescale MPC512x and MPC8308 DMA Controller
>> +
>> +The DMA controller in the Freescale MPC512x and MPC8308 SoCs can move
>> +blocks of memory contents between memory and peripherals or
>> +from memory to memory.
>> +
>> +Refer to the "Generic DMA Controller and DMA request bindings" in
>> +the dma/dma.txt file for a more detailed description of binding.
>> +
>> +* DMA controller
>> +
>> +Required properties:
>> +- compatible: Should be one of
>> +     "fsl,mpc5121-dma"
>> +     "fsl,mpc8308-dma", "fsl,mpc5121-dma"
>
> is this a duplicate?  looks funny, needs a fix
>
> or is it a requirement that for MPC8308 you need to provide both
> compatible strings?  that would be wrong, as MPC8308 certainly is
> not an MPC5121
>
> a quick search reveals: the drivers/dma/mpc512x_dma.c Linux
> driver implementation is wrong, it should match on both strings;
> expecting the MPC8308 to disguise as an MPC5121 when it's not is
> inappropriate (and only went unnoticed because of missing
> bindings, I guess)
I can try to fix that and add a new patch to the series.

>> +- reg: Address and size of the DMA controller's register set
>> +- interrupts: Interrupt for the DMA controller. Generic interrupt client node
>> +     is described in interrupt-controller/interrupts.txt
>
> 'interrupts' only works in combinations with 'interrupt-parent',
> that actual .dts files don't have the latter in the nodes is an
> implementation detail but not a binding's requirement
Excuse me, I didn't understand your point.

> and an alternative method of specifying interrupts was introduced
> recently, a reference to the common binding without naming one
> specific property name could be most appropriate
Excuse me, I haven't found such an example.

>> +
>> +Optional properties:
>> +- #dma-cells: The length of the DMA specifier, must be <1> since
>> +     the DMA controller uses a fixed assignment of request lines
>> +     per channel. Refer to dma/dma.txt for the detailed description
>> +     of this property
>
> I'm afraid that a generic/common document does not and cannot
> describe the specific semantics of this provider's cells
Ok, I see.

> this binding should explicitly mention that the number of cells
> needs to be one, and that this one cell is the DMA channel (which
> translates to "peripheral request line"), because these
> assigments are fixed in hardware
Ok.

>> +
>> +Example:
>> +
>> +     dma0: dma@14000 {
>> +             compatible = "fsl,mpc5121-dma";
>> +             reg = <0x14000 0x1800>;
>> +             interrupts = <65 0x8>;
>> +             #dma-cells = <1>;
>> +     };
>> +
>> +* DMA client
>
> the DMA provider's binding probably need not discuss client
> specs, a reference to the common binding should suffice if it's
> appropriate at all
Ok.

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

Re: [PATCH RFC v11 5/6] dma: mpc512x: add device tree binding document

[Gerhard Sittig]

On Fri, 2014-04-18 at 15:29 +0400, Alexander Popov wrote:
> 
> 2014-04-17 0:44 GMT+04:00 Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org>:
> > On Tue, 2014-04-15 at 14:54 +0400, Alexander Popov wrote:
> >>
> >> +- reg: Address and size of the DMA controller's register set
> >> +- interrupts: Interrupt for the DMA controller. Generic interrupt client node
> >> +     is described in interrupt-controller/interrupts.txt
> >
> > 'interrupts' only works in combinations with 'interrupt-parent',
> > that actual .dts files don't have the latter in the nodes is an
> > implementation detail but not a binding's requirement
> Excuse me, I didn't understand your point.
> 
> > and an alternative method of specifying interrupts was introduced
> > recently, a reference to the common binding without naming one
> > specific property name could be most appropriate
> Excuse me, I haven't found such an example.

The 'interrupts' property is not enough in itself, it always
needs the 'interrupt-parent' property, too.  Because the parent
(the interrupt controller) often is the same for multiple
interrupts, it usually gets "factored out" into a parent node in
the tree, and thus often gets missed in discussions.  Still the
'interrupt-parent' is strictly required for 'interrupts' to work.
Splitting both properties and putting them into nodes that are
rather distant from each other is just an implementation detail
of .dts files.  This should not be reflected in bindings.

The 'interrupts-extended' property was introduced only recently.
See bindings/interrupt-controller/interrupts.txt for details.
Although the motivation was to reference several parents from one
client node, I very much like the idea of having all of the
interrupt spec within a single property.  Being explicit is a
good thing, especially in setups with cascades.  I consider this
approach an improvement in readability and maintenance.

You might just want to document in the binding that interrupt
specs are required (or optional), which interrupts these are (the
above text could be sufficient if there is only one interrupt for
this IP block), and refer to the common binding for the syntax.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html