qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] aspeed/smc: add support for DMAs
@ 2017-02-13 14:44 Cédric Le Goater
  2017-02-13 14:44 ` [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property Cédric Le Goater
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Cédric Le Goater @ 2017-02-13 14:44 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, qemu-devel,
	Cédric Le Goater

Hello,

Here is a short series adding support for DMAs to the SMC controller
of the Aspeed SoCs. It uses coroutines to spawn the DMA requests in
bottom halves. This method is inspired from the block layer, the code
looks nice but maybe, this is unnecessarily complex. Paolo, could you
provide some feedback ?

Thanks,

C. 

Cédric Le Goater (2):
  aspeed/smc: add a 'sdram_base' property
  aspeed/smc: add support for DMAs

 hw/arm/aspeed_soc.c         |   5 +-
 hw/ssi/aspeed_smc.c         | 233 ++++++++++++++++++++++++++++++++++++++++++--
 include/hw/ssi/aspeed_smc.h |   3 +
 3 files changed, 233 insertions(+), 8 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property
  2017-02-13 14:44 [Qemu-devel] [PATCH 0/2] aspeed/smc: add support for DMAs Cédric Le Goater
@ 2017-02-13 14:44 ` Cédric Le Goater
  2017-02-20 13:51   ` Peter Maydell
  2017-02-13 14:44 ` [Qemu-devel] [PATCH 2/2] aspeed/smc: add support for DMAs Cédric Le Goater
  2017-02-13 17:20 ` [Qemu-devel] [PATCH 0/2] " Cédric Le Goater
  2 siblings, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2017-02-13 14:44 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, qemu-devel,
	Cédric Le Goater

The setting of the DRAM address of the DMA transaction depends on the
DRAM base address of the SoC. Let's add a property to give this
information to the SMC controller model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 hw/arm/aspeed_soc.c         | 5 ++++-
 hw/ssi/aspeed_smc.c         | 1 +
 include/hw/ssi/aspeed_smc.h | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 571e4f097b02..6df76382f007 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -258,7 +258,10 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
                        qdev_get_gpio_in(DEVICE(&s->vic), 12));
 
     /* FMC, The number of CS is set at the board level */
-    object_property_set_bool(OBJECT(&s->fmc), true, "realized", &err);
+    object_property_set_int(OBJECT(&s->fmc), sc->info->sdram_base, "sdram-base",
+                            &err);
+    object_property_set_bool(OBJECT(&s->fmc), true, "realized", &local_err);
+    error_propagate(&err, local_err);
     if (err) {
         error_propagate(errp, err);
         return;
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index cb515730c5ad..f6ecdc014436 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -798,6 +798,7 @@ static const VMStateDescription vmstate_aspeed_smc = {
 };
 
 static Property aspeed_smc_properties[] = {
+    DEFINE_PROP_UINT64("sdram-base", AspeedSMCState, sdram_base, 0),
     DEFINE_PROP_UINT32("num-cs", AspeedSMCState, num_cs, 1),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 1f557313fa93..2c375af7bcbb 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -97,6 +97,9 @@ typedef struct AspeedSMCState {
     uint8_t r_timings;
     uint8_t conf_enable_w0;
 
+    /* for DMA support */
+    uint64_t sdram_base;
+
     AspeedSMCFlash *flashes;
 } AspeedSMCState;
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 2/2] aspeed/smc: add support for DMAs
  2017-02-13 14:44 [Qemu-devel] [PATCH 0/2] aspeed/smc: add support for DMAs Cédric Le Goater
  2017-02-13 14:44 ` [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property Cédric Le Goater
@ 2017-02-13 14:44 ` Cédric Le Goater
  2017-02-13 17:20 ` [Qemu-devel] [PATCH 0/2] " Cédric Le Goater
  2 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2017-02-13 14:44 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, qemu-devel,
	Cédric Le Goater

Some of SMC controllers on the Aspeed SoCs support DMA to access the
flash modules. It can operate in a normal mode, to copy to or from the
flash module mapping window, or in a checksum calculation mode, to
evaluate the best clock settings for reads.

When DMA is enabled, a DMA request is built and passed on to a bottom
half to handle the memory transaction. The CPU is notified of the
completion with an IRQ if it was enabled.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 225 insertions(+), 7 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index f6ecdc014436..d73ab1537559 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -26,8 +26,10 @@
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "qemu/log.h"
+#include "qemu/coroutine.h"
 #include "include/qemu/error-report.h"
 #include "exec/address-spaces.h"
+#include "sysemu/dma.h"
 
 #include "hw/ssi/aspeed_smc.h"
 
@@ -106,10 +108,10 @@
 #define   DMA_CTRL_DELAY_SHIFT  8
 #define   DMA_CTRL_FREQ_MASK    0xf
 #define   DMA_CTRL_FREQ_SHIFT   4
-#define   DMA_CTRL_MODE         (1 << 3)
+#define   DMA_CTRL_CALIB        (1 << 3)
 #define   DMA_CTRL_CKSUM        (1 << 2)
-#define   DMA_CTRL_DIR          (1 << 1)
-#define   DMA_CTRL_EN           (1 << 0)
+#define   DMA_CTRL_WRITE        (1 << 1)
+#define   DMA_CTRL_ENABLE       (1 << 0)
 
 /* DMA Flash Side Address */
 #define R_DMA_FLASH_ADDR  (0x84 / 4)
@@ -141,6 +143,14 @@
 #define ASPEED_SOC_SPI_FLASH_BASE   0x30000000
 #define ASPEED_SOC_SPI2_FLASH_BASE  0x38000000
 
+/*
+ * DMA address and size encoding
+ */
+#define DMA_LENGTH(x)           (((x) & ~0xFE000003))
+#define DMA_DRAM_ADDR(base, x)  (((x) & ~0xE0000003) | base)
+#define DMA_FLASH_ADDR(x)       (((x) & ~0xE0000003) | \
+                                 ASPEED_SOC_FMC_FLASH_BASE)
+
 /* Flash opcodes. */
 #define SPI_OP_READ       0x03    /* Read data bytes (low frequency) */
 
@@ -618,9 +628,6 @@ static void aspeed_smc_reset(DeviceState *d)
 
     memset(s->regs, 0, sizeof s->regs);
 
-    /* Pretend DMA is done (u-boot initialization) */
-    s->regs[R_INTR_CTRL] = INTR_CTRL_DMA_STATUS;
-
     /* Unselect all slaves */
     for (i = 0; i < s->num_cs; ++i) {
         s->regs[s->r_ctrl0 + i] |= CTRL_CE_STOP_ACTIVE;
@@ -663,6 +670,11 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
         addr == s->r_timings ||
         addr == s->r_ce_ctrl ||
         addr == R_INTR_CTRL ||
+        (s->ctrl->has_dma && addr == R_DMA_CTRL) ||
+        (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) ||
+        (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) ||
+        (s->ctrl->has_dma && addr == R_DMA_LEN) ||
+        (s->ctrl->has_dma && addr == R_DMA_CHECKSUM) ||
         (addr >= R_SEG_ADDR0 && addr < R_SEG_ADDR0 + s->ctrl->max_slaves) ||
         (addr >= s->r_ctrl0 && addr < s->r_ctrl0 + s->num_cs)) {
         return s->regs[addr];
@@ -673,6 +685,200 @@ static uint64_t aspeed_smc_read(void *opaque, hwaddr addr, unsigned int size)
     }
 }
 
+typedef struct AspeedDmaCo {
+    AspeedSMCState *s;
+    int len;
+    uint32_t flash_addr;
+    uint32_t dram_addr;
+    uint32_t checksum;
+    bool direction;
+} AspeedDmaCo;
+
+static void coroutine_fn aspeed_smc_dma_done(AspeedDmaCo *dmaco)
+{
+    AspeedSMCState *s = dmaco->s;
+
+    s->regs[R_INTR_CTRL] |= INTR_CTRL_DMA_STATUS;
+    if (s->regs[R_INTR_CTRL] & INTR_CTRL_DMA_EN) {
+        qemu_irq_raise(s->irq);
+    }
+}
+
+static bool coroutine_fn aspeed_smc_dma_update(AspeedDmaCo *dmaco)
+{
+    AspeedSMCState *s = dmaco->s;
+    bool ret;
+
+    if (s->regs[R_DMA_CTRL] & DMA_CTRL_ENABLE) {
+        s->regs[R_DMA_FLASH_ADDR] = dmaco->flash_addr;
+        s->regs[R_DMA_DRAM_ADDR] = dmaco->dram_addr;
+        s->regs[R_DMA_LEN] = dmaco->len - 4;
+        s->regs[R_DMA_CHECKSUM] = dmaco->checksum;
+        ret = true;
+    } else {
+        ret = false;
+    }
+
+    return ret;
+}
+
+/*
+ * Accumulate the result of the reads in a register. It will be used
+ * later to do timing calibration.
+ */
+static void coroutine_fn aspeed_smc_dma_checksum(void* opaque)
+{
+    AspeedDmaCo *dmaco = opaque;
+    uint32_t data;
+
+    while (dmaco->len) {
+        /* check for disablement and update register values */
+        if (!aspeed_smc_dma_update(dmaco)) {
+            goto out;
+        }
+
+        cpu_physical_memory_read(dmaco->flash_addr, &data, 4);
+        dmaco->checksum += data;
+        dmaco->flash_addr += 4;
+        dmaco->len -= 4;
+    }
+
+    aspeed_smc_dma_done(dmaco);
+out:
+    g_free(dmaco);
+}
+
+static void coroutine_fn aspeed_smc_dma_rw(void* opaque)
+{
+    AspeedDmaCo *dmaco = opaque;
+    uint32_t data;
+
+    while (dmaco->len) {
+        /* check for disablement and update register values */
+        if (!aspeed_smc_dma_update(dmaco)) {
+            goto out;
+        }
+
+        if (dmaco->direction) {
+            dma_memory_read(&address_space_memory, dmaco->dram_addr, &data, 4);
+            cpu_physical_memory_write(dmaco->flash_addr, &data, 4);
+        } else {
+            cpu_physical_memory_read(dmaco->flash_addr, &data, 4);
+            dma_memory_write(&address_space_memory, dmaco->dram_addr,
+                             &data, 4);
+        }
+
+        dmaco->flash_addr += 4;
+        dmaco->dram_addr += 4;
+        dmaco->len -= 4;
+    }
+
+    aspeed_smc_dma_done(dmaco);
+out:
+    g_free(dmaco);
+}
+
+
+static void aspeed_smc_dma_stop(AspeedSMCState *s)
+{
+    /*
+     * When the DMA is disabled, INTR_CTRL_DMA_STATUS=0 means the
+     * engine is idle
+     */
+    s->regs[R_INTR_CTRL] &= ~INTR_CTRL_DMA_STATUS;
+    s->regs[R_DMA_CHECKSUM] = 0x0;
+    s->regs[R_DMA_FLASH_ADDR] = 0;
+    s->regs[R_DMA_DRAM_ADDR] = 0;
+    s->regs[R_DMA_LEN] = 0;
+
+    /*
+     * Lower DMA irq even in any case. The IRQ control register could
+     * have been cleared before disabling the DMA.
+     */
+    qemu_irq_lower(s->irq);
+}
+
+typedef struct AspeedDmaRequest {
+    Coroutine *co;
+    QEMUBH *bh;
+} AspeedDmaRequest;
+
+static void aspeed_smc_dma_run(void *opaque)
+{
+    AspeedDmaRequest *dmareq = opaque;
+
+    qemu_coroutine_enter(dmareq->co);
+    qemu_bh_delete(dmareq->bh);
+    g_free(dmareq);
+}
+
+static void aspeed_smc_dma_schedule(Coroutine *co)
+{
+    AspeedDmaRequest *dmareq;
+
+    dmareq = g_new0(AspeedDmaRequest, 1);
+
+    dmareq->co = co;
+    dmareq->bh = qemu_bh_new(aspeed_smc_dma_run, dmareq);
+    qemu_bh_schedule(dmareq->bh);
+}
+
+static void aspeed_smc_dma_start(void *opaque)
+{
+    AspeedSMCState *s = opaque;
+    AspeedDmaCo *dmaco;
+    Coroutine *co;
+
+    /* freed in the coroutine */
+    dmaco = g_new0(AspeedDmaCo, 1);
+
+    /* A DMA transaction has a minimum of 4 bytes */
+    dmaco->len        = s->regs[R_DMA_LEN] + 4;
+    dmaco->flash_addr = s->regs[R_DMA_FLASH_ADDR];
+    dmaco->dram_addr  = s->regs[R_DMA_DRAM_ADDR];
+    dmaco->direction  = (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE);
+    dmaco->s          = s;
+
+    if (s->regs[R_DMA_CTRL] & DMA_CTRL_CKSUM) {
+        co = qemu_coroutine_create(aspeed_smc_dma_checksum, dmaco);
+    } else {
+        co = qemu_coroutine_create(aspeed_smc_dma_rw, dmaco);
+    }
+
+    aspeed_smc_dma_schedule(co);
+}
+
+/*
+ * This is to run one DMA at a time. When INTR_CTRL_DMA_STATUS becomes
+ * 1, the DMA has completed and a new DMA can start even if the result
+ * of the previous was not collected.
+ */
+static bool aspeed_smc_dma_in_progress(AspeedSMCState *s)
+{
+    bool ret = (s->regs[R_DMA_CTRL] & DMA_CTRL_ENABLE) &&
+        !(s->regs[R_INTR_CTRL] & INTR_CTRL_DMA_STATUS);
+    return ret;
+}
+
+static void aspeed_smc_dma_ctrl(AspeedSMCState *s, uint64_t dma_ctrl)
+{
+    if (dma_ctrl & DMA_CTRL_ENABLE) {
+        if (aspeed_smc_dma_in_progress(s)) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA in progress\n",
+                          __func__);
+            return;
+        }
+
+        s->regs[R_DMA_CTRL] = dma_ctrl;
+
+        aspeed_smc_dma_start(s);
+    } else {
+        s->regs[R_DMA_CTRL] = dma_ctrl;
+
+        aspeed_smc_dma_stop(s);
+    }
+}
+
 static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
                              unsigned int size)
 {
@@ -696,6 +902,16 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
         if (value != s->regs[R_SEG_ADDR0 + cs]) {
             aspeed_smc_flash_set_segment(s, cs, value);
         }
+    } else if (addr == R_INTR_CTRL) {
+        s->regs[addr] = value;
+    } else if (s->ctrl->has_dma && addr == R_DMA_CTRL) {
+        aspeed_smc_dma_ctrl(s, value);
+    } else if (s->ctrl->has_dma && addr == R_DMA_DRAM_ADDR) {
+        s->regs[addr] = DMA_DRAM_ADDR(s->sdram_base, value);
+    } else if (s->ctrl->has_dma && addr == R_DMA_FLASH_ADDR) {
+        s->regs[addr] = DMA_FLASH_ADDR(value);
+    } else if (s->ctrl->has_dma && addr == R_DMA_LEN) {
+        s->regs[addr] = DMA_LENGTH(value);
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
@@ -728,6 +944,9 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     s->r_timings = s->ctrl->r_timings;
     s->conf_enable_w0 = s->ctrl->conf_enable_w0;
 
+    /* DMA irq */
+    sysbus_init_irq(sbd, &s->irq);
+
     /* Enforce some real HW limits */
     if (s->num_cs > s->ctrl->max_slaves) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: num_cs cannot exceed: %d\n",
@@ -738,7 +957,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
     s->spi = ssi_create_bus(dev, "spi");
 
     /* Setup cs_lines for slaves */
-    sysbus_init_irq(sbd, &s->irq);
     s->cs_lines = g_new0(qemu_irq, s->num_cs);
     ssi_auto_connect_slaves(dev, s->cs_lines, s->spi);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] aspeed/smc: add support for DMAs
  2017-02-13 14:44 [Qemu-devel] [PATCH 0/2] aspeed/smc: add support for DMAs Cédric Le Goater
  2017-02-13 14:44 ` [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property Cédric Le Goater
  2017-02-13 14:44 ` [Qemu-devel] [PATCH 2/2] aspeed/smc: add support for DMAs Cédric Le Goater
@ 2017-02-13 17:20 ` Cédric Le Goater
  2 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2017-02-13 17:20 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, qemu-devel

On 02/13/2017 03:44 PM, Cédric Le Goater wrote:
> Hello,
> 
> Here is a short series adding support for DMAs to the SMC controller
> of the Aspeed SoCs. It uses coroutines to spawn the DMA requests in
> bottom halves. This method is inspired from the block layer, the code
> looks nice but maybe, this is unnecessarily complex. Paolo, could you
> provide some feedback ?

I forgot the magic word here. Sorry about that. When ever you have some
time, please !

Thanks,

C. 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property
  2017-02-13 14:44 ` [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property Cédric Le Goater
@ 2017-02-20 13:51   ` Peter Maydell
  2017-02-20 14:30     ` Cédric Le Goater
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2017-02-20 13:51 UTC (permalink / raw)
  To: Cédric Le Goater
  Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, QEMU Developers

On 13 February 2017 at 14:44, Cédric Le Goater <clg@kaod.org> wrote:
> The setting of the DRAM address of the DMA transaction depends on the
> DRAM base address of the SoC. Let's add a property to give this
> information to the SMC controller model.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> --

This seems a bit weird -- what does it actually do in hardware?
(Does it really have the ability to dma anywhere in the
address space and just use an odd base address, or is it
DMA'ing to a more restricted address space?)

thanks
-- PMM

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property
  2017-02-20 13:51   ` Peter Maydell
@ 2017-02-20 14:30     ` Cédric Le Goater
  2017-02-20 14:38       ` Peter Maydell
  0 siblings, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2017-02-20 14:30 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, QEMU Developers

On 02/20/2017 02:51 PM, Peter Maydell wrote:
> On 13 February 2017 at 14:44, Cédric Le Goater <clg@kaod.org> wrote:
>> The setting of the DRAM address of the DMA transaction depends on the
>> DRAM base address of the SoC. Let's add a property to give this
>> information to the SMC controller model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>> --
> 
> This seems a bit weird -- what does it actually do in hardware?
> (Does it really have the ability to dma anywhere in the
> address space and just use an odd base address, or is it
> DMA'ing to a more restricted address space?)

For the AST2500, the valid address range for the DRAM side start
address of the DMA is [ 0x80000000 - 0xBFFFFFFF ] which is the 
full 1024M SDRAM space. It will wrap back in case of overflow.

For the AST2400, it is [ 0x40000000 - 0x5FFFFFFF ] which is the 
full 512M SDRAM space.

And the length of the DMA goes from 4 bytes to 32MB [ O - 0x7FFFFF ]

On real HW, the DRAM address of the DMA is taken as an offset from 
the beginning of the SDRAM address space. It does the same for 
the flash address btw. This is why we have the macros in the model : 

#define DMA_DRAM_ADDR(base, x)  (((x) & ~0xE0000003) | base)
#define DMA_FLASH_ADDR(x)       (((x) & ~0xE0000003) | \
                                 ASPEED_SOC_FMC_FLASH_BASE)


Thanks,

C.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property
  2017-02-20 14:30     ` Cédric Le Goater
@ 2017-02-20 14:38       ` Peter Maydell
  2017-02-20 16:33         ` Cédric Le Goater
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2017-02-20 14:38 UTC (permalink / raw)
  To: Cédric Le Goater
  Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, QEMU Developers

On 20 February 2017 at 14:30, Cédric Le Goater <clg@kaod.org> wrote:
> On 02/20/2017 02:51 PM, Peter Maydell wrote:
>> On 13 February 2017 at 14:44, Cédric Le Goater <clg@kaod.org> wrote:
>>> The setting of the DRAM address of the DMA transaction depends on the
>>> DRAM base address of the SoC. Let's add a property to give this
>>> information to the SMC controller model.
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>> --
>>
>> This seems a bit weird -- what does it actually do in hardware?
>> (Does it really have the ability to dma anywhere in the
>> address space and just use an odd base address, or is it
>> DMA'ing to a more restricted address space?)
>
> For the AST2500, the valid address range for the DRAM side start
> address of the DMA is [ 0x80000000 - 0xBFFFFFFF ] which is the
> full 1024M SDRAM space. It will wrap back in case of overflow.
>
> For the AST2400, it is [ 0x40000000 - 0x5FFFFFFF ] which is the
> full 512M SDRAM space.
>
> And the length of the DMA goes from 4 bytes to 32MB [ O - 0x7FFFFF ]

So if I do a DMA access to 0x800000 that's the same as an
access at 0x0, or does it fail?

I ask because the other option for modelling this kind of thing
would be that you make the DMA controller device take a link
property which is a MemoryRegion*. Then inside the DMA device
you create the AddressSpace* which you use to do DMA accesses,
and in the board code you pass the DMA device a suitably
constructed MemoryRegion that contains the things that the
device can DMA to.

You don't have to do it that way (and I don't think we
have much in the way of examples of doing it like that,
except for the ARM CPU handling of secure and nonsecure
memory address spaces), but if it seems to fit better
with what the hardware's doing it might be worth the effort.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property
  2017-02-20 14:38       ` Peter Maydell
@ 2017-02-20 16:33         ` Cédric Le Goater
  0 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2017-02-20 16:33 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Crosthwaite, Paolo Bonzini, qemu-arm, QEMU Developers

On 02/20/2017 03:38 PM, Peter Maydell wrote:
> On 20 February 2017 at 14:30, Cédric Le Goater <clg@kaod.org> wrote:
>> On 02/20/2017 02:51 PM, Peter Maydell wrote:
>>> On 13 February 2017 at 14:44, Cédric Le Goater <clg@kaod.org> wrote:
>>>> The setting of the DRAM address of the DMA transaction depends on the
>>>> DRAM base address of the SoC. Let's add a property to give this
>>>> information to the SMC controller model.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>>> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
>>>> --
>>>
>>> This seems a bit weird -- what does it actually do in hardware?
>>> (Does it really have the ability to dma anywhere in the
>>> address space and just use an odd base address, or is it
>>> DMA'ing to a more restricted address space?)
>>
>> For the AST2500, the valid address range for the DRAM side start
>> address of the DMA is [ 0x80000000 - 0xBFFFFFFF ] which is the
>> full 1024M SDRAM space. It will wrap back in case of overflow.
>>
>> For the AST2400, it is [ 0x40000000 - 0x5FFFFFFF ] which is the
>> full 512M SDRAM space.
>>
>> And the length of the DMA goes from 4 bytes to 32MB [ O - 0x7FFFFF ]

In fact, the above range is from the spec and it is wrong. I suppose
it is a left over from previous boards. Here is the current one :

	 [ 0 - 0x1fffffc ]
 
> So if I do a DMA access to 0x800000 that's the same as an
> access at 0x0, or does it fail?

The length value wraps back just like the addresses.

> I ask because the other option for modelling this kind of thing
> would be that you make the DMA controller device take a link
> property which is a MemoryRegion*. Then inside the DMA device
> you create the AddressSpace* which you use to do DMA accesses,
> and in the board code you pass the DMA device a suitably
> constructed MemoryRegion that contains the things that the
> device can DMA to.

ok. I think I understand. I have done something similar for 
the SCOM sideband bus of the PowerNV machine. It looks a like 
a good idea. The 'minus 4' on the DMA length might be a little 
annoying but I will give it a try.

Thanks,

C.  

> You don't have to do it that way (and I don't think we
> have much in the way of examples of doing it like that,
> except for the ARM CPU handling of secure and nonsecure
> memory address spaces), but if it seems to fit better
> with what the hardware's doing it might be worth the effort.
> 
> thanks
> -- PMM
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-02-20 16:33 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-13 14:44 [Qemu-devel] [PATCH 0/2] aspeed/smc: add support for DMAs Cédric Le Goater
2017-02-13 14:44 ` [Qemu-devel] [PATCH 1/2] aspeed/smc: add a 'sdram_base' property Cédric Le Goater
2017-02-20 13:51   ` Peter Maydell
2017-02-20 14:30     ` Cédric Le Goater
2017-02-20 14:38       ` Peter Maydell
2017-02-20 16:33         ` Cédric Le Goater
2017-02-13 14:44 ` [Qemu-devel] [PATCH 2/2] aspeed/smc: add support for DMAs Cédric Le Goater
2017-02-13 17:20 ` [Qemu-devel] [PATCH 0/2] " Cédric Le Goater

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).