Hi Mugunthan,

On 1 December 2015 at 04:13, Mugunthan V N mugunthanvnm@ti.com wrote:

Implement a DMA uclass so that the devices like ethernet, spi, mmc etc can offload the data transfers from/to the device and memory.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/dma/Kconfig | 15 ++++++++++++ drivers/dma/Makefile | 2 ++ drivers/dma/dma-uclass.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/dma.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 drivers/dma/dma-uclass.c create mode 100644 include/dma.h

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index e69de29..af15199 100644 --- a/drivers/dma/Kconfig +++ b/drivers/dma/Kconfig @@ -0,0 +1,15 @@ +menu "DMA Support"

+config DM_DMA

•   bool "Enable Driver Model for DMA drivers"
  

•   depends on DM
  

•   help
  

•     Enable driver model for DMA. DMA engines can do
  

•     asynchronous data transfers without involving the host
  

•     CPU. Currently, this framework can be used to offload
  

•     memory copies to and from devices like qspi, ethernet
  

•     etc Drivers provide methods to access the DMA devices
  

•     buses that is used to transfer data to and from memory.
  

•     The uclass interface is defined in include/dma.h.
  

+endmenu # menu "DMA Support" diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile index f95fe70..4ff22aa 100644 --- a/drivers/dma/Makefile +++ b/drivers/dma/Makefile @@ -5,6 +5,8 @@ # SPDX-License-Identifier: GPL-2.0+ #

+obj-$(CONFIG_DM_DMA) += dma-uclass.o

You can just use CONFIG_DMA. The DM_ prefix is for when we have an existing pre-DM interface. Eventually all the CONFIG_DM_... options will be removed. Unless you have a special reason?

obj-$(CONFIG_FSLDMAFEC) += MCD_tasksInit.o MCD_dmaApi.o MCD_tasks.o obj-$(CONFIG_APBH_DMA) += apbh_dma.o obj-$(CONFIG_FSL_DMA) += fsl_dma.o diff --git a/drivers/dma/dma-uclass.c b/drivers/dma/dma-uclass.c new file mode 100644 index 0000000..1afcbaa4 --- /dev/null +++ b/drivers/dma/dma-uclass.c @@ -0,0 +1,63 @@ +/*

• • Direct Memory Access U-Class driver
• • (C) Copyright 2015

• • Texas Instruments Incorporated, <www.ti.com>
    

• • Author: Mugunthan V N mugunthanvnm@ti.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dma.h> +#include <dm.h> +#include <dm/uclass-internal.h> +#include <errno.h>

+DECLARE_GLOBAL_DATA_PTR;

+int dma_memcpy(void *dst, void *src, size_t len) +{

•   struct udevice *dev;
  

•   const struct dma_ops *ops;
  

•   int ret;
  

•   for (ret = uclass_find_first_device(UCLASS_DMA, &dev); dev && !ret;
  

•        ret = uclass_find_next_device(&dev)) {
  

•           struct dma_dev_priv *uc_priv = dev_get_uclass_priv(dev);
  

•           if (uc_priv->supported & SUPPORTS_MEM_TO_MEM)
  

•                   break;
  

•   }
  

•   if (!dev) {
  

•           printf("No DMA device found that supports mem to mem transfers\n");
  

•           return -ENODEV;
  

•   }
  

Can you put the above code in a separate function like:

static int get_dma_device(struct udevice **devp)

also also please call device_probe() on it before returning it.

•   ops = device_get_ops(dev);
  

•   if (!ops->transfer)
  

•           return -ENOSYS;
  

•   /* Invalidate the area, so no writeback into the RAM races with DMA */
  

•   invalidate_dcache_range((unsigned long)dst, (unsigned long)dst +
  

•                           roundup(len, ARCH_DMA_MINALIGN));
  

•   return ops->transfer(dev, DMA_MEM_TO_MEM, dst, src, len);
  

+}

+int dma_post_bind(struct udevice *dev) +{

•   struct udevice *devp;
  

•   uclass_get_device_by_of_offset(UCLASS_DMA, dev->of_offset, &devp);
  

You are not allowed to do this in the post_bind() method. See above for how it should get probed.

•   return 0;
  

+}

+UCLASS_DRIVER(dma) = {

•   .id             = UCLASS_DMA,
  

•   .name           = "dma",
  

•   .flags          = DM_UC_FLAG_SEQ_ALIAS,
  

•   .per_device_auto_alloc_size = sizeof(struct dma_dev_priv),
  

•   .post_bind      = dma_post_bind,
  

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 27fa0b6..9f5fcae 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -30,6 +30,7 @@ enum uclass_id { UCLASS_CPU, /* CPU, typically part of an SoC */ UCLASS_CROS_EC, /* Chrome OS EC */ UCLASS_DISPLAY_PORT, /* Display port video */

•   UCLASS_DMA,             /* Direct Memory Access */
    UCLASS_RAM,             /* RAM controller */
    UCLASS_ETH,             /* Ethernet device */
    UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
  

diff --git a/include/dma.h b/include/dma.h new file mode 100644 index 0000000..d53e435 --- /dev/null +++ b/include/dma.h @@ -0,0 +1,64 @@ +/*

• • (C) Copyright 2015

• • Texas Instruments Incorporated, <www.ti.com>
    

• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _DMA_H_ +#define _DMA_H_

+/*

• • enum dma_direction - dma transfer direction indicator
• • @DMA_MEM_TO_MEM: Memcpy mode
• • @DMA_MEM_TO_DEV: From Memory to Device
• • @DMA_DEV_TO_MEM: From Device to Memory
• • @DMA_DEV_TO_DEV: From Device to Device
• */

+enum dma_direction {

•   DMA_MEM_TO_MEM,
  

•   DMA_MEM_TO_DEV,
  

•   DMA_DEV_TO_MEM,
  

•   DMA_DEV_TO_DEV,
  

•   DMA_TRANS_NONE,
  

What does that mean?

+};

+#define SUPPORTS_MEM_TO_MEM BIT(0) +#define SUPPORTS_MEM_TO_DEV BIT(1) +#define SUPPORTS_DEV_TO_MEM BIT(2) +#define SUPPORTS_DEV_TO_DEV BIT(3)

DMA_ prefix on these. Also you could put them in an enum so they are grouped.

+/*

• • struct dma_ops - Driver model DMA operations
• • The uclass interface is implemented by all DMA devices which use
• • driver model.
• */

+struct dma_ops {

•   /*
  

•    * Get the current timer count
  

Incorrect comment.

•    *
  

•    * @dev: The DMA device
  

•    * @direction: direction of data transfer should be one from
  

•                  enum dma_direction
  

•    * @dst: Destination pointer
  

•    * @src: Source pointer
  

Any alignment restructions?

•    * @len: Length of the data to be copied.
  

In bytes?

•    * @return: 0 if OK, -ve on error
  

•    */
  

•   int (*transfer)(struct udevice *dev, int direction, void *dst,
  

s/int direction/enum dma_direction direction/

•                   void *src, size_t len);
  

+};

+/*

• • struct dma_dev_priv - information about a device used by the uclass
• • @supported: mode of transfers that DMA can support

Please reference where these flags come from.

• */

+struct dma_dev_priv {

•   unsigned long supported;
  

int or uint should be enough.

+};

+int dma_memcpy(void *dst, void *src, size_t len);

+#endif /* _DMA_H_ */

2.6.3.368.gf34be46

Regards, Simon

Hi,

On 1 December 2015 at 04:13, Mugunthan V N mugunthanvnm@ti.com wrote:

Add dma memcpy api to the default spi_flash_copy_mmap(), so that dma will be used to copy data when DM_DMA is defined for the platform.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/spi/sf_ops.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c index 3a56d7f..3c83f8a 100644 --- a/drivers/mtd/spi/sf_ops.c +++ b/drivers/mtd/spi/sf_ops.c @@ -16,6 +16,7 @@ #include <watchdog.h> #include <linux/compiler.h> #include <linux/log2.h> +#include <dma.h>

#include "sf_internal.h"

@@ -389,6 +390,10 @@ int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,

void __weak spi_flash_copy_mmap(void *data, void *offset, size_t len) { +#ifdef CONFIG_DM_DMA

•   if (!dma_memcpy(data, offset, len))
  

•           return;
  

+#endif memcpy(data, offset, len); }

-- 2.6.3.368.gf34be46

This looks like one driver (SPI flash) calling a weak function in another (SPI). Why isn't this done with the driver interface?

Regards, Simon

Hi Magunthan,

On 2 December 2015 at 02:45, Mugunthan V N mugunthanvnm@ti.com wrote:

On Wednesday 02 December 2015 03:24 AM, Simon Glass wrote:

...

Hi,

On 1 December 2015 at 04:13, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Add dma memcpy api to the default spi_flash_copy_mmap(), so that dma will be used to copy data when DM_DMA is defined for the platform.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/spi/sf_ops.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/spi/sf_ops.c b/drivers/mtd/spi/sf_ops.c index 3a56d7f..3c83f8a 100644 --- a/drivers/mtd/spi/sf_ops.c +++ b/drivers/mtd/spi/sf_ops.c @@ -16,6 +16,7 @@ #include <watchdog.h> #include <linux/compiler.h> #include <linux/log2.h> +#include <dma.h>

#include "sf_internal.h"

@@ -389,6 +390,10 @@ int spi_flash_read_common(struct spi_flash *flash, const u8 *cmd,

void __weak spi_flash_copy_mmap(void *data, void *offset, size_t len) { +#ifdef CONFIG_DM_DMA

•   if (!dma_memcpy(data, offset, len))
  

•           return;
  

+#endif memcpy(data, offset, len); }

-- 2.6.3.368.gf34be46

This looks like one driver (SPI flash) calling a weak function in another (SPI). Why isn't this done with the driver interface?

This is suppose to be with SPI flash driver, as the flash driver can only decide whether to use memory map transfers or spi transfers.

It was kept as a weak function so that platforms with DMA support can have their own spi_flash_copy_mmap() to transfer data with DMA support. But for some reasons this implementations landed in spi driver files which is not the best place, it should have to be done in DMA drivers.

With CONFIG_DMA, this should go away with the above #ifdef CONFIG_DMA and the weak function attribute can be removed later.

OK that sounds good, please add a TODO for this.

Reviewed-by: Simon Glass sjg@chromium.org

Regards, Simon

Hi,

On 1 December 2015 at 04:13, Mugunthan V N mugunthanvnm@ti.com wrote:

When DM_DMA is defined the default spi_flash_copy_mmap() can handle dma memory copy, so compile out spi_flash_copy_mmap() from ti_qspi driver when DM_DMA config is defined.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/spi/ti_qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c index 945290e..f6cf9f4 100644 --- a/drivers/spi/ti_qspi.c +++ b/drivers/spi/ti_qspi.c @@ -277,7 +277,7 @@ static int __ti_qspi_xfer(struct ti_qspi_priv *priv, unsigned int bitlen, }

/* TODO: control from sf layer to here through dm-spi */ -#ifdef CONFIG_TI_EDMA3 +#if defined(CONFIG_TI_EDMA3) && !defined(CONFIG_DM_DMA) void spi_flash_copy_mmap(void *data, void *offset, size_t len) { unsigned int addr = (unsigned int) (data); -- 2.6.3.368.gf34be46

This is also odd - shouldn't it be an option for the SPI driver, perhaps in the device tree?

Regards, Simon

Hi Mugunthan,

On 1 December 2015 at 04:13, Mugunthan V N mugunthanvnm@ti.com wrote:

adopt ti-edma3 driver to device driver model

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/dma/ti-edma3.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/ti-edma3.c b/drivers/dma/ti-edma3.c index d6a427f..0eb8602 100644 --- a/drivers/dma/ti-edma3.c +++ b/drivers/dma/ti-edma3.c @@ -11,6 +11,9 @@

#include <asm/io.h> #include <common.h> +#include <dma.h> +#include <dm/device.h> +#include <asm/omap_common.h> #include <asm/ti-common/ti-edma3.h>

#define EDMA3_SL_BASE(slot) (0x4000 + ((slot) << 5)) @@ -31,6 +34,11 @@ #define EDMA3_QEESR 0x108c #define EDMA3_QSECR 0x1094

+/* ti qspi priv */

May as well drop this comment as it doesn't help.

+struct ti_edma3_priv {

•   u32 base;
  

+};

/**

• qedma3_start - start qdma on a channel
• @base: base address of edma

@@ -383,8 +391,8 @@ void qedma3_stop(u32 base, struct edma3_channel_config *cfg) __raw_writel(0, base + EDMA3_QCHMAP(cfg->chnum)); }

-void edma3_transfer(unsigned long edma3_base_addr, unsigned int

•               edma_slot_num, void *dst, void *src, size_t len)
  

+void __edma3_transfer(unsigned long edma3_base_addr, unsigned int edma_slot_num,

•                 void *dst, void *src, size_t len)
  

{ struct edma3_slot_config slot; struct edma3_channel_config edma_channel; @@ -460,3 +468,66 @@ void edma3_transfer(unsigned long edma3_base_addr, unsigned int qedma3_stop(edma3_base_addr, &edma_channel); } }

+#ifndef CONFIG_DM_DMA

+void edma3_transfer(unsigned long edma3_base_addr, unsigned int edma_slot_num,

•               void *dst, void *src, size_t len)
  

+{

•   __edma3_transfer(edma3_base_addr, edma_slot_num, dst, src, len);
  

+}

+#else

+static int ti_edma3_transfer(struct udevice *dev, int direction, void *dst,

•                        void *src, size_t len)
  

+{

•   struct ti_edma3_priv *priv = dev_get_priv(dev);
  

•   /* enable edma3 clocks */
  

•   enable_edma3_clocks();
  

•   __edma3_transfer(priv->base, 1, dst, src, len);
  

What happens to direction?

•   /* disable edma3 clocks */
  

•   disable_edma3_clocks();
  

•   return 0;
  

+}

+static int ti_edma3_probe(struct udevice *bus) +{

•   /* Nothing to do in probe */
  

You can omit this function.

•   return 0;
  

+}

+static int ti_edma3_ofdata_to_platdata(struct udevice *dev) +{

•   struct ti_edma3_priv *priv = dev_get_priv(dev);
  

•   struct dma_dev_priv *uc_priv = dev_get_uclass_priv(dev);
  

•   priv->base = dev_get_addr(dev);
  

•   uc_priv->supported = SUPPORTS_MEM_TO_MEM;
  

•   return 0;
  

+}

+static const struct dma_ops ti_edma3_ops = {

•   .transfer       = ti_edma3_transfer,
  

+};

+static const struct udevice_id ti_edma3_ids[] = {

•   { .compatible = "ti,edma3" },
  

•   { }
  

+};

+U_BOOT_DRIVER(ti_edma3) = {

•   .name   = "ti_edma3",
  

•   .id     = UCLASS_DMA,
  

•   .of_match = ti_edma3_ids,
  

•   .ops    = &ti_edma3_ops,
  

•   .ofdata_to_platdata = ti_edma3_ofdata_to_platdata,
  

•   .priv_auto_alloc_size = sizeof(struct ti_edma3_priv),
  

•   .probe  = ti_edma3_probe,
  

+};

+#endif /* CONFIG_DM_DMA */

2.6.3.368.gf34be46

Regards, Simon