Skip to contentSkip to conversation

dma: sudmac: add support for SUDMAC

PATCH Superseded Shimoda, Yoshihiro 3 participants 7 messages Aug 20, 2012 – Sep 18, 2012

Subsystems (2)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dma: sudmac: add support for SUDMAC

Some Renesas USB modules have SUDMAC. This patch supports it using the shdma-base driver.

---
 drivers/dma/Kconfig     |    8 +
 drivers/dma/Makefile    |    1 +
 drivers/dma/sh/Makefile |    1 +
 drivers/dma/sh/sudmac.c |  429 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sh/sudmac.h |   47 +++++
 include/linux/sudmac.h  |   68 ++++++++
 6 files changed, 554 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dma/sh/sudmac.c
 create mode 100644 drivers/dma/sh/sudmac.h
 create mode 100644 include/linux/sudmac.h
drivers/dma/Kconfig +8 -0
index d06ea29..fdd7bbe 100644
@@ -171,6 +171,14 @@ config SH_DMAE
 	help
 	  Enable support for the Renesas SuperH DMA controllers.
 
+config SUDMAC
+	tristate "Renesas SUDMAC support"
+	depends on (SUPERH && SH_DMA) || (ARM && ARCH_SHMOBILE)
+	depends on !SH_DMA_API
+	select DMA_ENGINE
+	help
+	  Enable support for the Renesas SUDMAC controllers.
+
 config COH901318
 	bool "ST-Ericsson COH901318 DMA support"
 	select DMA_ENGINE
drivers/dma/Makefile +1 -0
index 4cf6b12..b680e5e 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_SH_DMAE) += sh/
+obj-$(CONFIG_SUDMAC) += sh/
 obj-$(CONFIG_COH901318) += coh901318.o coh901318_lli.o
 obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
 obj-$(CONFIG_IMX_SDMA) += imx-sdma.o
drivers/dma/sh/Makefile +1 -0
index 54ae957..16f9225 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SH_DMAE) += shdma-base.o
 obj-$(CONFIG_SH_DMAE) += shdma.o
+obj-$(CONFIG_SUDMAC) += shdma-base.o sudmac.o
drivers/dma/sh/sudmac.c new file +429 -0

429 lines not on this page · raw message

drivers/dma/sh/sudmac.h new file +47 -0

47 lines not on this page · raw message

include/linux/sudmac.h new file +68 -0

68 lines not on this page · raw message

Signature · 1 line
1.7.1
#1 lore raw
Quoted 2 lines · Yoshihiro

Some Renesas USB modules have SUDMAC. This patch supports it using the shdma-base driver.

Hi Shimoda-san, Hi all,

I would like to enquire about the status of this patch. Is it still awaiting review?

Quoted 621 lines
---
 drivers/dma/Kconfig     |    8 +
 drivers/dma/Makefile    |    1 +
 drivers/dma/sh/Makefile |    1 +
 drivers/dma/sh/sudmac.c |  429 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sh/sudmac.h |   47 +++++
 include/linux/sudmac.h  |   68 ++++++++
 6 files changed, 554 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dma/sh/sudmac.c
 create mode 100644 drivers/dma/sh/sudmac.h
 create mode 100644 include/linux/sudmac.h
drivers/dma/Kconfig +8 -0
index d06ea29..fdd7bbe 100644
@@ -171,6 +171,14 @@ config SH_DMAE
 	help
 	  Enable support for the Renesas SuperH DMA controllers.
 
+config SUDMAC
+	tristate "Renesas SUDMAC support"
+	depends on (SUPERH && SH_DMA) || (ARM && ARCH_SHMOBILE)
+	depends on !SH_DMA_API
+	select DMA_ENGINE
+	help
+	  Enable support for the Renesas SUDMAC controllers.
+
 config COH901318
 	bool "ST-Ericsson COH901318 DMA support"
 	select DMA_ENGINE
drivers/dma/Makefile +1 -0
index 4cf6b12..b680e5e 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_SH_DMAE) += sh/
+obj-$(CONFIG_SUDMAC) += sh/
 obj-$(CONFIG_COH901318) += coh901318.o coh901318_lli.o
 obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
 obj-$(CONFIG_IMX_SDMA) += imx-sdma.o
drivers/dma/sh/Makefile +1 -0
index 54ae957..16f9225 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SH_DMAE) += shdma-base.o
 obj-$(CONFIG_SH_DMAE) += shdma.o
+obj-$(CONFIG_SUDMAC) += shdma-base.o sudmac.o
drivers/dma/sh/sudmac.c new file +429 -0
new file mode 100644
index 0000000..2454623
@@ -0,0 +1,429 @@
+/*
+ * Renesas SUDMAC support
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * based on drivers/dma/sh/shdma.c:
+ * Copyright (C) 2011-2012 Guennadi Liakhovetski <[email protected]>
+ * Copyright (C) 2009 Nobuhiro Iwamatsu <[email protected]>
+ * Copyright (C) 2009 Renesas Solutions, Inc. All rights reserved.
+ * Copyright (C) 2007 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/dmaengine.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/sudmac.h>
+
+#include "sudmac.h"
+
+#define SUDMAC_DRV_NAME "sudmac"
+
+static void sudmac_writel(struct sudmac_chan *sc, u32 data, u32 reg)
+{
+	iowrite32(data, sc->base + reg);
+}
+
+static u32 sudmac_readl(struct sudmac_chan *sc, u32 reg)
+{
+	return ioread32(sc->base + reg);
+}
+
+static bool sudmac_is_busy(struct sudmac_chan *sc)
+{
+	u32 den = sudmac_readl(sc, CH0DEN + sc->offset);
+
+	if (den)
+		return true; /* working */
+
+	return false; /* waiting */
+}
+
+static void sudmac_set_reg(struct sudmac_chan *sc, struct sudmac_regs *hw,
+			   struct shdma_desc *sdesc)
+{
+	sudmac_writel(sc, sc->cfg, CH0CFG + sc->offset);
+	sudmac_writel(sc, hw->ba, CH0BA + sc->offset);
+	sudmac_writel(sc, hw->bbc, CH0BBC + sc->offset);
+}
+
+static void sudmac_start(struct sudmac_chan *sc)
+{
+	u32 dintctrl = sudmac_readl(sc, DINTCTRL);
+
+	sudmac_writel(sc, dintctrl | sc->dint_end_bit, DINTCTRL);
+	sudmac_writel(sc, DEN, CH0DEN + sc->offset);
+}
+
+static void sudmac_start_xfer(struct shdma_chan *schan,
+			      struct shdma_desc *sdesc)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+
+	sudmac_set_reg(sc, &sd->hw, sdesc);
+	sudmac_start(sc);
+}
+
+static bool sudmac_channel_busy(struct shdma_chan *schan)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+
+	return sudmac_is_busy(sc);
+}
+
+static void sudmac_setup_xfer(struct shdma_chan *schan, int slave_id)
+{
+}
+
+static const struct sudmac_slave_config *sudmac_find_slave(
+	struct sudmac_chan *sc, int slave_id)
+{
+	struct sudmac_device *sdev = to_sdev(sc);
+	struct sudmac_pdata *pdata = sdev->pdata;
+	const struct sudmac_slave_config *cfg;
+	int i;
+
+	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
+		if (cfg->slave_id = slave_id)
+			return cfg;
+
+	return NULL;
+}
+
+static int sudmac_set_slave(struct shdma_chan *schan, int slave_id, bool try)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	const struct sudmac_slave_config *cfg = sudmac_find_slave(sc, slave_id);
+
+	if (!cfg)
+		return -ENODEV;
+
+	return 0;
+}
+
+static void sudmac_dma_halt(struct sudmac_chan *sc)
+{
+	u32 dintctrl = sudmac_readl(sc, DINTCTRL);
+
+	sudmac_writel(sc, 0, CH0DEN + sc->offset);
+	sudmac_writel(sc, dintctrl & ~sc->dint_end_bit, DINTCTRL);
+	sudmac_writel(sc, sc->dint_end_bit, DINTSTSCLR);
+}
+
+static int sudmac_desc_setup(struct shdma_chan *schan,
+			     struct shdma_desc *sdesc,
+			     dma_addr_t src, dma_addr_t dst, size_t *len)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+
+	dev_dbg(sc->shdma_chan.dev, "%s: src=%x, dst=%x, len=%d\n",
+		__func__, src, dst, *len);
+
+	if (*len > schan->max_xfer_len)
+		*len = schan->max_xfer_len;
+
+	if (dst)
+		sd->hw.ba = dst;
+	else if (src)
+		sd->hw.ba = src;
+	sd->hw.bbc = *len;
+
+	return 0;
+}
+
+static void sudmac_halt(struct shdma_chan *schan)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+
+	sudmac_dma_halt(sc);
+}
+
+static bool sudmac_chan_irq(struct shdma_chan *schan, int irq)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	u32 dintsts = sudmac_readl(sc, DINTSTS);
+
+	if (!(dintsts & sc->dint_end_bit))
+		return false;
+
+	/* DMA stop */
+	sudmac_dma_halt(sc);
+
+	return true;
+}
+
+static size_t sudmac_get_partial(struct shdma_chan *schan,
+				 struct shdma_desc *sdesc)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+	u32 cbc = sudmac_readl(sc, CH0CBC + sc->offset);
+
+	return sd->hw.bbc - cbc;
+}
+
+static bool sudmac_desc_completed(struct shdma_chan *schan,
+				  struct shdma_desc *sdesc)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+	u32 ca = sudmac_readl(sc, CH0CA + sc->offset);
+
+	return sd->hw.ba + sd->hw.bbc = ca;
+}
+
+static int __devinit sudmac_chan_probe(struct sudmac_device *su_dev, int id,
+				       int irq, unsigned long flags)
+{
+	struct shdma_dev *sdev = &su_dev->shdma_dev;
+	struct platform_device *pdev = to_platform_device(sdev->dma_dev.dev);
+	struct sudmac_chan *sc;
+	struct shdma_chan *schan;
+	int err;
+
+	sc = kzalloc(sizeof(struct sudmac_chan), GFP_KERNEL);
+	if (!sc) {
+		dev_err(sdev->dma_dev.dev,
+			"No free memory for allocating dma channels!\n");
+		return -ENOMEM;
+	}
+
+	schan = &sc->shdma_chan;
+	schan->max_xfer_len = 64 * 1024 * 1024 - 1;
+
+	shdma_chan_probe(sdev, schan, id);
+
+	sc->base = su_dev->chan_reg;
+
+	sc->offset = su_dev->pdata->channel->offset;
+	sc->cfg = su_dev->pdata->channel->config;
+	sc->dint_end_bit = su_dev->pdata->channel->dint_end_bit;
+
+	/* set up channel irq */
+	if (pdev->id >= 0)
+		snprintf(sc->dev_id, sizeof(sc->dev_id), "sudmac%d.%d",
+			 pdev->id, id);
+	else
+		snprintf(sc->dev_id, sizeof(sc->dev_id), "sudmac%d", id);
+
+	err = shdma_request_irq(schan, irq, flags, sc->dev_id);
+	if (err) {
+		dev_err(sdev->dma_dev.dev,
+			"DMA channel %d request_irq failed %d\n", id, err);
+		goto err_no_irq;
+	}
+
+	su_dev->chan[id] = sc;
+	return 0;
+
+err_no_irq:
+	/* remove from dmaengine device node */
+	shdma_chan_remove(schan);
+	kfree(sc);
+	return err;
+}
+
+static void sudmac_chan_remove(struct sudmac_device *su_dev)
+{
+	struct dma_device *dma_dev = &su_dev->shdma_dev.dma_dev;
+	struct shdma_chan *schan;
+	int i;
+
+	shdma_for_each_chan(schan, &su_dev->shdma_dev, i) {
+		struct sudmac_chan *sc = to_chan(schan);
+
+		BUG_ON(!schan);
+
+		shdma_free_irq(&sc->shdma_chan);
+		shdma_chan_remove(schan);
+		kfree(sc);
+	}
+	dma_dev->chancnt = 0;
+}
+
+static void sudmac_shutdown(struct platform_device *pdev)
+{
+}
+
+static int sudmac_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int sudmac_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int sudmac_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int sudmac_resume(struct device *dev)
+{
+	return 0;
+}
+#else
+#define sudmac_suspend NULL
+#define sudmac_resume NULL
+#endif
+
+const struct dev_pm_ops sudmac_pm = {
+	.suspend		= sudmac_suspend,
+	.resume			= sudmac_resume,
+	.runtime_suspend	= sudmac_runtime_suspend,
+	.runtime_resume		= sudmac_runtime_resume,
+};
+
+static dma_addr_t sudmac_slave_addr(struct shdma_chan *schan)
+{
+	/* SUDMAC doesn't need the address */
+	return 0;
+}
+
+static struct shdma_desc *sudmac_embedded_desc(void *buf, int i)
+{
+	return &((struct sudmac_desc *)buf)[i].shdma_desc;
+}
+
+static const struct shdma_ops sudmac_shdma_ops = {
+	.desc_completed = sudmac_desc_completed,
+	.halt_channel = sudmac_halt,
+	.channel_busy = sudmac_channel_busy,
+	.slave_addr = sudmac_slave_addr,
+	.desc_setup = sudmac_desc_setup,
+	.set_slave = sudmac_set_slave,
+	.setup_xfer = sudmac_setup_xfer,
+	.start_xfer = sudmac_start_xfer,
+	.embedded_desc = sudmac_embedded_desc,
+	.chan_irq = sudmac_chan_irq,
+	.get_partial = sudmac_get_partial,
+};
+
+static int __devinit sudmac_probe(struct platform_device *pdev)
+{
+	struct sudmac_pdata *pdata = pdev->dev.platform_data;
+	int err, i;
+	struct sudmac_device *su_dev;
+	struct dma_device *dma_dev;
+	struct resource *chan, *irq_res;
+
+	/* get platform data */
+	if (!pdata)
+		return -ENODEV;
+
+	chan = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!chan || !irq_res)
+		return -ENODEV;
+
+	err = -ENOMEM;
+	su_dev = kzalloc(sizeof(struct sudmac_device), GFP_KERNEL);
+	if (!su_dev) {
+		dev_err(&pdev->dev, "Not enough memory\n");
+		goto ealloc;
+	}
+
+	dma_dev = &su_dev->shdma_dev.dma_dev;
+
+	su_dev->chan_reg = ioremap(chan->start, resource_size(chan));
+	if (!su_dev->chan_reg)
+		goto emapchan;
+
+	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
+
+	su_dev->shdma_dev.ops = &sudmac_shdma_ops;
+	su_dev->shdma_dev.desc_size = sizeof(struct sudmac_desc);
+	err = shdma_init(&pdev->dev, &su_dev->shdma_dev, pdata->channel_num);
+	if (err < 0)
+		goto eshdma;
+
+	/* platform data */
+	su_dev->pdata = pdev->dev.platform_data;
+
+	platform_set_drvdata(pdev, su_dev);
+
+	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_get_sync(&pdev->dev);
+	if (err < 0)
+		dev_err(&pdev->dev, "%s(): GET = %d\n", __func__, err);
+
+	/* Create DMA Channel */
+	for (i = 0; i < pdata->channel_num; i++) {
+		err = sudmac_chan_probe(su_dev, i, irq_res->start, IRQF_SHARED);
+		if (err)
+			goto chan_probe_err;
+	}
+
+	pm_runtime_put(&pdev->dev);
+
+	err = dma_async_device_register(&su_dev->shdma_dev.dma_dev);
+	if (err < 0)
+		goto edmadevreg;
+
+	return err;
+
+edmadevreg:
+	pm_runtime_get(&pdev->dev);
+
+chan_probe_err:
+	sudmac_chan_remove(su_dev);
+
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	platform_set_drvdata(pdev, NULL);
+	shdma_cleanup(&su_dev->shdma_dev);
+eshdma:
+	iounmap(su_dev->chan_reg);
+emapchan:
+	kfree(su_dev);
+ealloc:
+
+	return err;
+}
+
+static int __devexit sudmac_remove(struct platform_device *pdev)
+{
+	struct sudmac_device *su_dev = platform_get_drvdata(pdev);
+	struct dma_device *dma_dev = &su_dev->shdma_dev.dma_dev;
+
+	dma_async_device_unregister(dma_dev);
+	pm_runtime_disable(&pdev->dev);
+	sudmac_chan_remove(su_dev);
+	shdma_cleanup(&su_dev->shdma_dev);
+	iounmap(su_dev->chan_reg);
+	platform_set_drvdata(pdev, NULL);
+	kfree(su_dev);
+
+	return 0;
+}
+
+static struct platform_driver sudmac_driver = {
+	.driver 	= {
+		.owner	= THIS_MODULE,
+		.pm	= &sudmac_pm,
+		.name	= SUDMAC_DRV_NAME,
+	},
+	.probe		= sudmac_probe,
+	.remove		= __devexit_p(sudmac_remove),
+	.shutdown	= sudmac_shutdown,
+};
+module_platform_driver(sudmac_driver);
+
+MODULE_AUTHOR("Yoshihiro Shimoda");
+MODULE_DESCRIPTION("Renesas SUDMAC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" SUDMAC_DRV_NAME);
drivers/dma/sh/sudmac.h new file +47 -0
new file mode 100644
index 0000000..4463b9a
@@ -0,0 +1,47 @@
+/*
+ * Renesas SUDMAC support
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef __DMA_SUDMAC_H
+#define __DMA_SUDMAC_H
+
+#define SUDMAC_MAX_CHANNELS 2
+
+struct sudmac_chan {
+	struct shdma_chan shdma_chan;
+	void __iomem *base;
+	char dev_id[16];	/* unique name per DMAC of channel */
+
+	u32 offset;		/* for CFG, BA, BBC, CA, CBC, DEN */
+	u32 cfg;
+	u32 dint_end_bit;
+};
+
+struct sudmac_device {
+	struct shdma_dev shdma_dev;
+	struct sudmac_chan *chan[SUDMAC_MAX_CHANNELS];
+	struct sudmac_pdata *pdata;
+	void __iomem *chan_reg;
+};
+
+struct sudmac_regs {
+	u32 ba;
+	u32 bbc;
+};
+
+struct sudmac_desc {
+	struct sudmac_regs hw;
+	struct shdma_desc shdma_desc;
+};
+
+#define to_chan(schan) container_of(schan, struct sudmac_chan, shdma_chan)
+#define to_desc(sdesc) container_of(sdesc, struct sudmac_desc, shdma_desc)
+#define to_sdev(sc) container_of(sc->shdma_chan.dma_chan.device, \
+				 struct sudmac_device, shdma_dev.dma_dev)
+
+#endif	/* __DMA_SUDMAC_H */
include/linux/sudmac.h new file +68 -0
new file mode 100644
index 0000000..064966a
@@ -0,0 +1,68 @@
+/*
+ * Header for the SUDMAC driver
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * based on include/linux/sh_dma.h:
+ * Copyright (C) 2010 Guennadi Liakhovetski <[email protected]>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef SUDMAC_H
+#define SUDMAC_H
+
+#include <linux/dmaengine.h>
+#include <linux/shdma-base.h>
+#include <linux/types.h>
+
+/* Used by slave DMA clients to request DMA to/from a specific peripheral */
+struct sudmac_slave {
+	struct shdma_slave	shdma_slave;	/* Set by the platform */
+};
+
+/*
+ * Supplied by platforms to specify, how a DMA channel has to be configured for
+ * a certain peripheral
+ */
+struct sudmac_slave_config {
+	int		slave_id;
+};
+
+struct sudmac_channel {
+	unsigned long	offset;
+	unsigned long	config;
+	unsigned long	dint_end_bit;
+};
+
+struct sudmac_pdata {
+	const struct sudmac_slave_config *slave;
+	int slave_num;
+	const struct sudmac_channel *channel;
+	int channel_num;
+};
+
+/* SUDMAC register */
+#define CH0CFG		0x00
+#define CH0BA		0x10
+#define CH0BBC		0x18
+#define CH0CA		0x20
+#define CH0CBC		0x28
+#define CH0DEN		0x30
+#define DSTSCLR		0x38
+#define DBUFCTRL	0x3C
+#define DINTCTRL	0x40
+#define DINTSTS		0x44
+#define DINTSTSCLR	0x48
+#define CH0SHCTRL	0x50
+
+/* Definitions for the SUDMAC */
+#define SENDBUFM	0x1000 /* b12: Transmit Buffer Mode */
+#define RCVENDM		0x0100 /* b8: Receive Data Transfer End Mode */
+#define LBA_WAIT	0x0030 /* b5-4: Local Bus Access Wait */
+#define DEN		0x0001 /* b0: DMA Transfer Enable */
+#define CH1ENDE		0x0002 /* b1: Ch1 DMA Transfer End Int Enable */
+#define CH0ENDE		0x0001 /* b0: Ch0 DMA Transfer End Int Enable */
+
+#endif
Signature · 5 lines
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw
In reply to Shimoda, Yoshihiro

Hi Simon-san,

Quoted 8 lines · Simon Horman
Quoted 2 lines · Yoshihiro

Some Renesas USB modules have SUDMAC. This patch supports it using the shdma-base driver.

Hi Shimoda-san, Hi all,

I would like to enquire about the status of this patch. Is it still awaiting review?

This patch is not reviewed from anyone yet. I am waiting for a review.

Best regards,
Yoshihiro Shimoda

#3 lore raw
In reply to Shimoda, Yoshihiro

Hi all

Quoted 8 lines · Simon Horman
Quoted 2 lines · Yoshihiro

Some Renesas USB modules have SUDMAC. This patch supports it using the shdma-base driver.

Hi Shimoda-san, Hi all,

I would like to enquire about the status of this patch. Is it still awaiting review?

I don't think my review is compulsory, and I don't have too many things to complain about here, anyway:-) Just a couple of minor points / questions, maybe, without going into SUDMAC operation details:

Quoted 135 lines
Quoted 135 lines
---
 drivers/dma/Kconfig     |    8 +
 drivers/dma/Makefile    |    1 +
 drivers/dma/sh/Makefile |    1 +
 drivers/dma/sh/sudmac.c |  429 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/dma/sh/sudmac.h |   47 +++++
 include/linux/sudmac.h  |   68 ++++++++
 6 files changed, 554 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dma/sh/sudmac.c
 create mode 100644 drivers/dma/sh/sudmac.h
 create mode 100644 include/linux/sudmac.h
drivers/dma/Kconfig +8 -0
index d06ea29..fdd7bbe 100644
@@ -171,6 +171,14 @@ config SH_DMAE
 	help
 	  Enable support for the Renesas SuperH DMA controllers.
 
+config SUDMAC
+	tristate "Renesas SUDMAC support"
+	depends on (SUPERH && SH_DMA) || (ARM && ARCH_SHMOBILE)
+	depends on !SH_DMA_API
+	select DMA_ENGINE
+	help
+	  Enable support for the Renesas SUDMAC controllers.
+
 config COH901318
 	bool "ST-Ericsson COH901318 DMA support"
 	select DMA_ENGINE
drivers/dma/Makefile +1 -0
index 4cf6b12..b680e5e 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
 obj-$(CONFIG_MX3_IPU) += ipu/
 obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
 obj-$(CONFIG_SH_DMAE) += sh/
+obj-$(CONFIG_SUDMAC) += sh/
 obj-$(CONFIG_COH901318) += coh901318.o coh901318_lli.o
 obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
 obj-$(CONFIG_IMX_SDMA) += imx-sdma.o
drivers/dma/sh/Makefile +1 -0
index 54ae957..16f9225 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_SH_DMAE) += shdma-base.o
 obj-$(CONFIG_SH_DMAE) += shdma.o
+obj-$(CONFIG_SUDMAC) += shdma-base.o sudmac.o
drivers/dma/sh/sudmac.c new file +77 -0
new file mode 100644
index 0000000..2454623
@@ -0,0 +1,429 @@
+/*
+ * Renesas SUDMAC support
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * based on drivers/dma/sh/shdma.c:
+ * Copyright (C) 2011-2012 Guennadi Liakhovetski <[email protected]>
+ * Copyright (C) 2009 Nobuhiro Iwamatsu <[email protected]>
+ * Copyright (C) 2009 Renesas Solutions, Inc. All rights reserved.
+ * Copyright (C) 2007 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/dmaengine.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/sudmac.h>
+
+#include "sudmac.h"
+
+#define SUDMAC_DRV_NAME "sudmac"
+
+static void sudmac_writel(struct sudmac_chan *sc, u32 data, u32 reg)
+{
+	iowrite32(data, sc->base + reg);
+}
+
+static u32 sudmac_readl(struct sudmac_chan *sc, u32 reg)
+{
+	return ioread32(sc->base + reg);
+}
+
+static bool sudmac_is_busy(struct sudmac_chan *sc)
+{
+	u32 den = sudmac_readl(sc, CH0DEN + sc->offset);
+
+	if (den)
+		return true; /* working */
+
+	return false; /* waiting */
+}
+
+static void sudmac_set_reg(struct sudmac_chan *sc, struct sudmac_regs *hw,
+			   struct shdma_desc *sdesc)
+{
+	sudmac_writel(sc, sc->cfg, CH0CFG + sc->offset);
+	sudmac_writel(sc, hw->ba, CH0BA + sc->offset);
+	sudmac_writel(sc, hw->bbc, CH0BBC + sc->offset);
+}
+
+static void sudmac_start(struct sudmac_chan *sc)
+{
+	u32 dintctrl = sudmac_readl(sc, DINTCTRL);
+
+	sudmac_writel(sc, dintctrl | sc->dint_end_bit, DINTCTRL);
+	sudmac_writel(sc, DEN, CH0DEN + sc->offset);
+}
+
+static void sudmac_start_xfer(struct shdma_chan *schan,
+			      struct shdma_desc *sdesc)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+
+	sudmac_set_reg(sc, &sd->hw, sdesc);
+	sudmac_start(sc);
+}
+
+static bool sudmac_channel_busy(struct shdma_chan *schan)
+{
Quoted 8 lines

drivers/dma/sh/sudmac.c line 85

Quoted 8 lines
drivers/dma/sh/sudmac.c new file +8 -0
+	struct sudmac_chan *sc = to_chan(schan);
+
+	return sudmac_is_busy(sc);
+}
+
+static void sudmac_setup_xfer(struct shdma_chan *schan, int slave_id)
+{
+}

Since you don't need this function, we could just make it optional in shdma-base.c, but this can be done later too, nothing critical.

Quoted 101 lines
Quoted 101 lines
drivers/dma/sh/sudmac.c new file +101 -0
+
+static const struct sudmac_slave_config *sudmac_find_slave(
+	struct sudmac_chan *sc, int slave_id)
+{
+	struct sudmac_device *sdev = to_sdev(sc);
+	struct sudmac_pdata *pdata = sdev->pdata;
+	const struct sudmac_slave_config *cfg;
+	int i;
+
+	for (i = 0, cfg = pdata->slave; i < pdata->slave_num; i++, cfg++)
+		if (cfg->slave_id = slave_id)
+			return cfg;
+
+	return NULL;
+}
+
+static int sudmac_set_slave(struct shdma_chan *schan, int slave_id, bool try)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	const struct sudmac_slave_config *cfg = sudmac_find_slave(sc, slave_id);
+
+	if (!cfg)
+		return -ENODEV;
+
+	return 0;
+}
+
+static void sudmac_dma_halt(struct sudmac_chan *sc)
+{
+	u32 dintctrl = sudmac_readl(sc, DINTCTRL);
+
+	sudmac_writel(sc, 0, CH0DEN + sc->offset);
+	sudmac_writel(sc, dintctrl & ~sc->dint_end_bit, DINTCTRL);
+	sudmac_writel(sc, sc->dint_end_bit, DINTSTSCLR);
+}
+
+static int sudmac_desc_setup(struct shdma_chan *schan,
+			     struct shdma_desc *sdesc,
+			     dma_addr_t src, dma_addr_t dst, size_t *len)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+
+	dev_dbg(sc->shdma_chan.dev, "%s: src=%x, dst=%x, len=%d\n",
+		__func__, src, dst, *len);
+
+	if (*len > schan->max_xfer_len)
+		*len = schan->max_xfer_len;
+
+	if (dst)
+		sd->hw.ba = dst;
+	else if (src)
+		sd->hw.ba = src;
+	sd->hw.bbc = *len;
+
+	return 0;
+}
+
+static void sudmac_halt(struct shdma_chan *schan)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+
+	sudmac_dma_halt(sc);
+}
+
+static bool sudmac_chan_irq(struct shdma_chan *schan, int irq)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	u32 dintsts = sudmac_readl(sc, DINTSTS);
+
+	if (!(dintsts & sc->dint_end_bit))
+		return false;
+
+	/* DMA stop */
+	sudmac_dma_halt(sc);
+
+	return true;
+}
+
+static size_t sudmac_get_partial(struct shdma_chan *schan,
+				 struct shdma_desc *sdesc)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+	u32 cbc = sudmac_readl(sc, CH0CBC + sc->offset);
+
+	return sd->hw.bbc - cbc;
+}
+
+static bool sudmac_desc_completed(struct shdma_chan *schan,
+				  struct shdma_desc *sdesc)
+{
+	struct sudmac_chan *sc = to_chan(schan);
+	struct sudmac_desc *sd = to_desc(sdesc);
+	u32 ca = sudmac_readl(sc, CH0CA + sc->offset);
+
+	return sd->hw.ba + sd->hw.bbc = ca;
+}
+
+static int __devinit sudmac_chan_probe(struct sudmac_device *su_dev, int id,
+				       int irq, unsigned long flags)
Quoted 8 lines

drivers/dma/sh/sudmac.c line 194

Quoted 8 lines
drivers/dma/sh/sudmac.c new file +8 -0
+{
+	struct shdma_dev *sdev = &su_dev->shdma_dev;
+	struct platform_device *pdev = to_platform_device(sdev->dma_dev.dev);
+	struct sudmac_chan *sc;
+	struct shdma_chan *schan;
+	int err;
+
+	sc = kzalloc(sizeof(struct sudmac_chan), GFP_KERNEL);

See below for a general comment to these...

Quoted 86 lines
Quoted 86 lines
drivers/dma/sh/sudmac.c new file +86 -0
+	if (!sc) {
+		dev_err(sdev->dma_dev.dev,
+			"No free memory for allocating dma channels!\n");
+		return -ENOMEM;
+	}
+
+	schan = &sc->shdma_chan;
+	schan->max_xfer_len = 64 * 1024 * 1024 - 1;
+
+	shdma_chan_probe(sdev, schan, id);
+
+	sc->base = su_dev->chan_reg;
+
+	sc->offset = su_dev->pdata->channel->offset;
+	sc->cfg = su_dev->pdata->channel->config;
+	sc->dint_end_bit = su_dev->pdata->channel->dint_end_bit;
+
+	/* set up channel irq */
+	if (pdev->id >= 0)
+		snprintf(sc->dev_id, sizeof(sc->dev_id), "sudmac%d.%d",
+			 pdev->id, id);
+	else
+		snprintf(sc->dev_id, sizeof(sc->dev_id), "sudmac%d", id);
+
+	err = shdma_request_irq(schan, irq, flags, sc->dev_id);
+	if (err) {
+		dev_err(sdev->dma_dev.dev,
+			"DMA channel %d request_irq failed %d\n", id, err);
+		goto err_no_irq;
+	}
+
+	su_dev->chan[id] = sc;
+	return 0;
+
+err_no_irq:
+	/* remove from dmaengine device node */
+	shdma_chan_remove(schan);
+	kfree(sc);
+	return err;
+}
+
+static void sudmac_chan_remove(struct sudmac_device *su_dev)
+{
+	struct dma_device *dma_dev = &su_dev->shdma_dev.dma_dev;
+	struct shdma_chan *schan;
+	int i;
+
+	shdma_for_each_chan(schan, &su_dev->shdma_dev, i) {
+		struct sudmac_chan *sc = to_chan(schan);
+
+		BUG_ON(!schan);
+
+		shdma_free_irq(&sc->shdma_chan);
+		shdma_chan_remove(schan);
+		kfree(sc);
+	}
+	dma_dev->chancnt = 0;
+}
+
+static void sudmac_shutdown(struct platform_device *pdev)
+{
+}
+
+static int sudmac_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int sudmac_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int sudmac_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int sudmac_resume(struct device *dev)
+{
+	return 0;
+}
+#else
+#define sudmac_suspend NULL
+#define sudmac_resume NULL
Quoted 8 lines

drivers/dma/sh/sudmac.c line 288

Quoted 8 lines
drivers/dma/sh/sudmac.c new file +8 -0
+#endif
+
+const struct dev_pm_ops sudmac_pm = {
+	.suspend		= sudmac_suspend,
+	.resume			= sudmac_resume,
+	.runtime_suspend	= sudmac_runtime_suspend,
+	.runtime_resume		= sudmac_runtime_resume,
+};

All the above (runtime-)PM callbacks: are they really all needed? I think some of them can be dropped with no functionality reduction.

Quoted 45 lines
Quoted 45 lines
drivers/dma/sh/sudmac.c new file +45 -0
+
+static dma_addr_t sudmac_slave_addr(struct shdma_chan *schan)
+{
+	/* SUDMAC doesn't need the address */
+	return 0;
+}
+
+static struct shdma_desc *sudmac_embedded_desc(void *buf, int i)
+{
+	return &((struct sudmac_desc *)buf)[i].shdma_desc;
+}
+
+static const struct shdma_ops sudmac_shdma_ops = {
+	.desc_completed = sudmac_desc_completed,
+	.halt_channel = sudmac_halt,
+	.channel_busy = sudmac_channel_busy,
+	.slave_addr = sudmac_slave_addr,
+	.desc_setup = sudmac_desc_setup,
+	.set_slave = sudmac_set_slave,
+	.setup_xfer = sudmac_setup_xfer,
+	.start_xfer = sudmac_start_xfer,
+	.embedded_desc = sudmac_embedded_desc,
+	.chan_irq = sudmac_chan_irq,
+	.get_partial = sudmac_get_partial,
+};
+
+static int __devinit sudmac_probe(struct platform_device *pdev)
+{
+	struct sudmac_pdata *pdata = pdev->dev.platform_data;
+	int err, i;
+	struct sudmac_device *su_dev;
+	struct dma_device *dma_dev;
+	struct resource *chan, *irq_res;
+
+	/* get platform data */
+	if (!pdata)
+		return -ENODEV;
+
+	chan = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!chan || !irq_res)
+		return -ENODEV;
+
+	err = -ENOMEM;
+	su_dev = kzalloc(sizeof(struct sudmac_device), GFP_KERNEL);
Quoted 8 lines

drivers/dma/sh/sudmac.c line 341

Quoted 8 lines
drivers/dma/sh/sudmac.c new file +8 -0
+	if (!su_dev) {
+		dev_err(&pdev->dev, "Not enough memory\n");
+		goto ealloc;
+	}
+
+	dma_dev = &su_dev->shdma_dev.dma_dev;
+
+	su_dev->chan_reg = ioremap(chan->start, resource_size(chan));

Recently it has become popular to use device-managed resource allocation:-) Indeed, it is rather convenient, so, you could consider using devm_kzalloc(), devm_request_irq(), devm_request_and_ioremap(), but this too would just be a minor improvement, IMHO.

Quoted 205 lines
Quoted 205 lines
drivers/dma/sh/sudmac.c new file +88 -0
+	if (!su_dev->chan_reg)
+		goto emapchan;
+
+	dma_cap_set(DMA_SLAVE, dma_dev->cap_mask);
+
+	su_dev->shdma_dev.ops = &sudmac_shdma_ops;
+	su_dev->shdma_dev.desc_size = sizeof(struct sudmac_desc);
+	err = shdma_init(&pdev->dev, &su_dev->shdma_dev, pdata->channel_num);
+	if (err < 0)
+		goto eshdma;
+
+	/* platform data */
+	su_dev->pdata = pdev->dev.platform_data;
+
+	platform_set_drvdata(pdev, su_dev);
+
+	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_get_sync(&pdev->dev);
+	if (err < 0)
+		dev_err(&pdev->dev, "%s(): GET = %d\n", __func__, err);
+
+	/* Create DMA Channel */
+	for (i = 0; i < pdata->channel_num; i++) {
+		err = sudmac_chan_probe(su_dev, i, irq_res->start, IRQF_SHARED);
+		if (err)
+			goto chan_probe_err;
+	}
+
+	pm_runtime_put(&pdev->dev);
+
+	err = dma_async_device_register(&su_dev->shdma_dev.dma_dev);
+	if (err < 0)
+		goto edmadevreg;
+
+	return err;
+
+edmadevreg:
+	pm_runtime_get(&pdev->dev);
+
+chan_probe_err:
+	sudmac_chan_remove(su_dev);
+
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	platform_set_drvdata(pdev, NULL);
+	shdma_cleanup(&su_dev->shdma_dev);
+eshdma:
+	iounmap(su_dev->chan_reg);
+emapchan:
+	kfree(su_dev);
+ealloc:
+
+	return err;
+}
+
+static int __devexit sudmac_remove(struct platform_device *pdev)
+{
+	struct sudmac_device *su_dev = platform_get_drvdata(pdev);
+	struct dma_device *dma_dev = &su_dev->shdma_dev.dma_dev;
+
+	dma_async_device_unregister(dma_dev);
+	pm_runtime_disable(&pdev->dev);
+	sudmac_chan_remove(su_dev);
+	shdma_cleanup(&su_dev->shdma_dev);
+	iounmap(su_dev->chan_reg);
+	platform_set_drvdata(pdev, NULL);
+	kfree(su_dev);
+
+	return 0;
+}
+
+static struct platform_driver sudmac_driver = {
+	.driver 	= {
+		.owner	= THIS_MODULE,
+		.pm	= &sudmac_pm,
+		.name	= SUDMAC_DRV_NAME,
+	},
+	.probe		= sudmac_probe,
+	.remove		= __devexit_p(sudmac_remove),
+	.shutdown	= sudmac_shutdown,
+};
+module_platform_driver(sudmac_driver);
+
+MODULE_AUTHOR("Yoshihiro Shimoda");
+MODULE_DESCRIPTION("Renesas SUDMAC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" SUDMAC_DRV_NAME);
drivers/dma/sh/sudmac.h new file +47 -0
new file mode 100644
index 0000000..4463b9a
@@ -0,0 +1,47 @@
+/*
+ * Renesas SUDMAC support
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef __DMA_SUDMAC_H
+#define __DMA_SUDMAC_H
+
+#define SUDMAC_MAX_CHANNELS 2
+
+struct sudmac_chan {
+	struct shdma_chan shdma_chan;
+	void __iomem *base;
+	char dev_id[16];	/* unique name per DMAC of channel */
+
+	u32 offset;		/* for CFG, BA, BBC, CA, CBC, DEN */
+	u32 cfg;
+	u32 dint_end_bit;
+};
+
+struct sudmac_device {
+	struct shdma_dev shdma_dev;
+	struct sudmac_chan *chan[SUDMAC_MAX_CHANNELS];
+	struct sudmac_pdata *pdata;
+	void __iomem *chan_reg;
+};
+
+struct sudmac_regs {
+	u32 ba;
+	u32 bbc;
+};
+
+struct sudmac_desc {
+	struct sudmac_regs hw;
+	struct shdma_desc shdma_desc;
+};
+
+#define to_chan(schan) container_of(schan, struct sudmac_chan, shdma_chan)
+#define to_desc(sdesc) container_of(sdesc, struct sudmac_desc, shdma_desc)
+#define to_sdev(sc) container_of(sc->shdma_chan.dma_chan.device, \
+				 struct sudmac_device, shdma_dev.dma_dev)
+
+#endif	/* __DMA_SUDMAC_H */
include/linux/sudmac.h new file +58 -0
new file mode 100644
index 0000000..064966a
@@ -0,0 +1,68 @@
+/*
+ * Header for the SUDMAC driver
+ *
+ * Copyright (C) 2012 Renesas Solutions Corp.
+ *
+ * based on include/linux/sh_dma.h:
+ * Copyright (C) 2010 Guennadi Liakhovetski <[email protected]>
+ *
+ * This is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ */
+#ifndef SUDMAC_H
+#define SUDMAC_H
+
+#include <linux/dmaengine.h>
+#include <linux/shdma-base.h>
+#include <linux/types.h>
+
+/* Used by slave DMA clients to request DMA to/from a specific peripheral */
+struct sudmac_slave {
+	struct shdma_slave	shdma_slave;	/* Set by the platform */
+};
+
+/*
+ * Supplied by platforms to specify, how a DMA channel has to be configured for
+ * a certain peripheral
+ */
+struct sudmac_slave_config {
+	int		slave_id;
+};
+
+struct sudmac_channel {
+	unsigned long	offset;
+	unsigned long	config;
+	unsigned long	dint_end_bit;
+};
+
+struct sudmac_pdata {
+	const struct sudmac_slave_config *slave;
+	int slave_num;
+	const struct sudmac_channel *channel;
+	int channel_num;
+};
+
+/* SUDMAC register */
+#define CH0CFG		0x00
+#define CH0BA		0x10
+#define CH0BBC		0x18
+#define CH0CA		0x20
+#define CH0CBC		0x28
+#define CH0DEN		0x30
+#define DSTSCLR		0x38
+#define DBUFCTRL	0x3C
+#define DINTCTRL	0x40
+#define DINTSTS		0x44
+#define DINTSTSCLR	0x48
+#define CH0SHCTRL	0x50
Quoted 8 lines

include/linux/sudmac.h line 66

Quoted 8 lines
include/linux/sudmac.h new file +8 -0
+
+/* Definitions for the SUDMAC */
+#define SENDBUFM	0x1000 /* b12: Transmit Buffer Mode */
+#define RCVENDM		0x0100 /* b8: Receive Data Transfer End Mode */
+#define LBA_WAIT	0x0030 /* b5-4: Local Bus Access Wait */
+#define DEN		0x0001 /* b0: DMA Transfer Enable */
+#define CH1ENDE		0x0002 /* b1: Ch1 DMA Transfer End Int Enable */
+#define CH0ENDE		0x0001 /* b0: Ch0 DMA Transfer End Int Enable */

Do we really need register addresses in a public header under include/linux/? If not - I'd really rather hide them in the private header and only keep the absolute minimum here.

Thanks
Guennadi

Quoted 5 lines
Quoted 5 lines
include/linux/sudmac.h new file +2 -0
+
+#endif
Signature · 2 lines
1.7.1
--

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

#4 lore raw
In reply to Shimoda, Yoshihiro

Hi Guennadi-san,

Quoted 8 lines · Guennadi Liakhovetski

Hi all

Quoted 4 lines · Simon Horman
Quoted 2 lines · Yoshihiro

Some Renesas USB modules have SUDMAC. This patch supports it using the shdma-base driver.

Quoted 8 lines
Quoted 4 lines

Hi Shimoda-san, Hi all,

I would like to enquire about the status of this patch. Is it still awaiting review?

I don't think my review is compulsory, and I don't have too many things to complain about here, anyway:-) Just a couple of minor points / questions, maybe, without going into SUDMAC operation details:

Thank you for the review!

< snip >

Quoted 6 lines
Quoted 3 lines

drivers/dma/sh/sudmac.c line 85

Quoted 3 lines
drivers/dma/sh/sudmac.c new file +3 -0
+static void sudmac_setup_xfer(struct shdma_chan *schan, int slave_id)
+{
+}

Since you don't need this function, we could just make it optional in shdma-base.c, but this can be done later too, nothing critical.

Since the shdma-base.c has the following code, I think that the sudmac driver needs this function:

int shdma_init(struct device *dev, struct shdma_dev *sdev,
int chan_num)

{
	struct dma_device *dma_dev = &sdev->dma_dev;

	/*
	 * Require all call-backs for now, they can trivially be made optional
	 * later as required
	 */
	if (!sdev->ops ||
	    !sdev->desc_size ||
	    !sdev->ops->embedded_desc ||
	    !sdev->ops->start_xfer ||
	    !sdev->ops->setup_xfer ||
	    !sdev->ops->set_slave ||
	    !sdev->ops->desc_setup ||
	    !sdev->ops->slave_addr ||
	    !sdev->ops->channel_busy ||
	    !sdev->ops->halt_channel ||
	    !sdev->ops->desc_completed)
		return -EINVAL;

< snip >

Quoted 10 lines
Quoted 6 lines

drivers/dma/sh/sudmac.c line 288

Quoted 6 lines
drivers/dma/sh/sudmac.c new file +6 -0
+const struct dev_pm_ops sudmac_pm = {
+	.suspend		= sudmac_suspend,
+	.resume			= sudmac_resume,
+	.runtime_suspend	= sudmac_runtime_suspend,
+	.runtime_resume		= sudmac_runtime_resume,
+};

All the above (runtime-)PM callbacks: are they really all needed? I think some of them can be dropped with no functionality reduction.

At the moment, those functions don't have any code. So, I will remove them.

< snip >

Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/dma/sh/sudmac.c new file +24 -0
+static int __devinit sudmac_probe(struct platform_device *pdev)
+{
+	struct sudmac_pdata *pdata = pdev->dev.platform_data;
+	int err, i;
+	struct sudmac_device *su_dev;
+	struct dma_device *dma_dev;
+	struct resource *chan, *irq_res;
+
+	/* get platform data */
+	if (!pdata)
+		return -ENODEV;
+
+	chan = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!chan || !irq_res)
+		return -ENODEV;
+
+	err = -ENOMEM;
+	su_dev = kzalloc(sizeof(struct sudmac_device), GFP_KERNEL);
+	if (!su_dev) {
+		dev_err(&pdev->dev, "Not enough memory\n");
+		goto ealloc;
+	}
+
Quoted 8 lines
Quoted 3 lines

drivers/dma/sh/sudmac.c line 341

Quoted 3 lines
drivers/dma/sh/sudmac.c new file +3 -0
+	dma_dev = &su_dev->shdma_dev.dma_dev;
+
+	su_dev->chan_reg = ioremap(chan->start, resource_size(chan));

Recently it has become popular to use device-managed resource allocation:-) Indeed, it is rather convenient, so, you could consider using devm_kzalloc(), devm_request_irq(), devm_request_and_ioremap(), but this too would just be a minor improvement, IMHO.

I understood it. I will modidy the code.

< snip >

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
include/linux/sudmac.h new file +17 -0
+/* SUDMAC register */
+#define CH0CFG		0x00
+#define CH0BA		0x10
+#define CH0BBC		0x18
+#define CH0CA		0x20
+#define CH0CBC		0x28
+#define CH0DEN		0x30
+#define DSTSCLR		0x38
+#define DBUFCTRL	0x3C
+#define DINTCTRL	0x40
+#define DINTSTS		0x44
+#define DINTSTSCLR	0x48
+#define CH0SHCTRL	0x50
+
+/* Definitions for the SUDMAC */
+#define SENDBUFM	0x1000 /* b12: Transmit Buffer Mode */
+#define RCVENDM		0x0100 /* b8: Receive Data Transfer End Mode */
Quoted 8 lines
Quoted 4 lines

include/linux/sudmac.h line 66

Quoted 4 lines
include/linux/sudmac.h new file +4 -0
+#define LBA_WAIT	0x0030 /* b5-4: Local Bus Access Wait */
+#define DEN		0x0001 /* b0: DMA Transfer Enable */
+#define CH1ENDE		0x0002 /* b1: Ch1 DMA Transfer End Int Enable */
+#define CH0ENDE		0x0001 /* b0: Ch0 DMA Transfer End Int Enable */

Do we really need register addresses in a public header under include/linux/? If not - I'd really rather hide them in the private header and only keep the absolute minimum here.

We need some definitions for the platform_data. But we don't need register addresses. I will modify the public header.

Best regards,
Yoshihiro Shimoda

Quoted 14 lines

Thanks
Guennadi

Quoted 5 lines
Quoted 5 lines
(quoted diff) +2 -0
+
+#endif
Signature · 2 lines
1.7.1
--

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

#5 lore raw
In reply to Shimoda, Yoshihiro
Quoted 46 lines · Yoshihiro

Hi Guennadi-san,

Quoted 8 lines · Guennadi Liakhovetski

Hi all

Quoted 4 lines · Simon Horman
Quoted 2 lines · Yoshihiro

Some Renesas USB modules have SUDMAC. This patch supports it using the shdma-base driver.

Quoted 8 lines
Quoted 4 lines

Hi Shimoda-san, Hi all,

I would like to enquire about the status of this patch. Is it still awaiting review?

I don't think my review is compulsory, and I don't have too many things to complain about here, anyway:-) Just a couple of minor points / questions, maybe, without going into SUDMAC operation details:

Thank you for the review!

< snip >

Quoted 6 lines
Quoted 3 lines

drivers/dma/sh/sudmac.c line 85

Quoted 3 lines
drivers/dma/sh/sudmac.c new file +3 -0
+static void sudmac_setup_xfer(struct shdma_chan *schan, int slave_id)
+{
+}

Since you don't need this function, we could just make it optional in shdma-base.c, but this can be done later too, nothing critical.

Since the shdma-base.c has the following code, I think that the sudmac driver needs this function:

int shdma_init(struct device *dev, struct shdma_dev *sdev,
int chan_num)

{
	struct dma_device *dma_dev = &sdev->dma_dev;

	/*
	 * Require all call-backs for now, they can trivially be made optional
	 * later as required
	 */
	if (!sdev->ops ||
	    !sdev->desc_size ||
	    !sdev->ops->embedded_desc ||
	    !sdev->ops->start_xfer ||
Quoted 8 lines
	    !sdev->ops->setup_xfer ||
	    !sdev->ops->set_slave ||
	    !sdev->ops->desc_setup ||
	    !sdev->ops->slave_addr ||
	    !sdev->ops->channel_busy ||
	    !sdev->ops->halt_channel ||
	    !sdev->ops->desc_completed)
		return -EINVAL;

Sure, I know that, that's why I've written "_make_ it optional," i.e., remove the check above for ops->setup_xfer and only call it if available.

Thanks
Guennadi

Quoted 83 lines

< snip >

Quoted 10 lines
Quoted 6 lines

drivers/dma/sh/sudmac.c line 288

Quoted 6 lines
drivers/dma/sh/sudmac.c new file +6 -0
+const struct dev_pm_ops sudmac_pm = {
+	.suspend		= sudmac_suspend,
+	.resume			= sudmac_resume,
+	.runtime_suspend	= sudmac_runtime_suspend,
+	.runtime_resume		= sudmac_runtime_resume,
+};

All the above (runtime-)PM callbacks: are they really all needed? I think some of them can be dropped with no functionality reduction.

At the moment, those functions don't have any code. So, I will remove them.

< snip >

Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/dma/sh/sudmac.c new file +24 -0
+static int __devinit sudmac_probe(struct platform_device *pdev)
+{
+	struct sudmac_pdata *pdata = pdev->dev.platform_data;
+	int err, i;
+	struct sudmac_device *su_dev;
+	struct dma_device *dma_dev;
+	struct resource *chan, *irq_res;
+
+	/* get platform data */
+	if (!pdata)
+		return -ENODEV;
+
+	chan = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!chan || !irq_res)
+		return -ENODEV;
+
+	err = -ENOMEM;
+	su_dev = kzalloc(sizeof(struct sudmac_device), GFP_KERNEL);
+	if (!su_dev) {
+		dev_err(&pdev->dev, "Not enough memory\n");
+		goto ealloc;
+	}
+
Quoted 8 lines
Quoted 3 lines

drivers/dma/sh/sudmac.c line 341

Quoted 3 lines
drivers/dma/sh/sudmac.c new file +3 -0
+	dma_dev = &su_dev->shdma_dev.dma_dev;
+
+	su_dev->chan_reg = ioremap(chan->start, resource_size(chan));

Recently it has become popular to use device-managed resource allocation:-) Indeed, it is rather convenient, so, you could consider using devm_kzalloc(), devm_request_irq(), devm_request_and_ioremap(), but this too would just be a minor improvement, IMHO.

I understood it. I will modidy the code.

< snip >

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
include/linux/sudmac.h new file +17 -0
+/* SUDMAC register */
+#define CH0CFG		0x00
+#define CH0BA		0x10
+#define CH0BBC		0x18
+#define CH0CA		0x20
+#define CH0CBC		0x28
+#define CH0DEN		0x30
+#define DSTSCLR		0x38
+#define DBUFCTRL	0x3C
+#define DINTCTRL	0x40
+#define DINTSTS		0x44
+#define DINTSTSCLR	0x48
+#define CH0SHCTRL	0x50
+
+/* Definitions for the SUDMAC */
+#define SENDBUFM	0x1000 /* b12: Transmit Buffer Mode */
+#define RCVENDM		0x0100 /* b8: Receive Data Transfer End Mode */
Quoted 8 lines
Quoted 4 lines

include/linux/sudmac.h line 66

Quoted 4 lines
include/linux/sudmac.h new file +4 -0
+#define LBA_WAIT	0x0030 /* b5-4: Local Bus Access Wait */
+#define DEN		0x0001 /* b0: DMA Transfer Enable */
+#define CH1ENDE		0x0002 /* b1: Ch1 DMA Transfer End Int Enable */
+#define CH0ENDE		0x0001 /* b0: Ch0 DMA Transfer End Int Enable */

Do we really need register addresses in a public header under include/linux/? If not - I'd really rather hide them in the private header and only keep the absolute minimum here.

We need some definitions for the platform_data. But we don't need register addresses. I will modify the public header.

Best regards,
Yoshihiro Shimoda

Quoted 8 lines
Quoted 8 lines

Thanks
Guennadi

Quoted 5 lines
Quoted 5 lines
(quoted diff) +2 -0
+
+#endif
Signature · 2 lines
1.7.1
--

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

#6 lore raw
In reply to Shimoda, Yoshihiro

Hi Guennadi-san,

Quoted 7 lines · Guennadi Liakhovetski
Quoted 5 lines · Yoshihiro

Hi Guennadi-san,

Quoted 2 lines · Guennadi Liakhovetski

Hi all

< snip >

Quoted 32 lines
Quoted 23 lines
Quoted 6 lines
Quoted 3 lines

drivers/dma/sh/sudmac.c line 85

Quoted 3 lines
drivers/dma/sh/sudmac.c new file +3 -0
+static void sudmac_setup_xfer(struct shdma_chan *schan, int slave_id)
+{
+}

Since you don't need this function, we could just make it optional in shdma-base.c, but this can be done later too, nothing critical.

Since the shdma-base.c has the following code, I think that the sudmac driver needs this function:

int shdma_init(struct device *dev, struct shdma_dev *sdev,
int chan_num)

{
	struct dma_device *dma_dev = &sdev->dma_dev;

	/*
	 * Require all call-backs for now, they can trivially be made optional
	 * later as required
	 */
	if (!sdev->ops ||
	    !sdev->desc_size ||
	    !sdev->ops->embedded_desc ||
	    !sdev->ops->start_xfer ||
Quoted 8 lines
	    !sdev->ops->setup_xfer ||
	    !sdev->ops->set_slave ||
	    !sdev->ops->desc_setup ||
	    !sdev->ops->slave_addr ||
	    !sdev->ops->channel_busy ||
	    !sdev->ops->halt_channel ||
	    !sdev->ops->desc_completed)
		return -EINVAL;
Quoted 2 lines

Sure, I know that, that's why I've written "_make_ it optional," i.e., remove the check above for ops->setup_xfer and only call it if available.

Thank you for the comment. I misunderstood your previous comment. I will keep the sudmac_setup_xfer() at the moment.

Best regards,
Yoshihiro Shimoda