DMAENGINE: generic slave channel control v2
in [Kernel]
|
Prev: [PATCH 2/3] drivers:staging:ti-st: cleanup code comments
Next: 2.6.32: <IRQ> __alloc_pages_nodemask+0x54b/0x595
From: Linus Walleij on 22 Jul 2010 06:50 2010/7/21 Linus Walleij <linus.walleij(a)stericsson.com>: > This adds an interface to the DMAengine to make it possible to > reconfigure a slave channel at runtime. We add a few foreseen > config parameters to the passed struct, with a void * pointer > for custom per-device or per-platform runtime slave data. BTW Vinod, if you're happy with this API, then please ACK it so Dan has some indication whether it'll fit the Moorestown too. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Koul, Vinod on 22 Jul 2010 12:20 > 2010/7/22 Linus Walleij <linus.walleij(a)stericsson.com>: > > > This adds an interface to the DMAengine to make it possible to > > reconfigure a slave channel at runtime. We add a few foreseen > > config parameters to the passed struct, with a void * pointer > > for custom per-device or per-platform runtime slave data. > > BTW Vinod, if you're happy with this API, then please ACK it so > Dan has some indication whether it'll fit the Moorestown too. Shouldn't this patch remove the private member in dma_chan structure Currently chan->private is used for sending slave or similar channel specific information. Now if we want to add struct dma_slave_config, then IMHO it would make sense to remove private variable and replace with dma_slave_config struture. That way we can reuse this struture there as well and if someone wants to add more stuff he can use the private_config. Dan, what do you think about this? Thanks -Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Walleij on 22 Jul 2010 18:00 2010/7/22 Koul, Vinod <vinod.koul(a)intel.com>: > Shouldn't this patch remove the private member in dma_chan structure I don't think so. > Currently chan->private is used for sending slave or similar channel specific > information. Now if we want to add struct dma_slave_config, then IMHO it > would make sense to remove private variable and replace with dma_slave_config > struture. That way we can reuse this struture there as well and if someone wants > to add more stuff he can use the private_config. That member is described like this: @private: private data for certain client-channel associations chan->private is supposed to only be used inside the dmaengine drivers themselves AFAICT. Some drivers (like the txx9dmac.c) use this to hold the state of the slave, whereas the void *private in the runtime config is supposed to contain some custom configuration struct which is created outside of the dmaengine framework and can be discarded after use (if you so wish), so that's vastly different. You could however argue (but this is another discussion altogether) that using chan->private to hold any states is superfluous since you could just have your struct dma_channel as a member of your custom channel struct and use container_of to dereference it (as we do in the coh901318 and ste_dma40 drivers) and then stockpile any other struct members or substructs into your custom struct but that's another issue, which would require major refactoring of these drivers. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dan Williams on 22 Jul 2010 19:20 On Thu, Jul 22, 2010 at 9:13 AM, Koul, Vinod <vinod.koul(a)intel.com> wrote: >> 2010/7/22 Linus Walleij <linus.walleij(a)stericsson.com>: >> >> > This adds an interface to the DMAengine to make it possible to >> > reconfigure a slave channel at runtime. We add a few foreseen >> > config parameters to the passed struct, with a void * pointer >> > for custom per-device or per-platform runtime slave data. >> >> BTW Vinod, if you're happy with this API, then please ACK it so >> Dan has some indication whether it'll fit the Moorestown too. > > Shouldn't this patch remove the private member in dma_chan structure > > Currently chan->private is used for sending slave or similar channel specific > information. Now if we want to add struct dma_slave_config, then IMHO it > would make sense to remove private variable and replace with dma_slave_config > struture. That way we can reuse this struture there as well and if someone wants > to add more stuff he can use the private_config. > > Dan, what do you think about this? If you take a look at the current usages of chan->private I don't think all of them are met by this interface. We have: struct at_dma_slave { struct device *dma_dev; dma_addr_t tx_reg; dma_addr_t rx_reg; enum at_dma_slave_width reg_width; u32 cfg; u32 ctrla; }; struct dw_dma_slave { struct device *dma_dev; dma_addr_t tx_reg; dma_addr_t rx_reg; enum dw_dma_slave_width reg_width; u32 cfg_hi; u32 cfg_lo; }; struct fsl_dma_slave { /* List of hardware address/length pairs */ struct list_head addresses; /* Support for extra controller features */ unsigned int request_count; unsigned int src_loop_size; unsigned int dst_loop_size; bool external_start; bool external_pause; }; struct dma_pl330_peri { /* * Peri_Req i/f of the DMAC that is * peripheral could be reached from. */ u8 peri_id; /* {0, 31} */ enum pl330_reqtype rqtype; /* For M->D and D->M Channels */ int burst_sz; /* in power of 2 */ dma_addr_t fifo_addr; }; struct sh_dmae_slave { unsigned int slave_id; /* Set by the platform */ struct device *dma_dev; /* Set by the platform */ const struct sh_dmae_slave_config *config; /* Set by the driver */ }; struct sh_dmae_slave_config { unsigned int slave_id; dma_addr_t addr; u32 chcr; char mid_rid; }; struct txx9dmac_slave { u64 tx_reg; u64 rx_reg; unsigned int reg_width; }; ....and I don't think this interface should try to meet all these requirements because there will always be arch-specific quirks that make things fall down. I think we should just start with an interface that is minimally useful for the drivers that want to take advantage of it. We could, since there is usually driver-specific knowledge known by the client in the dma-slave case, just require that a dma_slave_config be container_of() promoted to a driver specific config. This lets the non-esoteric platform configurations use the generic definition. -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dan Williams on 22 Jul 2010 19:40
Hi Linus, On Wed, Jul 21, 2010 at 12:22 PM, Linus Walleij <linus.walleij(a)stericsson.com> wrote: > This adds an interface to the DMAengine to make it possible to > reconfigure a slave channel at runtime. We add a few foreseen > config parameters to the passed struct, with a void * pointer > for custom per-device or per-platform runtime slave data. > > Signed-off-by: Linus Walleij <linus.walleij(a)stericsson.com> > --- > This version was revised after discussion, it splits the control > arguments in source/destination pairs to prepare for (among other > things) device to device transfers. > --- > �include/linux/dmaengine.h | � 75 +++++++++++++++++++++++++++++++++++++++++++++ > �1 files changed, 75 insertions(+), 0 deletions(-) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 5204f01..406b820 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -114,11 +114,17 @@ enum dma_ctrl_flags { > �* @DMA_TERMINATE_ALL: terminate all ongoing transfers > �* @DMA_PAUSE: pause ongoing transfers > �* @DMA_RESUME: resume paused transfer > + * @DMA_SLAVE_CONFIG: this command is only implemented by DMA controllers > + * that need to runtime reconfigure the slave channels (as opposed to passing > + * configuration data in statically from the platform). An additional > + * argument of struct dma_slave_config must be passed in with this > + * command. > �*/ > �enum dma_ctrl_cmd { > � � � �DMA_TERMINATE_ALL, > � � � �DMA_PAUSE, > � � � �DMA_RESUME, > + � � � DMA_SLAVE_CONFIG, > �}; > > �/** > @@ -199,6 +205,75 @@ struct dma_chan_dev { > � � � �atomic_t *idr_ref; > �}; > > +/** > + * enum dma_slave_buswidth - defines bus with of the DMA slave > + * device, source or target busses s/busses/buses/ > + */ > +enum dma_slave_buswidth { > + � � � DMA_SLAVE_BUSWIDTH_UNDEFINED = 0, > + � � � DMA_SLAVE_BUSWIDTH_1_BYTE = 1, > + � � � DMA_SLAVE_BUSWIDTH_2_BYTES = 2, > + � � � DMA_SLAVE_BUSWIDTH_4_BYTES = 4, > + � � � DMA_SLAVE_BUSWIDTH_8_BYTES = 8, > +}; > + > +/** > + * struct dma_slave_config - dma slave channel runtime config > + * @direction: whether the data shall go in or out on this slave > + * channel, right now. DMA_TO_DEVICE and DMA_FROM_DEVICE are > + * legal values, DMA_BIDIRECTIONAL is not acceptable since we > + * need to differentiate source and target addresses. > + * @src_addr: this is the physical address where DMA slave data > + * should be read (RX), if the source is memory this argument is > + * ignored. > + * @dst_addr: this is the physical address where DMA slave data > + * should be written (TX), if the source is memory this argument > + * is ignored. > + * @src_addr_width: this is the width in bytes of the source (RX) > + * register where DMA data shall be read. If the source > + * is memory this may be ignored depending on architecture. > + * Legal values: 1, 2, 4, 8. > + * @dst_addr_width: same as src_addr_width but for destination > + * target (TX) mutatis mutandis. /me wikipedias "mutatis mutandis", ah sort of latin for the s/// operator :-). > + * @src_maxburst: the maximum number of words (note: words, as in > + * units of the src_addr_width member, not bytes) that can be sent > + * in one burst to the device. Typically something like half the > + * FIFO depth on I/O peripherals so you don't overflow it. This > + * may or may not be applicable on memory sources. > + * @dst_maxburst: same as src_maxburst but for destination target > + * mutatus mutandis. s/mutatus/mutatis/ > + * @private_config: if you need to pass in specialized configuration > + * at runtime, apart from the generic things supported in this > + * struct, you provide it in this pointer and dereference it inside > + * your dmaengine driver to get the proper configuration bits out. > + * Will there be any users of this field initially, I'd just as soon leave it out? Like I mentioned to Vinod I think requiring dma_slave_config to be wrapped is a better way to extend this structure. -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |