From: "Ira W. Snyder" <iws@ovro•caltech.edu>
To: Scott Wood <scottwood@freescale•com>
Cc: herbert@gondor•apana.org.au, B04825@freescale•com,
linuxppc-dev@ozlabs•org, Vishnu@freescale•com,
Dipen.Dudhat@freescale•com, dan.j.williams@intel•com,
Maneesh.Gupta@freescale•com, R58472@freescale•com
Subject: Re: [PATCH 6/8] fsldma: simplify IRQ probing and handling
Date: Wed, 6 Jan 2010 10:39:51 -0800 [thread overview]
Message-ID: <20100106183951.GC26426@ovro.caltech.edu> (raw)
In-Reply-To: <20100106180226.GA27146@loki.buserror.net>
On Wed, Jan 06, 2010 at 12:02:26PM -0600, Scott Wood wrote:
> On Thu, Dec 31, 2009 at 10:10:44PM -0800, Ira W. Snyder wrote:
> > The IRQ probing is needlessly complex. All off the 83xx device trees in
> > arch/powerpc/boot/dts/ specify 5 interrupts per DMA controller: one for the
> > controller, and one for each channel. These interrupts are all attached to
> > the same IRQ line.
>
> The reason for this was to accommodate different types of drivers. A driver
> may want to only care about one channel (e.g. if that channel is used for
> something specific such as audio), in which case it can just look at the
> channel node.
>
> A driver that handles multiple channels, OTOH, may want to only register one
> interrupt handler that processes all channels, possibly using the summary
> register, on chips that do not have a different interrupt per channel. Such
> drivers would register the controller interrupt if there is one -- and if
> so, it would ignore the channel interrupts.
>
Ok. The original driver didn't do this, FYI. It would register all 5
interrupts: 1 controller + 4 channels. It is easy enough to have it
completely ignore per-channel interrupts if there is a controller
interrupt.
I don't think this would break any existing hardware. The 83xx all
shares one IRQ line, and the 85xx/86xx only have per-channel interrupts,
right? (I'm not an 85xx/86xx guy, I've only got 83xx experience). This
is what the device trees suggest, anyway.
> > This causes an interesting situation if two channels interrupt at the same
> > time. The controller's handler will handle the first channel, and the
> > channel handler will handle the remaining channels.
>
> The driver should not be registering handlers for both the controller
> interrupt and the channel interrupt.
>
> It looks like the other problem is that the controller interrupt handler
> is assuming only one bit will be set in the summary register. It should
> call the channel handler for each bit that is set.
>
Ok. I thought about doing this, but my changed approach seemed easier.
On the 83xx, we should only need to call the per-channel handler once
for each group of 8 bits. The original code used ffs(), which seems a
little wrong. Why call the handler twice if both EOSI and EOCDI are set
for a channel? Should I use a loop + mask, or is there some other neat
trick I can use?
> > The same can be accomplished on 83xx by removing the controller's interrupt
> > property from the device tree. Testing has not shown any problems with this
> > configuration.
>
> Yes, it will work, but the overhead is a little higher than if you only had
> the one handler and used the summary register.
>
Ok. All of the in-tree 83xx device trees have 5 interrupts listed. With
the changes described above, we'll only call request_irq() once in that
case, and use the per-controller interrupt.
I'll leave the documentation alone, with the exception of marking the
per-controller interrupt optional.
Ira
> > All in-tree device trees already have an interrupt property
> > specified for each channel.
> >
> > Signed-off-by: Ira W. Snyder <iws@ovro•caltech.edu>
> > ---
> > Documentation/powerpc/dts-bindings/fsl/dma.txt | 17 +++++---
> > drivers/dma/fsldma.c | 49 +++++++-----------------
> > 2 files changed, 25 insertions(+), 41 deletions(-)
> >
> > diff --git a/Documentation/powerpc/dts-bindings/fsl/dma.txt b/Documentation/powerpc/dts-bindings/fsl/dma.txt
> > index 0732cdd..d5d2d3d 100644
> > --- a/Documentation/powerpc/dts-bindings/fsl/dma.txt
> > +++ b/Documentation/powerpc/dts-bindings/fsl/dma.txt
> > @@ -12,6 +12,9 @@ Required properties:
> > - ranges : Should be defined as specified in 1) to describe the
> > DMA controller channels.
> > - cell-index : controller index. 0 for controller @ 0x8100
> > +
> > +Optional properties:
> > +
> > - interrupts : <interrupt mapping for DMA IRQ>
> > - interrupt-parent : optional, if needed for interrupt mapping
> >
> > @@ -23,11 +26,7 @@ Required properties:
> > "fsl,elo-dma-channel". However, see note below.
> > - reg : <registers mapping for channel>
> > - cell-index : dma channel index starts at 0.
> > -
> > -Optional properties:
> > - interrupts : <interrupt mapping for DMA channel IRQ>
> > - (on 83xx this is expected to be identical to
> > - the interrupts property of the parent node)
>
> It should indeed be the controller interrupt that is optional, but why
> remove the comment about 83xx? That's the only time you'll have a
> controller interrupt at all.
>
> > - interrupt-parent : optional, if needed for interrupt mapping
> >
> > Example:
> > @@ -37,28 +36,34 @@ Example:
> > compatible = "fsl,mpc8349-dma", "fsl,elo-dma";
> > reg = <0x82a8 4>;
> > ranges = <0 0x8100 0x1a4>;
> > - interrupt-parent = <&ipic>;
> > - interrupts = <71 8>;
> > cell-index = <0>;
> > dma-channel@0 {
> > compatible = "fsl,mpc8349-dma-channel", "fsl,elo-dma-channel";
> > cell-index = <0>;
> > reg = <0 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
> > };
> > dma-channel@80 {
> > compatible = "fsl,mpc8349-dma-channel", "fsl,elo-dma-channel";
> > cell-index = <1>;
> > reg = <0x80 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
> > };
> > dma-channel@100 {
> > compatible = "fsl,mpc8349-dma-channel", "fsl,elo-dma-channel";
> > cell-index = <2>;
> > reg = <0x100 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
> > };
> > dma-channel@180 {
> > compatible = "fsl,mpc8349-dma-channel", "fsl,elo-dma-channel";
> > cell-index = <3>;
> > reg = <0x180 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
>
> I'd rather the example provide both controller and channel interrupts.
>
> -Scott
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists•ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
next prev parent reply other threads:[~2010-01-06 18:39 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-01 6:10 fsldma: cleanup driver and fix async_tx compatibility Ira W. Snyder
2010-01-01 6:10 ` [PATCH 1/8] fsldma: reduce kernel text size Ira W. Snyder
2010-01-01 6:10 ` [PATCH 2/8] fsldma: remove unused structure members Ira W. Snyder
2010-01-01 6:10 ` [PATCH 3/8] fsldma: rename struct fsl_dma_chan to struct fsldma_chan Ira W. Snyder
2010-01-01 6:10 ` [PATCH 4/8] fsldma: rename dest to dst for uniformity Ira W. Snyder
2010-01-01 6:10 ` [PATCH 5/8] fsldma: clean up the OF subsystem routines Ira W. Snyder
2010-01-01 6:10 ` [PATCH 6/8] fsldma: simplify IRQ probing and handling Ira W. Snyder
2010-01-06 18:02 ` Scott Wood
2010-01-06 18:39 ` Ira W. Snyder [this message]
2010-01-06 20:51 ` Scott Wood
2010-01-01 6:10 ` [PATCH 7/8] fsldma: rename fsl_chan to fchan Ira W. Snyder
2010-01-06 18:04 ` Scott Wood
2010-01-06 18:19 ` Ira W. Snyder
2010-01-06 18:27 ` Scott Wood
2010-01-06 18:40 ` Ira W. Snyder
2010-01-01 6:10 ` [PATCH 8/8] fsldma: major cleanups and fixes Ira W. Snyder
2010-01-05 6:08 ` fsldma: cleanup driver and fix async_tx compatibility Dudhat Dipen-B09055
2010-01-11 5:47 ` Dudhat Dipen-B09055
2010-01-11 16:29 ` Ira W. Snyder
2010-02-02 7:50 ` Dudhat Dipen-B09055
2010-02-02 15:04 ` Dan Williams
-- strict thread matches above, loose matches on Subject: below --
2010-01-06 23:33 [PATCH 0/8 v2] " Ira W. Snyder
2010-01-06 23:34 ` [PATCH 6/8] fsldma: simplify IRQ probing and handling Ira W. Snyder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100106183951.GC26426@ovro.caltech.edu \
--to=iws@ovro$(echo .)caltech.edu \
--cc=B04825@freescale$(echo .)com \
--cc=Dipen.Dudhat@freescale$(echo .)com \
--cc=Maneesh.Gupta@freescale$(echo .)com \
--cc=R58472@freescale$(echo .)com \
--cc=Vishnu@freescale$(echo .)com \
--cc=dan.j.williams@intel$(echo .)com \
--cc=herbert@gondor$(echo .)apana.org.au \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=scottwood@freescale$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox