Patchwork [v7,1/3] MTD : add the common code for GPMI-NFC controller driver

login
register
mail settings
Submitter Shawn Guo
Date June 29, 2011, 2 p.m.
Message ID <20110629140038.GB25931@S2100-06.ap.freescale.net>
Download mbox | patch
Permalink /patch/102605/
State New
Headers show

Comments

Shawn Guo - June 29, 2011, 2 p.m.
On Wed, Jun 29, 2011 at 02:29:42PM +0200, Wolfram Sang wrote:
> 
> > > Still, the problem exists: When a second channel GPMI channel is
> > > requested, dmaengine will return -EBUSY, because the DMAIRQ is already
> > > taken.
> > >
> > Yes, we should change the DMA code, it is a DMA bug.
> > I ever submitted a patch about the issue:
> > http://patchwork.ozlabs.org/patch/87145/
> 
> That approach was rejected because it would register the same handler
> n-times where one time would do. Your other approach puts too much
> mach-specific details into the driver IMO and probably won't scale very
> well. Maybe we should add something to the private dma_data (like flags
> indicating SHARED) and then do some refcounting?
> 
I would suggest leave this gpmi specific quirk to gpmi driver to sort
out.  With the following mxs-dma change, it should work if gpmi driver
can pass the valid gpmi irq number for only one gpmi channel, and -1
for all others.
Wolfram Sang - June 29, 2011, 2:15 p.m.
On Wed, Jun 29, 2011 at 10:00:39PM +0800, Shawn Guo wrote:
> On Wed, Jun 29, 2011 at 02:29:42PM +0200, Wolfram Sang wrote:
> > 
> > > > Still, the problem exists: When a second channel GPMI channel is
> > > > requested, dmaengine will return -EBUSY, because the DMAIRQ is already
> > > > taken.
> > > >
> > > Yes, we should change the DMA code, it is a DMA bug.
> > > I ever submitted a patch about the issue:
> > > http://patchwork.ozlabs.org/patch/87145/
> > 
> > That approach was rejected because it would register the same handler
> > n-times where one time would do. Your other approach puts too much
> > mach-specific details into the driver IMO and probably won't scale very
> > well. Maybe we should add something to the private dma_data (like flags
> > indicating SHARED) and then do some refcounting?
> > 
> I would suggest leave this gpmi specific quirk to gpmi driver to sort
> out.  With the following mxs-dma change, it should work if gpmi driver
> can pass the valid gpmi irq number for only one gpmi channel, and -1
> for all others.

...which brings us right into the 'NO_IRQ is 0' discussion :)

Other than that, [thinking loud] this will help if all irq-sharing
channels are handled by the same driver. If not, we would just add
IRQF_SHARED (hopefully this will never be needed). Yup, sounds
reasonable to me. Will give it a second thought later, though.
Shawn Guo - June 29, 2011, 2:37 p.m.
On Wed, Jun 29, 2011 at 04:15:57PM +0200, Wolfram Sang wrote:
> On Wed, Jun 29, 2011 at 10:00:39PM +0800, Shawn Guo wrote:
> > On Wed, Jun 29, 2011 at 02:29:42PM +0200, Wolfram Sang wrote:
> > > 
> > > > > Still, the problem exists: When a second channel GPMI channel is
> > > > > requested, dmaengine will return -EBUSY, because the DMAIRQ is already
> > > > > taken.
> > > > >
> > > > Yes, we should change the DMA code, it is a DMA bug.
> > > > I ever submitted a patch about the issue:
> > > > http://patchwork.ozlabs.org/patch/87145/
> > > 
> > > That approach was rejected because it would register the same handler
> > > n-times where one time would do. Your other approach puts too much
> > > mach-specific details into the driver IMO and probably won't scale very
> > > well. Maybe we should add something to the private dma_data (like flags
> > > indicating SHARED) and then do some refcounting?
> > > 
> > I would suggest leave this gpmi specific quirk to gpmi driver to sort
> > out.  With the following mxs-dma change, it should work if gpmi driver
> > can pass the valid gpmi irq number for only one gpmi channel, and -1
> > for all others.
> 
> ...which brings us right into the 'NO_IRQ is 0' discussion :)
> 
Though I do not know what it means exactly, number 0 is an valid IRQ
on both mx23 and mx28 (see mx23.h and mx28.h).

> Other than that, [thinking loud] this will help if all irq-sharing
> channels are handled by the same driver. If not, we would just add
> IRQF_SHARED (hopefully this will never be needed). Yup, sounds
> reasonable to me. Will give it a second thought later, though.
> 
GPMI is the only mxs-dma user that gets irq-sharing.  So yes, all
irq-sharing channels are handled by the same driver, gpmi-nfc :)
Wolfram Sang - June 30, 2011, 4:46 a.m.
Hi Shawn,

> > > I would suggest leave this gpmi specific quirk to gpmi driver to sort
> > > out.  With the following mxs-dma change, it should work if gpmi driver
> > > can pass the valid gpmi irq number for only one gpmi channel, and -1
> > > for all others.
> > 
> > ...which brings us right into the 'NO_IRQ is 0' discussion :)
> > 
> Though I do not know what it means exactly, number 0 is an valid IRQ
> on both mx23 and mx28 (see mx23.h and mx28.h).

It could be remapped. It is a looong story. Start here if you are interested:

http://lkml.org/lkml/2005/11/21/221
http://yarchive.net/comp/linux/no_irq.html

Currently, it's a can of worms. Hopefully, irq_desc might help somewhen.

> > Other than that, [thinking loud] this will help if all irq-sharing
> > channels are handled by the same driver. If not, we would just add
> > IRQF_SHARED (hopefully this will never be needed). Yup, sounds
> > reasonable to me. Will give it a second thought later, though.
> > 
> GPMI is the only mxs-dma user that gets irq-sharing.  So yes, all
> irq-sharing channels are handled by the same driver, gpmi-nfc :)

Currently, yes. But we have to make it future-proof.

Regards,

   Wolfram
Shawn Guo - June 30, 2011, 5:28 a.m.
On Thu, Jun 30, 2011 at 06:46:55AM +0200, Wolfram Sang wrote:
> Hi Shawn,
> 
> > > > I would suggest leave this gpmi specific quirk to gpmi driver to sort
> > > > out.  With the following mxs-dma change, it should work if gpmi driver
> > > > can pass the valid gpmi irq number for only one gpmi channel, and -1
> > > > for all others.
> > > 
> > > ...which brings us right into the 'NO_IRQ is 0' discussion :)
> > > 
> > Though I do not know what it means exactly, number 0 is an valid IRQ
> > on both mx23 and mx28 (see mx23.h and mx28.h).
> 
> It could be remapped. It is a looong story. Start here if you are interested:
> 
> http://lkml.org/lkml/2005/11/21/221
> http://yarchive.net/comp/linux/no_irq.html
> 
> Currently, it's a can of worms. Hopefully, irq_desc might help somewhen.
> 
Thanks for the info.  It deserves a good study.  With your comment,
I would use macro NO_IRQ than -1 for the mxs-dma change.

> > > Other than that, [thinking loud] this will help if all irq-sharing
> > > channels are handled by the same driver. If not, we would just add
> > > IRQF_SHARED (hopefully this will never be needed). Yup, sounds
> > > reasonable to me. Will give it a second thought later, though.
> > > 
> > GPMI is the only mxs-dma user that gets irq-sharing.  So yes, all
> > irq-sharing channels are handled by the same driver, gpmi-nfc :)
> 
> Currently, yes. But we have to make it future-proof.
> 
For the SoCs coming in future, some, e.g. mx6 series, is the same
case, and some, like mx50, has separate irq for each gpmi channel,
which is even better.  So we can survive for quite a long time.

Patch

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index 88aad4f..ea27002 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -327,10 +327,12 @@  static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)

        memset(mxs_chan->ccw, 0, PAGE_SIZE);

-       ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
-                               0, "mxs-dma", mxs_dma);
-       if (ret)
-               goto err_irq;
+       if (mxs_chan->chan_irq >= 0) {
+               ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
+                                       0, "mxs-dma", mxs_dma);
+               if (ret)
+                       goto err_irq;
+       }

        ret = clk_enable(mxs_dma->clk);
        if (ret)