qemu-arm
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [Qemu-devel] [PATCH] hw: misc: Add Aspeed XDMA device

From: Eddie James
Subject: Re: [Qemu-arm] [Qemu-devel] [PATCH] hw: misc: Add Aspeed XDMA device
Date: Mon, 10 Jun 2019 13:00:51 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 

On 6/6/19 6:34 PM, Philippe Mathieu-Daudé wrote:

Hi Eddie,

On 6/4/19 12:09 AM, Eddie James wrote:

The XDMA engine embedded in the Aspeed SOCs performs PCI DMA operations
between the SOC (acting as a BMC) and a host processor in a server.

If I got your model correctly, it does no DMA operation but simply
answer correctly to the BMC, and set 'operation done' IRQ with no delay.
So this is a dummy device. Then it would be more useful having ignored
DMA ops traced with trace-events.



Thats correct. Good idea, I will add some tracing.





The XDMA engine exists on the AST2400, AST2500, and AST2600 SOCs, so
enable it for all of those.

Signed-off-by: Eddie James <address@hidden>
---
  hw/arm/aspeed_soc.c           |  14 ++++
  hw/misc/Makefile.objs         |   2 +-
  hw/misc/aspeed_xdma.c         | 156 ++++++++++++++++++++++++++++++++++++++++++
  include/hw/arm/aspeed_soc.h   |   2 +
  include/hw/misc/aspeed_xdma.h |  31 +++++++++
  5 files changed, 204 insertions(+), 1 deletion(-)
  create mode 100644 hw/misc/aspeed_xdma.c
  create mode 100644 include/hw/misc/aspeed_xdma.h

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index faff42b..b25bb18 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -31,6 +31,7 @@
  #define ASPEED_SOC_VIC_BASE         0x1E6C0000
  #define ASPEED_SOC_SDMC_BASE        0x1E6E0000
  #define ASPEED_SOC_SCU_BASE         0x1E6E2000
+#define ASPEED_SOC_XDMA_BASE        0x1E6E7000
  #define ASPEED_SOC_SRAM_BASE        0x1E720000
  #define ASPEED_SOC_TIMER_BASE       0x1E782000
  #define ASPEED_SOC_WDT_BASE         0x1E785000
@@ -159,6 +160,9 @@ static void aspeed_soc_init(Object *obj)
sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100), 
                            sizeof(s->ftgmac100), TYPE_FTGMAC100);
+
+    sysbus_init_child_obj(obj, "xdma", OBJECT(&s->xdma), sizeof(s->xdma),
+                          TYPE_ASPEED_XDMA);
  }
static void aspeed_soc_realize(DeviceState *dev, Error **errp) 
@@ -298,6 +302,16 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
**errp)
      sysbus_mmio_map(SYS_BUS_DEVICE(&s->ftgmac100), 0, ASPEED_SOC_ETH1_BASE);
      sysbus_connect_irq(SYS_BUS_DEVICE(&s->ftgmac100), 0,
                         qdev_get_gpio_in(DEVICE(&s->vic), 2));
+
+    /* XDMA */
+    object_property_set_bool(OBJECT(&s->xdma), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->xdma), 0, ASPEED_SOC_XDMA_BASE);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->xdma), 0,
+                       qdev_get_gpio_in(DEVICE(&s->vic), 6));
  }
static void aspeed_soc_class_init(ObjectClass *oc, void *data) 
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 77b9df9..a4483af 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -74,7 +74,7 @@ obj-$(CONFIG_ARMSSE_MHU) += armsse-mhu.o
obj-$(CONFIG_PVPANIC) += pvpanic.o 
  obj-$(CONFIG_AUX) += auxbus.o
-obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
+obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o aspeed_xdma.o
  obj-$(CONFIG_MSF2) += msf2-sysreg.o
  obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
diff --git a/hw/misc/aspeed_xdma.c b/hw/misc/aspeed_xdma.c 
new file mode 100644
index 0000000..fe3a32e
--- /dev/null
+++ b/hw/misc/aspeed_xdma.c
@@ -0,0 +1,156 @@
+/*
+ * ASPEED XDMA Controller
+ * Eddie James <address@hidden>
+ *
+ * Copyright (C) 2019 IBM Corp
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_xdma.h"
+#include "qapi/error.h"
+
+#define XDMA_BMC_CMDQ_ADDR         0x10
+#define XDMA_BMC_CMDQ_ENDP         0x14
+#define XDMA_BMC_CMDQ_WRP          0x18
+#define  XDMA_BMC_CMDQ_W_MASK      0x0003FFFF
+#define XDMA_BMC_CMDQ_RDP          0x1C
+#define  XDMA_BMC_CMDQ_RDP_MAGIC   0xEE882266
+#define XDMA_IRQ_ENG_CTRL          0x20
+#define  XDMA_IRQ_ENG_CTRL_US_COMP BIT(4)
+#define  XDMA_IRQ_ENG_CTRL_DS_COMP BIT(5)
+#define  XDMA_IRQ_ENG_CTRL_W_MASK  0xBFEFF07F
+#define XDMA_IRQ_ENG_STAT          0x24
+#define  XDMA_IRQ_ENG_STAT_US_COMP BIT(4)
+#define  XDMA_IRQ_ENG_STAT_DS_COMP BIT(5)
+#define  XDMA_IRQ_ENG_STAT_RESET   0xF8000000
+
+#define TO_REG(addr) ((addr) / sizeof(uint32_t))
+
+static uint64_t aspeed_xdma_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    uint32_t val = 0;
+    AspeedXDMAState *xdma = opaque;
+
+    if (addr < ASPEED_XDMA_REG_SIZE) {
+        val = xdma->regs[TO_REG(addr)];
+    }
+
+    return (uint64_t)val;
+}
+
+static void aspeed_xdma_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned int size)
+{
+    unsigned int idx;
+    uint32_t val32 = (uint32_t)val;
+    AspeedXDMAState *xdma = opaque;
+
+    if (addr >= ASPEED_XDMA_REG_SIZE) {
+        return;
+    }
+
+    switch (addr) {
+    case XDMA_BMC_CMDQ_ENDP:
+        xdma->regs[TO_REG(addr)] = val32 & XDMA_BMC_CMDQ_W_MASK;
+        break;
+    case XDMA_BMC_CMDQ_WRP:
+        idx = TO_REG(addr);
+        xdma->regs[idx] = val32 & XDMA_BMC_CMDQ_W_MASK;
+        xdma->regs[TO_REG(XDMA_BMC_CMDQ_RDP)] = xdma->regs[idx];

The two previous lines are odd. Are they inverted?
No, this should be correct. Since the command completes instantly, the engine "read pointer" is updated to the new "write pointer," with the expected result that no more commands need to be completed, since write == read. 




I guess I'd trace here:

            trace_aspeed_xdma_...(val, ...);


+
+        if (xdma->bmc_cmdq_readp_set) {
+            xdma->bmc_cmdq_readp_set = 0;
+        } else {
+            xdma->regs[TO_REG(XDMA_IRQ_ENG_STAT)] |=
+                XDMA_IRQ_ENG_STAT_US_COMP | XDMA_IRQ_ENG_STAT_DS_COMP;
+
+            if (xdma->regs[TO_REG(XDMA_IRQ_ENG_CTRL)] &
+                (XDMA_IRQ_ENG_CTRL_US_COMP | XDMA_IRQ_ENG_CTRL_DS_COMP))
+                qemu_irq_raise(xdma->irq);
+        }
+        break;
+    case XDMA_BMC_CMDQ_RDP:

Trace here too.


+        if (val32 == XDMA_BMC_CMDQ_RDP_MAGIC) {
+            xdma->bmc_cmdq_readp_set = 1;
+        }
+        break;
+    case XDMA_IRQ_ENG_CTRL:
+        xdma->regs[TO_REG(addr)] = val32 & XDMA_IRQ_ENG_CTRL_W_MASK;
+        break;
+    case XDMA_IRQ_ENG_STAT:
+        idx = TO_REG(addr);
+        if (val32 & (XDMA_IRQ_ENG_STAT_US_COMP | XDMA_IRQ_ENG_STAT_DS_COMP)) {
+            xdma->regs[TO_REG(addr)] &=

                           ^ idx



Thanks, will update.





+                ~(XDMA_IRQ_ENG_STAT_US_COMP | XDMA_IRQ_ENG_STAT_DS_COMP);
+            qemu_irq_lower(xdma->irq);
+        }
+        break;
+    default:
+        xdma->regs[TO_REG(addr)] = val32;
+        break;
+    }
+}
+
+static const MemoryRegionOps aspeed_xdma_ops = {
+    .read = aspeed_xdma_read,
+    .write = aspeed_xdma_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+};
+
+static void aspeed_xdma_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    AspeedXDMAState *xdma = ASPEED_XDMA(dev);
+
+    sysbus_init_irq(sbd, &xdma->irq);
+    memory_region_init_io(&xdma->iomem, OBJECT(xdma), &aspeed_xdma_ops, xdma,
+                          TYPE_ASPEED_XDMA, ASPEED_XDMA_MEM_SIZE);
+    sysbus_init_mmio(sbd, &xdma->iomem);
+}
+
+static void aspeed_xdma_reset(DeviceState *dev)
+{
+    AspeedXDMAState *xdma = ASPEED_XDMA(dev);
+
+    xdma->bmc_cmdq_readp_set = 0;
+    memset(xdma->regs, 0, ASPEED_XDMA_REG_SIZE);
+    xdma->regs[TO_REG(XDMA_IRQ_ENG_STAT)] = XDMA_IRQ_ENG_STAT_RESET;
+
+    qemu_irq_lower(xdma->irq);
+}
+
+static const VMStateDescription aspeed_xdma_vmstate = {
+    .name = TYPE_ASPEED_XDMA,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, AspeedXDMAState, ASPEED_XDMA_NUM_REGS),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static void aspeed_xdma_class_init(ObjectClass *classp, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(classp);
+
+    dc->realize = aspeed_xdma_realize;
+    dc->reset = aspeed_xdma_reset;
+    dc->vmsd = &aspeed_xdma_vmstate;
+}
+
+static const TypeInfo aspeed_xdma_info = {
+    .name          = TYPE_ASPEED_XDMA,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(AspeedXDMAState),
+    .class_init    = aspeed_xdma_class_init,
+};
+
+static void aspeed_xdma_register_type(void)
+{
+    type_register_static(&aspeed_xdma_info);
+}
+type_init(aspeed_xdma_register_type);
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 836b2ba..0329247 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -20,6 +20,7 @@
  #include "hw/ssi/aspeed_smc.h"
  #include "hw/watchdog/wdt_aspeed.h"
  #include "hw/net/ftgmac100.h"
+#include "hw/misc/aspeed_xdma.h"
#define ASPEED_SPIS_NUM 2 
  #define ASPEED_WDTS_NUM  3
@@ -40,6 +41,7 @@ typedef struct AspeedSoCState {
      AspeedSDMCState sdmc;
      AspeedWDTState wdt[ASPEED_WDTS_NUM];
      FTGMAC100State ftgmac100;
+    AspeedXDMAState xdma;
  } AspeedSoCState;
#define TYPE_ASPEED_SOC "aspeed-soc" 
diff --git a/include/hw/misc/aspeed_xdma.h b/include/hw/misc/aspeed_xdma.h
new file mode 100644
index 0000000..d19e9a7
--- /dev/null
+++ b/include/hw/misc/aspeed_xdma.h
@@ -0,0 +1,31 @@
+/*
+ * ASPEED XDMA Controller
+ * Eddie James <address@hidden>
+ *
+ * Copyright (C) 2019 IBM Corp.
+ * SPDX-License-Identifer: GPL-2.0-or-later
+ */
+
+#ifndef ASPEED_XDMA_H
+#define ASPEED_XDMA_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_ASPEED_XDMA "aspeed.xdma"
+#define ASPEED_XDMA(obj) OBJECT_CHECK(AspeedXDMAState, (obj), TYPE_ASPEED_XDMA)
+
+#define ASPEED_XDMA_MEM_SIZE 0x1000

Maybe you can keep ASPEED_XDMA_MEM_SIZE private in the source file.



Sure.





+#define ASPEED_XDMA_NUM_REGS (ASPEED_XDMA_REG_SIZE / sizeof(uint32_t))
+#define ASPEED_XDMA_REG_SIZE 0x7C

0x80?
Well 0x78 is the offset of the last register, so 0x7c would be the total register space. Of course the engine actually owns 4K bytes of register space but there's no reason to store all that. 





+
+typedef struct AspeedXDMAState {
+    SysBusDevice parent;
+
+    MemoryRegion iomem;
+    qemu_irq irq;
+
+    char bmc_cmdq_readp_set;
+    uint32_t regs[ASPEED_XDMA_NUM_REGS];
+} AspeedXDMAState;
+
+#endif /* ASPEED_XDMA_H */


Few questions, but otherwise LGTM.

Regards,

Phil.
reply via email to
[Prev in Thread] Current Thread [Next in Thread]