On Fri, Feb 11, 2011 at 04:51:35PM +0100, Arnd Bergmann wrote:

On Saturday 12 February 2011, Shawn Guo wrote:

quoted

On Thu, Feb 10, 2011 at 06:17:41PM +0100, Arnd Bergmann wrote:

quoted

On Friday 11 February 2011, Shawn Guo wrote:

quoted

quoted

quoted

+	struct mxs_dma_data		dma_data;

Why do you need host specific DMA structures? Please stick to
the generic dma-engine API if possible.

I'm sticking to the generic dmaengine api.  The mxs dma hardware
has separate irq line for every single dma channel, which needs to
be told by client driver.

I'm not convinced, it still sounds like a layering violation to
have specific information about the DMA controller in the
platform data of a driver using the dma engine API.

It sounds like something about the dma controller, but it really
belongs to dma client device e.g. ssp here.  Every single dma client
device has two dma related resources, dma channel and dma irq, which
should be defined in client device data.

That is true for the DMA controller you use, but it doesn't have
to be that way for all other DMA controllers, right? My point is
that the MMC driver should not make any assumptions about what
DMA controller is used, because a future SoC might combine it
with a different DMA controller, e.g. one that just just a single
interrupt for all channels.

Your thought is right.  But the fact is, from hw design point of
view, all client peripherals are coupled with mxs dma.  As a real
example, gpmi (mxs nand controller) is one mxs dma client device.
When gpmi is integrated into i.MX50 to replace nfc (original i.mx
nand controller), dma-aphb has to be brought into mx50 together,
though mx50 already gets sdma there.

I tried hard to decouple client driver from mxs dmaengine driver, but
it can not be that thorough. As dma peripheral only trigger error irq,
mxs dma "pio" mode and dma irq need to be handled by client and dma
driver in a coupled way.

quoted

quoted

Why can't you put the interrupt number into the platform data of
the dma engine device? Your filter function already identifies
the number of the DMA channel.

We have 16 channels for dma-apbh and dma-apbx respectively.  And each
channel has fixed peripheral device and irq.  You think we can define
2 x 16 x (channel number + channel irq) in dma engine driver?  I'm
afraid not.  The channel number can be identified in filter function
because dmaengine core code and mxs dma hw are indexing the channel
in the same way, so that the sw channel id can be used to address hw
channel, otherwise we have to pass hw channel id to dma driver just
like what we do with irq.

I don't understand. The sw channel id should be something that is
defined in an arbitrary way for each machine, and it should be the
only thing that a driver needs, right?

Yes, the sw channel id can be defined in an arbitrary way by
dmaengine, but in that case dmaegine can not use sw channel id to
access hw channel, because each mxs dma client device gets a specific
hw channel.  Of course, we can give device an arbitrary channel with
a fixed hw channel id attached on, and use hw id to access hw channel.
But in that case, we have one more info in device data to pass besides
irq.

I have not looked much at other dmaengine drivers, but I'd be
surprised if they require the device driver to be written
for a specific implementation. If that was the case, you would
not even need a dmaengine API but could just as well write
to the DMA controller registers from the device driver directly.

We need a specific implementation, but it's not so specific that we
have to access dma controller directly.  Even it is, we still need
an API/interface, as there are so many client devices need to do the
same thing, right? ;)

quoted

quoted

What I meant is that you take care to avoid getting into the
interrupt handler while holding the spinlock, but in the handler,
you don't check if the lock is held. It can't be correct to
serialize just half the cases.

Thanks for the explanation.  Please help review the fix below to see
if I understand the comment correctly.

        if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN)) {
                spin_lock_irqsave(&host->lock, flags);
                mmc_signal_sdio_irq(host->mmc);
                spin_unlock_irqrestore(&host->lock, flags);
        }

This is still wrong for two reasons:

* You now don't hold the lock while reading the 'stat' variable. The point of the
  lock is to make sure that sdio doesn't get turned off after reading stat but
  before signaling the sdio, so you have to hold it across both.
* In an interrupt controller, you should not disable interrupts again.
  It's harmless, but slow and confusing to the reader.

Sorry for the dumb.  So it should be something like the following?

        spin_lock(&host->lock);

        stat = readl(host->base + HW_SSP_CTRL1);
        writel(stat & MXS_MMC_IRQ_BITS,
                host->base + HW_SSP_CTRL1 + MXS_CLR_ADDR);

	[...]

        if ((stat & BM_SSP_CTRL1_SDIO_IRQ) && (stat & BM_SSP_CTRL1_SDIO_IRQ_EN))
                mmc_signal_sdio_irq(host->mmc);

        spin_unlock(&host->lock);

Regards,
Shawn