Re: [PATCH v3 04/10] hw/dma: Add the DMA control interface

[Peter Maydell]

On Wed, 24 Nov 2021 at 10:16, Francisco Iglesias
<francisco.iglesias@xilinx.com> wrote:
>
> Add an interface for controlling DMA models that are reused with other
> models. This allows a controlling model to start transfers through the
> DMA while reusing the DMA's handling of transfer state and completion
> signaling.
>
> Signed-off-by: Francisco Iglesias <francisco.iglesias@xilinx.com>
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Could you give an expanded sketch of the design here, please?
What sort of objects would implement this new interface? Who
calls it? Should all new DMA engine devices consider implementing it?

If it's likely to be widely useful we should consider having
documentation under docs/devel for the API.

> ---
>  hw/dma/dma-ctrl.c         | 31 ++++++++++++++++++++
>  hw/dma/meson.build        |  1 +
>  include/hw/dma/dma-ctrl.h | 74 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 106 insertions(+)
>  create mode 100644 hw/dma/dma-ctrl.c
>  create mode 100644 include/hw/dma/dma-ctrl.h
>
> diff --git a/hw/dma/dma-ctrl.c b/hw/dma/dma-ctrl.c
> new file mode 100644
> index 0000000000..4a9b68dac1
> --- /dev/null
> +++ b/hw/dma/dma-ctrl.c
> @@ -0,0 +1,31 @@
> +/*
> + * DMA control interface.
> + *
> + * Copyright (c) 2021 Xilinx Inc.
> + * Written by Francisco Iglesias <francisco.iglesias@xilinx.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#include "qemu/osdep.h"
> +#include "exec/hwaddr.h"
> +#include "hw/dma/dma-ctrl.h"
> +
> +void dma_ctrl_read_with_notify(DmaCtrl *dma_ctrl, hwaddr addr, uint32_t len,
> +                               DmaCtrlNotify *notify, bool start_dma)
> +{
> +    DmaCtrlClass *dcc =  DMA_CTRL_GET_CLASS(dma_ctrl);
> +    dcc->read(dma_ctrl, addr, len, notify, start_dma);
> +}
> +
> +static const TypeInfo dma_ctrl_info = {
> +    .name          = TYPE_DMA_CTRL,
> +    .parent        = TYPE_INTERFACE,
> +    .class_size = sizeof(DmaCtrlClass),
> +};
> +
> +static void dma_ctrl_register_types(void)
> +{
> +    type_register_static(&dma_ctrl_info);
> +}
> +
> +type_init(dma_ctrl_register_types)
> diff --git a/hw/dma/meson.build b/hw/dma/meson.build
> index f3f0661bc3..c0bc134046 100644
> --- a/hw/dma/meson.build
> +++ b/hw/dma/meson.build
> @@ -14,3 +14,4 @@ softmmu_ss.add(when: 'CONFIG_PXA2XX', if_true: 
> files('pxa2xx_dma.c'))
>  softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_dma.c'))
>  softmmu_ss.add(when: 'CONFIG_SIFIVE_PDMA', if_true: files('sifive_pdma.c'))
>  softmmu_ss.add(when: 'CONFIG_XLNX_CSU_DMA', if_true: files('xlnx_csu_dma.c'))
> +common_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('dma-ctrl.c'))
> diff --git a/include/hw/dma/dma-ctrl.h b/include/hw/dma/dma-ctrl.h
> new file mode 100644
> index 0000000000..498469395f
> --- /dev/null
> +++ b/include/hw/dma/dma-ctrl.h
> @@ -0,0 +1,74 @@
> +/*
> + * DMA control interface.
> + *
> + * Copyright (c) 2021 Xilinx Inc.
> + * Written by Francisco Iglesias <francisco.iglesias@xilinx.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef HW_DMA_CTRL_H
> +#define HW_DMA_CTRL_H
> +
> +#include "qemu-common.h"

Header files should not include qemu-common.h; the comment at the
top explains:

/*
 * This file is supposed to be included only by .c files. No header file should
 * depend on qemu-common.h, as this would easily lead to circular header
 * dependencies.
 *
 * If a header file uses a definition from qemu-common.h, that definition
 * must be moved to a separate header file, and the header that uses it
 * must include that header.
 */

> +#include "hw/hw.h"
> +#include "qom/object.h"
> +
> +#define TYPE_DMA_CTRL "dma-ctrl"
> +
> +#define DMA_CTRL_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(DmaCtrlClass, (klass), TYPE_DMA_CTRL)
> +#define DMA_CTRL_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(DmaCtrlClass, (obj), TYPE_DMA_CTRL)

Use the DECLARE_CLASS_CHECKERS macro rather than hand-writing these.

> +#define DMA_CTRL(obj) \
> +     INTERFACE_CHECK(DmaCtrl, (obj), TYPE_DMA_CTRL)
> +
> +typedef void (*dmactrl_notify_fn)(void *opaque);
> +
> +typedef struct DmaCtrlNotify {
> +    void *opaque;
> +    dmactrl_notify_fn cb;
> +} DmaCtrlNotify;
> +
> +typedef struct DmaCtrl {
> +    Object Parent;
> +} DmaCtrl;
> +
> +typedef struct DmaCtrlClass {

Can you include either "If" or "Interface" in the class/struct names
of interfaces, please? (We have examples of both, eg ArmLinuxBootIf
and IDAUInterface.) I think it makes it clearer that this is an interface
and not a real object.

> +    InterfaceClass parent;
> +
> +    /*
> +     * read: Start a read transfer on the DMA implementing the DMA control
> +     * interface
> +     *
> +     * @dma_ctrl: the DMA implementing this interface
> +     * @addr: the address to read
> +     * @len: the amount of bytes to read at 'addr'
> +     * @notify: the structure containg a callback to call and opaque pointer
> +     * to pass the callback when the transfer has been completed
> +     * @start_dma: true for starting the DMA transfer and false for just
> +     * refilling and proceding an already started transfer
> +     */
> +    void (*read)(DmaCtrl *dma_ctrl, hwaddr addr, uint32_t len,
> +                 DmaCtrlNotify *notify, bool start_dma);
> +} DmaCtrlClass;
> +
> +/*
> + * Start a read transfer on a DMA implementing the DMA control interface.
> + * The DMA will notify the caller that 'len' bytes have been read at 'addr'
> + * through the callback in the DmaCtrlNotify structure. For allowing 
> refilling
> + * an already started transfer the DMA notifies the caller before considering
> + * the transfer done (e.g. before setting done flags, generating IRQs and
> + * modifying other relevant internal device state).
> + *
> + * @dma_ctrl: the DMA implementing this interface
> + * @addr: the address to read
> + * @len: the amount of bytes to read at 'addr'
> + * @notify: the structure containing a callback to call and opaque pointer
> + * to pass the callback when the transfer has been completed
> + * @start_dma: true for starting the DMA transfer and false for just
> + * refilling and proceding an already started transfer
> + */
> +void dma_ctrl_read_with_notify(DmaCtrl *dma_ctrl, hwaddr addr, uint32_t len,
> +                   DmaCtrlNotify *notify, bool start_dma);
> +
> +#endif /* HW_DMA_CTRL_H */
> --
> 2.11.0

thanks
-- PMM