| Message ID | 1387886789-20249-5-git-send-email-a13xp0p0v88@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote: > > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt > @@ -0,0 +1,55 @@ > +* Freescale MPC512x DMA Controller > + > +[ ... ] > + > +DMA controller node properties: > + > +Required properties: > +- compatible: should be "fsl,mpc5121-dma" > +- reg: address and size of the DMA controller's register set > +- interrupts: interrupt spec for the DMA controller > + > +Optional properties: > +- #dma-cells: must be <1>, describes the number of integer cells > + needed to specify the 'dmas' property in client nodes, > + strongly recommended since common client helper code > + uses this property Given how time has passed and that we learned something in the meantime, I guess the device tree documentation would look different today than what was written back then. - I'd reference the generic interrupt bindings as well, so DTS authors need not guess what an interrupt spec looks like - the #dma-cells would become less confusing is it referenced the generic DMA binding, and just say that the value is "the length of the DMA specifier" for this provider - the property's being recommended should not get hidden in the description but should reflect in the group's caption - the binding doc shold not reference implementation details of one specific user (common client helper code) [ device tree binding doc policy? ] That one Linux driver handles both MPC5121 and MPC8308 hardware and implements the same binding in both cases should get reflected in the documentation as well. But I'm not certain whether adding MPC8308 into an MPX5121 document is better than duplicating MPC5121 information in another MPC8308 document. But it might be the lesser evil. Are there opinions, established preferences? Is an exhaustive list of compatible strings good enough since text search will match regardless of the document's filename in this case? There must have been this situation before of a component being used in one SoC and getting re-used in another SoC later, too. What's the best document to "get inspired from", i.e. how to best put this into binding document wording as well as filenames? > +[ ... ] > +Client node properties: > + > +Required properties: > +- dmas: list of DMA specifiers, consisting each of a handle > + for the DMA controller and integer cells to specify > + the channel used within the DMA controller > +- dma-names: list of identifier strings for the DMA specifiers, > + client device driver code uses these strings to > + have DMA channels looked up at the controller This certainly is wrong (it was before, I just wasn't aware back then). The phandle is not part of the specifier. And the binding should not discuss what driver code does. Since the DMA controller implements the semantics of the common DMA binding, it's unwise to duplicate the information here. Let me see how I can improve this document. Alex, it may be useful to split the code update and the binding document into separate patches. The current status already mixes the code extension and the binding update with the introduction of the document which was missing in the first place. That's why the binding doc patch is that late in the series. (Yes, my RFC "template" was rather dirty, which is why I flagged it as "RFC" in the first place.) I guess that I may have to introduce a binding doc reflecting the given current status, and update it later as new features become available. Or -- given that the hardware remains, all the knowledge is there already, just the implementations' capabilities change -- I might as well introduce a binding document including OF based DMA lookup. virtually yours Gerhard Sittig
[ dropping devicetree, we're DMA specific here ] On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote: > > --- a/drivers/dma/mpc512x_dma.c > +++ b/drivers/dma/mpc512x_dma.c > [ ... ] > @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op) > INIT_LIST_HEAD(&dma->channels); > dma_cap_set(DMA_MEMCPY, dma->cap_mask); > dma_cap_set(DMA_SLAVE, dma->cap_mask); > + dma_cap_set(DMA_PRIVATE, dma->cap_mask); > > for (i = 0; i < dma->chancnt; i++) { > mchan = &mdma->channels[i]; What are the implications of this? Is a comment due? I haven't found documentation about the DMA_PRIVATE flag, only saw commit 59b5ec21446b9 "dmaengine: introduce dma_request_channel and private channels". Alex, unless I'm missing something this one-line change is quite a change in semantics, and has dramatic influence on the code's behaviour (ignores the DMA controller when looking for channels that can do mem-to-mem transfers). Please reason about this change some more, and explain what it does and why it's needed. Consider the fact that this driver handles both MPC5121 as well as MPC8308 hardware. virtually yours Gerhard Sittig
Hello Gerhard. Thanks for your review. 2013/12/26 Gerhard Sittig <gsi@denx.de>: > [ dropping devicetree, we're DMA specific here ] > > On Tue, Dec 24, 2013 at 16:06 +0400, Alexander Popov wrote: >> >> --- a/drivers/dma/mpc512x_dma.c >> +++ b/drivers/dma/mpc512x_dma.c >> [ ... ] >> @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op) >> INIT_LIST_HEAD(&dma->channels); >> dma_cap_set(DMA_MEMCPY, dma->cap_mask); >> dma_cap_set(DMA_SLAVE, dma->cap_mask); >> + dma_cap_set(DMA_PRIVATE, dma->cap_mask); >> >> for (i = 0; i < dma->chancnt; i++) { >> mchan = &mdma->channels[i]; > > What are the implications of this? Is a comment due? I've involved DMA_PRIVATE flag because new of_dma_xlate_by_chan_id() uses dma_get_slave_channel() instead of dma_request_channel() (PATCH RFC v6 3/5). This flag is implicitly set in dma_request_channel(), but is not set in dma_get_slave_channel(). There are only two places in the mainline kernel, where dma_get_slave_channel() is used. I've picked up the idea at one of these places. Please look at this patch: http://www.spinics.net/lists/arm-kernel/msg268718.html > I haven't found documentation about the DMA_PRIVATE flag, only > saw commit 59b5ec21446b9 "dmaengine: introduce > dma_request_channel and private channels". Unfortunately I didn't find any description of DMA_PRIVATE flag too. But the comment at the beginning of drivers/dma/dmaengine.c may give a clue. Quotation: * subsystem can get access to a channel by calling dmaengine_get() followed * by dma_find_channel(), or if it has need for an exclusive channel it can call * dma_request_channel(). Once a channel is allocated a reference is taken * against its corresponding driver to disable removal. DMA_PRIVATE capability flag might indicate that the DMA controller can provide exclusive channels to its clients. Please correct me if I'm wrong. > Alex, unless I'm > missing something this one-line change is quite a change in > semantics, and has dramatic influence on the code's behaviour > (ignores the DMA controller when looking for channels that can do > mem-to-mem transfers) Excuse me, Gerhard, I don't see what you mean. Could you point to the corresponding code? > Consider the fact that this driver > handles both MPC5121 as well as MPC8308 hardware. Ah, yes, sorry. I should certainly fix this, if setting of DMA_PRIVATE flag is needed at all. Thanks! Best regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt new file mode 100644 index 0000000..a4867d5 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt @@ -0,0 +1,55 @@ +* Freescale MPC512x DMA Controller + +The DMA controller in the Freescale MPC512x SoC can move blocks of +memory contents between memory and peripherals or memory to memory. + +Refer to the "Generic DMA Controller and DMA request bindings" description +in the dma.txt file for a more detailled discussion of the binding. The +MPC512x DMA engine binding follows the common scheme, but doesn't provide +support for the optional channels and requests counters (those values are +derived from the detected hardware features) and has a fixed client +specifier length of 1 integer cell (the value is the DMA channel, since +the DMA controller uses a fixed assignment of request lines per channel). + + +DMA controller node properties: + +Required properties: +- compatible: should be "fsl,mpc5121-dma" +- reg: address and size of the DMA controller's register set +- interrupts: interrupt spec for the DMA controller + +Optional properties: +- #dma-cells: must be <1>, describes the number of integer cells + needed to specify the 'dmas' property in client nodes, + strongly recommended since common client helper code + uses this property + +Example: + + dma0: dma@14000 { + compatible = "fsl,mpc5121-dma"; + reg = <0x14000 0x1800>; + interrupts = <65 0x8>; + #dma-cells = <1>; + }; + + +Client node properties: + +Required properties: +- dmas: list of DMA specifiers, consisting each of a handle + for the DMA controller and integer cells to specify + the channel used within the DMA controller +- dma-names: list of identifier strings for the DMA specifiers, + client device driver code uses these strings to + have DMA channels looked up at the controller + +Example: + + sdhc@1500 { + compatible = "fsl,mpc5121-sdhc"; + /* ... */ + dmas = <&dma0 30>; + dma-names = "rx-tx"; + }; diff --git a/arch/powerpc/boot/dts/mpc5121.dtsi b/arch/powerpc/boot/dts/mpc5121.dtsi index 2d7cb04..15b7860 100644 --- a/arch/powerpc/boot/dts/mpc5121.dtsi +++ b/arch/powerpc/boot/dts/mpc5121.dtsi @@ -389,6 +389,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 a7e7749..8fabb52 100644 --- a/drivers/dma/mpc512x_dma.c +++ b/drivers/dma/mpc512x_dma.c @@ -50,6 +50,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> @@ -950,6 +951,7 @@ static int mpc_dma_probe(struct platform_device *op) INIT_LIST_HEAD(&dma->channels); dma_cap_set(DMA_MEMCPY, dma->cap_mask); dma_cap_set(DMA_SLAVE, dma->cap_mask); + dma_cap_set(DMA_PRIVATE, dma->cap_mask); for (i = 0; i < dma->chancnt; i++) { mchan = &mdma->channels[i]; @@ -1013,11 +1015,23 @@ static int mpc_dma_probe(struct platform_device *op) /* Register DMA engine */ dev_set_drvdata(dev, mdma); retval = dma_async_device_register(dma); - if (retval) { - devm_free_irq(dev, mdma->irq, mdma); - irq_dispose_mapping(mdma->irq); + if (retval) + goto out_irq; + + /* 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; + +out_irq: + devm_free_irq(dev, mdma->irq, mdma); + irq_dispose_mapping(mdma->irq); return retval; } @@ -1026,6 +1040,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); devm_free_irq(dev, mdma->irq, mdma); irq_dispose_mapping(mdma->irq);