| Message ID | 1262326246-936-7-git-send-email-iws@ovro.caltech.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 <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
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
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
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) - 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>; }; }; diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index 507b297..d8cc05b 100644 --- a/drivers/dma/fsldma.c +++ b/drivers/dma/fsldma.c @@ -1048,20 +1048,6 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data) return IRQ_HANDLED; } -static irqreturn_t fsldma_irq(int irq, void *data) -{ - struct fsldma_device *fdev = data; - int ch_nr; - u32 gsr; - - gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs) - : in_le32(fdev->regs); - ch_nr = (32 - ffs(gsr)) / 8; - - return fdev->chan[ch_nr] ? fsldma_chan_irq(irq, - fdev->chan[ch_nr]) : IRQ_NONE; -} - static void dma_do_tasklet(unsigned long data) { struct fsldma_chan *fsl_chan = (struct fsldma_chan *)data; @@ -1143,19 +1129,24 @@ static int __devinit fsl_dma_chan_probe(struct fsldma_device *fdev, fchan->common.device = &fdev->common; + /* find the IRQ line */ + fchan->irq = irq_of_parse_and_map(node, 0); + if (fchan->irq == NO_IRQ) { + dev_err(fdev->dev, "unable to find IRQ line\n"); + err = -EINVAL; + goto out_iounmap_regs; + } + /* Add the channel to DMA device channel list */ list_add_tail(&fchan->common.device_node, &fdev->common.channels); fdev->common.chancnt++; - fchan->irq = irq_of_parse_and_map(node, 0); - if (fchan->irq != NO_IRQ) { - err = request_irq(fchan->irq, &fsldma_chan_irq, - IRQF_SHARED, "fsldma-channel", fchan); - if (err) { - dev_err(fdev->dev, "unable to request IRQ " - "for channel %d\n", fchan->id); - goto out_list_del; - } + err = request_irq(fchan->irq, fsldma_chan_irq, IRQF_SHARED, + "fsldma-channel", fchan); + if (err) { + dev_err(fdev->dev, "unable to request IRQ for channel %d\n", + fchan->id); + goto out_list_del; } dev_info(fdev->dev, "#%d (%s), irq %d\n", fchan->id, compatible, @@ -1224,16 +1215,6 @@ static int __devinit fsldma_of_probe(struct of_device *op, fdev->common.device_terminate_all = fsl_dma_device_terminate_all; fdev->common.dev = &op->dev; - fdev->irq = irq_of_parse_and_map(op->node, 0); - if (fdev->irq != NO_IRQ) { - err = request_irq(fdev->irq, &fsldma_irq, IRQF_SHARED, - "fsldma-device", fdev); - if (err) { - dev_err(&op->dev, "unable to request IRQ\n"); - goto out_iounmap_regs; - } - } - dev_set_drvdata(&op->dev, fdev); /* @@ -1258,8 +1239,6 @@ static int __devinit fsldma_of_probe(struct of_device *op, dma_async_device_register(&fdev->common); return 0; -out_iounmap_regs: - iounmap(fdev->regs); out_free_fdev: kfree(fdev); out_return:
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. 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. Instead of this, just let the channel handler handle all channels, and ignore the controller handler completely. 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. 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(-)