Re: [PATCH v6 05/12] hw/dma: Add the DMA control interface

[Francisco Iglesias <frasse.iglesias@gmail.com>]

On [2022 Jan 18] Tue 23:01:42, Luc Michel wrote:
> Hi Francisco!
> 
> On 15:28 Fri 14 Jan     , Francisco Iglesias wrote:
> > An option on real hardware when embedding a DMA engine into a peripheral
> > is to make the peripheral control the engine through a custom DMA control
> > (hardware) interface between the two. Software drivers in this scenario
> > configure and trigger DMA operations through the controlling peripheral's
> > register API (for example, writing a specific bit in a register could
> > propagate down to a transfer start signal on the DMA control interface).
> > At the same time the status, results and interrupts for the transfer might
> > still be intended to be read and caught through the DMA engine's register
> > API (and signals).
> 
> I understand the goal you trying to achieve here. Having a generic
> interface between a peripheral and the internal DMA it's using for its
> memory transfers.
> 
> I wonder however how much this scenario can be generalized. I see that
> you have only one method in this interface, which is basically "perform
> a DMA transaction". Given the method's parameters, the peripheral can
> indicate what address/length it wants to read.
> 
> IIUC this is well suited to your case (the OSPI controller), because
> other DMA parameters are configured by other means (DMA or OSPI
> registers I guess).
> 
> But how much this will suite the next use case for this interface? Will
> the embedded DMA be configured the same way? Maybe the method will also
> require e.g., the destination buffer address?
> 
> My feeling is that this interface is quite ad-hoc for your case. It is
> either going to stay really simple as it is now, and won't necessarily
> fit the next similar use case, or is going to get pretty complicated to
> cover all cases. For an added value I'm not sure I can see because the
> pair "peripheral - DMA" seems tightly coupled to me. Do you foresee a
> case where you want to be able to e.g., instantiate DMA B instead of DMA
> A for the same controller?
> 
> I think you could have basically the same code by having a pointer to
> your XlnxCSUDMA object (instead of the iface) in the OSPI struct, and a
> function like xlnx_csu_dma_start_transfer declared in xlnx_csu_dma.h,
> having the same behaviour as dma_ctrl_if_read.
> 

Hi Luc,

> Maybe I'm wrong and you actually foresee cases where the genericity this
> interface gives is what you want? What do you think?

I see your point! Lets go with what you proposed above instead then! v7 will
have taken that direction!

Thank you again for reviewing!

Best regards,
Francisco Iglesias

> 
> Luc
> 
> > 
> > This patch adds a QEMU DMA control interface that can be used for
> > modelling above scenario. Through this new interface a peripheral model
> > embedding a DMA engine model will be able to directly initiate transfers
> > through the DMA. At the same time the transfer state, result and
> > completion signaling will be read and caught through the DMA engine
> > model's register API and signaling.
> > 
> > Signed-off-by: Francisco Iglesias <francisco.iglesias@xilinx.com>
> > ---
> >  hw/dma/dma-ctrl-if.c         | 30 +++++++++++++++++++++++
> >  hw/dma/meson.build           |  1 +
> >  include/hw/dma/dma-ctrl-if.h | 58 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644 hw/dma/dma-ctrl-if.c
> >  create mode 100644 include/hw/dma/dma-ctrl-if.h
> > 
> > diff --git a/hw/dma/dma-ctrl-if.c b/hw/dma/dma-ctrl-if.c
> > new file mode 100644
> > index 0000000000..895edac277
> > --- /dev/null
> > +++ b/hw/dma/dma-ctrl-if.c
> > @@ -0,0 +1,30 @@
> > +/*
> > + * 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-if.h"
> > +
> > +MemTxResult dma_ctrl_if_read(DmaCtrlIf *dma, hwaddr addr, uint32_t len)
> > +{
> > +    DmaCtrlIfClass *dcic =  DMA_CTRL_IF_GET_CLASS(dma);
> > +    return dcic->read(dma, addr, len);
> > +}
> > +
> > +static const TypeInfo dma_ctrl_if_info = {
> > +    .name          = TYPE_DMA_CTRL_IF,
> > +    .parent        = TYPE_INTERFACE,
> > +    .class_size = sizeof(DmaCtrlIfClass),
> > +};
> > +
> > +static void dma_ctrl_if_register_types(void)
> > +{
> > +    type_register_static(&dma_ctrl_if_info);
> > +}
> > +
> > +type_init(dma_ctrl_if_register_types)
> > diff --git a/hw/dma/meson.build b/hw/dma/meson.build
> > index f3f0661bc3..c43c067856 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-if.c'))
> > diff --git a/include/hw/dma/dma-ctrl-if.h b/include/hw/dma/dma-ctrl-if.h
> > new file mode 100644
> > index 0000000000..0662149e14
> > --- /dev/null
> > +++ b/include/hw/dma/dma-ctrl-if.h
> > @@ -0,0 +1,58 @@
> > +/*
> > + * 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_IF_H
> > +#define HW_DMA_CTRL_IF_H
> > +
> > +#include "hw/hw.h"
> > +#include "exec/memory.h"
> > +#include "qom/object.h"
> > +
> > +#define TYPE_DMA_CTRL_IF "dma-ctrl-if"
> > +typedef struct DmaCtrlIfClass DmaCtrlIfClass;
> > +DECLARE_CLASS_CHECKERS(DmaCtrlIfClass, DMA_CTRL_IF,
> > +                       TYPE_DMA_CTRL_IF)
> > +
> > +#define DMA_CTRL_IF(obj) \
> > +     INTERFACE_CHECK(DmaCtrlIf, (obj), TYPE_DMA_CTRL_IF)
> > +
> > +typedef struct DmaCtrlIf {
> > +    Object Parent;
> > +} DmaCtrlIf;
> > +
> > +typedef struct DmaCtrlIfClass {
> > +    InterfaceClass parent;
> > +
> > +    /*
> > +     * read: Start a read transfer on the DMA engine implementing the DMA
> > +     * control interface
> > +     *
> > +     * @dma_ctrl: the DMA engine implementing this interface
> > +     * @addr: the address to read
> > +     * @len: the number of bytes to read at 'addr'
> > +     *
> > +     * @return a MemTxResult indicating whether the operation succeeded ('len'
> > +     * bytes were read) or failed.
> > +     */
> > +    MemTxResult (*read)(DmaCtrlIf *dma, hwaddr addr, uint32_t len);
> > +} DmaCtrlIfClass;
> > +
> > +/*
> > + * Start a read transfer on a DMA engine implementing the DMA control
> > + * interface.
> > + *
> > + * @dma_ctrl: the DMA engine implementing this interface
> > + * @addr: the address to read
> > + * @len: the number of bytes to read at 'addr'
> > + *
> > + * @return a MemTxResult indicating whether the operation succeeded ('len'
> > + * bytes were read) or failed.
> > + */
> > +MemTxResult dma_ctrl_if_read(DmaCtrlIf *dma, hwaddr addr, uint32_t len);
> > +
> > +#endif /* HW_DMA_CTRL_IF_H */
> > -- 
> > 2.11.0
> > 
> > 
> 
> --