Hi Álvaro,

sorry for the late response.

On 05.03.2018 21:05, Álvaro Fernández Rojas wrote:
> This adds channels support for dma controllers that have multiple channels
> which can transfer data to/from different devices (enet, usb...).
> 
> Signed-off-by: Álvaro Fernández Rojas <nolt...@gmail.com>
> Reviewed-by: Simon Glass <s...@chromium.org>
> ---
>  v5: remove unneeded dma.h include
>  v4: no changes
>  v3: Introduce changes reported by Simon Glass:
>   - Improve dma-uclass.h documentation.
>   - Switch to live tree API.
> 
>  drivers/dma/Kconfig      |   7 ++
>  drivers/dma/dma-uclass.c | 188 
> +++++++++++++++++++++++++++++++++++++++++++++--
>  include/dma-uclass.h     |  78 ++++++++++++++++++++
>  include/dma.h            | 174 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 439 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 1b92c7789d..21b2c0dcaa 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -12,6 +12,13 @@ config DMA
>         buses that is used to transfer data to and from memory.
>         The uclass interface is defined in include/dma.h.
>  
> +config DMA_CHANNELS
> +     bool "Enable DMA channels support"
> +     depends on DMA
> +     help
> +       Enable channels support for DMA. Some DMA controllers have multiple
> +       channels which can either transfer data to/from different devices.
> +
>  config TI_EDMA3
>       bool "TI EDMA3 driver"
>       help
> diff --git a/drivers/dma/dma-uclass.c b/drivers/dma/dma-uclass.c
> index faa27a3a56..b5109aafc9 100644
> --- a/drivers/dma/dma-uclass.c
> +++ b/drivers/dma/dma-uclass.c
> @@ -1,23 +1,199 @@
>  /*
>   * Direct Memory Access U-Class driver
>   *
> - * (C) Copyright 2015
> - *     Texas Instruments Incorporated, <www.ti.com>
> - *
> - * Author: Mugunthan V N <mugunthan...@ti.com>
> + * Copyright (C) 2018 Álvaro Fernández Rojas <nolt...@gmail.com>
> + * Copyright (C) 2015 Texas Instruments Incorporated <www.ti.com>
> + * Written by Mugunthan V N <mugunthan...@ti.com>
>   *
>   * SPDX-License-Identifier:     GPL-2.0+
>   */
>  
>  #include <common.h>
>  #include <dm.h>
> -#include <dm/uclass-internal.h>
> -#include <dm/device-internal.h>
> +#include <dm/read.h>
>  #include <dma-uclass.h>
> +#include <dt-structs.h>
>  #include <errno.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +#ifdef CONFIG_DMA_CHANNELS
> +static inline struct dma_ops *dma_dev_ops(struct udevice *dev)
> +{
> +     return (struct dma_ops *)dev->driver->ops;
> +}
> +
> +# if CONFIG_IS_ENABLED(OF_CONTROL)
> +#  if CONFIG_IS_ENABLED(OF_PLATDATA)
> +int dma_get_by_index_platdata(struct udevice *dev, int index,
> +                           struct phandle_1_arg *cells, struct dma *dma)
> +{
> +     int ret;
> +
> +     if (index != 0)
> +             return -ENOSYS;
> +     ret = uclass_get_device(UCLASS_DMA, 0, &dma->dev);
> +     if (ret)
> +             return ret;
> +     dma->id = cells[0].id;
> +
> +     return 0;
> +}
> +#  else
> +static int dma_of_xlate_default(struct dma *dma,
> +                             struct ofnode_phandle_args *args)
> +{
> +     debug("%s(dma=%p)\n", __func__, dma);
> +
> +     if (args->args_count > 1) {
> +             pr_err("Invaild args_count: %d\n", args->args_count);
> +             return -EINVAL;
> +     }
> +
> +     if (args->args_count)
> +             dma->id = args->args[0];
> +     else
> +             dma->id = 0;
> +
> +     return 0;
> +}
> +
> +int dma_get_by_index(struct udevice *dev, int index, struct dma *dma)
> +{
> +     int ret;
> +     struct ofnode_phandle_args args;
> +     struct udevice *dev_dma;
> +     const struct dma_ops *ops;
> +
> +     debug("%s(dev=%p, index=%d, dma=%p)\n", __func__, dev, index, dma);
> +
> +     assert(dma);
> +     dma->dev = NULL;
> +
> +     ret = dev_read_phandle_with_args(dev, "dmas", "#dma-cells", 0, index,
> +                                      &args);
> +     if (ret) {
> +             pr_err("%s: dev_read_phandle_with_args failed: err=%d\n",
> +                    __func__, ret);
> +             return ret;
> +     }
> +
> +     ret = uclass_get_device_by_ofnode(UCLASS_DMA, args.node, &dev_dma);
> +     if (ret) {
> +             pr_err("%s: uclass_get_device_by_ofnode failed: err=%d\n",
> +                    __func__, ret);
> +             return ret;
> +     }
> +
> +     dma->dev = dev_dma;
> +
> +     ops = dma_dev_ops(dev_dma);
> +
> +     if (ops->of_xlate)
> +             ret = ops->of_xlate(dma, &args);
> +     else
> +             ret = dma_of_xlate_default(dma, &args);
> +     if (ret) {
> +             pr_err("of_xlate() failed: %d\n", ret);
> +             return ret;
> +     }
> +
> +     return dma_request(dev_dma, dma);
> +}
> +#  endif /* OF_PLATDATA */
> +
> +int dma_get_by_name(struct udevice *dev, const char *name, struct dma *dma)
> +{
> +     int index;
> +
> +     debug("%s(dev=%p, name=%s, dma=%p)\n", __func__, dev, name, dma);
> +     dma->dev = NULL;
> +
> +     index = dev_read_stringlist_search(dev, "dma-names", name);
> +     if (index < 0) {
> +             pr_err("dev_read_stringlist_search() failed: %d\n", index);
> +             return index;
> +     }
> +
> +     return dma_get_by_index(dev, index, dma);
> +}
> +# endif /* OF_CONTROL */
> +
> +int dma_request(struct udevice *dev, struct dma *dma)
> +{
> +     struct dma_ops *ops = dma_dev_ops(dev);
> +
> +     debug("%s(dev=%p, dma=%p)\n", __func__, dev, dma);
> +
> +     dma->dev = dev;
> +
> +     if (!ops->request)
> +             return 0;
> +
> +     return ops->request(dma);
> +}
> +
> +int dma_free(struct dma *dma)
> +{
> +     struct dma_ops *ops = dma_dev_ops(dma->dev);
> +
> +     debug("%s(dma=%p)\n", __func__, dma);
> +
> +     if (!ops->free)
> +             return 0;
> +
> +     return ops->free(dma);
> +}
> +
> +int dma_enable(struct dma *dma)
> +{
> +     struct dma_ops *ops = dma_dev_ops(dma->dev);
> +
> +     debug("%s(dma=%p)\n", __func__, dma);
> +
> +     if (!ops->enable)
> +             return -ENOSYS;
> +
> +     return ops->enable(dma);
> +}
> +
> +int dma_disable(struct dma *dma)
> +{
> +     struct dma_ops *ops = dma_dev_ops(dma->dev);
> +
> +     debug("%s(dma=%p)\n", __func__, dma);
> +
> +     if (!ops->disable)
> +             return -ENOSYS;
> +
> +     return ops->disable(dma);
> +}
> +
> +int dma_receive(struct dma *dma, void **dst)
> +{
> +     struct dma_ops *ops = dma_dev_ops(dma->dev);
> +
> +     debug("%s(dma=%p)\n", __func__, dma);
> +
> +     if (!ops->receive)
> +             return -1;
> +
> +     return ops->receive(dma, dst);
> +}
> +
> +int dma_send(struct dma *dma, void *src, size_t len)
> +{
> +     struct dma_ops *ops = dma_dev_ops(dma->dev);
> +
> +     debug("%s(dma=%p)\n", __func__, dma);
> +
> +     if (!ops->send)
> +             return -1;
> +
> +     return ops->send(dma, src, len);
> +}
> +#endif /* CONFIG_DMA_CHANNELS */
> +
>  int dma_get_device(u32 transfer_type, struct udevice **devp)
>  {
>       struct udevice *dev;
> diff --git a/include/dma-uclass.h b/include/dma-uclass.h
> index 3429f65ec4..b334adb68b 100644
> --- a/include/dma-uclass.h
> +++ b/include/dma-uclass.h
> @@ -13,6 +13,8 @@
>  
>  #include <dma.h>
>  
> +struct ofnode_phandle_args;
> +
>  /*
>   * struct dma_ops - Driver model DMA operations
>   *
> @@ -20,6 +22,82 @@
>   * driver model.
>   */
>  struct dma_ops {
> +#ifdef CONFIG_DMA_CHANNELS
> +     /**
> +      * of_xlate - Translate a client's device-tree (OF) DMA specifier.
> +      *
> +      * The DMA core calls this function as the first step in implementing
> +      * a client's dma_get_by_*() call.
> +      *
> +      * If this function pointer is set to NULL, the DMA core will use a
> +      * default implementation, which assumes #dma-cells = <1>, and that
> +      * the DT cell contains a simple integer DMA Channel.
> +      *
> +      * At present, the DMA API solely supports device-tree. If this
> +      * changes, other xxx_xlate() functions may be added to support those
> +      * other mechanisms.
> +      *
> +      * @dma: The dma struct to hold the translation result.
> +      * @args:       The dma specifier values from device tree.
> +      * @return 0 if OK, or a negative error code.
> +      */
> +     int (*of_xlate)(struct dma *dma,
> +                     struct ofnode_phandle_args *args);
> +     /**
> +      * request - Request a translated DMA.
> +      *
> +      * The DMA core calls this function as the second step in
> +      * implementing a client's dma_get_by_*() call, following a successful
> +      * xxx_xlate() call, or as the only step in implementing a client's
> +      * dma_request() call.
> +      *
> +      * @dma: The DMA struct to request; this has been filled in by
> +      *   a previoux xxx_xlate() function call, or by the caller of
> +      *   dma_request().
> +      * @return 0 if OK, or a negative error code.
> +      */
> +     int (*request)(struct dma *dma);
> +     /**
> +      * free - Free a previously requested dma.
> +      *
> +      * This is the implementation of the client dma_free() API.
> +      *
> +      * @dma: The DMA to free.
> +      * @return 0 if OK, or a negative error code.
> +      */
> +     int (*free)(struct dma *dma);
> +     /**
> +      * enable() - Enable a DMA Channel.
> +      *
> +      * @dma: The DMA Channel to manipulate.
> +      * @return zero on success, or -ve error code.
> +      */
> +     int (*enable)(struct dma *dma);
> +     /**
> +      * disable() - Disable a DMA Channel.
> +      *
> +      * @dma: The DMA Channel to manipulate.
> +      * @return zero on success, or -ve error code.
> +      */
> +     int (*disable)(struct dma *dma);
> +     /**
> +      * receive() - Receive a DMA transfer.
> +      *
> +      * @dma: The DMA Channel to manipulate.
> +      * @dst: The destination pointer.
> +      * @return zero on success, or -ve error code.
> +      */
> +     int (*receive)(struct dma *dma, void **dst);
> +     /**
> +      * send() - Send a DMA transfer.
> +      *
> +      * @dma: The DMA Channel to manipulate.
> +      * @src: The source pointer.
> +      * @len: Length of the data to be sent (number of bytes).
> +      * @return zero on success, or -ve error code.
> +      */
> +     int (*send)(struct dma *dma, void *src, size_t len);
> +#endif /* CONFIG_DMA_CHANNELS */

IMHO a DMA driver should not implement receive() and send() directly as
this involves generic code like flushing/invalidating caches,
synchronous waiting on descriptor status etc. This could be better
handled by dma-uclass code than repeating it in each driver.

I think a heavily simplified design of Linux's dmaengine would help. For
instance the driver interface could be modelled with prepare_cyclic(),
prepare_memcpy() and status(). The dma-uclass can then provide an
interface for those functions as well as additional wrapper functions
like dma_is_complete(), dma_sync_wait(), dma_receive() or dma_send().

Regardless of the design, I'd like to have at least separate functions
for preparing the buffers and polling the descriptor. For instance I
have a DMA-capable NAND controller, where the DMA transfer is only
started by writing some HW registers. This must happen between preparing
the buffers and DMA descriptors and polling the descriptors until the
transaction has finished.

Could you do that?
>       /**
>        * transfer() - Issue a DMA transfer. The implementation must
>        *   wait until the transfer is done.
> diff --git a/include/dma.h b/include/dma.h
> index 89320f10d9..bf8123fa9e 100644
> --- a/include/dma.h
> +++ b/include/dma.h
> @@ -1,6 +1,7 @@
>  /*
> - * (C) Copyright 2015
> - *     Texas Instruments Incorporated, <www.ti.com>
> + * Copyright (C) 2018 Álvaro Fernández Rojas <nolt...@gmail.com>
> + * Copyright (C) 2015 Texas Instruments Incorporated <www.ti.com>
> + * Written by Mugunthan V N <mugunthan...@ti.com>
>   *
>   * SPDX-License-Identifier:     GPL-2.0+
>   */
> @@ -8,6 +9,9 @@
>  #ifndef _DMA_H_
>  #define _DMA_H_
>  
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +
>  /*
>   * enum dma_direction - dma transfer direction indicator
>   * @DMA_MEM_TO_MEM: Memcpy mode
> @@ -37,6 +41,172 @@ struct dma_dev_priv {
>       u32 supported;
>  };
>  
> +#ifdef CONFIG_DMA_CHANNELS
> +/**
> + * A DMA is a feature of computer systems that allows certain hardware
> + * subsystems to access main system memory, independent of the CPU.
> + * DMA channels are typically generated externally to the HW module
> + * consuming them, by an entity this API calls a DMA provider. This API
> + * provides a standard means for drivers to enable and disable DMAs, and to
> + * copy, send and receive data using DMA.
> + *
> + * A driver that implements UCLASS_DMA is a DMA provider. A provider will
> + * often implement multiple separate DMAs, since the hardware it manages
> + * often has this capability. dma_uclass.h describes the interface which
> + * DMA providers must implement.
> + *
> + * DMA consumers/clients are the HW modules driven by the DMA channels. This
> + * header file describes the API used by drivers for those HW modules.
> + */
> +
> +struct udevice;
> +
> +/**
> + * struct dma - A handle to (allowing control of) a single DMA.
> + *
> + * Clients provide storage for DMA handles. The content of the structure is
> + * managed solely by the DMA API and DMA drivers. A DMA struct is
> + * initialized by "get"ing the DMA struct. The DMA struct is passed to all
> + * other DMA APIs to identify which DMA channel to operate upon.
> + *
> + * @dev: The device which implements the DMA channel.
> + * @id: The DMA channel ID within the provider.
> + *
> + * Currently, the DMA API assumes that a single integer ID is enough to
> + * identify and configure any DMA channel for any DMA provider. If this
> + * assumption becomes invalid in the future, the struct could be expanded to
> + * either (a) add more fields to allow DMA providers to store additional
> + * information, or (b) replace the id field with an opaque pointer, which the
> + * provider would dynamically allocated during its .of_xlate op, and process
> + * during is .request op. This may require the addition of an extra op to 
> clean
> + * up the allocation.
> + */
> +struct dma {
> +     struct udevice *dev;
> +     /*
> +      * Written by of_xlate. We assume a single id is enough for now. In the
> +      * future, we might add more fields here.
> +      */
> +     unsigned long id;
> +};
> +
> +# if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(DMA)
> +struct phandle_1_arg;
> +int dma_get_by_index_platdata(struct udevice *dev, int index,
> +                           struct phandle_1_arg *cells, struct dma *dma);
> +
> +/**
> + * dma_get_by_index - Get/request a DMA by integer index.
> + *
> + * This looks up and requests a DMA. The index is relative to the client
> + * device; each device is assumed to have n DMAs associated with it somehow,
> + * and this function finds and requests one of them. The mapping of client
> + * device DMA indices to provider DMAs may be via device-tree properties,
> + * board-provided mapping tables, or some other mechanism.
> + *
> + * @dev:     The client device.
> + * @index:   The index of the DMA to request, within the client's list of
> + *           DMA channels.
> + * @dma:     A pointer to a DMA struct to initialize.
> + * @return 0 if OK, or a negative error code.
> + */
> +int dma_get_by_index(struct udevice *dev, int index, struct dma *dma);
> +
> +/**
> + * dma_get_by_name - Get/request a DMA by name.
> + *
> + * This looks up and requests a DMA. The name is relative to the client
> + * device; each device is assumed to have n DMAs associated with it somehow,
> + * and this function finds and requests one of them. The mapping of client
> + * device DMA names to provider DMAs may be via device-tree properties,
> + * board-provided mapping tables, or some other mechanism.
> + *
> + * @dev:     The client device.
> + * @name:    The name of the DMA to request, within the client's list of
> + *           DMA channels.
> + * @dma:     A pointer to a DMA struct to initialize.
> + * @return 0 if OK, or a negative error code.
> + */
> +int dma_get_by_name(struct udevice *dev, const char *name, struct dma *dma);
> +# else
> +static inline int dma_get_by_index(struct udevice *dev, int index,
> +                                struct dma *dma)
> +{
> +     return -ENOSYS;
> +}
> +
> +static inline int dma_get_by_name(struct udevice *dev, const char *name,
> +                        struct dma *dma)
> +{
> +     return -ENOSYS;
> +}
> +# endif
> +
> +/**
> + * dma_request - Request a DMA by provider-specific ID.
> + *
> + * This requests a DMA using a provider-specific ID. Generally, this function
> + * should not be used, since dma_get_by_index/name() provide an interface 
> that
> + * better separates clients from intimate knowledge of DMA providers.
> + * However, this function may be useful in core SoC-specific code.
> + *
> + * @dev: The DMA provider device.
> + * @dma: A pointer to a DMA struct to initialize. The caller must
> + *    have already initialized any field in this struct which the
> + *    DMA provider uses to identify the DMA channel.
> + * @return 0 if OK, or a negative error code.
> + */
> +int dma_request(struct udevice *dev, struct dma *dma);
> +
> +/**
> + * dma_free - Free a previously requested DMA.
> + *
> + * @dma: A DMA struct that was previously successfully requested by
> + *    dma_request/get_by_*().
> + * @return 0 if OK, or a negative error code.
> + */
> +int dma_free(struct dma *dma);
> +
> +/**
> + * dma_enable() - Enable (turn on) a DMA channel.
> + *
> + * @dma: A DMA struct that was previously successfully requested by
> + *    dma_request/get_by_*().
> + * @return zero on success, or -ve error code.
> + */
> +int dma_enable(struct dma *dma);
> +
> +/**
> + * dma_disable() - Disable (turn off) a DMA channel.
> + *
> + * @dma: A DMA struct that was previously successfully requested by
> + *    dma_request/get_by_*().
> + * @return zero on success, or -ve error code.
> + */
> +int dma_disable(struct dma *dma);
> +
> +/**
> + * dma_receive() - Receive a DMA transfer.
> + *
> + * @dma: A DMA struct that was previously successfully requested by
> + *    dma_request/get_by_*().
> + * @dst: The destination pointer.
> + * @return zero on success, or -ve error code.
> + */
> +int dma_receive(struct dma *dma, void **dst);
> +
> +/**
> + * dma_send() - Send a DMA transfer.
> + *
> + * @dma: A DMA struct that was previously successfully requested by
> + *    dma_request/get_by_*().
> + * @src: The source pointer.
> + * @len: Length of the data to be sent (number of bytes).
> + * @return zero on success, or -ve error code.
> + */
> +int dma_send(struct dma *dma, void *src, size_t len);
> +#endif /* CONFIG_DMA_CHANNELS */
> +
>  /*
>   * dma_get_device - get a DMA device which supports transfer
>   * type of transfer_type
> 

-- 
- Daniel

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to