Re: [PATCH 1/2] dmaengine: TXx9 Soc DMA Controller driver

[Atsushi Nemoto]

On Mon, 16 Mar 2009 14:20:56 -0700, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> > Maybe I should move this DMA_CTRL_ACK setting to txx9dmac_desc_put()?
> 
> Perhaps a comment.  I think this scheme is ok, it just raised alarm
> bells as I read it.

OK, I will do.

> >> > +       disable_irq_nosync(irq);
> >> > +
> >> > +       return IRQ_HANDLED;
> >> > +}
> >>
> >> Why do you need to disable interrupts here?
> >
> > Because interrupts are not cleared until txx9dmac_tasklet() calls
> > txx9dmac_scan_descriptors() and it writes to CSR.  Touching CSR in
> > txx9dmac_interrupt() seems bad while dc->lock spinlock does not
> > protect from interrupts.  I chose calling disable_irq here instead of
> > replace all spin_lock with spin_lock_irqsave.
> 
> I believe in this case you are protected by the fact this IRQ handler
> will not race against itself, i.e. even though other interrupts are
> enabled this handler will be masked until it returns.

Yes, IRQ handler will be masked, but tasklet will not be masked.  If I
did not disable irq here, the kernel hangs just after returning from
this IRQ handler (and before tasklet routine is invoked).

> > I need the reserved_chan to make channel 3 named "dma0chan3".  If I
> > can chose chan_id for each channels in dma_device, the reserved_chan
> > is not needed.
> 
> Can you post the code that communicates chan_id to the routine calling
> dma_request_channel?  I am not understanding why you need to control
> chan_id.  Why not have the filter_fn passed to dma_request_channel
> ignore non-private devices?

You mean the filter_fn provided by client driver?  I don't want to let
client driver know which channel is used for memcpy.  And if
"dma0chan3" was not the Ch3 of the DMAC, it looks confusing...

Here is an excerpt from client under construction.

struct txx9aclc_dmadata {
	struct resource *dma_res;
	struct txx9dmac_slave dma_slave;
	struct dma_chan *dma_chan;
	...
};

static bool filter(struct dma_chan *chan, void *param)
{
	struct txx9aclc_dmadata *dmadata = param;

	if (strcmp(dev_name(chan->device->dev), dmadata->dma_res->name) == 0 &&
	    dmadata->dma_res->start == chan->chan_id) {
		chan->private = &dmadata->dma_slave;
		return true;
	}
	return false;
}

	struct txx9dmac_slave *ds = &dmadata->dma_slave;
	...
	dmadata->dma_res = platform_get_resource(pdev, IORESOURCE_DMA, 0);
	...
	dmadata->dma_chan = dma_request_channel(mask, filter, dmadata);

The IORESOURCE_DMA resource for the client device contains a name of a
DMA driver (dma_res->name) and its channel ID (dma_res->start).

---
Atsushi Nemoto
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/