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 <i...@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-chan...@0 {
> > compatible = "fsl,mpc8349-dma-channel",
> > "fsl,elo-dma-channel";
> > cell-index = <0>;
> > reg = <0 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
> > };
> > dma-chan...@80 {
> > compatible = "fsl,mpc8349-dma-channel",
> > "fsl,elo-dma-channel";
> > cell-index = <1>;
> > reg = <0x80 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
> > };
> > dma-chan...@100 {
> > compatible = "fsl,mpc8349-dma-channel",
> > "fsl,elo-dma-channel";
> > cell-index = <2>;
> > reg = <0x100 0x80>;
> > + interrupt-parent = <&ipic>;
> > + interrupts = <71 8>;
> > };
> > dma-chan...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
