diff mbox series

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

Message ID 20220114152841.1740-6-francisco.iglesias@xilinx.com (mailing list archive)
State New, archived
Headers show
Series Xilinx Versal's PMC SLCR and OSPI support | expand

Commit Message

Francisco Iglesias Jan. 14, 2022, 3:28 p.m. UTC 
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).

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

Comments

Luc Michel Jan. 18, 2022, 10:01 p.m. UTC | #1 
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.

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

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
> 
> 

--
Francisco Iglesias Jan. 21, 2022, 3:46 p.m. UTC | #2 
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
> > 
> > 
> 
> --
diff mbox series

Patch

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 */