* Re: [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine
[not found] <1372650378-2936-2-git-send-email-hongbo.zhang@freescale.com>
@ 2013-07-02 23:13 ` Scott Wood
2013-07-03 3:47 ` Hongbo Zhang
0 siblings, 1 reply; 4+ messages in thread
From: Scott Wood @ 2013-07-02 23:13 UTC (permalink / raw)
To: hongbo.zhang
Cc: vinod.koul, linux-kernel, vakul, Hongbo Zhang, djbw, linuxppc-dev
On 06/30/2013 10:46:18 PM, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
>=20
> This patch adds support to 8-channel DMA engine, thus the driver =20
> works for both
> the new 8-channel and the legacy 4-channel DMA engines.
>=20
> Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
> ---
> drivers/dma/fsldma.c | 48 =20
> ++++++++++++++++++++++++++++++++++--------------
> drivers/dma/fsldma.h | 4 ++--
> 2 files changed, 36 insertions(+), 16 deletions(-)
>=20
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 4fc2980..0f453ea 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1119,27 +1119,33 @@ static irqreturn_t fsldma_ctrl_irq(int irq, =20
> void *data)
> struct fsldma_device *fdev =3D data;
> struct fsldma_chan *chan;
> unsigned int handled =3D 0;
> - u32 gsr, mask;
> + u8 chan_sr[round_up(FSL_DMA_MAX_CHANS_PER_DEVICE, 4)];
> + u32 gsr;
> int i;
>=20
> - gsr =3D (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs)
> - : =20
> in_le32(fdev->regs);
> - mask =3D 0xff000000;
> - dev_dbg(fdev->dev, "IRQ: gsr 0x%.8x\n", gsr);
> + memset(&chan_sr, 0, sizeof(chan_sr));
> + gsr =3D (fdev->feature & FSL_DMA_BIG_ENDIAN) ? =20
> in_be32(fdev->regs0)
> + : =20
> in_le32(fdev->regs0);
> + memcpy(&chan_sr[0], &gsr, 4);
> + dev_dbg(fdev->dev, "IRQ: gsr0 0x%.8x\n", gsr);
> +
> + if (of_device_is_compatible(fdev->dev->of_node, =20
> "fsl,eloplus-dma2")) {
NACK; Figure out what sort of device you've got when you first probe =20
the device, and store the information for later. Do not call device =20
tree stuff in an interrupt handler.
> + gsr =3D (fdev->feature & FSL_DMA_BIG_ENDIAN) ?
> + in_be32(fdev->regs1) : in_le32(fdev->regs1);
> + memcpy(&chan_sr[4], &gsr, 4);
> + dev_dbg(fdev->dev, "IRQ: gsr1 0x%.8x\n", gsr);
> + }
Do these memcpy()s get inlined? If not (and maybe even if they do), =20
it'd be better to use a union instead.
Wait a second -- how are we even getting into this code on these new =20
DMA controllers? All 85xx-family DMA controllers use fsldma_chan_irq =20
directly.
> @@ -1341,13 +1349,22 @@ static int fsldma_of_probe(struct =20
> platform_device *op)
> INIT_LIST_HEAD(&fdev->common.channels);
>=20
> /* ioremap the registers for use */
> - fdev->regs =3D of_iomap(op->dev.of_node, 0);
> - if (!fdev->regs) {
> - dev_err(&op->dev, "unable to ioremap registers\n");
> + fdev->regs0 =3D of_iomap(op->dev.of_node, 0);
> + if (!fdev->regs0) {
> + dev_err(&op->dev, "unable to ioremap register0\n");
> err =3D -ENOMEM;
> goto out_free_fdev;
> }
>=20
> + if (of_device_is_compatible(op->dev.of_node, =20
> "fsl,eloplus-dma2")) {
Not "fsl,eloplusplus-dma"? :-)
More seriously, if we're sticking with this "elo" naming, maybe =20
"fsl,elo3-dma" would be better. It would be odd to have "2" in the =20
name of the third generation of this hardware.
> diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
> index f5c3879..880664d 100644
> --- a/drivers/dma/fsldma.h
> +++ b/drivers/dma/fsldma.h
> @@ -112,10 +112,10 @@ struct fsldma_chan_regs {
> };
>=20
> struct fsldma_chan;
> -#define FSL_DMA_MAX_CHANS_PER_DEVICE 4
> +#define FSL_DMA_MAX_CHANS_PER_DEVICE 8
>=20
> struct fsldma_device {
> - void __iomem *regs; /* DGSR register base */
> + void __iomem *regs0, *regs1; /* DGSR registers */
Either give these meaningful names, or use an array. Or both (dgsr[2]).
Or just get rid of this, since I don't see why we need DGSR1 at all, as =20
previously noted.
-Scott=
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine
2013-07-02 23:13 ` [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine Scott Wood
@ 2013-07-03 3:47 ` Hongbo Zhang
2013-07-03 16:41 ` Scott Wood
0 siblings, 1 reply; 4+ messages in thread
From: Hongbo Zhang @ 2013-07-03 3:47 UTC (permalink / raw)
To: Scott Wood
Cc: vinod.koul, devicetree-discuss, linux-kernel, vakul, djbw, linuxppc-dev
On 07/03/2013 07:13 AM, Scott Wood wrote:
> On 06/30/2013 10:46:18 PM, hongbo.zhang@freescale.com wrote:
>> From: Hongbo Zhang <hongbo.zhang@freescale.com>
>>
>> This patch adds support to 8-channel DMA engine, thus the driver
>> works for both
>> the new 8-channel and the legacy 4-channel DMA engines.
>>
>> Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
>> ---
>> drivers/dma/fsldma.c | 48
>> ++++++++++++++++++++++++++++++++++--------------
>> drivers/dma/fsldma.h | 4 ++--
>> 2 files changed, 36 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
>> index 4fc2980..0f453ea 100644
>> --- a/drivers/dma/fsldma.c
>> +++ b/drivers/dma/fsldma.c
>> @@ -1119,27 +1119,33 @@ static irqreturn_t fsldma_ctrl_irq(int irq,
>> void *data)
>> struct fsldma_device *fdev = data;
>> struct fsldma_chan *chan;
>> unsigned int handled = 0;
>> - u32 gsr, mask;
>> + u8 chan_sr[round_up(FSL_DMA_MAX_CHANS_PER_DEVICE, 4)];
>> + u32 gsr;
>> int i;
>>
>> - gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs)
>> - : in_le32(fdev->regs);
>> - mask = 0xff000000;
>> - dev_dbg(fdev->dev, "IRQ: gsr 0x%.8x\n", gsr);
>> + memset(&chan_sr, 0, sizeof(chan_sr));
>> + gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs0)
>> + : in_le32(fdev->regs0);
>> + memcpy(&chan_sr[0], &gsr, 4);
>> + dev_dbg(fdev->dev, "IRQ: gsr0 0x%.8x\n", gsr);
>> +
>> + if (of_device_is_compatible(fdev->dev->of_node,
>> "fsl,eloplus-dma2")) {
>
> NACK; Figure out what sort of device you've got when you first probe
> the device, and store the information for later. Do not call device
> tree stuff in an interrupt handler.
>
>> + gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ?
>> + in_be32(fdev->regs1) : in_le32(fdev->regs1);
>> + memcpy(&chan_sr[4], &gsr, 4);
>> + dev_dbg(fdev->dev, "IRQ: gsr1 0x%.8x\n", gsr);
>> + }
>
> Do these memcpy()s get inlined? If not (and maybe even if they do),
> it'd be better to use a union instead.
>
For this and the first comments: good catches, thank you.
But it is very likely I will remove these codes, see the last comments
of yours and mine.
> Wait a second -- how are we even getting into this code on these new
> DMA controllers? All 85xx-family DMA controllers use fsldma_chan_irq
> directly.
>
Right, we are using fsldma_chan_irq, this code never run.
I just see there is such code for elo/eloplus DMA controllers, so I
update it for the new 8-channel DMA.
>> @@ -1341,13 +1349,22 @@ static int fsldma_of_probe(struct
>> platform_device *op)
>> INIT_LIST_HEAD(&fdev->common.channels);
>>
>> /* ioremap the registers for use */
>> - fdev->regs = of_iomap(op->dev.of_node, 0);
>> - if (!fdev->regs) {
>> - dev_err(&op->dev, "unable to ioremap registers\n");
>> + fdev->regs0 = of_iomap(op->dev.of_node, 0);
>> + if (!fdev->regs0) {
>> + dev_err(&op->dev, "unable to ioremap register0\n");
>> err = -ENOMEM;
>> goto out_free_fdev;
>> }
>>
>> + if (of_device_is_compatible(op->dev.of_node, "fsl,eloplus-dma2")) {
>
> Not "fsl,eloplusplus-dma"? :-)
>
> More seriously, if we're sticking with this "elo" naming, maybe
> "fsl,elo3-dma" would be better. It would be odd to have "2" in the
> name of the third generation of this hardware.
>
It was really hard for me to name this new controller.
Yes "fsl,elo3-dma" seems better.
>> diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
>> index f5c3879..880664d 100644
>> --- a/drivers/dma/fsldma.h
>> +++ b/drivers/dma/fsldma.h
>> @@ -112,10 +112,10 @@ struct fsldma_chan_regs {
>> };
>>
>> struct fsldma_chan;
>> -#define FSL_DMA_MAX_CHANS_PER_DEVICE 4
>> +#define FSL_DMA_MAX_CHANS_PER_DEVICE 8
>>
>> struct fsldma_device {
>> - void __iomem *regs; /* DGSR register base */
>> + void __iomem *regs0, *regs1; /* DGSR registers */
>
> Either give these meaningful names, or use an array. Or both (dgsr[2]).
>
> Or just get rid of this, since I don't see why we need DGSR1 at all,
> as previously noted.
>
I choose the names regs* just to follow the previous pattern.
Here comes the key point: both the previous DGSR and the new DGSR0/DGSR1
are not actually used because we are using per channel irq.
I see we had such codes to handle DGSR, so I just follow the same
pattern to handle the new DGSR0/DGSR1.
Since getting rid of this unused DGSR1 is permitted, I'd like to remove
all the related codes, then this patch becomes simple :)
> -Scott
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine
2013-07-03 3:47 ` Hongbo Zhang
@ 2013-07-03 16:41 ` Scott Wood
0 siblings, 0 replies; 4+ messages in thread
From: Scott Wood @ 2013-07-03 16:41 UTC (permalink / raw)
To: Hongbo Zhang
Cc: vinod.koul, devicetree-discuss, linux-kernel, vakul, djbw, linuxppc-dev
On 07/02/2013 10:47:44 PM, Hongbo Zhang wrote:
> On 07/03/2013 07:13 AM, Scott Wood wrote:
>> Wait a second -- how are we even getting into this code on these new =20
>> DMA controllers? All 85xx-family DMA controllers use =20
>> fsldma_chan_irq directly.
>>=20
> Right, we are using fsldma_chan_irq, this code never run.
> I just see there is such code for elo/eloplus DMA controllers, so I =20
> update it for the new 8-channel DMA.
That code is used for elo (e.g. mpc83xx DMA), but not eloplus.
-Scott=
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine
2013-07-01 3:20 [PATCH 1/2] DMA: Freescale: Add new 8-channel DMA engine device tree nodes hongbo.zhang
@ 2013-07-01 3:20 ` hongbo.zhang
0 siblings, 0 replies; 4+ messages in thread
From: hongbo.zhang @ 2013-07-01 3:20 UTC (permalink / raw)
To: linuxppc-dev; +Cc: scottwood, Hongbo Zhang, linux-kernel, vakul
From: Hongbo Zhang <hongbo.zhang@freescale.com>
This patch adds support to 8-channel DMA engine, thus the driver works for both
the new 8-channel and the legacy 4-channel DMA engines.
Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
---
drivers/dma/fsldma.c | 48 ++++++++++++++++++++++++++++++++++--------------
drivers/dma/fsldma.h | 4 ++--
2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 4fc2980..0f453ea 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c
@@ -1119,27 +1119,33 @@ static irqreturn_t fsldma_ctrl_irq(int irq, void *data)
struct fsldma_device *fdev = data;
struct fsldma_chan *chan;
unsigned int handled = 0;
- u32 gsr, mask;
+ u8 chan_sr[round_up(FSL_DMA_MAX_CHANS_PER_DEVICE, 4)];
+ u32 gsr;
int i;
- gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs)
- : in_le32(fdev->regs);
- mask = 0xff000000;
- dev_dbg(fdev->dev, "IRQ: gsr 0x%.8x\n", gsr);
+ memset(&chan_sr, 0, sizeof(chan_sr));
+ gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ? in_be32(fdev->regs0)
+ : in_le32(fdev->regs0);
+ memcpy(&chan_sr[0], &gsr, 4);
+ dev_dbg(fdev->dev, "IRQ: gsr0 0x%.8x\n", gsr);
+
+ if (of_device_is_compatible(fdev->dev->of_node, "fsl,eloplus-dma2")) {
+ gsr = (fdev->feature & FSL_DMA_BIG_ENDIAN) ?
+ in_be32(fdev->regs1) : in_le32(fdev->regs1);
+ memcpy(&chan_sr[4], &gsr, 4);
+ dev_dbg(fdev->dev, "IRQ: gsr1 0x%.8x\n", gsr);
+ }
for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
chan = fdev->chan[i];
if (!chan)
continue;
- if (gsr & mask) {
+ if (chan_sr[i]) {
dev_dbg(fdev->dev, "IRQ: chan %d\n", chan->id);
fsldma_chan_irq(irq, chan);
handled++;
}
-
- gsr &= ~mask;
- mask >>= 8;
}
return IRQ_RETVAL(handled);
@@ -1261,7 +1267,9 @@ static int fsl_dma_chan_probe(struct fsldma_device *fdev,
WARN_ON(fdev->feature != chan->feature);
chan->dev = fdev->dev;
- chan->id = ((res.start - 0x100) & 0xfff) >> 7;
+ chan->id = (res.start & 0xfff) < 0x300 ?
+ ((res.start - 0x100) & 0xfff) >> 7 :
+ ((res.start - 0x200) & 0xfff) >> 7;
if (chan->id >= FSL_DMA_MAX_CHANS_PER_DEVICE) {
dev_err(fdev->dev, "too many channels for device\n");
err = -EINVAL;
@@ -1341,13 +1349,22 @@ static int fsldma_of_probe(struct platform_device *op)
INIT_LIST_HEAD(&fdev->common.channels);
/* ioremap the registers for use */
- fdev->regs = of_iomap(op->dev.of_node, 0);
- if (!fdev->regs) {
- dev_err(&op->dev, "unable to ioremap registers\n");
+ fdev->regs0 = of_iomap(op->dev.of_node, 0);
+ if (!fdev->regs0) {
+ dev_err(&op->dev, "unable to ioremap register0\n");
err = -ENOMEM;
goto out_free_fdev;
}
+ if (of_device_is_compatible(op->dev.of_node, "fsl,eloplus-dma2")) {
+ fdev->regs1 = of_iomap(op->dev.of_node, 1);
+ if (!fdev->regs1) {
+ dev_err(&op->dev, "unable to ioremap register1\n");
+ err = -ENOMEM;
+ goto out_free_fdev;
+ }
+ }
+
/* map the channel IRQ if it exists, but don't hookup the handler yet */
fdev->irq = irq_of_parse_and_map(op->dev.of_node, 0);
@@ -1427,7 +1444,9 @@ static int fsldma_of_remove(struct platform_device *op)
fsl_dma_chan_remove(fdev->chan[i]);
}
- iounmap(fdev->regs);
+ iounmap(fdev->regs0);
+ if (of_device_is_compatible(op->dev.of_node, "fsl,eloplus-dma2"))
+ iounmap(fdev->regs1);
dev_set_drvdata(&op->dev, NULL);
kfree(fdev);
@@ -1436,6 +1455,7 @@ static int fsldma_of_remove(struct platform_device *op)
static const struct of_device_id fsldma_of_ids[] = {
{ .compatible = "fsl,eloplus-dma", },
+ { .compatible = "fsl,eloplus-dma2", },
{ .compatible = "fsl,elo-dma", },
{}
};
diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
index f5c3879..880664d 100644
--- a/drivers/dma/fsldma.h
+++ b/drivers/dma/fsldma.h
@@ -112,10 +112,10 @@ struct fsldma_chan_regs {
};
struct fsldma_chan;
-#define FSL_DMA_MAX_CHANS_PER_DEVICE 4
+#define FSL_DMA_MAX_CHANS_PER_DEVICE 8
struct fsldma_device {
- void __iomem *regs; /* DGSR register base */
+ void __iomem *regs0, *regs1; /* DGSR registers */
struct device *dev;
struct dma_device common;
struct fsldma_chan *chan[FSL_DMA_MAX_CHANS_PER_DEVICE];
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-07-03 16:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1372650378-2936-2-git-send-email-hongbo.zhang@freescale.com>
2013-07-02 23:13 ` [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine Scott Wood
2013-07-03 3:47 ` Hongbo Zhang
2013-07-03 16:41 ` Scott Wood
2013-07-01 3:20 [PATCH 1/2] DMA: Freescale: Add new 8-channel DMA engine device tree nodes hongbo.zhang
2013-07-01 3:20 ` [PATCH 2/2] DMA: Freescale: update driver to support 8-channel DMA engine hongbo.zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).