PL-330 DMA driver

PL-330 DMA driver

[jassi brar]

Hello,

  Many Samsung SoCs have PL-330 as the DMA controller and the driver
is missing in the mainline.

A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
based on the DMA API(drivers/dma/), but the patches weren't accepted.
Perhaps the driver should go into arch/arm/common/ ? Or inconspicuously
into arch/arm/plat-s3cxxxx/ ?

Could maintainers please suggest what is the best place for the PL330
controller driver?

Regards

PL-330 DMA driver

[Joonyoung Shim]

On 2/17/2010 2:50 PM, jassi brar wrote:
> Hello,
> 
>   Many Samsung SoCs have PL-330 as the DMA controller and the driver
> is missing in the mainline.
> 
> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
> based on the DMA API(drivers/dma/), but the patches weren't accepted.
> Perhaps the driver should go into arch/arm/common/ ? Or inconspicuously
> into arch/arm/plat-s3cxxxx/ ?
> 

First we need a decision about we use which dma api. I think it's better 
we use the DMA API, but i agree about using s3c dma api for
compatibility of the existing s3c dma api too.

> Could maintainers please suggest what is the best place for the PL330
> controller driver?
> 
> Regards
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

PL-330 DMA driver

[jassi brar]

On Wed, Feb 17, 2010 at 4:07 PM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> On 2/17/2010 2:50 PM, jassi brar wrote:
>> Hello,
>>
>> ? Many Samsung SoCs have PL-330 as the DMA controller and the driver
>> is missing in the mainline.
>>
>> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
>> based on the DMA API(drivers/dma/), but the patches weren't accepted.
>> Perhaps the driver should go into arch/arm/common/ ? Or inconspicuously
>> into arch/arm/plat-s3cxxxx/ ?
> First we need a decision about we use which dma api. I think it's better
> we use the DMA API, but i agree about using s3c dma api for
> compatibility of the existing s3c dma api too.
Also, PL330 is 'configurable' (one implementation can be
different from the other) that too adds one reason for it to be in
plat-samsung
Still, I would like maintainers to suggest.

PL-330 DMA driver

[Linus Walleij]

2010/2/17 jassi brar <jassisinghbrar@gmail.com>:

> Could maintainers please suggest what is the best place for the PL330
> controller driver?

drivers/dma/amba-pl033.c would be my bet.

I have patches to support MMCI (PL180/PL181) using the generic DMA
engine pending, I will attempt to submit patches for spi/amba-pl022.c and
serial/amba-pl011.c when I get the time.

In drivers/dma/coh901318.c we found that implementing a generic
DMA engine over some random custom DMA controller is actually
quite straight forward. If you're making it custom I think seriously
you're doing something wrong.

The key to making the DMA controller wholly abstract is to configure
the driver with platform data containing information on how to
set up/tear down/refill/etc any DMA channel on the system, so
the DMA engine knows all about all channels, and the driver
using it just knows that it's requesting some rx or tx channel with
some platform filter (all from platform data).

This way replacing the DMA enginge, as in resynthesize your SoC
with some totally different DMA controller, and the driver using it
still works.

There may be DMA controllers so esoteric that they cannot be
handled by DMA engine, what do I know, I haven't seen one
yet.

Linus Walleij

PL-330 DMA driver

[Russell King - ARM Linux]

On Wed, Feb 17, 2010 at 07:26:00PM +0100, Linus Walleij wrote:
> 2010/2/17 jassi brar <jassisinghbrar@gmail.com>:
> 
> > Could maintainers please suggest what is the best place for the PL330
> > controller driver?
> 
> drivers/dma/amba-pl033.c would be my bet.
> 
> I have patches to support MMCI (PL180/PL181) using the generic DMA
> engine pending, I will attempt to submit patches for spi/amba-pl022.c and
> serial/amba-pl011.c when I get the time.

One of the problems with the DMA engine APIs are that the data needed to
setup the non-device side is opaque - in other words, it's specific to
the DMA engine being used.

This means primecell drivers can't define this data; they don't know what
kind of DMA engine will be used - and I'm not sure passing it in from
the platform side of things makes much sense either.

Unless... we define a base structure for DMA engines used with primecell
peripherals...

PL-330 DMA driver

[Guennadi Liakhovetski]

On Wed, 17 Feb 2010, Russell King - ARM Linux wrote:

> On Wed, Feb 17, 2010 at 07:26:00PM +0100, Linus Walleij wrote:
> > 2010/2/17 jassi brar <jassisinghbrar@gmail.com>:
> > 
> > > Could maintainers please suggest what is the best place for the PL330
> > > controller driver?
> > 
> > drivers/dma/amba-pl033.c would be my bet.
> > 
> > I have patches to support MMCI (PL180/PL181) using the generic DMA
> > engine pending, I will attempt to submit patches for spi/amba-pl022.c and
> > serial/amba-pl011.c when I get the time.
> 
> One of the problems with the DMA engine APIs are that the data needed to
> setup the non-device side is opaque - in other words, it's specific to
> the DMA engine being used.
> 
> This means primecell drivers can't define this data; they don't know what
> kind of DMA engine will be used - and I'm not sure passing it in from
> the platform side of things makes much sense either.
> 
> Unless... we define a base structure for DMA engines used with primecell
> peripherals...

Having implemented support for IDMAC on i.MX31 under drivers/dma and slave 
DMA support for drivers/dma/shdma.c, my current opinion is, that basically 
only for controllers, that provide generic DMA operations (memcpy, 
xor,...) it makes sense to put them under dmaengine, and even then your 
slave DMA support will have to struggle. If a DMA controller only supports 
slave type DMA (to / from onboard and external peripherals) it normally 
doesn't make sense to implement it using dmaengine API. Granted, inventing 
a good proprietary API might be difficult, but having done that reasonably 
well, you end up with an API, better suitable for your hardware. It might 
even make sense to implement generic functions using dmaengine API and 
slave DMA using hardware-specific extensions.

Of course, there's also a problem of reusing peripheral drivers with 
different DMA controllers...

my 2p.

Thanks
Guennadi
---
Guennadi Liakhovetski

PL-330 DMA driver

[Linus Walleij]

2010/2/17 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> One of the problems with the DMA engine APIs are that the data needed to
> setup the non-device side is opaque - in other words, it's specific to
> the DMA engine being used.
>
> This means primecell drivers can't define this data; they don't know what
> kind of DMA engine will be used - and I'm not sure passing it in from
> the platform side of things makes much sense either.
>
> Unless... we define a base structure for DMA engines used with primecell
> peripherals...

Yes I get the point. I haven't seen much such non-device side stuff
that's actually needed for the driver to specify in the platforms I've
seen. I would say that things like burst size, throttling mechanisms
etc are not generic at all and that knowledge has to be tied to the
channel and DMA engine, however the address and the width of the
endpoint is very generic.

For U300 we have two such parameters in the
DMA engine config for each DMA channel. It's the memory address
where the DMA transaction ends up, like (for PL180) this:

const struct coh_dma_channel chan_config[U300_DMA_CHANNELS] = {
 		.priority_high = 0,
> 		.dev_addr =  U300_MMCSD_BASE + 0x080,
 		.param.config = COH901318_CX_CFG_CH_DISABLE |
(...)
			COH901318_CX_CTRL_BURST_COUNT_32_BYTES | \
>			COH901318_CX_CTRL_SRC_BUS_SIZE_32_BITS | \
			COH901318_CX_CTRL_SRC_ADDR_INC_ENABLE | \
>			COH901318_CX_CTRL_DST_BUS_SIZE_32_BITS | \

It would indeed be better if the physical .dev_addr for TX or RX
could come in from the PrimeCell itself.

This is the most obvious thing, I guess things like burst size could
be equally relevant on some peripheral but haven't seen such a
thing yet.

Would it be good if I went in and defined some struct in
include/linux/amba/bus.h like this:

struct amba_dma_params {
	bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
	void *dma_rx_param;
	void *dma_tx_param;
	dma_addr_t phy_rx_addr;
	dma_addr_t phy_tx_addr;
	int rx_width;
	int tx_width;
};

dma_addr_t could just as well be struct resource (as the
platform_device does it) if that is preferred. But why...
Then we include that into e.g. struct mmci_platform_data
etc.

We could pass a subset of this for as the void * parameter
to the dma_request_channel(), for example this:

struct amba_dma_channel_request {
	void *dma_param;
	dma_addr_t phy_addr;
	int width;
	/* extend here with stuff needed to request PrimeCell channels */
};

It's also quite simple to make a pair of macros for wrapping the
rx or tx part into an amba_dma_channel_request.

In this case the DMA engine implementation
has to know which channels are primecell channels and
dereference these into amba_dma_params. This could be
the defined base structure of DMA engines supporting primecells
I guess.

Otherwise we can require that all DMA engines that
support PrimeCells also support an additional function call:
dma_request_amba_channel(dma_cap_mask_t *mask,
  dma_filter_fn fn,
  struct amba_dma_channel_request *ar);

I can make a quick fix on the MMCI driver as example for either
path, but I need some rough consensus.

Yours,
Linus Walleij

PL-330 DMA driver

[Linus Walleij]

2010/2/17 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:

> Of course, there's also a problem of reusing peripheral drivers with
> different DMA controllers...

That's what I'm facing with MMCI and a host of other peripherals.

We have to either:

(1) Use something, i.e the DMA engine slave

(2) Fork the driver entirely to some ste-mmci.c and delete all generic
  code. (Repeat for each vendor using a primecell!)

(3) Sprinkle mmci.c with #ifdef ARCH_U300 for our DMA engine.
  Or, not so bad, atleast sprinkle the smallish part I have now
  broken out in mmci.c that deals with DMA transfers.

I think the current patch does (1) and it actually works, then if
it gets burdensome to others, using a generic DMA engine can
be a special case of (3) since that works for us so we're on the right
path in two cases and (2) doesn't look good to me.

Yours,
Linus Walleij

PL-330 DMA driver

[jassi brar]

On Thu, Feb 18, 2010 at 6:53 AM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/2/17 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>
>> Of course, there's also a problem of reusing peripheral drivers with
>> different DMA controllers...
>
> That's what I'm facing with MMCI and a host of other peripherals.
>
> We have to either:
>
> (1) Use something, i.e the DMA engine slave
>
> (2) Fork the driver entirely to some ste-mmci.c and delete all generic
> ?code. (Repeat for each vendor using a primecell!)
>
> (3) Sprinkle mmci.c with #ifdef ARCH_U300 for our DMA engine.
> ?Or, not so bad, atleast sprinkle the smallish part I have now
> ?broken out in mmci.c that deals with DMA transfers.
>
> I think the current patch does (1) and it actually works, then if
> it gets burdensome to others, using a generic DMA engine can
> be a special case of (3) since that works for us so we're on the right
> path in two cases and (2) doesn't look good to me.
>
> Yours,
> Linus Walleij

CCing the S3C maintainer (Ben Dooks)  to share his opinion.

PL-330 DMA driver

[Dan Williams]

On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote:
> Hello,
>
> ?Many Samsung SoCs have PL-330 as the DMA controller and the driver
> is missing in the mainline.
>
> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
> based on the DMA API(drivers/dma/), but the patches weren't accepted.

Looking back at the archives seems I missed this patch.  So it was not
rejected just overlooked.  Do we want to move forward on that patch as
it stands, or go a different direction?  If it is resubmitted I will
get it in the queue for 2.6.34.

--
Dan

PL-330 DMA driver

[jassi brar]

On Thu, Feb 18, 2010 at 3:24 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote:
>> Hello,
>>
>> ?Many Samsung SoCs have PL-330 as the DMA controller and the driver
>> is missing in the mainline.
>>
>> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
>> based on the DMA API(drivers/dma/), but the patches weren't accepted.
>
> Looking back at the archives seems I missed this patch. ?So it was not
> rejected just overlooked. ?Do we want to move forward on that patch as
> it stands, or go a different direction? ?If it is resubmitted I will
> get it in the queue for 2.6.34.
Great, but you might want to wait for the platform maintainer's (Ben Dooks)
opinion as it is going to have impact on extant dma client drivers of the
platform which so far shared one dma API across various SOCs.

PL-330 DMA driver

[Russell King - ARM Linux]

On Wed, Feb 17, 2010 at 11:24:28PM -0700, Dan Williams wrote:
> On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote:
> > Hello,
> >
> > ?Many Samsung SoCs have PL-330 as the DMA controller and the driver
> > is missing in the mainline.
> >
> > A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
> > based on the DMA API(drivers/dma/), but the patches weren't accepted.
> 
> Looking back at the archives seems I missed this patch.  So it was not
> rejected just overlooked.  Do we want to move forward on that patch as
> it stands, or go a different direction?  If it is resubmitted I will
> get it in the queue for 2.6.34.

If it's a generic ARM primecell, it's likely it will be used elsewhere as
well (primecells are gaining popularity at the moment.)  Having it not
be specific to S3C would be a big advantage, unless of course everyone
specifically wants to have divergent copies of the same code.

PL-330 DMA driver

[Joonyoung Shim]

On 2/18/2010 3:24 PM, Dan Williams wrote:
> On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote:
>> Hello,
>>
>> ?any Samsung SoCs have PL-330 as the DMA controller and the driver
>> is missing in the mainline.
>>
>> A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
>> based on the DMA API(drivers/dma/), but the patches weren't accepted.
> 
> Looking back at the archives seems I missed this patch.  So it was not
> rejected just overlooked.  Do we want to move forward on that patch as
> it stands, or go a different direction?  If it is resubmitted I will
> get it in the queue for 2.6.34.
> 

You can find the prior patch of pl330 from the below url.
http://patchwork.kernel.org/patch/47847/

If needs, i can resubmit patch based from async_tx repo after some test.

PL-330 DMA driver

[Linus Walleij]

2010/2/18 Joonyoung Shim <jy0922.shim@samsung.com>:

> You can find the prior patch of pl330 from the below url.
> http://patchwork.kernel.org/patch/47847/

This one has a number of review issues, I'll put them in here, hope
you can fix them for the next patch if you're at it now:

+#include <linux/platform_device.h>

No, please, #include <linux/amba/bus.h> instead. That's how
we register PrimeCells. More on that at the end.

+#include <plat/dma.h>

Move that dma.h to include/linux/amba/pl330.h and include as
#include <linux/amba/pl330.h>
And also include it in the patch or we have no chance to know
how struct pl330_platform_data looks (it is used a lot in the
driver).

+static unsigned int pl330_get_reg(struct pl330_device *pl330_dev,
+		unsigned int reg)
+{
+	void __iomem *base = pl330_dev->reg_base;
+
+	return readl(base + reg);
+}
+
+static void pl330_set_reg(struct pl330_device *pl330_dev, unsigned int reg,
+		unsigned int val)
+{
+	void __iomem *base = pl330_dev->reg_base;
+
+	writel(val, base + reg);
+}

Is this kind of abstraction really useful? Isn't it easier in any case to
just writel(FOO, pl330_dev->reg_base + BAR); ?
In case you really keep them, make them inline.

+static void pl330_dump_regs(struct pl330_chan *pl330_ch)
+{
+	struct device *dev = pl330_ch->pl330_dev->common.dev;
+	unsigned int val;
+	unsigned int id = pl330_ch->id;
+
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DS);
+	dev_dbg(dev, "PL330_DS:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DPC);
+	dev_dbg(dev, "PL330_DPC:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTEN);
+	dev_dbg(dev, "PL330_INTEN:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_ES);
+	dev_dbg(dev, "PL330_ES:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTSTATUS);
+	dev_dbg(dev, "PL330_INTSTATUS:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSM);
+	dev_dbg(dev, "PL330_FSM:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSC);
+	dev_dbg(dev, "PL330_FSC:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTM);
+	dev_dbg(dev, "PL330_FTM:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTC(id));
+	dev_dbg(dev, "PL330_FTC(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CS(id));
+	dev_dbg(dev, "PL330_CS(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CPC(id));
+	dev_dbg(dev, "PL330_CPC(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_SA(id));
+	dev_dbg(dev, "PL330_SA(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DA(id));
+	dev_dbg(dev, "PL330_DA(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CC(id));
+	dev_dbg(dev, "PL330_CC(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC0(id));
+	dev_dbg(dev, "PL330_LC0(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC1(id));
+	dev_dbg(dev, "PL330_LC1(%d):\t\t0x%08x\n", id, val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DBGSTATUS);
+	dev_dbg(dev, "PL330_DBGSTATUS:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR0);
+	dev_dbg(dev, "PL330_CR0:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR1);
+	dev_dbg(dev, "PL330_CR1:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR2);
+	dev_dbg(dev, "PL330_CR2:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR3);
+	dev_dbg(dev, "PL330_CR3:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR4);
+	dev_dbg(dev, "PL330_CR4:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CRDN);
+	dev_dbg(dev, "PL330_CRDN:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID0);
+	dev_dbg(dev, "PL330_PERIPH_ID0:\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID1);
+	dev_dbg(dev, "PL330_PERIPH_ID1:\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID2);
+	dev_dbg(dev, "PL330_PERIPH_ID2:\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID3);
+	dev_dbg(dev, "PL330_PERIPH_ID3:\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID0);
+	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID1);
+	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID2);
+	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
+	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID3);
+	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
+}

Turn this into a table.

struct pl330_regdump {
  char *name;
  u16 reg;
}

static const struct pl330_regdump dumpregs[] = {
 {
    .name = "PL330_DS",
    .reg = PL330_DS,
 },
 (....)
};

int i;
for (i = 0; i < ARRAY_SIZE(dumpregs); i++) {
  struct pl330_regdump *rd = &dumpregs[i];
  dev_dbg(dev, "%s:\t\t0x%08x\n",
    rd->name,
    readl(pl330_ch->pl330_dev->base + rd->reg));
}

Easy! (Beware of bugs in above code, just typing...)

+/* instruction set functions */
(...)

All these inlines make me think of serious rollerskating races.
Are they really necessary?

+	if (loop_size_rest)
+		dev_dbg(dev, "TODO\n");

Hm. Perhaps this can be a bit more descriptive...

+static struct dma_async_tx_descriptor *
+pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+		unsigned int sg_len, enum dma_data_direction direction,
+		unsigned long flags)
+{
+	struct pl330_chan *pl330_ch = to_pl330_chan(chan);
+	struct pl330_register_cc *pl330_reg_cc = &pl330_ch->pl330_reg_cc;
+	struct pl330_dma_slave *dma_slave = chan->private;
+	struct pl330_desc *desc;
+	struct scatterlist *sg;
+	unsigned int inst_size = 0;
+	unsigned int i;
+
+	BUG_ON(!dma_slave);
+	BUG_ON(direction == DMA_BIDIRECTIONAL);

Does the PL330 really prohibit bidirectional channels?

+static int pl330_probe(struct platform_device *pdev)

Add __init macro.

+static int pl330_remove(struct platform_device *pdev)

Add __exit macro.

+static struct platform_driver pl330_driver = {
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "pl330",
+	},
+	.probe		= pl330_probe,
+	.remove		= pl330_remove,
+};

This should be converted into an amba_device per <linux/amba/bus.h> as for
all other primecells. That inevitably involves changing the probe and
remove code a bit, and to register it differently, but we'll be thankful.
(See any other PrimeCell driver for examples, e.g. drivers/spi/amba-pl022.c
or drivers/mmc/host/mmci.c)

Yours,
Linus Walleij

PL-330 DMA driver

[Joonyoung Shim]

On 2/19/2010 2:55 AM, Linus Walleij wrote:
> 2010/2/18 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
>> You can find the prior patch of pl330 from the below url.
>> http://patchwork.kernel.org/patch/47847/
> 
> This one has a number of review issues, I'll put them in here, hope
> you can fix them for the next patch if you're at it now:
> 
> +#include <linux/platform_device.h>
> 
> No, please, #include <linux/amba/bus.h> instead. That's how
> we register PrimeCells. More on that at the end.
> 

OK, i will convert to amba_device it.

> +#include <plat/dma.h>
> 
> Move that dma.h to include/linux/amba/pl330.h and include as
> #include <linux/amba/pl330.h>
> And also include it in the patch or we have no chance to know
> how struct pl330_platform_data looks (it is used a lot in the
> driver).
> 

OK, i did this because of some plat specific define. I will move and
split it.

> +static unsigned int pl330_get_reg(struct pl330_device *pl330_dev,
> +		unsigned int reg)
> +{
> +	void __iomem *base = pl330_dev->reg_base;
> +
> +	return readl(base + reg);
> +}
> +
> +static void pl330_set_reg(struct pl330_device *pl330_dev, unsigned int reg,
> +		unsigned int val)
> +{
> +	void __iomem *base = pl330_dev->reg_base;
> +
> +	writel(val, base + reg);
> +}
> 
> Is this kind of abstraction really useful? Isn't it easier in any case to
> just writel(FOO, pl330_dev->reg_base + BAR); ?
> In case you really keep them, make them inline.
> 

I think too the abstraction is unnecessary.

> +static void pl330_dump_regs(struct pl330_chan *pl330_ch)
> +{
> +	struct device *dev = pl330_ch->pl330_dev->common.dev;
> +	unsigned int val;
> +	unsigned int id = pl330_ch->id;
> +
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DS);
> +	dev_dbg(dev, "PL330_DS:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DPC);
> +	dev_dbg(dev, "PL330_DPC:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTEN);
> +	dev_dbg(dev, "PL330_INTEN:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_ES);
> +	dev_dbg(dev, "PL330_ES:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTSTATUS);
> +	dev_dbg(dev, "PL330_INTSTATUS:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSM);
> +	dev_dbg(dev, "PL330_FSM:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSC);
> +	dev_dbg(dev, "PL330_FSC:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTM);
> +	dev_dbg(dev, "PL330_FTM:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTC(id));
> +	dev_dbg(dev, "PL330_FTC(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CS(id));
> +	dev_dbg(dev, "PL330_CS(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CPC(id));
> +	dev_dbg(dev, "PL330_CPC(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_SA(id));
> +	dev_dbg(dev, "PL330_SA(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DA(id));
> +	dev_dbg(dev, "PL330_DA(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CC(id));
> +	dev_dbg(dev, "PL330_CC(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC0(id));
> +	dev_dbg(dev, "PL330_LC0(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC1(id));
> +	dev_dbg(dev, "PL330_LC1(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DBGSTATUS);
> +	dev_dbg(dev, "PL330_DBGSTATUS:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR0);
> +	dev_dbg(dev, "PL330_CR0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR1);
> +	dev_dbg(dev, "PL330_CR1:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR2);
> +	dev_dbg(dev, "PL330_CR2:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR3);
> +	dev_dbg(dev, "PL330_CR3:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR4);
> +	dev_dbg(dev, "PL330_CR4:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CRDN);
> +	dev_dbg(dev, "PL330_CRDN:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID0);
> +	dev_dbg(dev, "PL330_PERIPH_ID0:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID1);
> +	dev_dbg(dev, "PL330_PERIPH_ID1:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID2);
> +	dev_dbg(dev, "PL330_PERIPH_ID2:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID3);
> +	dev_dbg(dev, "PL330_PERIPH_ID3:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID0);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID1);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID2);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID3);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +}
> 
> Turn this into a table.
> 
> struct pl330_regdump {
>   char *name;
>   u16 reg;
> }
> 
> static const struct pl330_regdump dumpregs[] = {
>  {
>     .name = "PL330_DS",
>     .reg = PL330_DS,
>  },
>  (....)
> };
> 
> int i;
> for (i = 0; i < ARRAY_SIZE(dumpregs); i++) {
>   struct pl330_regdump *rd = &dumpregs[i];
>   dev_dbg(dev, "%s:\t\t0x%08x\n",
>     rd->name,
>     readl(pl330_ch->pl330_dev->base + rd->reg));
> }
> 
> Easy! (Beware of bugs in above code, just typing...)
> 

OK, i will change it.

> +/* instruction set functions */
> (...)
> 


> All these inlines make me think of serious rollerskating races.
> Are they really necessary?
> 

If inline is a problem, i can remove inline.

> +	if (loop_size_rest)
> +		dev_dbg(dev, "TODO\n");
> 
> Hm. Perhaps this can be a bit more descriptive...
> 

Yes, this is thing to implement additionally, i will add more
description.

> +static struct dma_async_tx_descriptor *
> +pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_data_direction direction,
> +		unsigned long flags)
> +{
> +	struct pl330_chan *pl330_ch = to_pl330_chan(chan);
> +	struct pl330_register_cc *pl330_reg_cc = &pl330_ch->pl330_reg_cc;
> +	struct pl330_dma_slave *dma_slave = chan->private;
> +	struct pl330_desc *desc;
> +	struct scatterlist *sg;
> +	unsigned int inst_size = 0;
> +	unsigned int i;
> +
> +	BUG_ON(!dma_slave);
> +	BUG_ON(direction == DMA_BIDIRECTIONAL);
> 
> Does the PL330 really prohibit bidirectional channels?
> 
> +static int pl330_probe(struct platform_device *pdev)
> 
> Add __init macro.
> 

OK.

> +static int pl330_remove(struct platform_device *pdev)
> 
> Add __exit macro.
> 

OK.

> +static struct platform_driver pl330_driver = {
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "pl330",
> +	},
> +	.probe		= pl330_probe,
> +	.remove		= pl330_remove,
> +};
> 
> This should be converted into an amba_device per <linux/amba/bus.h> as for
> all other primecells. That inevitably involves changing the probe and
> remove code a bit, and to register it differently, but we'll be thankful.
> (See any other PrimeCell driver for examples, e.g. drivers/spi/amba-pl022.c
> or drivers/mmc/host/mmci.c)
> 

I see, i will refer other primecell drivers.

I will fix above things at the next patch, but i cannot start right now 
it, will try to do it as soon as possible.

Thanks for your review.

PL-330 DMA driver

[Ben Dooks]

On Thu, Feb 18, 2010 at 10:32:03AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 17, 2010 at 11:24:28PM -0700, Dan Williams wrote:
> > On Tue, Feb 16, 2010 at 10:50 PM, jassi brar <jassisinghbrar@gmail.com> wrote:
> > > Hello,
> > >
> > > ?Many Samsung SoCs have PL-330 as the DMA controller and the driver
> > > is missing in the mainline.
> > >
> > > A few months ago, Joonyoung Shim of Samsung, attempted the driver for PL-330
> > > based on the DMA API(drivers/dma/), but the patches weren't accepted.
> > 
> > Looking back at the archives seems I missed this patch.  So it was not
> > rejected just overlooked.  Do we want to move forward on that patch as
> > it stands, or go a different direction?  If it is resubmitted I will
> > get it in the queue for 2.6.34.
> 
> If it's a generic ARM primecell, it's likely it will be used elsewhere as
> well (primecells are gaining popularity at the moment.)  Having it not
> be specific to S3C would be a big advantage, unless of course everyone
> specifically wants to have divergent copies of the same code.

Which is great, until you find out that the S3C PL080 isn't a vanilla
PL080, and has extra registers.

I'm in favour of some form of generic DMA API, but we're going to have
to sort out whether it is dmaengine, dmaengine with some updates or
something entirely new.

The current dmaengine API doesn't have any support for circular buffer
mangement, and from what I remeber of reading stuff last time, adding
to the queue from a buffer done callback is a no-no.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

PL-330 DMA driver

[Ben Dooks]

On Thu, Feb 18, 2010 at 06:55:54PM +0100, Linus Walleij wrote:
> 2010/2/18 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
> > You can find the prior patch of pl330 from the below url.
> > http://patchwork.kernel.org/patch/47847/
> 
> This one has a number of review issues, I'll put them in here, hope
> you can fix them for the next patch if you're at it now:
> 
> +#include <linux/platform_device.h>
> 
> No, please, #include <linux/amba/bus.h> instead. That's how
> we register PrimeCells. More on that at the end.
> 
> +#include <plat/dma.h>
> 
> Move that dma.h to include/linux/amba/pl330.h and include as
> #include <linux/amba/pl330.h>
> And also include it in the patch or we have no chance to know
> how struct pl330_platform_data looks (it is used a lot in the
> driver).
> 
> +static unsigned int pl330_get_reg(struct pl330_device *pl330_dev,
> +		unsigned int reg)
> +{
> +	void __iomem *base = pl330_dev->reg_base;
> +
> +	return readl(base + reg);
> +}
> +
> +static void pl330_set_reg(struct pl330_device *pl330_dev, unsigned int reg,
> +		unsigned int val)
> +{
> +	void __iomem *base = pl330_dev->reg_base;
> +
> +	writel(val, base + reg);
> +}
> 
> Is this kind of abstraction really useful? Isn't it easier in any case to
> just writel(FOO, pl330_dev->reg_base + BAR); ?
> In case you really keep them, make them inline.
> 
> +static void pl330_dump_regs(struct pl330_chan *pl330_ch)
> +{
> +	struct device *dev = pl330_ch->pl330_dev->common.dev;
> +	unsigned int val;
> +	unsigned int id = pl330_ch->id;
> +
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DS);
> +	dev_dbg(dev, "PL330_DS:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DPC);
> +	dev_dbg(dev, "PL330_DPC:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTEN);
> +	dev_dbg(dev, "PL330_INTEN:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_ES);
> +	dev_dbg(dev, "PL330_ES:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_INTSTATUS);
> +	dev_dbg(dev, "PL330_INTSTATUS:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSM);
> +	dev_dbg(dev, "PL330_FSM:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FSC);
> +	dev_dbg(dev, "PL330_FSC:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTM);
> +	dev_dbg(dev, "PL330_FTM:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_FTC(id));
> +	dev_dbg(dev, "PL330_FTC(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CS(id));
> +	dev_dbg(dev, "PL330_CS(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CPC(id));
> +	dev_dbg(dev, "PL330_CPC(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_SA(id));
> +	dev_dbg(dev, "PL330_SA(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DA(id));
> +	dev_dbg(dev, "PL330_DA(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CC(id));
> +	dev_dbg(dev, "PL330_CC(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC0(id));
> +	dev_dbg(dev, "PL330_LC0(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_LC1(id));
> +	dev_dbg(dev, "PL330_LC1(%d):\t\t0x%08x\n", id, val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_DBGSTATUS);
> +	dev_dbg(dev, "PL330_DBGSTATUS:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR0);
> +	dev_dbg(dev, "PL330_CR0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR1);
> +	dev_dbg(dev, "PL330_CR1:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR2);
> +	dev_dbg(dev, "PL330_CR2:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR3);
> +	dev_dbg(dev, "PL330_CR3:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CR4);
> +	dev_dbg(dev, "PL330_CR4:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_CRDN);
> +	dev_dbg(dev, "PL330_CRDN:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID0);
> +	dev_dbg(dev, "PL330_PERIPH_ID0:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID1);
> +	dev_dbg(dev, "PL330_PERIPH_ID1:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID2);
> +	dev_dbg(dev, "PL330_PERIPH_ID2:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PERIPH_ID3);
> +	dev_dbg(dev, "PL330_PERIPH_ID3:\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID0);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID1);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID2);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +	val = pl330_get_reg(pl330_ch->pl330_dev, PL330_PCELL_ID3);
> +	dev_dbg(dev, "PL330_PCELL_ID0:\t\t0x%08x\n", val);
> +}
> 
> Turn this into a table.
> 
> struct pl330_regdump {
>   char *name;
>   u16 reg;
> }
> 
> static const struct pl330_regdump dumpregs[] = {
>  {
>     .name = "PL330_DS",
>     .reg = PL330_DS,
>  },
>  (....)
> };
> 
> int i;
> for (i = 0; i < ARRAY_SIZE(dumpregs); i++) {
>   struct pl330_regdump *rd = &dumpregs[i];
>   dev_dbg(dev, "%s:\t\t0x%08x\n",
>     rd->name,
>     readl(pl330_ch->pl330_dev->base + rd->reg));
> }
> 
> Easy! (Beware of bugs in above code, just typing...)
> 
> +/* instruction set functions */
> (...)
> 
> All these inlines make me think of serious rollerskating races.
> Are they really necessary?
> 
> +	if (loop_size_rest)
> +		dev_dbg(dev, "TODO\n");
> 
> Hm. Perhaps this can be a bit more descriptive...
> 
> +static struct dma_async_tx_descriptor *
> +pl330_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		unsigned int sg_len, enum dma_data_direction direction,
> +		unsigned long flags)
> +{
> +	struct pl330_chan *pl330_ch = to_pl330_chan(chan);
> +	struct pl330_register_cc *pl330_reg_cc = &pl330_ch->pl330_reg_cc;
> +	struct pl330_dma_slave *dma_slave = chan->private;
> +	struct pl330_desc *desc;
> +	struct scatterlist *sg;
> +	unsigned int inst_size = 0;
> +	unsigned int i;
> +
> +	BUG_ON(!dma_slave);
> +	BUG_ON(direction == DMA_BIDIRECTIONAL);
> 
> Does the PL330 really prohibit bidirectional channels?
> 
> +static int pl330_probe(struct platform_device *pdev)
> 
> Add __init macro.
> 
> +static int pl330_remove(struct platform_device *pdev)
> 
> Add __exit macro.
> 
> +static struct platform_driver pl330_driver = {
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "pl330",
> +	},
> +	.probe		= pl330_probe,
> +	.remove		= pl330_remove,
> +};
> 
> This should be converted into an amba_device per <linux/amba/bus.h> as for
> all other primecells. That inevitably involves changing the probe and
> remove code a bit, and to register it differently, but we'll be thankful.
> (See any other PrimeCell driver for examples, e.g. drivers/spi/amba-pl022.c
> or drivers/mmc/host/mmci.c)

I don't see any real reason to go and use amba devices, especially as
this'll be one of the few on many of the s3c/s5p platforms.
 
> Yours,
> Linus Walleij
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

PL-330 DMA driver

[Russell King - ARM Linux]

On Wed, Feb 24, 2010 at 12:44:22AM +0000, Ben Dooks wrote:
> Which is great, until you find out that the S3C PL080 isn't a vanilla
> PL080, and has extra registers.

We're not going to go hacking drivers to have multiple different DMA
interfaces inside them just because SoC maintainers want their own
private DMA interfaces.  We need to come up with a better solution to
this problem.

As I've said, the reality is that ARM's primecell IP is starting to
appear in multiple different SoCs, so we need to solve this problem
now.