Re: [PATCH 6/8] fsldma: simplify IRQ probing and handling
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.
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.
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.
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
___
Re: [PATCH 6/8] fsldma: simplify IRQ probing and handling
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 {
Re: [PATCH 6/8] fsldma: simplify IRQ probing and handling
Ira W. Snyder wrote: 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. Right. 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? Sorry, I meant call it once per channel that has bits set. Should I use a loop + mask, or is there some other neat trick I can use? After you process one channel, it shouldn't have any bits set (and if it does, it's a new event that needs to be processed), so you could use the current ffs approach with a while (summary reg != 0) loop around it. 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. Hmm, that description is specific to 83xx, and such chips really should have the controller interrupt specified. The per-channel interrupt should not be optional, though. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
