[U-Boot] [PATCH v5 02/15] dma: add channels support

Grygorii Strashko
2018-03-08T23:07:41Z

Hi Álvaro,

On 03/05/2018 02:05 PM, Á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 <noltari@gmail.com>
> Reviewed-by: Simon Glass <sjg@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(-)

Small note first. I don't know if this is common practice for u-boot -
but Isn't it preferable to send new version of the series *not as reply* to the old one?

I've tried this and, in general, it works for me (unfortunately I can't post code yet).
Thanks a lot for you work.

But... (it's always but ;)
[...]
> + /**
> + * 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);

Can we have additional *optional* parameter in above two callbacks and in dma_receive/dma_send() API?
Like:
int (*send)(struct dma *dma, void *src, size_t len, void *metadata)
int (*receive)(struct dma *dma, void **dst, void **metadata);

Reason:
It's not a common practice to implement Networking DMA using generic DMA frameworks, simply
because Networking DMA HW is terribly different between different SoCs :(, so it's mostly impossible
to fit all of them in any generic DMA framework (at least this has never ever worked for Linux kernel):
- totally different HW rings/queues impl, multi-queues & multi DMA channels
- special requirements for IRQ handling
- necessity to pass additional information within each Net DMA descriptor
- availability and support of different Net traffic HW acceleration features

But in case of u-boot, it, theoretically, might work because most of Net
drivers have much more simplified implementation comparing to Linux kernel
- UP, polling, one RX/TX channel and disabled Net traffic HW acceleration features.

As result, Proposed here interface can be used with much more different HW and
drivers (and especially Net drivers), but only if it will be possible to pass
additional DMA driver's specific information from DMA user to DMA driver
per each send/rsv request.
For example, two TI driver cpsw.c and keystone_net.c required to pass
minimum one additional parameter with each sending packet - Destination Port number,
which must be passed to DMA in DMA descriptor. And Source Port number
has to be passed back with each received packet.

! I'm not insisting here ! and be happy to hear third opinion.

(if proposition will not be accepted - .. :( I'll just need to do the same later,
but there might be hight number of active users of this interface in u-boot already)

> +#endif /* CONFIG_DMA_CHANNELS */
> /**
> * 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 <noltari@gmail.com>
> + * Copyright (C) 2015 Texas Instruments Incorporated <www.ti.com>
> + * Written by Mugunthan V N <mugunthanvnm@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
>

--
regards,
-grygorii