Skip to contentSkip to conversation

Floppy controller: break relation with PC-style DMA

RFC In review Hervé Poussineau 2 participants 4 messages Nov 22, 2008

lore
Share & tools
lore

Hervé Poussineau

author
#0 lore raw

RFC 1/1 Floppy controller: break relation with PC-style DMA

Hi,

Currently, floppy disk controller is tied to PC-style DMA controller.
Attached patch is an attempt to break the link between those, by introducing function pointers for dma operations (read, write, hold, release).

This will allow me to fix fdc DMA handling on MIPS Magnum/Acer Pica 61, where fdc is connected to rc4030 DMA controller.

Please comment.

Hervé

fdc.c +118 -40
Index: hw/fdc.c
===================================================================
@@ -471,7 +471,12 @@
     uint8_t version;
     /* HW */
     qemu_irq irq;
-    int dma_chann;
+    fdcdma_memory_read_write dma_memory_read;
+    fdcdma_memory_read_write dma_memory_write;
+    fdcdma_hold_release dma_hold;
+    fdcdma_hold_release dma_release;
+    void *dma_opaque;
+    int i8257_dma_chann;
     target_phys_addr_t io_base;
     /* Controller state */
     QEMUTimer *result_timer;

@@ -771,7 +776,7 @@
         fdctrl->sra |= FD_SRA_nDRV2;
     fdctrl->cur_drv = 0;
     fdctrl->dor = FD_DOR_nRESET;
-    fdctrl->dor |= (fdctrl->dma_chann != -1) ? FD_DOR_DMAEN : 0;
+    fdctrl->dor |= (fdctrl->dma_memory_read && fdctrl->dma_memory_write) ? FD_DOR_DMAEN : 0;
     fdctrl->msr = FD_MSR_RQM;
     /* FIFO state */
     fdctrl->data_pos = 0;

@@ -1068,8 +1073,8 @@
     fdctrl->fifo[5] = cur_drv->sect;
     fdctrl->fifo[6] = FD_SECTOR_SC;
     fdctrl->data_dir = FD_DIR_READ;
-    if (!(fdctrl->msr & FD_MSR_NONDMA)) {
-        DMA_release_DREQ(fdctrl->dma_chann);
+    if (!(fdctrl->msr & FD_MSR_NONDMA) && fdctrl->dma_release) {
+        fdctrl->dma_release(fdctrl->dma_opaque);
     }
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;

@@ -1144,29 +1149,15 @@
     }
     fdctrl->eot = fdctrl->fifo[6];
     if (fdctrl->dor & FD_DOR_DMAEN) {
-        int dma_mode;
-        /* DMA transfer are enabled. Check if DMA channel is well programmed */
-        dma_mode = DMA_get_channel_mode(fdctrl->dma_chann);
-        dma_mode = (dma_mode >> 2) & 3;
-        FLOPPY_DPRINTF("dma_mode=%d direction=%d (%d - %d)\n",
-                       dma_mode, direction,
-                       (128 << fdctrl->fifo[5]) *
-                       (cur_drv->last_sect - ks + 1), fdctrl->data_len);
-        if (((direction == FD_DIR_SCANE || direction == FD_DIR_SCANL ||
-              direction == FD_DIR_SCANH) && dma_mode == 0) ||
-            (direction == FD_DIR_WRITE && dma_mode == 2) ||
-            (direction == FD_DIR_READ && dma_mode == 1)) {
-            /* No access is allowed until DMA transfer has completed */
+        /* DMA transfer are enabled */
+        if (fdctrl->dma_hold) {
+            fdctrl->dma_hold(fdctrl->dma_opaque);
+        } else {
+            /* Do the transfer synchronously... */
             fdctrl->msr &= ~FD_MSR_RQM;
-            /* Now, we just have to wait for the DMA controller to
-             * recall us...
-             */
-            DMA_hold_DREQ(fdctrl->dma_chann);
-            DMA_schedule(fdctrl->dma_chann);
-            return;
-        } else {
-            FLOPPY_ERROR("dma_mode=%d direction=%d\n", dma_mode, direction);
+            fdctrl_transfer_handler(fdctrl, -1, 0, fdctrl->data_len);
         }
+        return;
     }
     FLOPPY_DPRINTF("start non-DMA transfer\n");
     fdctrl->msr |= FD_MSR_NONDMA;

@@ -1174,8 +1165,6 @@
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */
     fdctrl_raise_irq(fdctrl, 0x00);
-
-    return;
 }
 
 /* Prepare a transfer of deleted data */

@@ -1241,13 +1230,11 @@
         switch (fdctrl->data_dir) {
         case FD_DIR_READ:
             /* READ commands */
-            DMA_write_memory (nchan, fdctrl->fifo + rel_pos,
-                              fdctrl->data_pos, len);
+            fdctrl->dma_memory_write(fdctrl->dma_opaque, fdctrl->fifo + rel_pos, len);
             break;
         case FD_DIR_WRITE:
             /* WRITE commands */
-            DMA_read_memory (nchan, fdctrl->fifo + rel_pos,
-                             fdctrl->data_pos, len);
+            fdctrl->dma_memory_read(fdctrl->dma_opaque, fdctrl->fifo + rel_pos, len);
             if (bdrv_write(cur_drv->bs, fd_sector(cur_drv),
                            fdctrl->fifo, 1) < 0) {
                 FLOPPY_ERROR("writing sector %d\n", fd_sector(cur_drv));

@@ -1260,7 +1247,7 @@
             {
                 uint8_t tmpbuf[FD_SECTOR_LEN];
                 int ret;
-                DMA_read_memory (nchan, tmpbuf, fdctrl->data_pos, len);
+                fdctrl->dma_memory_read(fdctrl->dma_opaque, tmpbuf, len);
                 ret = memcmp(tmpbuf, fdctrl->fifo + rel_pos, len);
                 if (ret == 0) {
                     status2 = FD_SR2_SEH;

@@ -1833,7 +1820,12 @@
 }
 
 /* Init functions */
-static fdctrl_t *fdctrl_init_common (qemu_irq irq, int dma_chann,
+static fdctrl_t *fdctrl_init_common (qemu_irq irq,
+                                     fdcdma_memory_read_write dma_memory_read,
+                                     fdcdma_memory_read_write dma_memory_write,
+                                     fdcdma_hold_release dma_hold,
+                                     fdcdma_hold_release dma_release,
+                                     void *dma_opaque,
                                      target_phys_addr_t io_base,
                                      BlockDriverState **fds)
 {

@@ -1862,12 +1854,13 @@
 
     fdctrl->version = 0x90; /* Intel 82078 controller */
     fdctrl->irq = irq;
-    fdctrl->dma_chann = dma_chann;
+    fdctrl->dma_memory_read = dma_memory_read;
+    fdctrl->dma_memory_write = dma_memory_write;
+    fdctrl->dma_hold = dma_hold;
+    fdctrl->dma_release = dma_release;
+    fdctrl->dma_opaque = dma_opaque;
     fdctrl->io_base = io_base;
     fdctrl->config = FD_CONFIG_EIS | FD_CONFIG_EFIFO; /* Implicit seek, polling & FIFO enabled */
-    if (fdctrl->dma_chann != -1) {
-        DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
-    }
     for (i = 0; i < MAX_FD; i++) {
         fd_init(&fdctrl->drives[i], fds[i]);
     }

@@ -1881,14 +1874,21 @@
     return fdctrl;
 }
 
-fdctrl_t *fdctrl_init (qemu_irq irq, int dma_chann, int mem_mapped,
+fdctrl_t *fdctrl_init (qemu_irq irq,
+                       fdcdma_memory_read_write dma_memory_read,
+                       fdcdma_memory_read_write dma_memory_write,
+                       fdcdma_hold_release dma_hold,
+                       fdcdma_hold_release dma_release,
+                       void *dma_opaque, int mem_mapped,
                        target_phys_addr_t io_base,
                        BlockDriverState **fds)
 {
     fdctrl_t *fdctrl;
     int io_mem;
 
-    fdctrl = fdctrl_init_common(irq, dma_chann, io_base, fds);
+    fdctrl = fdctrl_init_common(irq, dma_memory_read, dma_memory_write,
+                                dma_hold, dma_release,
+                                dma_opaque, io_base, fds);
 
     fdctrl->sun4m = 0;
     if (mem_mapped) {

@@ -1915,7 +1915,7 @@
     fdctrl_t *fdctrl;
     int io_mem;
 
-    fdctrl = fdctrl_init_common(irq, -1, io_base, fds);
+    fdctrl = fdctrl_init_common(irq, NULL, NULL, NULL, NULL, NULL, io_base, fds);
     fdctrl->sun4m = 1;
     io_mem = cpu_register_io_memory(0, fdctrl_mem_read_strict,
                                     fdctrl_mem_write_strict,

@@ -1925,3 +1925,81 @@
 
     return fdctrl;
 }
+
+static void i8257_dma_read(void *opaque, uint8_t *buf, int len)
+{
+    fdctrl_t *fdctrl = opaque;
+    DMA_read_memory(fdctrl->i8257_dma_chann, buf, fdctrl->data_pos, len);
+}
+
+static void i8257_dma_write(void *opaque, uint8_t *buf, int len)
+{
+    fdctrl_t *fdctrl = opaque;
+    DMA_write_memory(fdctrl->i8257_dma_chann, buf, fdctrl->data_pos, len);
+}
+
+static void i8257_dma_hold(void *opaque)
+{
+    fdctrl_t *fdctrl = opaque;
+    int dma_mode;
+    int direction = fdctrl->data_dir;
+
+    /* DMA transfer are enabled. Check if DMA channel is well programmed */
+    dma_mode = DMA_get_channel_mode(fdctrl->i8257_dma_chann);
+    dma_mode = (dma_mode >> 2) & 3;
+    FLOPPY_DPRINTF("dma_mode=%d direction=%d (%d - %d)\n",
+                   dma_mode, direction,
+                   (128 << fdctrl->fifo[5]) *
+                   (cur_drv->last_sect - fdctrl->fifo[4] + 1), fdctrl->data_len);
+    if (((direction == FD_DIR_SCANE || direction == FD_DIR_SCANL ||
+          direction == FD_DIR_SCANH) && dma_mode == 0) ||
+        (direction == FD_DIR_WRITE && dma_mode == 2) ||
+        (direction == FD_DIR_READ && dma_mode == 1)) {
+        /* No access is allowed until DMA transfer has completed */
+        fdctrl->msr &= ~FD_MSR_RQM;
+        /* Now, we just have to wait for the DMA controller to
+         * recall us...
+         */
+        DMA_hold_DREQ(fdctrl->i8257_dma_chann);
+        DMA_schedule(fdctrl->i8257_dma_chann);
+    } else {
+        FLOPPY_ERROR("dma_mode=%d direction=%d\n", dma_mode, direction);
+    }
+}
+
+static void i8257_dma_release(void *opaque)
+{
+    fdctrl_t *fdctrl = opaque;
+    DMA_release_DREQ(fdctrl->i8257_dma_chann);
+}
+
+fdctrl_t *fdctrl_i8257_init (qemu_irq irq, int i8257_dma_chann,
+                             int mem_mapped,
+                             target_phys_addr_t io_base,
+                             BlockDriverState **fds)
+{
+    fdcdma_memory_read_write dma_memory_read = NULL;
+    fdcdma_memory_read_write dma_memory_write = NULL;
+    fdcdma_hold_release dma_hold = NULL;
+    fdcdma_hold_release dma_release = NULL;
+    fdctrl_t *fdctrl;
+
+    if (i8257_dma_chann != -1) {
+        dma_memory_read = i8257_dma_read;
+        dma_memory_write = i8257_dma_write;
+        dma_hold = i8257_dma_hold;
+        dma_release = i8257_dma_release;
+    }
+
+    fdctrl = fdctrl_init(irq, dma_memory_read, dma_memory_write,
+                         i8257_dma_hold, i8257_dma_release, NULL,
+                         mem_mapped, io_base, fds);
+
+    if (i8257_dma_chann != -1) {
+        fdctrl->dma_opaque = fdctrl;
+        fdctrl->i8257_dma_chann = i8257_dma_chann;
+        DMA_register_channel(fdctrl->i8257_dma_chann, &fdctrl_transfer_handler, fdctrl);
+    }
+
+    return fdctrl;
+}
\ No newline at end of file
fdc.h +12 -1
Index: hw/fdc.h
===================================================================
@@ -3,9 +3,20 @@
 
 typedef struct fdctrl_t fdctrl_t;
 
-fdctrl_t *fdctrl_init (qemu_irq irq, int dma_chann, int mem_mapped,
+typedef void (*fdcdma_memory_read_write)(void *opaque, uint8_t *buf, int len);
+typedef void (*fdcdma_hold_release)(void *opaque);
+fdctrl_t *fdctrl_init (qemu_irq irq,
+                       fdcdma_memory_read_write dma_memory_read,
+                       fdcdma_memory_read_write dma_memory_write,
+                       fdcdma_hold_release dma_hold,
+                       fdcdma_hold_release dma_release,
+                       void *dma_opaque, int mem_mapped,
                        target_phys_addr_t io_base,
                        BlockDriverState **fds);
+fdctrl_t *fdctrl_i8257_init (qemu_irq irq, int i8257_dma_chann,
+                             int mem_mapped,
+                             target_phys_addr_t io_base,
+                             BlockDriverState **fds);
 fdctrl_t *sun4m_fdctrl_init (qemu_irq irq, target_phys_addr_t io_base,
                              BlockDriverState **fds, qemu_irq *fdc_tc);
 int fdctrl_get_drive_type(fdctrl_t *fdctrl, int drive_num);
mips_jazz.c +1 -1
Index: hw/mips_jazz.c
===================================================================
@@ -221,7 +221,7 @@
         else
             fds[n] = NULL;
     }
-    fdctrl_init(rc4030[1], 0, 1, 0x80003000, fds);
+    fdctrl_init(rc4030[1], NULL, NULL, NULL, NULL, NULL, 1, 0x80003000, fds);
 
     /* Real time clock */
     rtc_init(0x70, i8259[8]);
mips_malta.c +1 -1
Index: hw/mips_malta.c
===================================================================
@@ -929,7 +929,7 @@
        else
            fd[i] = NULL;
     }
-    floppy_controller = fdctrl_init(i8259[6], 2, 0, 0x3f0, fd);
+    floppy_controller = fdctrl_i8257_init(i8259[6], 2, 0, 0x3f0, fd);
 
     /* Sound card */
 #ifdef HAS_AUDIO
pc.c +1 -1
Index: hw/pc.c
===================================================================
@@ -1052,7 +1052,7 @@
 	else
 	    fd[i] = NULL;
     }
-    floppy_controller = fdctrl_init(i8259[6], 2, 0, 0x3f0, fd);
+    floppy_controller = fdctrl_i8257_init(i8259[6], 2, 0, 0x3f0, fd);
 
     cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device, hd);
ppc_prep.c +1 -1
Index: hw/ppc_prep.c
===================================================================
@@ -709,7 +709,7 @@
         else
             fd[i] = NULL;
     }
-    fdctrl_init(i8259[6], 2, 0, 0x3f0, fd);
+    fdctrl_i8257_init(i8259[6], 2, 0, 0x3f0, fd);
 
     /* Register speaker port */
     register_ioport_read(0x61, 1, 1, speaker_ioport_read, NULL);
sun4m.c +1 -1
Index: hw/sun4m.c
===================================================================
@@ -1589,7 +1589,7 @@
 
     if (hwdef->fd_base != (target_phys_addr_t)-1) {
         /* there is zero or one floppy drive */
-        fd[1] = fd[0] = NULL;
+        memset(fd, 0, sizeof(fd));
         drive_index = drive_get_index(IF_FLOPPY, 0, 0);
         if (drive_index != -1)
             fd[0] = drives_table[drive_index].bdrv;
sun4u.c +1 -1
Index: hw/sun4u.c
===================================================================
@@ -508,7 +508,7 @@
        else
            fd[i] = NULL;
     }
-    floppy_controller = fdctrl_init(NULL/*6*/, 2, 0, 0x3f0, fd);
+    floppy_controller = fdctrl_i8257_init(NULL/*6*/, 2, 0, 0x3f0, fd);
     nvram = m48t59_init(NULL/*8*/, 0, 0x0074, NVRAM_SIZE, 59);
     sun4u_NVRAM_set_params(nvram, NVRAM_SIZE, "Sun4u", RAM_size, boot_devices,
                            KERNEL_LOAD_ADDR, kernel_size,
#1 lore raw
Quoted 9 lines · Hervé Poussineau

Hi,

Currently, floppy disk controller is tied to PC-style DMA controller. Attached patch is an attempt to break the link between those, by
introducing function pointers for dma operations (read, write, hold, release).

This will allow me to fix fdc DMA handling on MIPS Magnum/Acer Pica 61,
where fdc is connected to rc4030 DMA controller.

Isn't it possible to move the i8257 DMA helpers to for example dma.c? Then I could eliminate some dummy DMA_xx functions from sun4m.c and I think fdctrl_i8257_init would not be needed.

You are calling fdctrl->dma_memory_write and _read without checking that they are not NULL.

author
#2 lore raw
Quoted 11 lines · Blue Swirl
Quoted 6 lines · Hervé Poussineau

Hi,

Currently, floppy disk controller is tied to PC-style DMA controller. Attached patch is an attempt to break the link between those, by
introducing function pointers for dma operations (read, write, hold, release).

Isn't it possible to move the i8257 DMA helpers to for example dma.c? Then I could eliminate some dummy DMA_xx functions from sun4m.c and I think fdctrl_i8257_init would not be needed.

That's a little bit hard for 3 reasons:
a) i8257_dma_hold() checks if DMA is well programmed before doing the transfer. I suppose the check should be moved to DMA engine, or at least to the _read and _write handlers.
b) There is also the problem at initialization, where each i8257 client calls DMA_register_channel() to register itself, and gives a callback function (fdctrl_transfer_handler for fdc.c).
c) This callback will then be called to feed destination DMA buffer part by part (see dma_pos and dma_len in fdctrl_transfer_handler), instead of in one big transfer. That also explains the data_pos parameter when calling DMA_read_memory/DMA_write_memory

Yet, I've no good idea to solve problem b) and c).

I suppose we really need a generic DMA framework, as it seems each DMA controller is using its own scheme.
For me, a first step in this direction is to break links between devices and DMA engines, like it has already be done for esp adapter. Then, something good can emerge. You should also notice that I chose _read and _write callbacks for fdc to be exactly the same as in esp.

Quoted 2 lines

You are calling fdctrl->dma_memory_write and _read without checking that they are not NULL.

Ok, I'll add checks for fdctrl->dma_memory_write and _read.
In all cases, one without the other should be an error at initialization, and then, DMA should be avoided/prevented if they are not defined.

Hervé

#3 lore raw
Quoted 17 lines · Hervé Poussineau
Quoted 10 lines · Blue Swirl
Quoted 7 lines · Hervé Poussineau

Hi,

Currently, floppy disk controller is tied to PC-style DMA controller. Attached patch is an attempt to break the link between those, by
introducing function pointers for dma operations (read, write, hold, release).

Quoted 4 lines

Isn't it possible to move the i8257 DMA helpers to for example dma.c? Then I could eliminate some dummy DMA_xx functions from sun4m.c and I think fdctrl_i8257_init would not be needed.

Quoted 4 lines

That's a little bit hard for 3 reasons:
a) i8257_dma_hold() checks if DMA is well programmed before doing the
transfer. I suppose the check should be moved to DMA engine, or at least to the _read and _write handlers.

The DMA engine should be a better place.

Quoted 3 lines

b) There is also the problem at initialization, where each i8257 client
calls DMA_register_channel() to register itself, and gives a callback function (fdctrl_transfer_handler for fdc.c).

How about pushing registering of the callback to board level, for example introduce a function for getting the transfer callback.

Quoted 4 lines

c) This callback will then be called to feed destination DMA buffer part by
part (see dma_pos and dma_len in fdctrl_transfer_handler), instead of in one big transfer. That also explains the data_pos parameter when calling DMA_read_memory/DMA_write_memory

There is something fishy with the interface, real devices using DMA have no idea about DMA position and they can't give DMA controller any instructions.

Quoted 9 lines

Yet, I've no good idea to solve problem b) and c).

I suppose we really need a generic DMA framework, as it seems each DMA
controller is using its own scheme.
For me, a first step in this direction is to break links between devices
and DMA engines, like it has already be done for esp adapter. Then, something good can emerge.
You should also notice that I chose _read and _write callbacks for fdc to
be exactly the same as in esp.

Great! Maybe I should revive some of my generic DMA patches.