qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 4/4] ppc440_uc: Basic emulation of


From: BALATON Zoltan
Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 4/4] ppc440_uc: Basic emulation of PPC440 DMA controller
Date: Thu, 28 Jun 2018 16:25:03 +0200 (CEST)
User-agent: Alpine 2.21 (BSF 202 2017-01-01)

On Thu, 28 Jun 2018, Cédric Le Goater wrote:
On 06/24/2018 01:20 PM, BALATON Zoltan wrote:
PPC440 SoCs such as the AMCC 460EX have a DMA controller which is used
by AmigaOS on the sam460ex. Implement the parts used by AmigaOS so it
can get further booting on the sam460ex machine.

Signed-off-by: BALATON Zoltan <address@hidden>
---
 hw/ppc/ppc440.h    |   1 +
 hw/ppc/ppc440_uc.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/sam460ex.c  |   3 +
 3 files changed, 219 insertions(+)

diff --git a/hw/ppc/ppc440.h b/hw/ppc/ppc440.h
index ad27db1..7cef936 100644
--- a/hw/ppc/ppc440.h
+++ b/hw/ppc/ppc440.h
@@ -21,6 +21,7 @@ void ppc440_sdram_init(CPUPPCState *env, int nbanks,
                        hwaddr *ram_bases, hwaddr *ram_sizes,
                        int do_init);
 void ppc4xx_ahb_init(CPUPPCState *env);
+void ppc4xx_dma_init(CPUPPCState *env, int dcr_base);
 void ppc460ex_pcie_init(CPUPPCState *env);

 #endif /* PPC440_H */
diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
index 123f4ac..38aadd9 100644
--- a/hw/ppc/ppc440_uc.c
+++ b/hw/ppc/ppc440_uc.c
@@ -13,6 +13,7 @@
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "cpu.h"
 #include "hw/hw.h"
 #include "exec/address-spaces.h"
@@ -803,6 +804,220 @@ void ppc4xx_ahb_init(CPUPPCState *env)
 }

 /*****************************************************************************/
+/* DMA controller */
+
+#define DMA0_CR_CE  (1 << 31)
+#define DMA0_CR_PW  (1 << 26 | 1 << 25)
+#define DMA0_CR_DAI (1 << 24)
+#define DMA0_CR_SAI (1 << 23)
+#define DMA0_CR_DEC (1 << 2)
+
+enum {
+    DMA0_CR  = 0x00,
+    DMA0_CT,
+    DMA0_SAH,
+    DMA0_SAL,
+    DMA0_DAH,
+    DMA0_DAL,
+    DMA0_SGH,
+    DMA0_SGL,
+
+    DMA0_SR  = 0x20,
+    DMA0_SGC = 0x23,
+    DMA0_SLP = 0x25,
+    DMA0_POL = 0x26,
+};
+
+typedef struct {
+    uint32_t cr;
+    uint32_t ct;
+    uint64_t sa;
+    uint64_t da;
+    uint64_t sg;
+} ppc4xx_dma_ch_t;

You need to use CamelCase for the type names.

OK.

+typedef struct {
+    int base;
+    ppc4xx_dma_ch_t ch[4];
+    uint32_t sr;
+} ppc4xx_dma_t;


Can't you QOM'ify the model ?

Maybe I could but none of these SoC models are QOMified yet so maybe it would be better to do that as separate patch after thinking about how to model this correctly. So now this just follows the existing way which I prefer versus mixing QOM and old style devices models in a single SoC without clear plan to QOMify everything. Hope this won't block this patch and then QOMification could be done as separate cleanup series. (These SoC models will need to be cleaned up eventually but for now getting it work was my priority.)

+static uint32_t dcr_read_dma(void *opaque, int dcrn)
+{
+    ppc4xx_dma_t *dma = opaque;
+    uint32_t val = 0;
+    int addr = dcrn - dma->base;
+    int chnl = addr / 8;
+
+    switch (addr) {
+    case 0x00 ... 0x1f:
+        switch (addr % 8) {
+        case DMA0_CR:
+            val = dma->ch[chnl].cr;
+            break;
+        case DMA0_CT:
+            val = dma->ch[chnl].ct;
+            break;
+        case DMA0_SAH:
+            val = dma->ch[chnl].sa >> 32;
+            break;
+        case DMA0_SAL:
+            val = dma->ch[chnl].sa;
+            break;
+        case DMA0_DAH:
+            val = dma->ch[chnl].da >> 32;
+            break;
+        case DMA0_DAL:
+            val = dma->ch[chnl].da;
+            break;
+        case DMA0_SGH:
+            val = dma->ch[chnl].sg >> 32;
+            break;
+        case DMA0_SGL:
+            val = dma->ch[chnl].sg;
+            break;
+        }
+        break;
+    case DMA0_SR:
+        val = dma->sr;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
+                      __func__, dcrn, chnl, addr);
+    }
+
+    return val;
+}
+
+static void dcr_write_dma(void *opaque, int dcrn, uint32_t val)
+{
+    ppc4xx_dma_t *dma = opaque;
+    int addr = dcrn - dma->base;
+    int chnl = addr / 8;
+
+    switch (addr) {
+    case 0x00 ... 0x1f:
+        switch (addr % 8) {
+        case DMA0_CR:
+            dma->ch[chnl].cr = val;
+            if (val & DMA0_CR_CE) {
+                int count = dma->ch[chnl].ct & 0xffff;
+
+                if (count) {
+                    int width, i, sidx, didx;
+                    uint8_t *rptr, *wptr;
+                    hwaddr rlen, wlen;
+
+                    width = 1 << ((val & DMA0_CR_PW) >> 25);
+                    rptr = cpu_physical_memory_map(dma->ch[chnl].sa, &rlen, 0);
+                    wptr = cpu_physical_memory_map(dma->ch[chnl].da, &wlen, 1);

you don't need necessarily to map. You could just use cpu_physical_memory_rw

Would that also update dirty bitmap as unmap does? Also this way I can use memmove to optimise common case so this looked better but I'm not sure about which is the best way to implement this.


+                    if (!(val & DMA0_CR_DEC) &&
+                        val & DMA0_CR_SAI && val & DMA0_CR_DAI) {

some extra () would help reading the if.

This is personal taste I guess, I've seen other reviewers ask for less () in the past. I don't mind either way, David what do you prefer as maintainer?

+                        /* optimise common case */
+                        memmove(wptr, rptr, count * width);
+                        sidx = didx = count * width;
+                    } else {
+                        /* do it the slow way */
+                        for (sidx = didx = i = 0; i < count; i++) {
+                            uint64_t v = ldn_le_p(rptr + sidx, width);
+                            stn_le_p(wptr + didx, width, v);
+                            if (val & DMA0_CR_SAI) {
+                                sidx += width;
+                            }
+                            if (val & DMA0_CR_DAI) {
+                                didx += width;
+                            }
+                        }
+                    }
+                    cpu_physical_memory_unmap(wptr, wlen, 1, didx);
+                    cpu_physical_memory_unmap(rptr, rlen, 0, sidx);
+                }
+            }

I would put all this code doing the dma in its own routine.

I could do that but it's pretty simple so maybe does not worth splitting out yet. Maybe when model is ever improved to handle more exotic cases and this gets mode complex, if a guest actually using those found.

Thank you for the review,
BALATON Zoltan

+            break;
+        case DMA0_CT:
+            dma->ch[chnl].ct = val;
+            break;
+        case DMA0_SAH:
+            dma->ch[chnl].sa &= 0xffffffffULL;
+            dma->ch[chnl].sa |= (uint64_t)val << 32;
+            break;
+        case DMA0_SAL:
+            dma->ch[chnl].sa &= 0xffffffff00000000ULL;
+            dma->ch[chnl].sa |= val;
+            break;
+        case DMA0_DAH:
+            dma->ch[chnl].da &= 0xffffffffULL;
+            dma->ch[chnl].da |= (uint64_t)val << 32;
+            break;
+        case DMA0_DAL:
+            dma->ch[chnl].da &= 0xffffffff00000000ULL;
+            dma->ch[chnl].da |= val;
+            break;
+        case DMA0_SGH:
+            dma->ch[chnl].sg &= 0xffffffffULL;
+            dma->ch[chnl].sg |= (uint64_t)val << 32;
+            break;
+        case DMA0_SGL:
+            dma->ch[chnl].sg &= 0xffffffff00000000ULL;
+            dma->ch[chnl].sg |= val;
+            break;
+        }
+        break;
+    case DMA0_SR:
+        dma->sr &= ~val;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: unimplemented register %x (%d, %x)\n",
+                      __func__, dcrn, chnl, addr);
+    }
+}
+
+static void ppc4xx_dma_reset(void *opaque)
+{
+    ppc4xx_dma_t *dma = opaque;
+    int dma_base = dma->base;
+
+    memset(dma, 0, sizeof(*dma));
+    dma->base = dma_base;
+}
+
+void ppc4xx_dma_init(CPUPPCState *env, int dcr_base)
+{
+    ppc4xx_dma_t *dma;
+    int i;
+
+    dma = g_malloc0(sizeof(*dma));

Can't you QOM'ify the model ? A part from that it looks correct.

Thanks,

C.

+    dma->base = dcr_base;
+    qemu_register_reset(&ppc4xx_dma_reset, dma);
+    for (i = 0; i < 4; i++) {
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CR,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_CT,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAH,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SAL,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAH,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_DAL,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGH,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+        ppc_dcr_register(env, dcr_base + i * 8 + DMA0_SGL,
+                         dma, &dcr_read_dma, &dcr_write_dma);
+    }
+    ppc_dcr_register(env, dcr_base + DMA0_SR,
+                     dma, &dcr_read_dma, &dcr_write_dma);
+    ppc_dcr_register(env, dcr_base + DMA0_SGC,
+                     dma, &dcr_read_dma, &dcr_write_dma);
+    ppc_dcr_register(env, dcr_base + DMA0_SLP,
+                     dma, &dcr_read_dma, &dcr_write_dma);
+    ppc_dcr_register(env, dcr_base + DMA0_POL,
+                     dma, &dcr_read_dma, &dcr_write_dma);
+}
+
+/*****************************************************************************/
 /* PCI Express controller */
 /* FIXME: This is not complete and does not work, only implemented partially
  * to allow firmware and guests to find an empty bus. Cards should use PCI.
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index dc730cc..4f9248e 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -477,6 +477,9 @@ static void sam460ex_init(MachineState *machine)
     /* MAL */
     ppc4xx_mal_init(env, 4, 16, &uic[2][3]);

+    /* DMA */
+    ppc4xx_dma_init(env, 0x200);
+
     /* 256K of L2 cache as memory */
     ppc4xx_l2sram_init(env);
     /* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */





reply via email to

[Prev in Thread] Current Thread [Next in Thread]