Re: [RFC 1/2] usb/musb dma: add cppi41 dma driver
From: Sergei Shtylyov
Date: Mon Jul 08 2013 - 08:16:20 EST
On 08.07.2013 12:52, Sebastian Andrzej Siewior wrote:
On 07/07/2013 04:55 PM, Sergei Shtylyov wrote:
Hello.
Hello Sergei,
On 05-07-2013 20:12, Sebastian Andrzej Siewior wrote:
This is a first shot of the cppi41 DMA driver.
Where have you been when I submitted my drivers back in 2009? :-)
Not here it seems :) There is a driver I got which seem to work but it
is not using the dma engine and is not really in shape so I'm doing
this.
And the current musb's cppi dma engine (3.1, different logic etc.) is
3.0 I think.
not using dmaengine and the network driver (cpsw) which is also using
cppi 3.1 is having its own implementation of the cppi-dma part. So I
think dma enggine implementation is a must here.
Not at all. I'm not familiar with CPSW but DaVinci EMAC uses CPPI
3.0 too but it's completely regisyter incompatible with USB. CPPI 3.0
doesn't seem to be universal DMA engine, hence drivers/dma/ driver
doesn't seem feasible.
diff --git a/arch/arm/boot/dts/am33xx.dtsi
b/arch/arm/boot/dts/am33xx.dtsi
index bb2298c..fc29b54 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -349,6 +349,18 @@
[...]
+ compatible = "ti,am3359-cppi41";
+ reg = <0x47400000 0x1000 /* usbss */
USB register are not a part of CPPI 4.1 DMA. They are not generic and
are different on e.g. DA8xx/OMAP-L1x. Besides this range is conflicting
with the next node.
I will shorten them for the range conflict. usbss is only used for
interrupt mask on/off. If there are different, a different compatible
string will carry the difference.
You don't quite understand. CPPI 4.1 specification as such (which I
guess you haven't seen) doesn't include any interrupt registers.
I think I will also add the usb
string to it since a possible network engine will look different in
terms of the queue used (which I plan to move away from DT).
There are out of tree platform which uses CPPI 4.1 not only for USB
but e.g. for Ethernet.
diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
new file mode 100644
index 0000000..80dcb56
--- /dev/null
+++ b/drivers/dma/cppi41.c
@@ -0,0 +1,777 @@
[...]
+struct cppi41_dd {
+ struct dma_device ddev;
+
+ void *qmgr_scratch;
+ dma_addr_t scratch_phys;
+
+ struct cppi41_desc *cd;
+ dma_addr_t descs_phys;
+ struct cppi41_channel *chan_busy[ALLOC_DECS_NUM];
+
+ void __iomem *usbss_mem;
Shouldn't be here.
+ void __iomem *ctrl_mem;
+ void __iomem *sched_mem;
+ void __iomem *qmgr_mem;
+ unsigned int irq;
Shouldn't be here either.
What is wrong with the four mem pointer here?
I meant only IRQ (interrupt registers are not part of CPPI 4.1 spec).
+static void cppi_writel(u32 val, void *__iomem *mem)
+{
+ writel(val, mem);
+}
+
+static u32 cppi_readl(void *__iomem *mem)
+{
+ u32 val;
+ val = readl(mem);
+ return val;
+}
I don't see much sense in these functions. Besides, they should
probably be using __raw_{read|write}().
I had printk() before posting for debugging. Using raw would require an
explicit cache flush before triggering the engine, right?
Don't think so -- I wasn't using it.
+static irqreturn_t cppi41_irq(int irq, void *data)
+{
+ struct cppi41_dd *cdd = data;
+ struct cppi41_channel *c;
+ u32 status;
+ int i;
+
+ status = cppi_readl(cdd->usbss_mem + USBSS_IRQ_STATUS);
+ if (!(status & USBSS_IRQ_PD_COMP))
+ return IRQ_NONE;
No, this can't be here.
again. Why not?
This register is not part of CPPI 4.1 spec.
+static u32 get_host_pd0(u32 length)
+{
+ u32 reg;
+
+ reg = DESC_TYPE_HOST << DESC_TYPE;
+ reg |= length;
+
+ return reg;
+}
+
+static u32 get_host_pd1(struct cppi41_channel *c)
+{
+ u32 reg;
+
+ reg = 0;
+
+ return reg;
+}
+
+static u32 get_host_pd2(struct cppi41_channel *c)
+{
+ u32 reg;
+
+ reg = 5 << 26; /* USB TYPE */
The driver shouldn't be tied to USB at all.
For now it is USB only. Once we git different types we will have
different compatible strings for that. But that shift could by hidden
behind a define so to comment could vanish as well.
I repeat: CPPI 4.1 is universal DMA engine. It's a pity that it's
implemented as a part of USB peripheral on all in-tree platforms but
it's implemented as an autonomous module on out-of-tree platforms.
+static int cppi41_dma_probe(struct platform_device *pdev)
+{
[...]
+ irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (!irq)
+ goto err_irq;
+
+ cppi_writel(USBSS_IRQ_PD_COMP, cdd->usbss_mem + USBSS_IRQ_ENABLER);
Shouldn't be here.
[...]
+static const struct of_device_id cppi41_dma_ids[] = {
+ { .compatible = "ti,am3359-cppi41", },
CPPI 4.1 driver should be generic, not SoC specific.
So you want another driver around it to handle the SoC specific stuff?
This can be handled as part of MUSB DMA driver in our case.
diff --git a/drivers/usb/musb/musb_dmaeng.c
b/drivers/usb/musb/musb_dmaeng.c
new file mode 100644
index 0000000..c12f42a
--- /dev/null
+++ b/drivers/usb/musb/musb_dmaeng.c
@@ -0,0 +1,294 @@
[...]
+static struct dma_channel *cppi41_dma_channel_allocate(struct
dma_controller *c,
+ struct musb_hw_ep *hw_ep, u8 is_tx)
+{
+ struct cppi41_dma_controller *controller = container_of(c,
+ struct cppi41_dma_controller, controller);
+ struct cppi41_dma_channel *cppi41_channel = NULL;
+ struct musb *musb = controller->musb;
+ u8 ch_num = hw_ep->epnum - 1;
+
+ if (ch_num >= MUSB_DMA_NUM_CHANNELS)
+ return NULL;
+
+ if (!is_tx)
+ return NULL;
+ if (is_tx)
You've just returned on '!is_tx'.
As I said earlier, I have only TX completed so for RX channels there is
nothing to do. If you want this to be removed wait for the next version
which has RX also implemented :)
I don't see why this *if* (which couldn't happen) is in current
version exactly.
[...]
+ musb_dma = &cppi41_channel->channel;
+ musb_dma->private_data = cppi41_channel;
+ musb_dma->status = MUSB_DMA_STATUS_FREE;
+ musb_dma->max_len = SZ_4M;
Really?
The TRM says somewhere that it can handle a single transfer up to 4MiB.
Strange, my DA8xx driver didn't seem to include such limitation.
Sebastian
PS: Are you aware that TI has written CPPI 4.2 DMA engine driver in
their Arago project?
http://linux.davincidsp.com/pipermail/davinci-linux-open-source/2013-February/026345.html
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/