Re: [PATCH 2/2] dmaengine: usb-dmac: Add Renesas USB DMA Controller (USB-DMAC) driver

[Vinod Koul <vinod.koul@intel.com>]

On Mon, Feb 09, 2015 at 05:14:05PM +0900, Yoshihiro Shimoda wrote:
> +struct usb_dmac_chan {
> +	struct dma_chan chan;
> +	void __iomem *iomem;
> +	unsigned int index;
> +
> +	spinlock_t lock;
> +
> +	struct {
> +		struct list_head free;
> +		struct list_head pending;
> +		struct list_head active;
> +		struct list_head done;
> +		struct list_head wait;
> +		struct usb_dmac_desc *running;
> +		struct usb_dmac_desc *last_done;
> +
> +		struct list_head chunks_free;
> +
> +		struct list_head pages;
Thats too many lists, do we need so many? Shouldn't free and done be same
thing. Similarly whats meant by wait here?
Do you submit multiple descriptors to HW?

> +static dma_cookie_t usb_dmac_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> +	struct usb_dmac_chan *chan = to_usb_dmac_chan(tx->chan);
> +	struct usb_dmac_desc *desc = to_usb_dmac_desc(tx);
> +	unsigned long flags;
> +	dma_cookie_t cookie;
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	cookie = dma_cookie_assign(tx);
> +
> +	dev_dbg(chan->chan.device->dev, "chan%u: submit #%d@%p\n",
> +		chan->index, tx->cookie, desc);
> +
> +	list_add_tail(&desc->node, &chan->desc.pending);
> +	desc->running = list_first_entry(&desc->chunks,
> +					 struct usb_dmac_xfer_chunk, node);
what is this required for?

> +static void usb_dmac_desc_put(struct usb_dmac_chan *chan,
> +			      struct usb_dmac_desc *desc)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +	list_splice_tail_init(&desc->chunks, &chan->desc.chunks_free);
> +	list_add_tail(&desc->node, &chan->desc.free);
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +}
> +
> +static void usb_dmac_desc_recycle_acked(struct usb_dmac_chan *chan)
what is meant by acked here, and by whom?

> +{
> +	struct usb_dmac_desc *desc, *_desc;
> +	unsigned long flags;
> +	LIST_HEAD(list);
> +
> +	/*
> +	 * We have to temporarily move all descriptors from the wait list to a
> +	 * local list as iterating over the wait list, even with
> +	 * list_for_each_entry_safe, isn't safe if we release the channel lock
> +	 * around the usb_dmac_desc_put() call.
> +	 */
> +	spin_lock_irqsave(&chan->lock, flags);
> +	list_splice_init(&chan->desc.wait, &list);
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	list_for_each_entry_safe(desc, _desc, &list, node) {
> +		if (async_tx_test_ack(&desc->async_tx)) {
Hmmm, this part is not correct. ACK is for async_tx API and not for slave
dmaengine drivers.

> +static int usb_dmac_alloc_chan_resources(struct dma_chan *chan)
> +{
> +	struct usb_dmac_chan *uchan = to_usb_dmac_chan(chan);
> +	int ret;
> +
> +	INIT_LIST_HEAD(&uchan->desc.chunks_free);
> +	INIT_LIST_HEAD(&uchan->desc.pages);
> +
> +	/* Preallocate descriptors. */
> +	ret = usb_dmac_xfer_chunk_alloc(uchan, GFP_KERNEL);
GFP_NOWAIT

> +	if (ret < 0)
> +		return -ENOMEM;
> +
> +	ret = usb_dmac_desc_alloc(uchan, GFP_KERNEL);
Ditto

> +static int usb_dmac_chan_terminate_all(struct dma_chan *chan)
> +{
> +	struct usb_dmac_chan *uchan = to_usb_dmac_chan(chan);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&uchan->lock, flags);
> +	usb_dmac_chan_halt(uchan);
> +	spin_unlock_irqrestore(&uchan->lock, flags);
> +
> +	/*
> +	 * FIXME: No new interrupt can occur now, but the IRQ thread might still
> +	 * be running.
> +	 */
and when ?

> +static unsigned int
> +usb_dmac_chan_get_residue_if_complete(struct usb_dmac_chan *chan)
> +{
> +	struct usb_dmac_desc *desc = chan->desc.last_done;
> +	struct usb_dmac_xfer_chunk *chunk = desc ? desc->running : NULL;
> +
> +	if (!chunk)
> +		return 0;
> +
> +	return usb_dmac_chan_get_last_residue(chan, chunk, desc->direction);
and this calls for completed descriptor, wont reading HW be wrong here as
you might have submitted another descriptor?


> +static enum dma_status usb_dmac_tx_status(struct dma_chan *chan,
> +					  dma_cookie_t cookie,
> +					  struct dma_tx_state *txstate)
> +{
> +	struct usb_dmac_chan *uchan = to_usb_dmac_chan(chan);
> +	enum dma_status status;
> +	unsigned long flags;
> +	unsigned int residue;
> +
> +	status = dma_cookie_status(chan, cookie, txstate);
> +	/* a client driver will get residue after DMA_COMPLETE */
> +	if (!txstate)
> +		return status;
> +
> +	spin_lock_irqsave(&uchan->lock, flags);
> +	if (status = DMA_COMPLETE)
> +		residue = usb_dmac_chan_get_residue_if_complete(uchan);
if it is completed then residue should be zero, so why are we computing this

> +
> +#ifdef CONFIG_PM_SLEEP
> +static int usb_dmac_sleep_suspend(struct device *dev)
> +{
> +	/*
> +	 * TODO: Wait for the current transfer to complete and stop the device.
> +	 */
> +	return 0;
> +}
> +
> +static int usb_dmac_sleep_resume(struct device *dev)
> +{
> +	/* TODO: Resume transfers, if any. */
> +	return 0;
> +}
> +#endif
what is the point of these?

> +
> +#ifdef CONFIG_PM
> +static int usb_dmac_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
ditto

> +static int usb_dmac_chan_probe(struct usb_dmac *dmac,
> +			       struct usb_dmac_chan *uchan,
> +			       unsigned int index)
> +{
> +	struct platform_device *pdev = to_platform_device(dmac->dev);
> +	struct dma_chan *chan = &uchan->chan;
> +	char pdev_irqname[5];
> +	char *irqname;
> +	int irq;
> +	int ret;
> +
> +	uchan->index = index;
> +	uchan->iomem = dmac->iomem + USB_DMAC_CHAN_OFFSET(index);
> +
> +	spin_lock_init(&uchan->lock);
> +
> +	INIT_LIST_HEAD(&uchan->desc.free);
> +	INIT_LIST_HEAD(&uchan->desc.pending);
> +	INIT_LIST_HEAD(&uchan->desc.active);
> +	INIT_LIST_HEAD(&uchan->desc.done);
> +	INIT_LIST_HEAD(&uchan->desc.wait);
> +
> +	/* Request the channel interrupt. */
> +	sprintf(pdev_irqname, "ch%u", index);
> +	irq = platform_get_irq_byname(pdev, pdev_irqname);
> +	if (irq < 0) {
> +		dev_err(dmac->dev, "no IRQ specified for channel %u\n", index);
> +		return -ENODEV;
> +	}
> +
> +	irqname = devm_kasprintf(dmac->dev, GFP_KERNEL, "%s:%u",
> +				 dev_name(dmac->dev), index);
> +	if (!irqname)
> +		return -ENOMEM;
> +
> +	ret = devm_request_threaded_irq(dmac->dev, irq, usb_dmac_isr_channel,
> +					usb_dmac_isr_channel_thread,
> +					IRQF_SHARED,
> +					irqname, uchan);
DMA engine API expects that you are running a tasklet not a thread


> +
> +static int usb_dmac_remove(struct platform_device *pdev)
> +{
> +	struct usb_dmac *dmac = platform_get_drvdata(pdev);
> +
> +	of_dma_controller_free(pdev->dev.of_node);
> +	dma_async_device_unregister(&dmac->engine);
> +
> +	pm_runtime_disable(&pdev->dev);
and you have not freed or disabled your irq, neither ensured all
threads/tasklets running are stopped

Overall i feel descriptor management is overtly complicated. Can you see if
you can you use virt-dma infrastructure, that will ease you descriptor management
a lot

Thanks
-- 
~Vinod