All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup
@ 2013-06-30  1:26 Alexander Graf
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 01/15] PPC: Mac: Fix guest exported tbfreq values Alexander Graf
                   ` (13 more replies)
  0 siblings, 14 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:26 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

Recently there has been a lot of progress on the OpenBIOS side to get Mac OS X
to boot.

For a while now it seemed there was only very little to make it a fully working
guest os in QEMU.

This patch set is the result of this. With this I can successfully boot Mac OS X
10.2 to 10.4 with the g3beige machine all the way to the GUI. I was not able to
boot 10.0 or 10.1, both of which crashed in interrupt controller registration.
10.5 does not include drivers for g3beige anymore. Everything as of 10.6 is x86
only.

The mac99 target doesn't look quite as good, but also very close. FWIW only
minor issues in our NVRAM layout and via-cuda emulation keep us from using that
one.

Please don't try to run this with KVM yet. Mac OS X uses mixed mode (half real,
half paged) extensively, which happens to break badly in KVM.

For reference, here are a few pictures:

  https://dl.dropboxusercontent.com/u/8976842/Screen%20Shot%202013-06-29%20at%2021.25.38.png
  https://dl.dropboxusercontent.com/u/8976842/Screen%20Shot%202013-06-29%20at%2005.21.03.png

If you want to try this out, please apply the patches on top of my ppc-next
queue. Or just use this git repo:

  git://github.com/agraf/qemu.git macos-v1


Enjoy!

Alex

Alexander Graf (15):
  PPC: Mac: Fix guest exported tbfreq values
  PPC: g3beige:  Move secondary IDE bus to mac-io
  PPC: Macio: Replace tabs with spaces
  PPC: dbdma: Replace tabs with spaces
  PPC: Mac: Add debug prints in macio and dbdma code
  PPC: dbdma: Fix debug print
  PPC: dbdma: Allow new commands in RUN state
  PPC: dbdma: Move defines into header file
  PPC: dbdma: Introduce kick function
  PPC: dbdma: Move static bh variable to device struct
  PPC: dbdma: macio: Add DMA callback
  PPC: dbdma: Move processing to io
  PPC: dbdma: Wait for DMA until we have data
  PPC: dbdma: Support unaligned DMA access
  PPC: Update PPC OpenBIOS

 hw/ide/macio.c             | 232 ++++++++++++++++++++++++++++++++++++++++++---
 hw/misc/macio/mac_dbdma.c  | 191 ++++++++++---------------------------
 hw/misc/macio/macio.c      |  95 +++++++++++--------
 hw/ppc/mac.h               |   3 +
 hw/ppc/mac_newworld.c      |   5 +-
 hw/ppc/mac_oldworld.c      |  22 ++---
 include/hw/ppc/mac_dbdma.h | 124 ++++++++++++++++++++++++
 pc-bios/openbios-ppc       | Bin 733972 -> 1357906 bytes
 8 files changed, 465 insertions(+), 207 deletions(-)

-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 01/15] PPC: Mac: Fix guest exported tbfreq values
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
@ 2013-06-30  1:26 ` Alexander Graf
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io Alexander Graf
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:26 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

We can tell the guest the frequency of its time base through fwcfg.

However, we tell it a different value from the speed tb actually runs
at. Let's fix it and make the tbfreq initialization and the fwcfg exposure
use the same values.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ppc/mac_newworld.c | 5 +++--
 hw/ppc/mac_oldworld.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 3badfa3..253089a 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -71,6 +71,7 @@
 
 #define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
+#define TBFREQ (100UL * 1000UL * 1000UL)
 
 /* debug UniNorth */
 //#define DEBUG_UNIN
@@ -191,7 +192,7 @@ static void ppc_core99_init(QEMUMachineInitArgs *args)
         env = &cpu->env;
 
         /* Set time-base frequency to 100 Mhz */
-        cpu_ppc_tb_init(env, 100UL * 1000UL * 1000UL);
+        cpu_ppc_tb_init(env, TBFREQ);
         qemu_register_reset(ppc_core99_reset, cpu);
     }
 
@@ -460,7 +461,7 @@ static void ppc_core99_init(QEMUMachineInitArgs *args)
         fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_KVM_PID, getpid());
 #endif
     } else {
-        fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_TBFREQ, get_ticks_per_sec());
+        fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_TBFREQ, TBFREQ);
     }
     /* Mac OS X requires a "known good" clock-frequency value; pass it one. */
     fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_CLOCKFREQ, 266000000);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 8faff30..2aab54c 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -45,6 +45,7 @@
 
 #define MAX_IDE_BUS 2
 #define CFG_ADDR 0xf0000510
+#define TBFREQ 16600000UL
 
 static int fw_cfg_boot_set(void *opaque, const char *boot_device)
 {
@@ -114,7 +115,7 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
         env = &cpu->env;
 
         /* Set time-base frequency to 16.6 Mhz */
-        cpu_ppc_tb_init(env,  16600000UL);
+        cpu_ppc_tb_init(env,  TBFREQ);
         qemu_register_reset(ppc_heathrow_reset, cpu);
     }
 
@@ -331,7 +332,7 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
         fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_KVM_PID, getpid());
 #endif
     } else {
-        fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_TBFREQ, get_ticks_per_sec());
+        fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_TBFREQ, TBFREQ);
     }
     /* Mac OS X requires a "known good" clock-frequency value; pass it one. */
     fw_cfg_add_i32(fw_cfg, FW_CFG_PPC_CLOCKFREQ, 266000000);
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 01/15] PPC: Mac: Fix guest exported tbfreq values Alexander Graf
@ 2013-06-30  1:26 ` Alexander Graf
  2013-06-30  6:33   ` Andreas Färber
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 03/15] PPC: Macio: Replace tabs with spaces Alexander Graf
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:26 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

On a real G3 Beige the secondary IDE bus lives on the mac-io chip, not
on some random PCI device. Move it there to become more compatible.

While at it, also clean up the IDE channel connection logic.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - fix IRQ mapping
---
 hw/ide/macio.c        |  2 +-
 hw/misc/macio/macio.c | 95 +++++++++++++++++++++++++++++----------------------
 hw/ppc/mac_oldworld.c | 17 +++++----
 3 files changed, 64 insertions(+), 50 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index a1952b0..7a1c573 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -363,7 +363,7 @@ static void macio_ide_register_types(void)
     type_register_static(&macio_ide_type_info);
 }
 
-/* hd_table must contain 4 block drivers */
+/* hd_table must contain 2 block drivers */
 void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
 {
     int i;
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index fd4c8e5..d9971e2 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -51,11 +51,9 @@ typedef struct OldWorldMacIOState {
     /*< private >*/
     MacIOState parent_obj;
     /*< public >*/
-
-    qemu_irq irqs[3];
-
+    qemu_irq irqs[5];
     MacIONVRAMState nvram;
-    MACIOIDEState ide;
+    MACIOIDEState ide[2];
 } OldWorldMacIOState;
 
 #define NEWWORLD_MACIO(obj) \
@@ -147,18 +145,32 @@ static int macio_common_initfn(PCIDevice *d)
     return 0;
 }
 
+static int macio_initfn_ide(MacIOState *s, MACIOIDEState *ide, qemu_irq irq0,
+                            qemu_irq irq1, int dmaid)
+{
+    SysBusDevice *sysbus_dev;
+
+    sysbus_dev = SYS_BUS_DEVICE(ide);
+    sysbus_connect_irq(sysbus_dev, 0, irq0);
+    sysbus_connect_irq(sysbus_dev, 1, irq1);
+    macio_ide_register_dma(ide, s->dbdma, dmaid);
+    return qdev_init(DEVICE(ide));
+}
+
 static int macio_oldworld_initfn(PCIDevice *d)
 {
     MacIOState *s = MACIO(d);
     OldWorldMacIOState *os = OLDWORLD_MACIO(d);
     SysBusDevice *sysbus_dev;
+    int i;
+    int cur_irq = 0;
     int ret = macio_common_initfn(d);
     if (ret < 0) {
         return ret;
     }
 
     sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
-    sysbus_connect_irq(sysbus_dev, 0, os->irqs[0]);
+    sysbus_connect_irq(sysbus_dev, 0, os->irqs[cur_irq++]);
 
     ret = qdev_init(DEVICE(&os->nvram));
     if (ret < 0) {
@@ -174,23 +186,39 @@ static int macio_oldworld_initfn(PCIDevice *d)
         memory_region_add_subregion(&s->bar, 0x00000, s->pic_mem);
     }
 
-    sysbus_dev = SYS_BUS_DEVICE(&os->ide);
-    sysbus_connect_irq(sysbus_dev, 0, os->irqs[1]);
-    sysbus_connect_irq(sysbus_dev, 1, os->irqs[2]);
-    macio_ide_register_dma(&os->ide, s->dbdma, 0x16);
-    ret = qdev_init(DEVICE(&os->ide));
-    if (ret < 0) {
-        return ret;
+    /* IDE buses */
+    for (i = 0; i < ARRAY_SIZE(os->ide); i++) {
+        qemu_irq irq0 = os->irqs[cur_irq++];
+        qemu_irq irq1 = os->irqs[cur_irq++];
+
+        ret = macio_initfn_ide(s, &os->ide[i], irq0, irq1, 0x16 + (i * 4));
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     return 0;
 }
 
+static void macio_init_ide(MacIOState *s, MACIOIDEState *ide, int index)
+{
+    gchar *name;
+
+    object_initialize(ide, TYPE_MACIO_IDE);
+    qdev_set_parent_bus(DEVICE(ide), sysbus_get_default());
+    memory_region_add_subregion(&s->bar, 0x1f000 + ((index + 1) * 0x1000),
+                                &ide->mem);
+    name = g_strdup_printf("ide[%i]", index);
+    object_property_add_child(OBJECT(s), name, OBJECT(ide), NULL);
+    g_free(name);
+}
+
 static void macio_oldworld_init(Object *obj)
 {
     MacIOState *s = MACIO(obj);
     OldWorldMacIOState *os = OLDWORLD_MACIO(obj);
     DeviceState *dev;
+    int i;
 
     qdev_init_gpio_out(DEVICE(obj), os->irqs, ARRAY_SIZE(os->irqs));
 
@@ -199,10 +227,9 @@ static void macio_oldworld_init(Object *obj)
     qdev_prop_set_uint32(dev, "size", 0x2000);
     qdev_prop_set_uint32(dev, "it_shift", 4);
 
-    object_initialize(&os->ide, TYPE_MACIO_IDE);
-    qdev_set_parent_bus(DEVICE(&os->ide), sysbus_get_default());
-    memory_region_add_subregion(&s->bar, 0x1f000 + (1 * 0x1000), &os->ide.mem);
-    object_property_add_child(obj, "ide", OBJECT(&os->ide), NULL);
+    for (i = 0; i < 2; i++) {
+        macio_init_ide(s, &os->ide[i], i);
+    }
 }
 
 static int macio_newworld_initfn(PCIDevice *d)
@@ -210,35 +237,30 @@ static int macio_newworld_initfn(PCIDevice *d)
     MacIOState *s = MACIO(d);
     NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
     SysBusDevice *sysbus_dev;
+    int i;
+    int cur_irq = 0;
     int ret = macio_common_initfn(d);
     if (ret < 0) {
         return ret;
     }
 
     sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
-    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[0]);
+    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[cur_irq++]);
 
     if (s->pic_mem) {
         /* OpenPIC */
         memory_region_add_subregion(&s->bar, 0x40000, s->pic_mem);
     }
 
-    sysbus_dev = SYS_BUS_DEVICE(&ns->ide[0]);
-    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[1]);
-    sysbus_connect_irq(sysbus_dev, 1, ns->irqs[2]);
-    macio_ide_register_dma(&ns->ide[0], s->dbdma, 0x16);
-    ret = qdev_init(DEVICE(&ns->ide[0]));
-    if (ret < 0) {
-        return ret;
-    }
+    /* IDE buses */
+    for (i = 0; i < ARRAY_SIZE(ns->ide); i++) {
+        qemu_irq irq0 = ns->irqs[cur_irq++];
+        qemu_irq irq1 = ns->irqs[cur_irq++];
 
-    sysbus_dev = SYS_BUS_DEVICE(&ns->ide[1]);
-    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[3]);
-    sysbus_connect_irq(sysbus_dev, 1, ns->irqs[4]);
-    macio_ide_register_dma(&ns->ide[1], s->dbdma, 0x1a);
-    ret = qdev_init(DEVICE(&ns->ide[1]));
-    if (ret < 0) {
-        return ret;
+        ret = macio_initfn_ide(s, &ns->ide[i], irq0, irq1, 0x16 + (i * 4));
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     return 0;
@@ -249,18 +271,11 @@ static void macio_newworld_init(Object *obj)
     MacIOState *s = MACIO(obj);
     NewWorldMacIOState *ns = NEWWORLD_MACIO(obj);
     int i;
-    gchar *name;
 
     qdev_init_gpio_out(DEVICE(obj), ns->irqs, ARRAY_SIZE(ns->irqs));
 
     for (i = 0; i < 2; i++) {
-        object_initialize(&ns->ide[i], TYPE_MACIO_IDE);
-        qdev_set_parent_bus(DEVICE(&ns->ide[i]), sysbus_get_default());
-        memory_region_add_subregion(&s->bar, 0x1f000 + ((i + 1) * 0x1000),
-                                    &ns->ide[i].mem);
-        name = g_strdup_printf("ide[%i]", i);
-        object_property_add_child(obj, name, OBJECT(&ns->ide[i]), NULL);
-        g_free(name);
+        macio_init_ide(s, &ns->ide[i], i);
     }
 }
 
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 2aab54c..7422eb9 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -268,20 +268,19 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
     macio = pci_create(pci_bus, -1, TYPE_OLDWORLD_MACIO);
     dev = DEVICE(macio);
     qdev_connect_gpio_out(dev, 0, pic[0x12]); /* CUDA */
-    qdev_connect_gpio_out(dev, 1, pic[0x0D]); /* IDE */
-    qdev_connect_gpio_out(dev, 2, pic[0x02]); /* IDE DMA */
+    qdev_connect_gpio_out(dev, 1, pic[0x0D]); /* IDE-0 */
+    qdev_connect_gpio_out(dev, 2, pic[0x02]); /* IDE-0 DMA */
+    qdev_connect_gpio_out(dev, 3, pic[0x0E]); /* IDE-1 */
+    qdev_connect_gpio_out(dev, 4, pic[0x03]); /* IDE-1 DMA */
     macio_init(macio, pic_mem, escc_bar);
 
-    /* First IDE channel is a MAC IDE on the MacIO bus */
     macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
-                                                        "ide"));
+                                                        "ide[0]"));
     macio_ide_init_drives(macio_ide, hd);
 
-    /* Second IDE channel is a CMD646 on the PCI bus */
-    hd[0] = hd[MAX_IDE_DEVS];
-    hd[1] = hd[MAX_IDE_DEVS + 1];
-    hd[3] = hd[2] = NULL;
-    pci_cmd646_ide_init(pci_bus, hd, 0);
+    macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
+                                                        "ide[1]"));
+    macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
 
     dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
     adb_bus = qdev_get_child_bus(dev, "adb.0");
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 03/15] PPC: Macio: Replace tabs with spaces
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 01/15] PPC: Mac: Fix guest exported tbfreq values Alexander Graf
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io Alexander Graf
@ 2013-06-30  1:26 ` Alexander Graf
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 04/15] PPC: dbdma: " Alexander Graf
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:26 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

s/^I/        /g on the file.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/macio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 7a1c573..82409dc 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -55,7 +55,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
         s->packet_transfer_size -= s->io_buffer_size;
 
         s->io_buffer_index += s->io_buffer_size;
-	s->lba += s->io_buffer_index >> 11;
+        s->lba += s->io_buffer_index >> 11;
         s->io_buffer_index &= 0x7ff;
     }
 
@@ -97,7 +97,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     if (ret < 0) {
         m->aiocb = NULL;
         qemu_sglist_destroy(&s->sg);
-	ide_dma_error(s);
+        ide_dma_error(s);
         goto done;
     }
 
@@ -136,11 +136,11 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
         m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
-		                 pmac_ide_transfer_cb, io);
+                                 pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_WRITE:
         m->aiocb = dma_bdrv_write(s->bs, &s->sg, sector_num,
-		                  pmac_ide_transfer_cb, io);
+                                  pmac_ide_transfer_cb, io);
         break;
     case IDE_DMA_TRIM:
         m->aiocb = dma_bdrv_io(s->bs, &s->sg, sector_num,
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 04/15] PPC: dbdma: Replace tabs with spaces
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (2 preceding siblings ...)
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 03/15] PPC: Macio: Replace tabs with spaces Alexander Graf
@ 2013-06-30  1:26 ` Alexander Graf
  2013-06-30  6:35   ` Andreas Färber
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code Alexander Graf
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:26 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

s/^I/        /g on the file with a few manual tweaks to align things.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c | 102 +++++++++++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 2fc7f87..ab174f5 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -85,75 +85,75 @@
 
 /* Bits in control and status registers */
 
-#define RUN	0x8000
-#define PAUSE	0x4000
-#define FLUSH	0x2000
-#define WAKE	0x1000
-#define DEAD	0x0800
-#define ACTIVE	0x0400
-#define BT	0x0100
-#define DEVSTAT	0x00ff
+#define RUN        0x8000
+#define PAUSE      0x4000
+#define FLUSH      0x2000
+#define WAKE       0x1000
+#define DEAD       0x0800
+#define ACTIVE     0x0400
+#define BT         0x0100
+#define DEVSTAT    0x00ff
 
 /*
  * DBDMA command structure.  These fields are all little-endian!
  */
 
 typedef struct dbdma_cmd {
-    uint16_t req_count;	  /* requested byte transfer count */
-    uint16_t command;	  /* command word (has bit-fields) */
-    uint32_t phy_addr;	  /* physical data address */
-    uint32_t cmd_dep;	  /* command-dependent field */
-    uint16_t res_count;	  /* residual count after completion */
-    uint16_t xfer_status; /* transfer status */
+    uint16_t req_count;          /* requested byte transfer count */
+    uint16_t command;            /* command word (has bit-fields) */
+    uint32_t phy_addr;           /* physical data address */
+    uint32_t cmd_dep;            /* command-dependent field */
+    uint16_t res_count;          /* residual count after completion */
+    uint16_t xfer_status;        /* transfer status */
 } dbdma_cmd;
 
 /* DBDMA command values in command field */
 
 #define COMMAND_MASK    0xf000
-#define OUTPUT_MORE	0x0000	/* transfer memory data to stream */
-#define OUTPUT_LAST	0x1000	/* ditto followed by end marker */
-#define INPUT_MORE	0x2000	/* transfer stream data to memory */
-#define INPUT_LAST	0x3000	/* ditto, expect end marker */
-#define STORE_WORD	0x4000	/* write word (4 bytes) to device reg */
-#define LOAD_WORD	0x5000	/* read word (4 bytes) from device reg */
-#define DBDMA_NOP	0x6000	/* do nothing */
-#define DBDMA_STOP	0x7000	/* suspend processing */
+#define OUTPUT_MORE     0x0000        /* transfer memory data to stream */
+#define OUTPUT_LAST     0x1000        /* ditto followed by end marker */
+#define INPUT_MORE      0x2000        /* transfer stream data to memory */
+#define INPUT_LAST      0x3000        /* ditto, expect end marker */
+#define STORE_WORD      0x4000        /* write word (4 bytes) to device reg */
+#define LOAD_WORD       0x5000        /* read word (4 bytes) from device reg */
+#define DBDMA_NOP       0x6000        /* do nothing */
+#define DBDMA_STOP      0x7000        /* suspend processing */
 
 /* Key values in command field */
 
 #define KEY_MASK        0x0700
-#define KEY_STREAM0	0x0000	/* usual data stream */
-#define KEY_STREAM1	0x0100	/* control/status stream */
-#define KEY_STREAM2	0x0200	/* device-dependent stream */
-#define KEY_STREAM3	0x0300	/* device-dependent stream */
-#define KEY_STREAM4	0x0400	/* reserved */
-#define KEY_REGS	0x0500	/* device register space */
-#define KEY_SYSTEM	0x0600	/* system memory-mapped space */
-#define KEY_DEVICE	0x0700	/* device memory-mapped space */
+#define KEY_STREAM0     0x0000        /* usual data stream */
+#define KEY_STREAM1     0x0100        /* control/status stream */
+#define KEY_STREAM2     0x0200        /* device-dependent stream */
+#define KEY_STREAM3     0x0300        /* device-dependent stream */
+#define KEY_STREAM4     0x0400        /* reserved */
+#define KEY_REGS        0x0500        /* device register space */
+#define KEY_SYSTEM      0x0600        /* system memory-mapped space */
+#define KEY_DEVICE      0x0700        /* device memory-mapped space */
 
 /* Interrupt control values in command field */
 
 #define INTR_MASK       0x0030
-#define INTR_NEVER	0x0000	/* don't interrupt */
-#define INTR_IFSET	0x0010	/* intr if condition bit is 1 */
-#define INTR_IFCLR	0x0020	/* intr if condition bit is 0 */
-#define INTR_ALWAYS	0x0030	/* always interrupt */
+#define INTR_NEVER      0x0000        /* don't interrupt */
+#define INTR_IFSET      0x0010        /* intr if condition bit is 1 */
+#define INTR_IFCLR      0x0020        /* intr if condition bit is 0 */
+#define INTR_ALWAYS     0x0030        /* always interrupt */
 
 /* Branch control values in command field */
 
 #define BR_MASK         0x000c
-#define BR_NEVER	0x0000	/* don't branch */
-#define BR_IFSET	0x0004	/* branch if condition bit is 1 */
-#define BR_IFCLR	0x0008	/* branch if condition bit is 0 */
-#define BR_ALWAYS	0x000c	/* always branch */
+#define BR_NEVER        0x0000        /* don't branch */
+#define BR_IFSET        0x0004        /* branch if condition bit is 1 */
+#define BR_IFCLR        0x0008        /* branch if condition bit is 0 */
+#define BR_ALWAYS       0x000c        /* always branch */
 
 /* Wait control values in command field */
 
 #define WAIT_MASK       0x0003
-#define WAIT_NEVER	0x0000	/* don't wait */
-#define WAIT_IFSET	0x0001	/* wait if condition bit is 1 */
-#define WAIT_IFCLR	0x0002	/* wait if condition bit is 0 */
-#define WAIT_ALWAYS	0x0003	/* always wait */
+#define WAIT_NEVER      0x0000        /* don't wait */
+#define WAIT_IFSET      0x0001        /* wait if condition bit is 1 */
+#define WAIT_IFCLR      0x0002        /* wait if condition bit is 0 */
+#define WAIT_ALWAYS     0x0003        /* always wait */
 
 typedef struct DBDMA_channel {
     int channel;
@@ -558,11 +558,11 @@ static void channel_run(DBDMA_channel *ch)
     switch (cmd) {
     case DBDMA_NOP:
         nop(ch);
-	return;
+        return;
 
     case DBDMA_STOP:
         stop(ch);
-	return;
+        return;
     }
 
     key = le16_to_cpu(current->command) & 0x0700;
@@ -578,19 +578,19 @@ static void channel_run(DBDMA_channel *ch)
     switch (cmd) {
     case OUTPUT_MORE:
         start_output(ch, key, phy_addr, req_count, 0);
-	return;
+        return;
 
     case OUTPUT_LAST:
         start_output(ch, key, phy_addr, req_count, 1);
-	return;
+        return;
 
     case INPUT_MORE:
         start_input(ch, key, phy_addr, req_count, 0);
-	return;
+        return;
 
     case INPUT_LAST:
         start_input(ch, key, phy_addr, req_count, 1);
-	return;
+        return;
     }
 
     if (key < KEY_REGS) {
@@ -615,11 +615,11 @@ static void channel_run(DBDMA_channel *ch)
     switch (cmd) {
     case LOAD_WORD:
         load_word(ch, key, phy_addr, req_count);
-	return;
+        return;
 
     case STORE_WORD:
         store_word(ch, key, phy_addr, req_count);
-	return;
+        return;
     }
 }
 
@@ -720,7 +720,7 @@ static void dbdma_write(void *opaque, hwaddr addr,
 
     if (reg == DBDMA_CMDPTR_LO &&
         (ch->regs[DBDMA_STATUS] & (RUN | ACTIVE)))
-	return;
+        return;
 
     ch->regs[reg] = value;
 
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (3 preceding siblings ...)
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 04/15] PPC: dbdma: " Alexander Graf
@ 2013-06-30  1:26 ` Alexander Graf
  2013-06-30  6:42   ` Andreas Färber
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 06/15] PPC: dbdma: Fix debug print Alexander Graf
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:26 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

The macio code is basically undebuggable as it stands today, with no
debug prints anywhere whatsoever. DBDMA was better, but I needed a
few more to create reasonable logs that tell me where breakage is.

Add a DPRINTF macro in the macio source file and add a bunch of debug
prints that are all disabled by default of course.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/macio.c            | 39 ++++++++++++++++++++++++++++++++++++++-
 hw/misc/macio/mac_dbdma.c | 12 ++++++++++--
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 82409dc..5cbc923 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -30,6 +30,17 @@
 
 #include <hw/ide/internal.h>
 
+/* debug MACIO */
+// #define DEBUG_MACIO
+
+#ifdef DEBUG_MACIO
+#define MACIO_DPRINTF(fmt, ...)                                 \
+    do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0)
+#else
+#define MACIO_DPRINTF(fmt, ...)
+#endif
+
+
 /***********************************************************/
 /* MacIO based PowerPC IDE */
 
@@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
         goto done;
     }
 
+    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
+
     if (s->io_buffer_size > 0) {
         m->aiocb = NULL;
         qemu_sglist_destroy(&s->sg);
@@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
         s->io_buffer_index &= 0x7ff;
     }
 
-    if (s->packet_transfer_size <= 0)
+    /* end of transfer ? */
+    if (s->packet_transfer_size <= 0) {
+        MACIO_DPRINTF("end of transfer\n");
         ide_atapi_cmd_ok(s);
+    }
 
+    /* end of DMA ? */
     if (io->len == 0) {
+        MACIO_DPRINTF("end of DMA\n");
         goto done;
     }
 
     /* launch next transfer */
 
+    MACIO_DPRINTF("io->len = %#x\n", io->len);
+
     s->io_buffer_size = io->len;
 
     qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
@@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     io->addr += io->len;
     io->len = 0;
 
+    MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
+                  (s->lba << 2) + (s->io_buffer_index >> 9),
+                  s->packet_transfer_size, s->dma_cmd);
+
     m->aiocb = dma_bdrv_read(s->bs, &s->sg,
                              (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9),
                              pmac_ide_atapi_transfer_cb, io);
     return;
 
 done:
+    MACIO_DPRINTF("done DMA\n");
     bdrv_acct_done(s->bs, &s->acct);
     io->dma_end(opaque);
 }
@@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     int64_t sector_num;
 
     if (ret < 0) {
+        MACIO_DPRINTF("DMA error\n");
         m->aiocb = NULL;
         qemu_sglist_destroy(&s->sg);
         ide_dma_error(s);
@@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     }
 
     sector_num = ide_get_sector(s);
+    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
     if (s->io_buffer_size > 0) {
         m->aiocb = NULL;
         qemu_sglist_destroy(&s->sg);
@@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
     /* end of transfer ? */
     if (s->nsector == 0) {
+        MACIO_DPRINTF("end of transfer\n");
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
     }
 
     /* end of DMA ? */
     if (io->len == 0) {
+        MACIO_DPRINTF("end of DMA\n");
         goto done;
     }
 
@@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     s->io_buffer_index = 0;
     s->io_buffer_size = io->len;
 
+
+    MACIO_DPRINTF("io->len = %#x\n", io->len);
+
     qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
                      &address_space_memory);
     qemu_sglist_add(&s->sg, io->addr, io->len);
     io->addr += io->len;
     io->len = 0;
 
+    MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n",
+                  sector_num, n, s->nsector, s->dma_cmd);
+
     switch (s->dma_cmd) {
     case IDE_DMA_READ:
         m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
@@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io)
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
 
+    MACIO_DPRINTF("\n", __LINE__);
+
     s->io_buffer_size = 0;
     if (s->drive_kind == IDE_CD) {
         bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index ab174f5..903604d 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
         return;
     case INTR_ALWAYS: /* always interrupt */
         qemu_irq_raise(ch->irq);
+        DBDMA_DPRINTF("conditional_interrupt: raise\n");
         return;
     }
 
@@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch)
 
     switch(intr) {
     case INTR_IFSET:  /* intr if condition bit is 1 */
-        if (cond)
+        if (cond) {
             qemu_irq_raise(ch->irq);
+            DBDMA_DPRINTF("conditional_interrupt: raise\n");
+        }
         return;
     case INTR_IFCLR:  /* intr if condition bit is 0 */
-        if (!cond)
+        if (!cond) {
             qemu_irq_raise(ch->irq);
+            DBDMA_DPRINTF("conditional_interrupt: raise\n");
+        }
         return;
     }
 }
@@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io)
     DBDMA_channel *ch = io->channel;
     dbdma_cmd *current = &ch->current;
 
+    DBDMA_DPRINTF("%s\n", __func__, __LINE__);
+
     if (conditional_wait(ch))
         goto wait;
 
@@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
      * are not implemented in the mac-io chip
      */
 
+    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
     if (!addr || key > KEY_STREAM3) {
         kill_channel(ch);
         return;
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 06/15] PPC: dbdma: Fix debug print
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (4 preceding siblings ...)
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 07/15] PPC: dbdma: Allow new commands in RUN state Alexander Graf
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

There was a debug print that didn't compile for me because the format
and the arguments weren't in sync. Fix it up.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 903604d..fb4cf23 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -720,7 +720,8 @@ static void dbdma_write(void *opaque, hwaddr addr,
     DBDMA_channel *ch = &s->channels[channel];
     int reg = (addr - (channel << DBDMA_CHANNEL_SHIFT)) >> 2;
 
-    DBDMA_DPRINTF("writel 0x" TARGET_FMT_plx " <= 0x%08x\n", addr, value);
+    DBDMA_DPRINTF("writel 0x" TARGET_FMT_plx " <= 0x%08"PRIx64"\n",
+                  addr, value);
     DBDMA_DPRINTF("channel 0x%x reg 0x%x\n",
                   (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
 
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 07/15] PPC: dbdma: Allow new commands in RUN state
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (5 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 06/15] PPC: dbdma: Fix debug print Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 08/15] PPC: dbdma: Move defines into header file Alexander Graf
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

The DBDMA controller can not change its command stream while it's
actively streaming data, true. But the fact that it's in RUN state
doesn't actually indicate anything. It could just as well be in
WAIT while in RUN. And then it's legal to change commands.

This fixes a real world issue I've encountered with Mac OS X.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index fb4cf23..566185d 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -725,11 +725,11 @@ static void dbdma_write(void *opaque, hwaddr addr,
     DBDMA_DPRINTF("channel 0x%x reg 0x%x\n",
                   (uint32_t)addr >> DBDMA_CHANNEL_SHIFT, reg);
 
-    /* cmdptr cannot be modified if channel is RUN or ACTIVE */
+    /* cmdptr cannot be modified if channel is ACTIVE */
 
-    if (reg == DBDMA_CMDPTR_LO &&
-        (ch->regs[DBDMA_STATUS] & (RUN | ACTIVE)))
+    if (reg == DBDMA_CMDPTR_LO && (ch->regs[DBDMA_STATUS] & ACTIVE)) {
         return;
+    }
 
     ch->regs[reg] = value;
 
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 08/15] PPC: dbdma: Move defines into header file
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (6 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 07/15] PPC: dbdma: Allow new commands in RUN state Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 09/15] PPC: dbdma: Introduce kick function Alexander Graf
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

We usually keep struct and constant definitions in header files. Move
them there to stay consistent and to make access to fields easier.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c  | 117 --------------------------------------------
 include/hw/ppc/mac_dbdma.h | 118 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+), 117 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 566185d..c2f15d8 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -54,123 +54,6 @@
 /*
  */
 
-/*
- * DBDMA control/status registers.  All little-endian.
- */
-
-#define DBDMA_CONTROL         0x00
-#define DBDMA_STATUS          0x01
-#define DBDMA_CMDPTR_HI       0x02
-#define DBDMA_CMDPTR_LO       0x03
-#define DBDMA_INTR_SEL        0x04
-#define DBDMA_BRANCH_SEL      0x05
-#define DBDMA_WAIT_SEL        0x06
-#define DBDMA_XFER_MODE       0x07
-#define DBDMA_DATA2PTR_HI     0x08
-#define DBDMA_DATA2PTR_LO     0x09
-#define DBDMA_RES1            0x0A
-#define DBDMA_ADDRESS_HI      0x0B
-#define DBDMA_BRANCH_ADDR_HI  0x0C
-#define DBDMA_RES2            0x0D
-#define DBDMA_RES3            0x0E
-#define DBDMA_RES4            0x0F
-
-#define DBDMA_REGS            16
-#define DBDMA_SIZE            (DBDMA_REGS * sizeof(uint32_t))
-
-#define DBDMA_CHANNEL_SHIFT   7
-#define DBDMA_CHANNEL_SIZE    (1 << DBDMA_CHANNEL_SHIFT)
-
-#define DBDMA_CHANNELS        (0x1000 >> DBDMA_CHANNEL_SHIFT)
-
-/* Bits in control and status registers */
-
-#define RUN        0x8000
-#define PAUSE      0x4000
-#define FLUSH      0x2000
-#define WAKE       0x1000
-#define DEAD       0x0800
-#define ACTIVE     0x0400
-#define BT         0x0100
-#define DEVSTAT    0x00ff
-
-/*
- * DBDMA command structure.  These fields are all little-endian!
- */
-
-typedef struct dbdma_cmd {
-    uint16_t req_count;          /* requested byte transfer count */
-    uint16_t command;            /* command word (has bit-fields) */
-    uint32_t phy_addr;           /* physical data address */
-    uint32_t cmd_dep;            /* command-dependent field */
-    uint16_t res_count;          /* residual count after completion */
-    uint16_t xfer_status;        /* transfer status */
-} dbdma_cmd;
-
-/* DBDMA command values in command field */
-
-#define COMMAND_MASK    0xf000
-#define OUTPUT_MORE     0x0000        /* transfer memory data to stream */
-#define OUTPUT_LAST     0x1000        /* ditto followed by end marker */
-#define INPUT_MORE      0x2000        /* transfer stream data to memory */
-#define INPUT_LAST      0x3000        /* ditto, expect end marker */
-#define STORE_WORD      0x4000        /* write word (4 bytes) to device reg */
-#define LOAD_WORD       0x5000        /* read word (4 bytes) from device reg */
-#define DBDMA_NOP       0x6000        /* do nothing */
-#define DBDMA_STOP      0x7000        /* suspend processing */
-
-/* Key values in command field */
-
-#define KEY_MASK        0x0700
-#define KEY_STREAM0     0x0000        /* usual data stream */
-#define KEY_STREAM1     0x0100        /* control/status stream */
-#define KEY_STREAM2     0x0200        /* device-dependent stream */
-#define KEY_STREAM3     0x0300        /* device-dependent stream */
-#define KEY_STREAM4     0x0400        /* reserved */
-#define KEY_REGS        0x0500        /* device register space */
-#define KEY_SYSTEM      0x0600        /* system memory-mapped space */
-#define KEY_DEVICE      0x0700        /* device memory-mapped space */
-
-/* Interrupt control values in command field */
-
-#define INTR_MASK       0x0030
-#define INTR_NEVER      0x0000        /* don't interrupt */
-#define INTR_IFSET      0x0010        /* intr if condition bit is 1 */
-#define INTR_IFCLR      0x0020        /* intr if condition bit is 0 */
-#define INTR_ALWAYS     0x0030        /* always interrupt */
-
-/* Branch control values in command field */
-
-#define BR_MASK         0x000c
-#define BR_NEVER        0x0000        /* don't branch */
-#define BR_IFSET        0x0004        /* branch if condition bit is 1 */
-#define BR_IFCLR        0x0008        /* branch if condition bit is 0 */
-#define BR_ALWAYS       0x000c        /* always branch */
-
-/* Wait control values in command field */
-
-#define WAIT_MASK       0x0003
-#define WAIT_NEVER      0x0000        /* don't wait */
-#define WAIT_IFSET      0x0001        /* wait if condition bit is 1 */
-#define WAIT_IFCLR      0x0002        /* wait if condition bit is 0 */
-#define WAIT_ALWAYS     0x0003        /* always wait */
-
-typedef struct DBDMA_channel {
-    int channel;
-    uint32_t regs[DBDMA_REGS];
-    qemu_irq irq;
-    DBDMA_io io;
-    DBDMA_rw rw;
-    DBDMA_flush flush;
-    dbdma_cmd current;
-    int processing;
-} DBDMA_channel;
-
-typedef struct {
-    MemoryRegion mem;
-    DBDMA_channel channels[DBDMA_CHANNELS];
-} DBDMAState;
-
 #ifdef DEBUG_DBDMA
 static void dump_dbdma_cmd(dbdma_cmd *cmd)
 {
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 691263e..90be5d9 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -39,6 +39,124 @@ struct DBDMA_io {
     DBDMA_end dma_end;
 };
 
+/*
+ * DBDMA control/status registers.  All little-endian.
+ */
+
+#define DBDMA_CONTROL         0x00
+#define DBDMA_STATUS          0x01
+#define DBDMA_CMDPTR_HI       0x02
+#define DBDMA_CMDPTR_LO       0x03
+#define DBDMA_INTR_SEL        0x04
+#define DBDMA_BRANCH_SEL      0x05
+#define DBDMA_WAIT_SEL        0x06
+#define DBDMA_XFER_MODE       0x07
+#define DBDMA_DATA2PTR_HI     0x08
+#define DBDMA_DATA2PTR_LO     0x09
+#define DBDMA_RES1            0x0A
+#define DBDMA_ADDRESS_HI      0x0B
+#define DBDMA_BRANCH_ADDR_HI  0x0C
+#define DBDMA_RES2            0x0D
+#define DBDMA_RES3            0x0E
+#define DBDMA_RES4            0x0F
+
+#define DBDMA_REGS            16
+#define DBDMA_SIZE            (DBDMA_REGS * sizeof(uint32_t))
+
+#define DBDMA_CHANNEL_SHIFT   7
+#define DBDMA_CHANNEL_SIZE    (1 << DBDMA_CHANNEL_SHIFT)
+
+#define DBDMA_CHANNELS        (0x1000 >> DBDMA_CHANNEL_SHIFT)
+
+/* Bits in control and status registers */
+
+#define RUN        0x8000
+#define PAUSE      0x4000
+#define FLUSH      0x2000
+#define WAKE       0x1000
+#define DEAD       0x0800
+#define ACTIVE     0x0400
+#define BT         0x0100
+#define DEVSTAT    0x00ff
+
+/*
+ * DBDMA command structure.  These fields are all little-endian!
+ */
+
+typedef struct dbdma_cmd {
+    uint16_t req_count;          /* requested byte transfer count */
+    uint16_t command;            /* command word (has bit-fields) */
+    uint32_t phy_addr;           /* physical data address */
+    uint32_t cmd_dep;            /* command-dependent field */
+    uint16_t res_count;          /* residual count after completion */
+    uint16_t xfer_status;        /* transfer status */
+} dbdma_cmd;
+
+/* DBDMA command values in command field */
+
+#define COMMAND_MASK    0xf000
+#define OUTPUT_MORE     0x0000        /* transfer memory data to stream */
+#define OUTPUT_LAST     0x1000        /* ditto followed by end marker */
+#define INPUT_MORE      0x2000        /* transfer stream data to memory */
+#define INPUT_LAST      0x3000        /* ditto, expect end marker */
+#define STORE_WORD      0x4000        /* write word (4 bytes) to device reg */
+#define LOAD_WORD       0x5000        /* read word (4 bytes) from device reg */
+#define DBDMA_NOP       0x6000        /* do nothing */
+#define DBDMA_STOP      0x7000        /* suspend processing */
+
+/* Key values in command field */
+
+#define KEY_MASK        0x0700
+#define KEY_STREAM0     0x0000        /* usual data stream */
+#define KEY_STREAM1     0x0100        /* control/status stream */
+#define KEY_STREAM2     0x0200        /* device-dependent stream */
+#define KEY_STREAM3     0x0300        /* device-dependent stream */
+#define KEY_STREAM4     0x0400        /* reserved */
+#define KEY_REGS        0x0500        /* device register space */
+#define KEY_SYSTEM      0x0600        /* system memory-mapped space */
+#define KEY_DEVICE      0x0700        /* device memory-mapped space */
+
+/* Interrupt control values in command field */
+
+#define INTR_MASK       0x0030
+#define INTR_NEVER      0x0000        /* don't interrupt */
+#define INTR_IFSET      0x0010        /* intr if condition bit is 1 */
+#define INTR_IFCLR      0x0020        /* intr if condition bit is 0 */
+#define INTR_ALWAYS     0x0030        /* always interrupt */
+
+/* Branch control values in command field */
+
+#define BR_MASK         0x000c
+#define BR_NEVER        0x0000        /* don't branch */
+#define BR_IFSET        0x0004        /* branch if condition bit is 1 */
+#define BR_IFCLR        0x0008        /* branch if condition bit is 0 */
+#define BR_ALWAYS       0x000c        /* always branch */
+
+/* Wait control values in command field */
+
+#define WAIT_MASK       0x0003
+#define WAIT_NEVER      0x0000        /* don't wait */
+#define WAIT_IFSET      0x0001        /* wait if condition bit is 1 */
+#define WAIT_IFCLR      0x0002        /* wait if condition bit is 0 */
+#define WAIT_ALWAYS     0x0003        /* always wait */
+
+typedef struct DBDMA_channel {
+    int channel;
+    uint32_t regs[DBDMA_REGS];
+    qemu_irq irq;
+    DBDMA_io io;
+    DBDMA_rw rw;
+    DBDMA_flush flush;
+    dbdma_cmd current;
+    int processing;
+} DBDMA_channel;
+
+typedef struct {
+    MemoryRegion mem;
+    DBDMA_channel channels[DBDMA_CHANNELS];
+} DBDMAState;
+
+/* Externally callable functions */
 
 void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
                             DBDMA_rw rw, DBDMA_flush flush,
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 09/15] PPC: dbdma: Introduce kick function
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (7 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 08/15] PPC: dbdma: Move defines into header file Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 10/15] PPC: dbdma: Move static bh variable to device struct Alexander Graf
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

The DBDMA engine really is running all the time, waiting for input. However
we don't want to waste cycles constantly polling.

So introduce a kick function that data providers can call to notify the
DBDMA controller of new input.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c  | 5 +++++
 include/hw/ppc/mac_dbdma.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index c2f15d8..6830a85 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -536,6 +536,11 @@ static void DBDMA_run_bh(void *opaque)
     DBDMA_run(s);
 }
 
+void DBDMA_kick(DBDMAState *dbdma)
+{
+    qemu_bh_schedule(dbdma_bh);
+}
+
 void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
                             DBDMA_rw rw, DBDMA_flush flush,
                             void *opaque)
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 90be5d9..aaeab10 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -161,6 +161,7 @@ typedef struct {
 void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
                             DBDMA_rw rw, DBDMA_flush flush,
                             void *opaque);
+void DBDMA_kick(DBDMAState *dbdma);
 void* DBDMA_init (MemoryRegion **dbdma_mem);
 
 #endif
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 10/15] PPC: dbdma: Move static bh variable to device struct
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (8 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 09/15] PPC: dbdma: Introduce kick function Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 11/15] PPC: dbdma: macio: Add DMA callback Alexander Graf
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

The DBDMA controller has a bottom half to asynchronously process DMA
request queues.

This bh was stored as a gross static variable. Move it into the device
struct instead.

While at it, move all users of it to the new generic kick function.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c  | 24 +++++++++++++++---------
 include/hw/ppc/mac_dbdma.h |  1 +
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 6830a85..324ac54 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -54,6 +54,11 @@
 /*
  */
 
+static DBDMAState *dbdma_from_ch(DBDMA_channel *ch)
+{
+    return container_of(ch, DBDMAState, channels[ch->channel]);
+}
+
 #ifdef DEBUG_DBDMA
 static void dump_dbdma_cmd(dbdma_cmd *cmd)
 {
@@ -248,7 +253,6 @@ static void conditional_branch(DBDMA_channel *ch)
     }
 }
 
-static QEMUBH *dbdma_bh;
 static void channel_run(DBDMA_channel *ch);
 
 static void dbdma_end(DBDMA_io *io)
@@ -365,7 +369,7 @@ static void load_word(DBDMA_channel *ch, int key, uint32_t addr,
     next(ch);
 
 wait:
-    qemu_bh_schedule(dbdma_bh);
+    DBDMA_kick(dbdma_from_ch(ch));
 }
 
 static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
@@ -403,7 +407,7 @@ static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
     next(ch);
 
 wait:
-    qemu_bh_schedule(dbdma_bh);
+    DBDMA_kick(dbdma_from_ch(ch));
 }
 
 static void nop(DBDMA_channel *ch)
@@ -420,7 +424,7 @@ static void nop(DBDMA_channel *ch)
     conditional_branch(ch);
 
 wait:
-    qemu_bh_schedule(dbdma_bh);
+    DBDMA_kick(dbdma_from_ch(ch));
 }
 
 static void stop(DBDMA_channel *ch)
@@ -538,7 +542,7 @@ static void DBDMA_run_bh(void *opaque)
 
 void DBDMA_kick(DBDMAState *dbdma)
 {
-    qemu_bh_schedule(dbdma_bh);
+    qemu_bh_schedule(dbdma->bh);
 }
 
 void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
@@ -594,10 +598,12 @@ dbdma_control_write(DBDMA_channel *ch)
 
     ch->regs[DBDMA_STATUS] = status;
 
-    if (status & ACTIVE)
-        qemu_bh_schedule(dbdma_bh);
-    if ((status & FLUSH) && ch->flush)
+    if (status & ACTIVE) {
+        DBDMA_kick(dbdma_from_ch(ch));
+    }
+    if ((status & FLUSH) && ch->flush) {
         ch->flush(&ch->io);
+    }
 }
 
 static void dbdma_write(void *opaque, hwaddr addr,
@@ -750,7 +756,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
     vmstate_register(NULL, -1, &vmstate_dbdma, s);
     qemu_register_reset(dbdma_reset, s);
 
-    dbdma_bh = qemu_bh_new(DBDMA_run_bh, s);
+    s->bh = qemu_bh_new(DBDMA_run_bh, s);
 
     return s;
 }
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index aaeab10..eb8e0f0 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -154,6 +154,7 @@ typedef struct DBDMA_channel {
 typedef struct {
     MemoryRegion mem;
     DBDMA_channel channels[DBDMA_CHANNELS];
+    QEMUBH *bh;
 } DBDMAState;
 
 /* Externally callable functions */
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 11/15] PPC: dbdma: macio: Add DMA callback
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (9 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 10/15] PPC: dbdma: Move static bh variable to device struct Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io Alexander Graf
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

We need to know when the IDE core starts a DMA transfer. Add a notifier
function so we have the chance to start transmitting data.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/macio.c | 40 ++++++++++++++++++++++++++++++++++++++++
 hw/ppc/mac.h   |  2 ++
 2 files changed, 42 insertions(+)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 5cbc923..bd1b972 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -359,11 +359,50 @@ static void macio_ide_reset(DeviceState *dev)
     ide_bus_reset(&d->bus);
 }
 
+static int ide_nop(IDEDMA *dma)
+{
+    return 0;
+}
+
+static int ide_nop_int(IDEDMA *dma, int x)
+{
+    return 0;
+}
+
+static void ide_nop_restart(void *opaque, int x, RunState y)
+{
+}
+
+static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
+                            BlockDriverCompletionFunc *cb)
+{
+    MACIOIDEState *m = container_of(dma, MACIOIDEState, dma);
+
+    MACIO_DPRINTF("\n", __LINE__);
+    DBDMA_kick(m->dbdma);
+}
+
+static const IDEDMAOps dbdma_ops = {
+    .start_dma      = ide_dbdma_start,
+    .start_transfer = ide_nop,
+    .prepare_buf    = ide_nop_int,
+    .rw_buf         = ide_nop_int,
+    .set_unit       = ide_nop_int,
+    .add_status     = ide_nop_int,
+    .set_inactive   = ide_nop,
+    .restart_cb     = ide_nop_restart,
+    .reset          = ide_nop,
+};
+
 static void macio_ide_realizefn(DeviceState *dev, Error **errp)
 {
     MACIOIDEState *s = MACIO_IDE(dev);
 
     ide_init2(&s->bus, s->irq);
+
+    /* Register DMA callbacks */
+    s->dma.ops = &dbdma_ops;
+    s->bus.dma = &s->dma;
 }
 
 static void macio_ide_initfn(Object *obj)
@@ -414,6 +453,7 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
 
 void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
 {
+    s->dbdma = dbdma;
     DBDMA_register_channel(dbdma, channel, s->dma_irq,
                            pmac_ide_transfer, pmac_ide_flush, s);
 }
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index 54efaed..27c4ca3 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -131,6 +131,8 @@ typedef struct MACIOIDEState {
     MemoryRegion mem;
     IDEBus bus;
     BlockDriverAIOCB *aiocb;
+    IDEDMA dma;
+    void *dbdma;
 } MACIOIDEState;
 
 void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (10 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 11/15] PPC: dbdma: macio: Add DMA callback Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  6:48   ` Andreas Färber
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 13/15] PPC: dbdma: Wait for DMA until we have data Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 14/15] PPC: dbdma: Support unaligned DMA access Alexander Graf
  13 siblings, 1 reply; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

Soon we will introduce intermediate processing pauses which will
allow the bottom half to restart a DMA request that couldn't be
fulfilled yet.

For that to work, move the processing variable into the io struct
which is what DMA providers work with.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/misc/macio/mac_dbdma.c  | 10 ++++++----
 include/hw/ppc/mac_dbdma.h |  3 ++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 324ac54..91b9eaf 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -275,7 +275,9 @@ static void dbdma_end(DBDMA_io *io)
     conditional_branch(ch);
 
 wait:
-    ch->processing = 0;
+    /* Indicate that we're ready for a new DMA round */
+    ch->io.processing = 0;
+
     if ((ch->regs[DBDMA_STATUS] & RUN) &&
         (ch->regs[DBDMA_STATUS] & ACTIVE))
         channel_run(ch);
@@ -301,7 +303,7 @@ static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
     ch->io.is_last = is_last;
     ch->io.dma_end = dbdma_end;
     ch->io.is_dma_out = 1;
-    ch->processing = 1;
+    ch->io.processing = 1;
     if (ch->rw) {
         ch->rw(&ch->io);
     }
@@ -327,7 +329,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
     ch->io.is_last = is_last;
     ch->io.dma_end = dbdma_end;
     ch->io.is_dma_out = 0;
-    ch->processing = 1;
+    ch->io.processing = 1;
     if (ch->rw) {
         ch->rw(&ch->io);
     }
@@ -525,7 +527,7 @@ static void DBDMA_run(DBDMAState *s)
     for (channel = 0; channel < DBDMA_CHANNELS; channel++) {
         DBDMA_channel *ch = &s->channels[channel];
         uint32_t status = ch->regs[DBDMA_STATUS];
-        if (!ch->processing && (status & RUN) && (status & ACTIVE)) {
+        if (!ch->io.processing && (status & RUN) && (status & ACTIVE)) {
             channel_run(ch);
         }
     }
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index eb8e0f0..8ad1b6e 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -37,6 +37,8 @@ struct DBDMA_io {
     int is_last;
     int is_dma_out;
     DBDMA_end dma_end;
+    /* DMA is in progress, don't start another one */
+    int processing;
 };
 
 /*
@@ -148,7 +150,6 @@ typedef struct DBDMA_channel {
     DBDMA_rw rw;
     DBDMA_flush flush;
     dbdma_cmd current;
-    int processing;
 } DBDMA_channel;
 
 typedef struct {
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 13/15] PPC: dbdma: Wait for DMA until we have data
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (11 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 14/15] PPC: dbdma: Support unaligned DMA access Alexander Graf
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

We should only start processing DMA requests when we have data to process.
Hold off working through the DMA shuffling until the IDE core told us that
it's ready.

This is required because the guest can program the DMA engine or the IDE
transfer first. Both are legal.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/macio.c | 19 +++++++++++++++++++
 hw/ppc/mac.h   |  1 +
 2 files changed, 20 insertions(+)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index bd1b972..095e1f1 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -59,6 +59,14 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
         goto done;
     }
 
+    if (!m->dma_active) {
+        MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
+                      s->nsector, io->len, s->status);
+        /* data not ready yet, wait for the channel to get restarted */
+        io->processing = 0;
+        return;
+    }
+
     MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
 
     if (s->io_buffer_size > 0) {
@@ -76,6 +84,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     if (s->packet_transfer_size <= 0) {
         MACIO_DPRINTF("end of transfer\n");
         ide_atapi_cmd_ok(s);
+        m->dma_active = false;
     }
 
     /* end of DMA ? */
@@ -127,6 +136,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         goto done;
     }
 
+    if (!m->dma_active) {
+        MACIO_DPRINTF("waiting for data (%#x - %#x - %x)\n",
+                      s->nsector, io->len, s->status);
+        /* data not ready yet, wait for the channel to get restarted */
+        io->processing = 0;
+        return;
+    }
+
     sector_num = ide_get_sector(s);
     MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
     if (s->io_buffer_size > 0) {
@@ -143,6 +160,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         MACIO_DPRINTF("end of transfer\n");
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
+        m->dma_active = false;
     }
 
     /* end of DMA ? */
@@ -379,6 +397,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
     MACIOIDEState *m = container_of(dma, MACIOIDEState, dma);
 
     MACIO_DPRINTF("\n", __LINE__);
+    m->dma_active = true;
     DBDMA_kick(m->dbdma);
 }
 
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index 27c4ca3..1e578dd 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -133,6 +133,7 @@ typedef struct MACIOIDEState {
     BlockDriverAIOCB *aiocb;
     IDEDMA dma;
     void *dbdma;
+    bool dma_active;
 } MACIOIDEState;
 
 void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
-- 
1.8.1.4

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

* [Qemu-devel] [PATCH 14/15] PPC: dbdma: Support unaligned DMA access
  2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
                   ` (12 preceding siblings ...)
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 13/15] PPC: dbdma: Wait for DMA until we have data Alexander Graf
@ 2013-06-30  1:27 ` Alexander Graf
  13 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30  1:27 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-devel

The DBDMA engine really just reads bytes from a producing device (IDE
in our case) and shoves these bytes into memory. It doesn't care whether
any alignment takes place or not.

Our code today however assumes that block accesses always happen on
sector (512 byte) boundaries. This is a fair assumption for most cases.

However, Mac OS X really likes to do unaligned, incomplete accesses
that it finishes with the next DMA request.

So we need to read / write the unaligned bits independent of the actual
asynchronous request, because that one can only handle 512-byte-aligned
data. We also need to cache these unaligned sectors until the next DMA
request, at which point the data might be successfully flushed from the
pipe.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/macio.c             | 128 ++++++++++++++++++++++++++++++++++++++++++---
 include/hw/ppc/mac_dbdma.h |   3 ++
 2 files changed, 124 insertions(+), 7 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 095e1f1..202df06 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -51,11 +51,13 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     DBDMA_io *io = opaque;
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
+    int unaligned;
 
     if (ret < 0) {
         m->aiocb = NULL;
         qemu_sglist_destroy(&s->sg);
         ide_atapi_io_error(s, ret);
+        io->remainder_len = 0;
         goto done;
     }
 
@@ -80,8 +82,30 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
         s->io_buffer_index &= 0x7ff;
     }
 
+    s->io_buffer_size = io->len;
+
+    MACIO_DPRINTF("remainder: %d io->len: %d size: %d\n", io->remainder_len,
+                  io->len, s->packet_transfer_size);
+    if (io->remainder_len && io->len) {
+        /* guest wants the rest of its previous transfer */
+        int remainder_len = MIN(io->remainder_len, io->len);
+
+        MACIO_DPRINTF("copying remainder %d bytes\n", remainder_len);
+
+        cpu_physical_memory_write(io->addr, io->remainder + 0x200 -
+                                  remainder_len, remainder_len);
+
+        io->addr += remainder_len;
+        io->len -= remainder_len;
+        s->io_buffer_size = remainder_len;
+        io->remainder_len -= remainder_len;
+        /* treat remainder as individual transfer, start again */
+        pmac_ide_atapi_transfer_cb(opaque, 0);
+        return;
+    }
+
     /* end of transfer ? */
-    if (s->packet_transfer_size <= 0) {
+    if (!s->packet_transfer_size) {
         MACIO_DPRINTF("end of transfer\n");
         ide_atapi_cmd_ok(s);
         m->dma_active = false;
@@ -95,14 +119,40 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 
     /* launch next transfer */
 
-    MACIO_DPRINTF("io->len = %#x\n", io->len);
+    /* handle unaligned accesses first, get them over with and only do the
+       remaining bulk transfer using our async DMA helpers */
+    unaligned = io->len & 0x1ff;
+    if (unaligned) {
+        int sector_num = (s->lba << 2) + (s->io_buffer_index >> 9);
+        int nsector = io->len >> 9;
 
-    s->io_buffer_size = io->len;
+        MACIO_DPRINTF("precopying unaligned %d bytes to %#lx\n",
+                      unaligned, io->addr + io->len - unaligned);
+
+        bdrv_read(s->bs, sector_num + nsector, io->remainder, 1);
+        cpu_physical_memory_write(io->addr + io->len - unaligned,
+                                  io->remainder, unaligned);
+
+        io->len -= unaligned;
+    }
+
+    MACIO_DPRINTF("io->len = %#x\n", io->len);
 
     qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
                      &address_space_memory);
     qemu_sglist_add(&s->sg, io->addr, io->len);
-    io->addr += io->len;
+    io->addr += s->io_buffer_size;
+    io->remainder_len = MIN(s->packet_transfer_size - s->io_buffer_size,
+                            (0x200 - unaligned) & 0x1ff);
+    MACIO_DPRINTF("set remainder to: %d\n", io->remainder_len);
+
+    /* We would read no data from the block layer, thus not get a callback.
+       Just fake completion manually. */
+    if (!io->len) {
+        pmac_ide_atapi_transfer_cb(opaque, 0);
+        return;
+    }
+
     io->len = 0;
 
     MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
@@ -125,14 +175,16 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     DBDMA_io *io = opaque;
     MACIOIDEState *m = io->opaque;
     IDEState *s = idebus_active_if(&m->bus);
-    int n;
+    int n = 0;
     int64_t sector_num;
+    int unaligned;
 
     if (ret < 0) {
         MACIO_DPRINTF("DMA error\n");
         m->aiocb = NULL;
         qemu_sglist_destroy(&s->sg);
         ide_dma_error(s);
+        io->remainder_len = 0;
         goto done;
     }
 
@@ -155,8 +207,34 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
         s->nsector -= n;
     }
 
+    MACIO_DPRINTF("remainder: %d io->len: %d nsector: %d sector_num: %ld\n",
+                  io->remainder_len, io->len, s->nsector, sector_num);
+    if (io->remainder_len && io->len) {
+        /* guest wants the rest of its previous transfer */
+        int remainder_len = MIN(io->remainder_len, io->len);
+        uint8_t *p = &io->remainder[0x200 - remainder_len];
+
+        MACIO_DPRINTF("copying remainder %d bytes at %#lx\n",
+                      remainder_len, io->addr);
+
+        switch (s->dma_cmd) {
+        case IDE_DMA_READ:
+            cpu_physical_memory_write(io->addr, p, remainder_len);
+            break;
+        case IDE_DMA_WRITE:
+            cpu_physical_memory_read(io->addr, p, remainder_len);
+            bdrv_write(s->bs, sector_num - 1, io->remainder, 1);
+            break;
+        case IDE_DMA_TRIM:
+            break;
+        }
+        io->addr += remainder_len;
+        io->len -= remainder_len;
+        io->remainder_len -= remainder_len;
+    }
+
     /* end of transfer ? */
-    if (s->nsector == 0) {
+    if (s->nsector == 0 && !io->remainder_len) {
         MACIO_DPRINTF("end of transfer\n");
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
@@ -174,13 +252,49 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     s->io_buffer_index = 0;
     s->io_buffer_size = io->len;
 
+    /* handle unaligned accesses first, get them over with and only do the
+       remaining bulk transfer using our async DMA helpers */
+    unaligned = io->len & 0x1ff;
+    if (unaligned) {
+        int nsector = io->len >> 9;
+
+        MACIO_DPRINTF("precopying unaligned %d bytes to %#lx\n",
+                      unaligned, io->addr + io->len - unaligned);
+
+        switch (s->dma_cmd) {
+        case IDE_DMA_READ:
+            bdrv_read(s->bs, sector_num + nsector, io->remainder, 1);
+            cpu_physical_memory_write(io->addr + io->len - unaligned,
+                                      io->remainder, unaligned);
+            break;
+        case IDE_DMA_WRITE:
+            /* cache the contents in our io struct */
+            cpu_physical_memory_read(io->addr + io->len - unaligned,
+                                     io->remainder, unaligned);
+            break;
+        case IDE_DMA_TRIM:
+            break;
+        }
+
+        io->len -= unaligned;
+    }
 
     MACIO_DPRINTF("io->len = %#x\n", io->len);
 
     qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
                      &address_space_memory);
     qemu_sglist_add(&s->sg, io->addr, io->len);
-    io->addr += io->len;
+    io->addr += io->len + unaligned;
+    io->remainder_len = (0x200 - unaligned) & 0x1ff;
+    MACIO_DPRINTF("set remainder to: %d\n", io->remainder_len);
+
+    /* We would read no data from the block layer, thus not get a callback.
+       Just fake completion manually. */
+    if (!io->len) {
+        pmac_ide_transfer_cb(opaque, 0);
+        return;
+    }
+
     io->len = 0;
 
     MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n",
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 8ad1b6e..ad4d826 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -39,6 +39,9 @@ struct DBDMA_io {
     DBDMA_end dma_end;
     /* DMA is in progress, don't start another one */
     int processing;
+    /* unaligned last sector of a request */
+    uint8_t remainder[0x200];
+    int remainder_len;
 };
 
 /*
-- 
1.8.1.4

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

* Re: [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io Alexander Graf
@ 2013-06-30  6:33   ` Andreas Färber
  2013-06-30 23:18     ` Alexander Graf
  0 siblings, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2013-06-30  6:33 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel

Am 30.06.2013 03:26, schrieb Alexander Graf:
> On a real G3 Beige the secondary IDE bus lives on the mac-io chip, not
> on some random PCI device. Move it there to become more compatible.
> 
> While at it, also clean up the IDE channel connection logic.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v1 -> v2:
> 
>   - fix IRQ mapping
> ---
>  hw/ide/macio.c        |  2 +-
>  hw/misc/macio/macio.c | 95 +++++++++++++++++++++++++++++----------------------
>  hw/ppc/mac_oldworld.c | 17 +++++----
>  3 files changed, 64 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index a1952b0..7a1c573 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -363,7 +363,7 @@ static void macio_ide_register_types(void)
>      type_register_static(&macio_ide_type_info);
>  }
>  
> -/* hd_table must contain 4 block drivers */
> +/* hd_table must contain 2 block drivers */
>  void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
>  {
>      int i;
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index fd4c8e5..d9971e2 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -51,11 +51,9 @@ typedef struct OldWorldMacIOState {
>      /*< private >*/
>      MacIOState parent_obj;
>      /*< public >*/
> -
> -    qemu_irq irqs[3];
> -
> +    qemu_irq irqs[5];
>      MacIONVRAMState nvram;
> -    MACIOIDEState ide;
> +    MACIOIDEState ide[2];
>  } OldWorldMacIOState;

I had asked you to please keep my spacing: It separates parent field,
local fields (IRQs) and child fields.

>  
>  #define NEWWORLD_MACIO(obj) \
> @@ -147,18 +145,32 @@ static int macio_common_initfn(PCIDevice *d)
>      return 0;
>  }
>  
> +static int macio_initfn_ide(MacIOState *s, MACIOIDEState *ide, qemu_irq irq0,
> +                            qemu_irq irq1, int dmaid)

macio_realize_ide would be a better name since PCIDevices still use
their legacy "initfn".

Otherwise looks good.

Andreas

> +{
> +    SysBusDevice *sysbus_dev;
> +
> +    sysbus_dev = SYS_BUS_DEVICE(ide);
> +    sysbus_connect_irq(sysbus_dev, 0, irq0);
> +    sysbus_connect_irq(sysbus_dev, 1, irq1);
> +    macio_ide_register_dma(ide, s->dbdma, dmaid);
> +    return qdev_init(DEVICE(ide));
> +}
> +
>  static int macio_oldworld_initfn(PCIDevice *d)
>  {
>      MacIOState *s = MACIO(d);
>      OldWorldMacIOState *os = OLDWORLD_MACIO(d);
>      SysBusDevice *sysbus_dev;
> +    int i;
> +    int cur_irq = 0;
>      int ret = macio_common_initfn(d);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
> -    sysbus_connect_irq(sysbus_dev, 0, os->irqs[0]);
> +    sysbus_connect_irq(sysbus_dev, 0, os->irqs[cur_irq++]);
>  
>      ret = qdev_init(DEVICE(&os->nvram));
>      if (ret < 0) {
> @@ -174,23 +186,39 @@ static int macio_oldworld_initfn(PCIDevice *d)
>          memory_region_add_subregion(&s->bar, 0x00000, s->pic_mem);
>      }
>  
> -    sysbus_dev = SYS_BUS_DEVICE(&os->ide);
> -    sysbus_connect_irq(sysbus_dev, 0, os->irqs[1]);
> -    sysbus_connect_irq(sysbus_dev, 1, os->irqs[2]);
> -    macio_ide_register_dma(&os->ide, s->dbdma, 0x16);
> -    ret = qdev_init(DEVICE(&os->ide));
> -    if (ret < 0) {
> -        return ret;
> +    /* IDE buses */
> +    for (i = 0; i < ARRAY_SIZE(os->ide); i++) {
> +        qemu_irq irq0 = os->irqs[cur_irq++];
> +        qemu_irq irq1 = os->irqs[cur_irq++];
> +
> +        ret = macio_initfn_ide(s, &os->ide[i], irq0, irq1, 0x16 + (i * 4));
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      return 0;
>  }
>  
> +static void macio_init_ide(MacIOState *s, MACIOIDEState *ide, int index)
> +{
> +    gchar *name;
> +
> +    object_initialize(ide, TYPE_MACIO_IDE);
> +    qdev_set_parent_bus(DEVICE(ide), sysbus_get_default());
> +    memory_region_add_subregion(&s->bar, 0x1f000 + ((index + 1) * 0x1000),
> +                                &ide->mem);
> +    name = g_strdup_printf("ide[%i]", index);
> +    object_property_add_child(OBJECT(s), name, OBJECT(ide), NULL);
> +    g_free(name);
> +}
> +
>  static void macio_oldworld_init(Object *obj)
>  {
>      MacIOState *s = MACIO(obj);
>      OldWorldMacIOState *os = OLDWORLD_MACIO(obj);
>      DeviceState *dev;
> +    int i;
>  
>      qdev_init_gpio_out(DEVICE(obj), os->irqs, ARRAY_SIZE(os->irqs));
>  
> @@ -199,10 +227,9 @@ static void macio_oldworld_init(Object *obj)
>      qdev_prop_set_uint32(dev, "size", 0x2000);
>      qdev_prop_set_uint32(dev, "it_shift", 4);
>  
> -    object_initialize(&os->ide, TYPE_MACIO_IDE);
> -    qdev_set_parent_bus(DEVICE(&os->ide), sysbus_get_default());
> -    memory_region_add_subregion(&s->bar, 0x1f000 + (1 * 0x1000), &os->ide.mem);
> -    object_property_add_child(obj, "ide", OBJECT(&os->ide), NULL);
> +    for (i = 0; i < 2; i++) {
> +        macio_init_ide(s, &os->ide[i], i);
> +    }
>  }
>  
>  static int macio_newworld_initfn(PCIDevice *d)
> @@ -210,35 +237,30 @@ static int macio_newworld_initfn(PCIDevice *d)
>      MacIOState *s = MACIO(d);
>      NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
>      SysBusDevice *sysbus_dev;
> +    int i;
> +    int cur_irq = 0;
>      int ret = macio_common_initfn(d);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
> -    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[0]);
> +    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[cur_irq++]);
>  
>      if (s->pic_mem) {
>          /* OpenPIC */
>          memory_region_add_subregion(&s->bar, 0x40000, s->pic_mem);
>      }
>  
> -    sysbus_dev = SYS_BUS_DEVICE(&ns->ide[0]);
> -    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[1]);
> -    sysbus_connect_irq(sysbus_dev, 1, ns->irqs[2]);
> -    macio_ide_register_dma(&ns->ide[0], s->dbdma, 0x16);
> -    ret = qdev_init(DEVICE(&ns->ide[0]));
> -    if (ret < 0) {
> -        return ret;
> -    }
> +    /* IDE buses */
> +    for (i = 0; i < ARRAY_SIZE(ns->ide); i++) {
> +        qemu_irq irq0 = ns->irqs[cur_irq++];
> +        qemu_irq irq1 = ns->irqs[cur_irq++];
>  
> -    sysbus_dev = SYS_BUS_DEVICE(&ns->ide[1]);
> -    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[3]);
> -    sysbus_connect_irq(sysbus_dev, 1, ns->irqs[4]);
> -    macio_ide_register_dma(&ns->ide[1], s->dbdma, 0x1a);
> -    ret = qdev_init(DEVICE(&ns->ide[1]));
> -    if (ret < 0) {
> -        return ret;
> +        ret = macio_initfn_ide(s, &ns->ide[i], irq0, irq1, 0x16 + (i * 4));
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      return 0;
> @@ -249,18 +271,11 @@ static void macio_newworld_init(Object *obj)
>      MacIOState *s = MACIO(obj);
>      NewWorldMacIOState *ns = NEWWORLD_MACIO(obj);
>      int i;
> -    gchar *name;
>  
>      qdev_init_gpio_out(DEVICE(obj), ns->irqs, ARRAY_SIZE(ns->irqs));
>  
>      for (i = 0; i < 2; i++) {
> -        object_initialize(&ns->ide[i], TYPE_MACIO_IDE);
> -        qdev_set_parent_bus(DEVICE(&ns->ide[i]), sysbus_get_default());
> -        memory_region_add_subregion(&s->bar, 0x1f000 + ((i + 1) * 0x1000),
> -                                    &ns->ide[i].mem);
> -        name = g_strdup_printf("ide[%i]", i);
> -        object_property_add_child(obj, name, OBJECT(&ns->ide[i]), NULL);
> -        g_free(name);
> +        macio_init_ide(s, &ns->ide[i], i);
>      }
>  }
>  
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 2aab54c..7422eb9 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -268,20 +268,19 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
>      macio = pci_create(pci_bus, -1, TYPE_OLDWORLD_MACIO);
>      dev = DEVICE(macio);
>      qdev_connect_gpio_out(dev, 0, pic[0x12]); /* CUDA */
> -    qdev_connect_gpio_out(dev, 1, pic[0x0D]); /* IDE */
> -    qdev_connect_gpio_out(dev, 2, pic[0x02]); /* IDE DMA */
> +    qdev_connect_gpio_out(dev, 1, pic[0x0D]); /* IDE-0 */
> +    qdev_connect_gpio_out(dev, 2, pic[0x02]); /* IDE-0 DMA */
> +    qdev_connect_gpio_out(dev, 3, pic[0x0E]); /* IDE-1 */
> +    qdev_connect_gpio_out(dev, 4, pic[0x03]); /* IDE-1 DMA */
>      macio_init(macio, pic_mem, escc_bar);
>  
> -    /* First IDE channel is a MAC IDE on the MacIO bus */
>      macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
> -                                                        "ide"));
> +                                                        "ide[0]"));
>      macio_ide_init_drives(macio_ide, hd);
>  
> -    /* Second IDE channel is a CMD646 on the PCI bus */
> -    hd[0] = hd[MAX_IDE_DEVS];
> -    hd[1] = hd[MAX_IDE_DEVS + 1];
> -    hd[3] = hd[2] = NULL;
> -    pci_cmd646_ide_init(pci_bus, hd, 0);
> +    macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
> +                                                        "ide[1]"));
> +    macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
>  
>      dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
>      adb_bus = qdev_get_child_bus(dev, "adb.0");
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

* Re: [Qemu-devel] [PATCH 04/15] PPC: dbdma: Replace tabs with spaces
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 04/15] PPC: dbdma: " Alexander Graf
@ 2013-06-30  6:35   ` Andreas Färber
  2013-06-30 11:21     ` Alexander Graf
  0 siblings, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2013-06-30  6:35 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel

Am 30.06.2013 03:26, schrieb Alexander Graf:
> s/^I/        /g on the file with a few manual tweaks to align things.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/misc/macio/mac_dbdma.c | 102 +++++++++++++++++++++++-----------------------
>  1 file changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 2fc7f87..ab174f5 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -85,75 +85,75 @@
>  
>  /* Bits in control and status registers */
>  
> -#define RUN	0x8000
> -#define PAUSE	0x4000
> -#define FLUSH	0x2000
> -#define WAKE	0x1000
> -#define DEAD	0x0800
> -#define ACTIVE	0x0400
> -#define BT	0x0100
> -#define DEVSTAT	0x00ff
> +#define RUN        0x8000
> +#define PAUSE      0x4000
> +#define FLUSH      0x2000
> +#define WAKE       0x1000
> +#define DEAD       0x0800
> +#define ACTIVE     0x0400
> +#define BT         0x0100
> +#define DEVSTAT    0x00ff
>  
>  /*
>   * DBDMA command structure.  These fields are all little-endian!
>   */
>  
>  typedef struct dbdma_cmd {
> -    uint16_t req_count;	  /* requested byte transfer count */
> -    uint16_t command;	  /* command word (has bit-fields) */
> -    uint32_t phy_addr;	  /* physical data address */
> -    uint32_t cmd_dep;	  /* command-dependent field */
> -    uint16_t res_count;	  /* residual count after completion */
> -    uint16_t xfer_status; /* transfer status */
> +    uint16_t req_count;          /* requested byte transfer count */
> +    uint16_t command;            /* command word (has bit-fields) */
> +    uint32_t phy_addr;           /* physical data address */
> +    uint32_t cmd_dep;            /* command-dependent field */
> +    uint16_t res_count;          /* residual count after completion */
> +    uint16_t xfer_status;        /* transfer status */
>  } dbdma_cmd;
>  
>  /* DBDMA command values in command field */
>  
>  #define COMMAND_MASK    0xf000
> -#define OUTPUT_MORE	0x0000	/* transfer memory data to stream */
> -#define OUTPUT_LAST	0x1000	/* ditto followed by end marker */
> -#define INPUT_MORE	0x2000	/* transfer stream data to memory */
> -#define INPUT_LAST	0x3000	/* ditto, expect end marker */
> -#define STORE_WORD	0x4000	/* write word (4 bytes) to device reg */
> -#define LOAD_WORD	0x5000	/* read word (4 bytes) from device reg */
> -#define DBDMA_NOP	0x6000	/* do nothing */
> -#define DBDMA_STOP	0x7000	/* suspend processing */
> +#define OUTPUT_MORE     0x0000        /* transfer memory data to stream */
> +#define OUTPUT_LAST     0x1000        /* ditto followed by end marker */
> +#define INPUT_MORE      0x2000        /* transfer stream data to memory */
> +#define INPUT_LAST      0x3000        /* ditto, expect end marker */
> +#define STORE_WORD      0x4000        /* write word (4 bytes) to device reg */
> +#define LOAD_WORD       0x5000        /* read word (4 bytes) from device reg */
> +#define DBDMA_NOP       0x6000        /* do nothing */
> +#define DBDMA_STOP      0x7000        /* suspend processing */
>  
>  /* Key values in command field */
>  
>  #define KEY_MASK        0x0700
> -#define KEY_STREAM0	0x0000	/* usual data stream */
> -#define KEY_STREAM1	0x0100	/* control/status stream */
> -#define KEY_STREAM2	0x0200	/* device-dependent stream */
> -#define KEY_STREAM3	0x0300	/* device-dependent stream */
> -#define KEY_STREAM4	0x0400	/* reserved */
> -#define KEY_REGS	0x0500	/* device register space */
> -#define KEY_SYSTEM	0x0600	/* system memory-mapped space */
> -#define KEY_DEVICE	0x0700	/* device memory-mapped space */
> +#define KEY_STREAM0     0x0000        /* usual data stream */
> +#define KEY_STREAM1     0x0100        /* control/status stream */
> +#define KEY_STREAM2     0x0200        /* device-dependent stream */
> +#define KEY_STREAM3     0x0300        /* device-dependent stream */
> +#define KEY_STREAM4     0x0400        /* reserved */
> +#define KEY_REGS        0x0500        /* device register space */
> +#define KEY_SYSTEM      0x0600        /* system memory-mapped space */
> +#define KEY_DEVICE      0x0700        /* device memory-mapped space */
>  
>  /* Interrupt control values in command field */
>  
>  #define INTR_MASK       0x0030
> -#define INTR_NEVER	0x0000	/* don't interrupt */
> -#define INTR_IFSET	0x0010	/* intr if condition bit is 1 */
> -#define INTR_IFCLR	0x0020	/* intr if condition bit is 0 */
> -#define INTR_ALWAYS	0x0030	/* always interrupt */
> +#define INTR_NEVER      0x0000        /* don't interrupt */
> +#define INTR_IFSET      0x0010        /* intr if condition bit is 1 */
> +#define INTR_IFCLR      0x0020        /* intr if condition bit is 0 */
> +#define INTR_ALWAYS     0x0030        /* always interrupt */
>  
>  /* Branch control values in command field */
>  
>  #define BR_MASK         0x000c
> -#define BR_NEVER	0x0000	/* don't branch */
> -#define BR_IFSET	0x0004	/* branch if condition bit is 1 */
> -#define BR_IFCLR	0x0008	/* branch if condition bit is 0 */
> -#define BR_ALWAYS	0x000c	/* always branch */
> +#define BR_NEVER        0x0000        /* don't branch */
> +#define BR_IFSET        0x0004        /* branch if condition bit is 1 */
> +#define BR_IFCLR        0x0008        /* branch if condition bit is 0 */
> +#define BR_ALWAYS       0x000c        /* always branch */
>  
>  /* Wait control values in command field */
>  
>  #define WAIT_MASK       0x0003
> -#define WAIT_NEVER	0x0000	/* don't wait */
> -#define WAIT_IFSET	0x0001	/* wait if condition bit is 1 */
> -#define WAIT_IFCLR	0x0002	/* wait if condition bit is 0 */
> -#define WAIT_ALWAYS	0x0003	/* always wait */
> +#define WAIT_NEVER      0x0000        /* don't wait */
> +#define WAIT_IFSET      0x0001        /* wait if condition bit is 1 */
> +#define WAIT_IFCLR      0x0002        /* wait if condition bit is 0 */
> +#define WAIT_ALWAYS     0x0003        /* always wait */
>  
>  typedef struct DBDMA_channel {
>      int channel;
> @@ -558,11 +558,11 @@ static void channel_run(DBDMA_channel *ch)
>      switch (cmd) {
>      case DBDMA_NOP:
>          nop(ch);
> -	return;
> +        return;
>  
>      case DBDMA_STOP:
>          stop(ch);
> -	return;
> +        return;
>      }
>  
>      key = le16_to_cpu(current->command) & 0x0700;
> @@ -578,19 +578,19 @@ static void channel_run(DBDMA_channel *ch)
>      switch (cmd) {
>      case OUTPUT_MORE:
>          start_output(ch, key, phy_addr, req_count, 0);
> -	return;
> +        return;
>  
>      case OUTPUT_LAST:
>          start_output(ch, key, phy_addr, req_count, 1);
> -	return;
> +        return;
>  
>      case INPUT_MORE:
>          start_input(ch, key, phy_addr, req_count, 0);
> -	return;
> +        return;
>  
>      case INPUT_LAST:
>          start_input(ch, key, phy_addr, req_count, 1);
> -	return;
> +        return;
>      }
>  
>      if (key < KEY_REGS) {
> @@ -615,11 +615,11 @@ static void channel_run(DBDMA_channel *ch)
>      switch (cmd) {
>      case LOAD_WORD:
>          load_word(ch, key, phy_addr, req_count);
> -	return;
> +        return;
>  
>      case STORE_WORD:
>          store_word(ch, key, phy_addr, req_count);
> -	return;
> +        return;
>      }
>  }
>  
> @@ -720,7 +720,7 @@ static void dbdma_write(void *opaque, hwaddr addr,
>  
>      if (reg == DBDMA_CMDPTR_LO &&
>          (ch->regs[DBDMA_STATUS] & (RUN | ACTIVE)))
> -	return;
> +        return;

Add braces while at it? Otherwise fine.

Andreas

>  
>      ch->regs[reg] = value;
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

* Re: [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code
  2013-06-30  1:26 ` [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code Alexander Graf
@ 2013-06-30  6:42   ` Andreas Färber
  2013-06-30 23:30     ` Alexander Graf
  0 siblings, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2013-06-30  6:42 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel

Am 30.06.2013 03:26, schrieb Alexander Graf:
> The macio code is basically undebuggable as it stands today, with no
> debug prints anywhere whatsoever. DBDMA was better, but I needed a
> few more to create reasonable logs that tell me where breakage is.
> 
> Add a DPRINTF macro in the macio source file and add a bunch of debug
> prints that are all disabled by default of course.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ide/macio.c            | 39 ++++++++++++++++++++++++++++++++++++++-
>  hw/misc/macio/mac_dbdma.c | 12 ++++++++++--
>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 82409dc..5cbc923 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -30,6 +30,17 @@
>  
>  #include <hw/ide/internal.h>
>  
> +/* debug MACIO */
> +// #define DEBUG_MACIO
> +
> +#ifdef DEBUG_MACIO
> +#define MACIO_DPRINTF(fmt, ...)                                 \
> +    do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0)
> +#else
> +#define MACIO_DPRINTF(fmt, ...)
> +#endif

Please use the pattern you suggested yourself of having an if
(DEBUG_MACIO_ENABLED) {...} inside the macro rather than a second
MACIO_DPRINTF(), so that the newly added debug output doesn'T bitrot.

Andreas

> +
> +
>  /***********************************************************/
>  /* MacIO based PowerPC IDE */
>  
> @@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>          goto done;
>      }
>  
> +    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
> +
>      if (s->io_buffer_size > 0) {
>          m->aiocb = NULL;
>          qemu_sglist_destroy(&s->sg);
> @@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>          s->io_buffer_index &= 0x7ff;
>      }
>  
> -    if (s->packet_transfer_size <= 0)
> +    /* end of transfer ? */
> +    if (s->packet_transfer_size <= 0) {
> +        MACIO_DPRINTF("end of transfer\n");
>          ide_atapi_cmd_ok(s);
> +    }
>  
> +    /* end of DMA ? */
>      if (io->len == 0) {
> +        MACIO_DPRINTF("end of DMA\n");
>          goto done;
>      }

Both comments duplicate your debug output module question mark. :)

>  
>      /* launch next transfer */
>  
> +    MACIO_DPRINTF("io->len = %#x\n", io->len);
> +
>      s->io_buffer_size = io->len;
>  
>      qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
> @@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>      io->addr += io->len;
>      io->len = 0;
>  
> +    MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
> +                  (s->lba << 2) + (s->io_buffer_index >> 9),
> +                  s->packet_transfer_size, s->dma_cmd);
> +
>      m->aiocb = dma_bdrv_read(s->bs, &s->sg,
>                               (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9),
>                               pmac_ide_atapi_transfer_cb, io);
>      return;
>  
>  done:
> +    MACIO_DPRINTF("done DMA\n");
>      bdrv_acct_done(s->bs, &s->acct);
>      io->dma_end(opaque);
>  }
> @@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>      int64_t sector_num;
>  
>      if (ret < 0) {
> +        MACIO_DPRINTF("DMA error\n");
>          m->aiocb = NULL;
>          qemu_sglist_destroy(&s->sg);
>          ide_dma_error(s);
> @@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>      }
>  
>      sector_num = ide_get_sector(s);
> +    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
>      if (s->io_buffer_size > 0) {
>          m->aiocb = NULL;
>          qemu_sglist_destroy(&s->sg);
> @@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>  
>      /* end of transfer ? */
>      if (s->nsector == 0) {
> +        MACIO_DPRINTF("end of transfer\n");
>          s->status = READY_STAT | SEEK_STAT;
>          ide_set_irq(s->bus);
>      }
>  
>      /* end of DMA ? */
>      if (io->len == 0) {
> +        MACIO_DPRINTF("end of DMA\n");
>          goto done;
>      }
>  
> @@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>      s->io_buffer_index = 0;
>      s->io_buffer_size = io->len;
>  
> +

Intentionally two white lines?

> +    MACIO_DPRINTF("io->len = %#x\n", io->len);
> +
>      qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>                       &address_space_memory);
>      qemu_sglist_add(&s->sg, io->addr, io->len);
>      io->addr += io->len;
>      io->len = 0;
>  
> +    MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n",
> +                  sector_num, n, s->nsector, s->dma_cmd);
> +
>      switch (s->dma_cmd) {
>      case IDE_DMA_READ:
>          m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
> @@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io)
>      MACIOIDEState *m = io->opaque;
>      IDEState *s = idebus_active_if(&m->bus);
>  
> +    MACIO_DPRINTF("\n", __LINE__);

The argument is unused.

> +
>      s->io_buffer_size = 0;
>      if (s->drive_kind == IDE_CD) {
>          bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index ab174f5..903604d 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
>          return;
>      case INTR_ALWAYS: /* always interrupt */
>          qemu_irq_raise(ch->irq);
> +        DBDMA_DPRINTF("conditional_interrupt: raise\n");

Use %s and __func__ in case we ever rename it? More instances below. For
dbdma_end() you did.

>          return;
>      }
>  
> @@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch)
>  
>      switch(intr) {
>      case INTR_IFSET:  /* intr if condition bit is 1 */
> -        if (cond)
> +        if (cond) {
>              qemu_irq_raise(ch->irq);
> +            DBDMA_DPRINTF("conditional_interrupt: raise\n");
> +        }
>          return;
>      case INTR_IFCLR:  /* intr if condition bit is 0 */
> -        if (!cond)
> +        if (!cond) {
>              qemu_irq_raise(ch->irq);
> +            DBDMA_DPRINTF("conditional_interrupt: raise\n");
> +        }
>          return;
>      }
>  }
> @@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io)
>      DBDMA_channel *ch = io->channel;
>      dbdma_cmd *current = &ch->current;
>  
> +    DBDMA_DPRINTF("%s\n", __func__, __LINE__);

Unused argument.

> +
>      if (conditional_wait(ch))
>          goto wait;
>  
> @@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
>       * are not implemented in the mac-io chip
>       */
>  
> +    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);

PRIx32 for addr

>      if (!addr || key > KEY_STREAM3) {
>          kill_channel(ch);
>          return;

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

* Re: [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io
  2013-06-30  1:27 ` [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io Alexander Graf
@ 2013-06-30  6:48   ` Andreas Färber
  2013-06-30 23:35     ` Alexander Graf
  0 siblings, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2013-06-30  6:48 UTC (permalink / raw)
  To: Alexander Graf
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel

Am 30.06.2013 03:27, schrieb Alexander Graf:
> Soon we will introduce intermediate processing pauses which will
> allow the bottom half to restart a DMA request that couldn't be
> fulfilled yet.
> 
> For that to work, move the processing variable into the io struct
> which is what DMA providers work with.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/misc/macio/mac_dbdma.c  | 10 ++++++----
>  include/hw/ppc/mac_dbdma.h |  3 ++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 324ac54..91b9eaf 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -275,7 +275,9 @@ static void dbdma_end(DBDMA_io *io)
>      conditional_branch(ch);
>  
>  wait:
> -    ch->processing = 0;
> +    /* Indicate that we're ready for a new DMA round */
> +    ch->io.processing = 0;
> +
>      if ((ch->regs[DBDMA_STATUS] & RUN) &&
>          (ch->regs[DBDMA_STATUS] & ACTIVE))
>          channel_run(ch);
> @@ -301,7 +303,7 @@ static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
>      ch->io.is_last = is_last;
>      ch->io.dma_end = dbdma_end;
>      ch->io.is_dma_out = 1;
> -    ch->processing = 1;
> +    ch->io.processing = 1;
>      if (ch->rw) {
>          ch->rw(&ch->io);
>      }
> @@ -327,7 +329,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
>      ch->io.is_last = is_last;
>      ch->io.dma_end = dbdma_end;
>      ch->io.is_dma_out = 0;
> -    ch->processing = 1;
> +    ch->io.processing = 1;
>      if (ch->rw) {
>          ch->rw(&ch->io);
>      }
> @@ -525,7 +527,7 @@ static void DBDMA_run(DBDMAState *s)
>      for (channel = 0; channel < DBDMA_CHANNELS; channel++) {
>          DBDMA_channel *ch = &s->channels[channel];
>          uint32_t status = ch->regs[DBDMA_STATUS];
> -        if (!ch->processing && (status & RUN) && (status & ACTIVE)) {
> +        if (!ch->io.processing && (status & RUN) && (status & ACTIVE)) {
>              channel_run(ch);
>          }
>      }
> diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
> index eb8e0f0..8ad1b6e 100644
> --- a/include/hw/ppc/mac_dbdma.h
> +++ b/include/hw/ppc/mac_dbdma.h
> @@ -37,6 +37,8 @@ struct DBDMA_io {
>      int is_last;
>      int is_dma_out;
>      DBDMA_end dma_end;
> +    /* DMA is in progress, don't start another one */
> +    int processing;

Can it be changed to bool (its users to true/false) while at it?

Andreas

>  };
>  
>  /*
> @@ -148,7 +150,6 @@ typedef struct DBDMA_channel {
>      DBDMA_rw rw;
>      DBDMA_flush flush;
>      dbdma_cmd current;
> -    int processing;
>  } DBDMA_channel;
>  
>  typedef struct {

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

* Re: [Qemu-devel] [PATCH 04/15] PPC: dbdma: Replace tabs with spaces
  2013-06-30  6:35   ` Andreas Färber
@ 2013-06-30 11:21     ` Alexander Graf
  0 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30 11:21 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel



Am 30.06.2013 um 08:35 schrieb Andreas Färber <afaerber@suse.de>:

> Am 30.06.2013 03:26, schrieb Alexander Graf:
>> s/^I/        /g on the file with a few manual tweaks to align things.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/misc/macio/mac_dbdma.c | 102 +++++++++++++++++++++++-----------------------
>> 1 file changed, 51 insertions(+), 51 deletions(-)
>> 
>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>> index 2fc7f87..ab174f5 100644
>> --- a/hw/misc/macio/mac_dbdma.c
>> +++ b/hw/misc/macio/mac_dbdma.c
>> @@ -85,75 +85,75 @@
>> 
>> /* Bits in control and status registers */
>> 
>> -#define RUN    0x8000
>> -#define PAUSE    0x4000
>> -#define FLUSH    0x2000
>> -#define WAKE    0x1000
>> -#define DEAD    0x0800
>> -#define ACTIVE    0x0400
>> -#define BT    0x0100
>> -#define DEVSTAT    0x00ff
>> +#define RUN        0x8000
>> +#define PAUSE      0x4000
>> +#define FLUSH      0x2000
>> +#define WAKE       0x1000
>> +#define DEAD       0x0800
>> +#define ACTIVE     0x0400
>> +#define BT         0x0100
>> +#define DEVSTAT    0x00ff
>> 
>> /*
>>  * DBDMA command structure.  These fields are all little-endian!
>>  */
>> 
>> typedef struct dbdma_cmd {
>> -    uint16_t req_count;      /* requested byte transfer count */
>> -    uint16_t command;      /* command word (has bit-fields) */
>> -    uint32_t phy_addr;      /* physical data address */
>> -    uint32_t cmd_dep;      /* command-dependent field */
>> -    uint16_t res_count;      /* residual count after completion */
>> -    uint16_t xfer_status; /* transfer status */
>> +    uint16_t req_count;          /* requested byte transfer count */
>> +    uint16_t command;            /* command word (has bit-fields) */
>> +    uint32_t phy_addr;           /* physical data address */
>> +    uint32_t cmd_dep;            /* command-dependent field */
>> +    uint16_t res_count;          /* residual count after completion */
>> +    uint16_t xfer_status;        /* transfer status */
>> } dbdma_cmd;
>> 
>> /* DBDMA command values in command field */
>> 
>> #define COMMAND_MASK    0xf000
>> -#define OUTPUT_MORE    0x0000    /* transfer memory data to stream */
>> -#define OUTPUT_LAST    0x1000    /* ditto followed by end marker */
>> -#define INPUT_MORE    0x2000    /* transfer stream data to memory */
>> -#define INPUT_LAST    0x3000    /* ditto, expect end marker */
>> -#define STORE_WORD    0x4000    /* write word (4 bytes) to device reg */
>> -#define LOAD_WORD    0x5000    /* read word (4 bytes) from device reg */
>> -#define DBDMA_NOP    0x6000    /* do nothing */
>> -#define DBDMA_STOP    0x7000    /* suspend processing */
>> +#define OUTPUT_MORE     0x0000        /* transfer memory data to stream */
>> +#define OUTPUT_LAST     0x1000        /* ditto followed by end marker */
>> +#define INPUT_MORE      0x2000        /* transfer stream data to memory */
>> +#define INPUT_LAST      0x3000        /* ditto, expect end marker */
>> +#define STORE_WORD      0x4000        /* write word (4 bytes) to device reg */
>> +#define LOAD_WORD       0x5000        /* read word (4 bytes) from device reg */
>> +#define DBDMA_NOP       0x6000        /* do nothing */
>> +#define DBDMA_STOP      0x7000        /* suspend processing */
>> 
>> /* Key values in command field */
>> 
>> #define KEY_MASK        0x0700
>> -#define KEY_STREAM0    0x0000    /* usual data stream */
>> -#define KEY_STREAM1    0x0100    /* control/status stream */
>> -#define KEY_STREAM2    0x0200    /* device-dependent stream */
>> -#define KEY_STREAM3    0x0300    /* device-dependent stream */
>> -#define KEY_STREAM4    0x0400    /* reserved */
>> -#define KEY_REGS    0x0500    /* device register space */
>> -#define KEY_SYSTEM    0x0600    /* system memory-mapped space */
>> -#define KEY_DEVICE    0x0700    /* device memory-mapped space */
>> +#define KEY_STREAM0     0x0000        /* usual data stream */
>> +#define KEY_STREAM1     0x0100        /* control/status stream */
>> +#define KEY_STREAM2     0x0200        /* device-dependent stream */
>> +#define KEY_STREAM3     0x0300        /* device-dependent stream */
>> +#define KEY_STREAM4     0x0400        /* reserved */
>> +#define KEY_REGS        0x0500        /* device register space */
>> +#define KEY_SYSTEM      0x0600        /* system memory-mapped space */
>> +#define KEY_DEVICE      0x0700        /* device memory-mapped space */
>> 
>> /* Interrupt control values in command field */
>> 
>> #define INTR_MASK       0x0030
>> -#define INTR_NEVER    0x0000    /* don't interrupt */
>> -#define INTR_IFSET    0x0010    /* intr if condition bit is 1 */
>> -#define INTR_IFCLR    0x0020    /* intr if condition bit is 0 */
>> -#define INTR_ALWAYS    0x0030    /* always interrupt */
>> +#define INTR_NEVER      0x0000        /* don't interrupt */
>> +#define INTR_IFSET      0x0010        /* intr if condition bit is 1 */
>> +#define INTR_IFCLR      0x0020        /* intr if condition bit is 0 */
>> +#define INTR_ALWAYS     0x0030        /* always interrupt */
>> 
>> /* Branch control values in command field */
>> 
>> #define BR_MASK         0x000c
>> -#define BR_NEVER    0x0000    /* don't branch */
>> -#define BR_IFSET    0x0004    /* branch if condition bit is 1 */
>> -#define BR_IFCLR    0x0008    /* branch if condition bit is 0 */
>> -#define BR_ALWAYS    0x000c    /* always branch */
>> +#define BR_NEVER        0x0000        /* don't branch */
>> +#define BR_IFSET        0x0004        /* branch if condition bit is 1 */
>> +#define BR_IFCLR        0x0008        /* branch if condition bit is 0 */
>> +#define BR_ALWAYS       0x000c        /* always branch */
>> 
>> /* Wait control values in command field */
>> 
>> #define WAIT_MASK       0x0003
>> -#define WAIT_NEVER    0x0000    /* don't wait */
>> -#define WAIT_IFSET    0x0001    /* wait if condition bit is 1 */
>> -#define WAIT_IFCLR    0x0002    /* wait if condition bit is 0 */
>> -#define WAIT_ALWAYS    0x0003    /* always wait */
>> +#define WAIT_NEVER      0x0000        /* don't wait */
>> +#define WAIT_IFSET      0x0001        /* wait if condition bit is 1 */
>> +#define WAIT_IFCLR      0x0002        /* wait if condition bit is 0 */
>> +#define WAIT_ALWAYS     0x0003        /* always wait */
>> 
>> typedef struct DBDMA_channel {
>>     int channel;
>> @@ -558,11 +558,11 @@ static void channel_run(DBDMA_channel *ch)
>>     switch (cmd) {
>>     case DBDMA_NOP:
>>         nop(ch);
>> -    return;
>> +        return;
>> 
>>     case DBDMA_STOP:
>>         stop(ch);
>> -    return;
>> +        return;
>>     }
>> 
>>     key = le16_to_cpu(current->command) & 0x0700;
>> @@ -578,19 +578,19 @@ static void channel_run(DBDMA_channel *ch)
>>     switch (cmd) {
>>     case OUTPUT_MORE:
>>         start_output(ch, key, phy_addr, req_count, 0);
>> -    return;
>> +        return;
>> 
>>     case OUTPUT_LAST:
>>         start_output(ch, key, phy_addr, req_count, 1);
>> -    return;
>> +        return;
>> 
>>     case INPUT_MORE:
>>         start_input(ch, key, phy_addr, req_count, 0);
>> -    return;
>> +        return;
>> 
>>     case INPUT_LAST:
>>         start_input(ch, key, phy_addr, req_count, 1);
>> -    return;
>> +        return;
>>     }
>> 
>>     if (key < KEY_REGS) {
>> @@ -615,11 +615,11 @@ static void channel_run(DBDMA_channel *ch)
>>     switch (cmd) {
>>     case LOAD_WORD:
>>         load_word(ch, key, phy_addr, req_count);
>> -    return;
>> +        return;
>> 
>>     case STORE_WORD:
>>         store_word(ch, key, phy_addr, req_count);
>> -    return;
>> +        return;
>>     }
>> }
>> 
>> @@ -720,7 +720,7 @@ static void dbdma_write(void *opaque, hwaddr addr,
>> 
>>     if (reg == DBDMA_CMDPTR_LO &&
>>         (ch->regs[DBDMA_STATUS] & (RUN | ACTIVE)))
>> -    return;
>> +        return;
> 
> Add braces while at it? Otherwise fine.

This patch is supposed to be mechanical. I've added braces at this spot in a later patch, when I actually touched the code.

Alex

> 
> Andreas
> 
>> 
>>     ch->regs[reg] = value;
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

* Re: [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io
  2013-06-30  6:33   ` Andreas Färber
@ 2013-06-30 23:18     ` Alexander Graf
  0 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30 23:18 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel


On 30.06.2013, at 08:33, Andreas Färber wrote:

> Am 30.06.2013 03:26, schrieb Alexander Graf:
>> On a real G3 Beige the secondary IDE bus lives on the mac-io chip, not
>> on some random PCI device. Move it there to become more compatible.
>> 
>> While at it, also clean up the IDE channel connection logic.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> 
>> ---
>> 
>> v1 -> v2:
>> 
>>  - fix IRQ mapping
>> ---
>> hw/ide/macio.c        |  2 +-
>> hw/misc/macio/macio.c | 95 +++++++++++++++++++++++++++++----------------------
>> hw/ppc/mac_oldworld.c | 17 +++++----
>> 3 files changed, 64 insertions(+), 50 deletions(-)
>> 
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index a1952b0..7a1c573 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -363,7 +363,7 @@ static void macio_ide_register_types(void)
>>     type_register_static(&macio_ide_type_info);
>> }
>> 
>> -/* hd_table must contain 4 block drivers */
>> +/* hd_table must contain 2 block drivers */
>> void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
>> {
>>     int i;
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index fd4c8e5..d9971e2 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -51,11 +51,9 @@ typedef struct OldWorldMacIOState {
>>     /*< private >*/
>>     MacIOState parent_obj;
>>     /*< public >*/
>> -
>> -    qemu_irq irqs[3];
>> -
>> +    qemu_irq irqs[5];
>>     MacIONVRAMState nvram;
>> -    MACIOIDEState ide;
>> +    MACIOIDEState ide[2];
>> } OldWorldMacIOState;
> 
> I had asked you to please keep my spacing: It separates parent field,
> local fields (IRQs) and child fields.

Ok, fixed. If it's so dear to you, please send a follow-up patch fixing struct NewWorldMacIOState then.

> 
>> 
>> #define NEWWORLD_MACIO(obj) \
>> @@ -147,18 +145,32 @@ static int macio_common_initfn(PCIDevice *d)
>>     return 0;
>> }
>> 
>> +static int macio_initfn_ide(MacIOState *s, MACIOIDEState *ide, qemu_irq irq0,
>> +                            qemu_irq irq1, int dmaid)
> 
> macio_realize_ide would be a better name since PCIDevices still use
> their legacy "initfn".

I've tried to keep the naming consistent. macio_initfn_ide gets called from macio_oldworld_initfn and macio_newworld_initfn. I'd find a sudden naming switch to "realize" utterly confusing.


Alex

> 
> Otherwise looks good.
> 
> Andreas
> 
>> +{
>> +    SysBusDevice *sysbus_dev;
>> +
>> +    sysbus_dev = SYS_BUS_DEVICE(ide);
>> +    sysbus_connect_irq(sysbus_dev, 0, irq0);
>> +    sysbus_connect_irq(sysbus_dev, 1, irq1);
>> +    macio_ide_register_dma(ide, s->dbdma, dmaid);
>> +    return qdev_init(DEVICE(ide));
>> +}
>> +
>> static int macio_oldworld_initfn(PCIDevice *d)
>> {
>>     MacIOState *s = MACIO(d);
>>     OldWorldMacIOState *os = OLDWORLD_MACIO(d);
>>     SysBusDevice *sysbus_dev;
>> +    int i;
>> +    int cur_irq = 0;
>>     int ret = macio_common_initfn(d);
>>     if (ret < 0) {
>>         return ret;
>>     }
>> 
>>     sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
>> -    sysbus_connect_irq(sysbus_dev, 0, os->irqs[0]);
>> +    sysbus_connect_irq(sysbus_dev, 0, os->irqs[cur_irq++]);
>> 
>>     ret = qdev_init(DEVICE(&os->nvram));
>>     if (ret < 0) {
>> @@ -174,23 +186,39 @@ static int macio_oldworld_initfn(PCIDevice *d)
>>         memory_region_add_subregion(&s->bar, 0x00000, s->pic_mem);
>>     }
>> 
>> -    sysbus_dev = SYS_BUS_DEVICE(&os->ide);
>> -    sysbus_connect_irq(sysbus_dev, 0, os->irqs[1]);
>> -    sysbus_connect_irq(sysbus_dev, 1, os->irqs[2]);
>> -    macio_ide_register_dma(&os->ide, s->dbdma, 0x16);
>> -    ret = qdev_init(DEVICE(&os->ide));
>> -    if (ret < 0) {
>> -        return ret;
>> +    /* IDE buses */
>> +    for (i = 0; i < ARRAY_SIZE(os->ide); i++) {
>> +        qemu_irq irq0 = os->irqs[cur_irq++];
>> +        qemu_irq irq1 = os->irqs[cur_irq++];
>> +
>> +        ret = macio_initfn_ide(s, &os->ide[i], irq0, irq1, 0x16 + (i * 4));
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>     }
>> 
>>     return 0;
>> }
>> 
>> +static void macio_init_ide(MacIOState *s, MACIOIDEState *ide, int index)
>> +{
>> +    gchar *name;
>> +
>> +    object_initialize(ide, TYPE_MACIO_IDE);
>> +    qdev_set_parent_bus(DEVICE(ide), sysbus_get_default());
>> +    memory_region_add_subregion(&s->bar, 0x1f000 + ((index + 1) * 0x1000),
>> +                                &ide->mem);
>> +    name = g_strdup_printf("ide[%i]", index);
>> +    object_property_add_child(OBJECT(s), name, OBJECT(ide), NULL);
>> +    g_free(name);
>> +}
>> +
>> static void macio_oldworld_init(Object *obj)
>> {
>>     MacIOState *s = MACIO(obj);
>>     OldWorldMacIOState *os = OLDWORLD_MACIO(obj);
>>     DeviceState *dev;
>> +    int i;
>> 
>>     qdev_init_gpio_out(DEVICE(obj), os->irqs, ARRAY_SIZE(os->irqs));
>> 
>> @@ -199,10 +227,9 @@ static void macio_oldworld_init(Object *obj)
>>     qdev_prop_set_uint32(dev, "size", 0x2000);
>>     qdev_prop_set_uint32(dev, "it_shift", 4);
>> 
>> -    object_initialize(&os->ide, TYPE_MACIO_IDE);
>> -    qdev_set_parent_bus(DEVICE(&os->ide), sysbus_get_default());
>> -    memory_region_add_subregion(&s->bar, 0x1f000 + (1 * 0x1000), &os->ide.mem);
>> -    object_property_add_child(obj, "ide", OBJECT(&os->ide), NULL);
>> +    for (i = 0; i < 2; i++) {
>> +        macio_init_ide(s, &os->ide[i], i);
>> +    }
>> }
>> 
>> static int macio_newworld_initfn(PCIDevice *d)
>> @@ -210,35 +237,30 @@ static int macio_newworld_initfn(PCIDevice *d)
>>     MacIOState *s = MACIO(d);
>>     NewWorldMacIOState *ns = NEWWORLD_MACIO(d);
>>     SysBusDevice *sysbus_dev;
>> +    int i;
>> +    int cur_irq = 0;
>>     int ret = macio_common_initfn(d);
>>     if (ret < 0) {
>>         return ret;
>>     }
>> 
>>     sysbus_dev = SYS_BUS_DEVICE(&s->cuda);
>> -    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[0]);
>> +    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[cur_irq++]);
>> 
>>     if (s->pic_mem) {
>>         /* OpenPIC */
>>         memory_region_add_subregion(&s->bar, 0x40000, s->pic_mem);
>>     }
>> 
>> -    sysbus_dev = SYS_BUS_DEVICE(&ns->ide[0]);
>> -    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[1]);
>> -    sysbus_connect_irq(sysbus_dev, 1, ns->irqs[2]);
>> -    macio_ide_register_dma(&ns->ide[0], s->dbdma, 0x16);
>> -    ret = qdev_init(DEVICE(&ns->ide[0]));
>> -    if (ret < 0) {
>> -        return ret;
>> -    }
>> +    /* IDE buses */
>> +    for (i = 0; i < ARRAY_SIZE(ns->ide); i++) {
>> +        qemu_irq irq0 = ns->irqs[cur_irq++];
>> +        qemu_irq irq1 = ns->irqs[cur_irq++];
>> 
>> -    sysbus_dev = SYS_BUS_DEVICE(&ns->ide[1]);
>> -    sysbus_connect_irq(sysbus_dev, 0, ns->irqs[3]);
>> -    sysbus_connect_irq(sysbus_dev, 1, ns->irqs[4]);
>> -    macio_ide_register_dma(&ns->ide[1], s->dbdma, 0x1a);
>> -    ret = qdev_init(DEVICE(&ns->ide[1]));
>> -    if (ret < 0) {
>> -        return ret;
>> +        ret = macio_initfn_ide(s, &ns->ide[i], irq0, irq1, 0x16 + (i * 4));
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>     }
>> 
>>     return 0;
>> @@ -249,18 +271,11 @@ static void macio_newworld_init(Object *obj)
>>     MacIOState *s = MACIO(obj);
>>     NewWorldMacIOState *ns = NEWWORLD_MACIO(obj);
>>     int i;
>> -    gchar *name;
>> 
>>     qdev_init_gpio_out(DEVICE(obj), ns->irqs, ARRAY_SIZE(ns->irqs));
>> 
>>     for (i = 0; i < 2; i++) {
>> -        object_initialize(&ns->ide[i], TYPE_MACIO_IDE);
>> -        qdev_set_parent_bus(DEVICE(&ns->ide[i]), sysbus_get_default());
>> -        memory_region_add_subregion(&s->bar, 0x1f000 + ((i + 1) * 0x1000),
>> -                                    &ns->ide[i].mem);
>> -        name = g_strdup_printf("ide[%i]", i);
>> -        object_property_add_child(obj, name, OBJECT(&ns->ide[i]), NULL);
>> -        g_free(name);
>> +        macio_init_ide(s, &ns->ide[i], i);
>>     }
>> }
>> 
>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>> index 2aab54c..7422eb9 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -268,20 +268,19 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
>>     macio = pci_create(pci_bus, -1, TYPE_OLDWORLD_MACIO);
>>     dev = DEVICE(macio);
>>     qdev_connect_gpio_out(dev, 0, pic[0x12]); /* CUDA */
>> -    qdev_connect_gpio_out(dev, 1, pic[0x0D]); /* IDE */
>> -    qdev_connect_gpio_out(dev, 2, pic[0x02]); /* IDE DMA */
>> +    qdev_connect_gpio_out(dev, 1, pic[0x0D]); /* IDE-0 */
>> +    qdev_connect_gpio_out(dev, 2, pic[0x02]); /* IDE-0 DMA */
>> +    qdev_connect_gpio_out(dev, 3, pic[0x0E]); /* IDE-1 */
>> +    qdev_connect_gpio_out(dev, 4, pic[0x03]); /* IDE-1 DMA */
>>     macio_init(macio, pic_mem, escc_bar);
>> 
>> -    /* First IDE channel is a MAC IDE on the MacIO bus */
>>     macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
>> -                                                        "ide"));
>> +                                                        "ide[0]"));
>>     macio_ide_init_drives(macio_ide, hd);
>> 
>> -    /* Second IDE channel is a CMD646 on the PCI bus */
>> -    hd[0] = hd[MAX_IDE_DEVS];
>> -    hd[1] = hd[MAX_IDE_DEVS + 1];
>> -    hd[3] = hd[2] = NULL;
>> -    pci_cmd646_ide_init(pci_bus, hd, 0);
>> +    macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
>> +                                                        "ide[1]"));
>> +    macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
>> 
>>     dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
>>     adb_bus = qdev_get_child_bus(dev, "adb.0");
>> 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

* Re: [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code
  2013-06-30  6:42   ` Andreas Färber
@ 2013-06-30 23:30     ` Alexander Graf
  0 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30 23:30 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel


On 30.06.2013, at 08:42, Andreas Färber wrote:

> Am 30.06.2013 03:26, schrieb Alexander Graf:
>> The macio code is basically undebuggable as it stands today, with no
>> debug prints anywhere whatsoever. DBDMA was better, but I needed a
>> few more to create reasonable logs that tell me where breakage is.
>> 
>> Add a DPRINTF macro in the macio source file and add a bunch of debug
>> prints that are all disabled by default of course.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ide/macio.c            | 39 ++++++++++++++++++++++++++++++++++++++-
>> hw/misc/macio/mac_dbdma.c | 12 ++++++++++--
>> 2 files changed, 48 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index 82409dc..5cbc923 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -30,6 +30,17 @@
>> 
>> #include <hw/ide/internal.h>
>> 
>> +/* debug MACIO */
>> +// #define DEBUG_MACIO
>> +
>> +#ifdef DEBUG_MACIO
>> +#define MACIO_DPRINTF(fmt, ...)                                 \
>> +    do { printf("MACIO: %s: " fmt , __func__, ## __VA_ARGS__); } while (0)
>> +#else
>> +#define MACIO_DPRINTF(fmt, ...)
>> +#endif
> 
> Please use the pattern you suggested yourself of having an if
> (DEBUG_MACIO_ENABLED) {...} inside the macro rather than a second
> MACIO_DPRINTF(), so that the newly added debug output doesn'T bitrot.

Very good point. Thanks a lot!


> 
>> +
>> +
>> /***********************************************************/
>> /* MacIO based PowerPC IDE */
>> 
>> @@ -48,6 +59,8 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>>         goto done;
>>     }
>> 
>> +    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
>> +
>>     if (s->io_buffer_size > 0) {
>>         m->aiocb = NULL;
>>         qemu_sglist_destroy(&s->sg);
>> @@ -59,15 +72,22 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>>         s->io_buffer_index &= 0x7ff;
>>     }
>> 
>> -    if (s->packet_transfer_size <= 0)
>> +    /* end of transfer ? */
>> +    if (s->packet_transfer_size <= 0) {
>> +        MACIO_DPRINTF("end of transfer\n");
>>         ide_atapi_cmd_ok(s);
>> +    }
>> 
>> +    /* end of DMA ? */
>>     if (io->len == 0) {
>> +        MACIO_DPRINTF("end of DMA\n");
>>         goto done;
>>     }
> 
> Both comments duplicate your debug output module question mark. :)

Yeah, I've just synced the comments between ATAPI and ATA here. But you're right - I should just remove them altogether.

> 
>> 
>>     /* launch next transfer */
>> 
>> +    MACIO_DPRINTF("io->len = %#x\n", io->len);
>> +
>>     s->io_buffer_size = io->len;
>> 
>>     qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>> @@ -76,12 +96,17 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>>     io->addr += io->len;
>>     io->len = 0;
>> 
>> +    MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
>> +                  (s->lba << 2) + (s->io_buffer_index >> 9),
>> +                  s->packet_transfer_size, s->dma_cmd);
>> +
>>     m->aiocb = dma_bdrv_read(s->bs, &s->sg,
>>                              (int64_t)(s->lba << 2) + (s->io_buffer_index >> 9),
>>                              pmac_ide_atapi_transfer_cb, io);
>>     return;
>> 
>> done:
>> +    MACIO_DPRINTF("done DMA\n");
>>     bdrv_acct_done(s->bs, &s->acct);
>>     io->dma_end(opaque);
>> }
>> @@ -95,6 +120,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>>     int64_t sector_num;
>> 
>>     if (ret < 0) {
>> +        MACIO_DPRINTF("DMA error\n");
>>         m->aiocb = NULL;
>>         qemu_sglist_destroy(&s->sg);
>>         ide_dma_error(s);
>> @@ -102,6 +128,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>>     }
>> 
>>     sector_num = ide_get_sector(s);
>> +    MACIO_DPRINTF("io_buffer_size = %#x\n", s->io_buffer_size);
>>     if (s->io_buffer_size > 0) {
>>         m->aiocb = NULL;
>>         qemu_sglist_destroy(&s->sg);
>> @@ -113,12 +140,14 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>> 
>>     /* end of transfer ? */
>>     if (s->nsector == 0) {
>> +        MACIO_DPRINTF("end of transfer\n");
>>         s->status = READY_STAT | SEEK_STAT;
>>         ide_set_irq(s->bus);
>>     }
>> 
>>     /* end of DMA ? */
>>     if (io->len == 0) {
>> +        MACIO_DPRINTF("end of DMA\n");
>>         goto done;
>>     }
>> 
>> @@ -127,12 +156,18 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>>     s->io_buffer_index = 0;
>>     s->io_buffer_size = io->len;
>> 
>> +
> 
> Intentionally two white lines?

Nope, I'm sure that gets resolved somewhere later in the patch series. But I'll fix it up here too ;).

> 
>> +    MACIO_DPRINTF("io->len = %#x\n", io->len);
>> +
>>     qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
>>                      &address_space_memory);
>>     qemu_sglist_add(&s->sg, io->addr, io->len);
>>     io->addr += io->len;
>>     io->len = 0;
>> 
>> +    MACIO_DPRINTF("sector_num=%" PRId64 " n=%d, nsector=%d, cmd_cmd=%d\n",
>> +                  sector_num, n, s->nsector, s->dma_cmd);
>> +
>>     switch (s->dma_cmd) {
>>     case IDE_DMA_READ:
>>         m->aiocb = dma_bdrv_read(s->bs, &s->sg, sector_num,
>> @@ -162,6 +197,8 @@ static void pmac_ide_transfer(DBDMA_io *io)
>>     MACIOIDEState *m = io->opaque;
>>     IDEState *s = idebus_active_if(&m->bus);
>> 
>> +    MACIO_DPRINTF("\n", __LINE__);
> 
> The argument is unused.

which proves the point that DPRINTF code shouldn't be left to bitrot ;).

> 
>> +
>>     s->io_buffer_size = 0;
>>     if (s->drive_kind == IDE_CD) {
>>         bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>> index ab174f5..903604d 100644
>> --- a/hw/misc/macio/mac_dbdma.c
>> +++ b/hw/misc/macio/mac_dbdma.c
>> @@ -233,6 +233,7 @@ static void conditional_interrupt(DBDMA_channel *ch)
>>         return;
>>     case INTR_ALWAYS: /* always interrupt */
>>         qemu_irq_raise(ch->irq);
>> +        DBDMA_DPRINTF("conditional_interrupt: raise\n");
> 
> Use %s and __func__ in case we ever rename it? More instances below. For
> dbdma_end() you did.

Ah, out of scope of this hunk is another debug print that already hardcodes the name. But I'll just change to __func__ ;).

> 
>>         return;
>>     }
>> 
>> @@ -245,12 +246,16 @@ static void conditional_interrupt(DBDMA_channel *ch)
>> 
>>     switch(intr) {
>>     case INTR_IFSET:  /* intr if condition bit is 1 */
>> -        if (cond)
>> +        if (cond) {
>>             qemu_irq_raise(ch->irq);
>> +            DBDMA_DPRINTF("conditional_interrupt: raise\n");
>> +        }
>>         return;
>>     case INTR_IFCLR:  /* intr if condition bit is 0 */
>> -        if (!cond)
>> +        if (!cond) {
>>             qemu_irq_raise(ch->irq);
>> +            DBDMA_DPRINTF("conditional_interrupt: raise\n");
>> +        }
>>         return;
>>     }
>> }
>> @@ -368,6 +373,8 @@ static void dbdma_end(DBDMA_io *io)
>>     DBDMA_channel *ch = io->channel;
>>     dbdma_cmd *current = &ch->current;
>> 
>> +    DBDMA_DPRINTF("%s\n", __func__, __LINE__);
> 
> Unused argument.
> 
>> +
>>     if (conditional_wait(ch))
>>         goto wait;
>> 
>> @@ -422,6 +429,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
>>      * are not implemented in the mac-io chip
>>      */
>> 
>> +    DBDMA_DPRINTF("addr 0x%x key 0x%x\n", addr, key);
> 
> PRIx32 for addr

Oh? Is there any system out there which does not work with %x and uint32_t?


Alex

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

* Re: [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io
  2013-06-30  6:48   ` Andreas Färber
@ 2013-06-30 23:35     ` Alexander Graf
  0 siblings, 0 replies; 23+ messages in thread
From: Alexander Graf @ 2013-06-30 23:35 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Kevin Wolf, programmingkidx, mark.cave-ayland, qemu-ppc, qemu-devel


On 30.06.2013, at 08:48, Andreas Färber wrote:

> Am 30.06.2013 03:27, schrieb Alexander Graf:
>> Soon we will introduce intermediate processing pauses which will
>> allow the bottom half to restart a DMA request that couldn't be
>> fulfilled yet.
>> 
>> For that to work, move the processing variable into the io struct
>> which is what DMA providers work with.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/misc/macio/mac_dbdma.c  | 10 ++++++----
>> include/hw/ppc/mac_dbdma.h |  3 ++-
>> 2 files changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
>> index 324ac54..91b9eaf 100644
>> --- a/hw/misc/macio/mac_dbdma.c
>> +++ b/hw/misc/macio/mac_dbdma.c
>> @@ -275,7 +275,9 @@ static void dbdma_end(DBDMA_io *io)
>>     conditional_branch(ch);
>> 
>> wait:
>> -    ch->processing = 0;
>> +    /* Indicate that we're ready for a new DMA round */
>> +    ch->io.processing = 0;
>> +
>>     if ((ch->regs[DBDMA_STATUS] & RUN) &&
>>         (ch->regs[DBDMA_STATUS] & ACTIVE))
>>         channel_run(ch);
>> @@ -301,7 +303,7 @@ static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
>>     ch->io.is_last = is_last;
>>     ch->io.dma_end = dbdma_end;
>>     ch->io.is_dma_out = 1;
>> -    ch->processing = 1;
>> +    ch->io.processing = 1;
>>     if (ch->rw) {
>>         ch->rw(&ch->io);
>>     }
>> @@ -327,7 +329,7 @@ static void start_input(DBDMA_channel *ch, int key, uint32_t addr,
>>     ch->io.is_last = is_last;
>>     ch->io.dma_end = dbdma_end;
>>     ch->io.is_dma_out = 0;
>> -    ch->processing = 1;
>> +    ch->io.processing = 1;
>>     if (ch->rw) {
>>         ch->rw(&ch->io);
>>     }
>> @@ -525,7 +527,7 @@ static void DBDMA_run(DBDMAState *s)
>>     for (channel = 0; channel < DBDMA_CHANNELS; channel++) {
>>         DBDMA_channel *ch = &s->channels[channel];
>>         uint32_t status = ch->regs[DBDMA_STATUS];
>> -        if (!ch->processing && (status & RUN) && (status & ACTIVE)) {
>> +        if (!ch->io.processing && (status & RUN) && (status & ACTIVE)) {
>>             channel_run(ch);
>>         }
>>     }
>> diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
>> index eb8e0f0..8ad1b6e 100644
>> --- a/include/hw/ppc/mac_dbdma.h
>> +++ b/include/hw/ppc/mac_dbdma.h
>> @@ -37,6 +37,8 @@ struct DBDMA_io {
>>     int is_last;
>>     int is_dma_out;
>>     DBDMA_end dma_end;
>> +    /* DMA is in progress, don't start another one */
>> +    int processing;
> 
> Can it be changed to bool (its users to true/false) while at it?

Sure!


Alex

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

end of thread, other threads:[~2013-06-30 23:35 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-30  1:26 [Qemu-devel] [PATCH 00/15] PPC: Mac OS X guest bringup Alexander Graf
2013-06-30  1:26 ` [Qemu-devel] [PATCH 01/15] PPC: Mac: Fix guest exported tbfreq values Alexander Graf
2013-06-30  1:26 ` [Qemu-devel] [PATCH 02/15] PPC: g3beige: Move secondary IDE bus to mac-io Alexander Graf
2013-06-30  6:33   ` Andreas Färber
2013-06-30 23:18     ` Alexander Graf
2013-06-30  1:26 ` [Qemu-devel] [PATCH 03/15] PPC: Macio: Replace tabs with spaces Alexander Graf
2013-06-30  1:26 ` [Qemu-devel] [PATCH 04/15] PPC: dbdma: " Alexander Graf
2013-06-30  6:35   ` Andreas Färber
2013-06-30 11:21     ` Alexander Graf
2013-06-30  1:26 ` [Qemu-devel] [PATCH 05/15] PPC: Mac: Add debug prints in macio and dbdma code Alexander Graf
2013-06-30  6:42   ` Andreas Färber
2013-06-30 23:30     ` Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 06/15] PPC: dbdma: Fix debug print Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 07/15] PPC: dbdma: Allow new commands in RUN state Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 08/15] PPC: dbdma: Move defines into header file Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 09/15] PPC: dbdma: Introduce kick function Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 10/15] PPC: dbdma: Move static bh variable to device struct Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 11/15] PPC: dbdma: macio: Add DMA callback Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 12/15] PPC: dbdma: Move processing to io Alexander Graf
2013-06-30  6:48   ` Andreas Färber
2013-06-30 23:35     ` Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 13/15] PPC: dbdma: Wait for DMA until we have data Alexander Graf
2013-06-30  1:27 ` [Qemu-devel] [PATCH 14/15] PPC: dbdma: Support unaligned DMA access Alexander Graf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.