Patchwork [2/5] floppy: move dma setup + drive connect to fdctrl_init_common()

login
register
mail settings
Submitter Gerd Hoffmann
Date Sept. 16, 2009, 8:25 p.m.
Message ID <1253132744-10492-3-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/33737/
State Superseded
Headers show

Comments

Gerd Hoffmann - Sept. 16, 2009, 8:25 p.m.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/fdc.c |   33 ++++++++++++---------------------
 1 files changed, 12 insertions(+), 21 deletions(-)
Markus Armbruster - Sept. 18, 2009, 2:48 p.m.
Gerd Hoffmann <kraxel@redhat.com> writes:

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/fdc.c |   33 ++++++++++++---------------------
>  1 files changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index ea3b8ac..537db66 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1842,23 +1842,14 @@ static void fdctrl_connect_drives(fdctrl_t *fdctrl)
>  
>  fdctrl_t *fdctrl_init_isa(DriveInfo **fds)
>  {
> -    fdctrl_t *fdctrl;
>      ISADevice *dev;
> -    int dma_chann = 2;
>  
>      dev = isa_create("isa-fdc");
>      qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]);
>      qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]);
>      if (qdev_init(&dev->qdev) != 0)
>          return NULL;
> -    fdctrl = &(DO_UPCAST(fdctrl_isabus_t, busdev, dev)->state);
> -
> -    fdctrl->dma_chann = dma_chann;
> -    DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
> -
> -    fdctrl_connect_drives(fdctrl);
> -
> -    return fdctrl;
> +    return &(DO_UPCAST(fdctrl_isabus_t, busdev, dev)->state);

Redundant parenthesis.

>  }
>  
>  fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
> @@ -1870,19 +1861,16 @@ fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>      fdctrl_sysbus_t *sys;
>  
>      dev = qdev_create(NULL, "sysbus-fdc");
> +    sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
> +    fdctrl = &sys->state;
> +    fdctrl->dma_chann = dma_chann; /* FIXME */

What needs to be fixed here?  Could that be explained in the comment?

>      qdev_prop_set_drive(dev, "driveA", fds[0]);
>      qdev_prop_set_drive(dev, "driveB", fds[1]);
>      if (qdev_init(dev) != 0)
>          return NULL;
> -    sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
> -    fdctrl = &sys->state;
>      sysbus_connect_irq(&sys->busdev, 0, irq);
>      sysbus_mmio_map(&sys->busdev, 0, mmio_base);
>  
> -    fdctrl->dma_chann = dma_chann;
> -    DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
> -    fdctrl_connect_drives(fdctrl);
> -
>      return fdctrl;
>  }
>  
> @@ -1901,11 +1889,7 @@ fdctrl_t *sun4m_fdctrl_init (qemu_irq irq, target_phys_addr_t io_base,
>      fdctrl = &sys->state;
>      sysbus_connect_irq(&sys->busdev, 0, irq);
>      sysbus_mmio_map(&sys->busdev, 0, io_base);
> -    *fdc_tc = qdev_get_gpio_in(dev, 0);
> -
> -    fdctrl->dma_chann = -1;
> -
> -    fdctrl_connect_drives(fdctrl);
> +    *fdc_tc = qdev_get_gpio_in(dev, 0); /* FIXME */

Same question.

>  
>      return fdctrl;
>  }
> @@ -1937,6 +1921,10 @@ static int fdctrl_init_common(fdctrl_t *fdctrl)
>      fdctrl->config = FD_CONFIG_EIS | FD_CONFIG_EFIFO; /* Implicit seek, polling & FIFO enabled */
>      fdctrl->num_floppies = MAX_FD;
>  
> +    if (fdctrl->dma_chann != -1)
> +        DMA_register_channel(fdctrl->dma_chann, &fdctrl_transfer_handler, fdctrl);
> +    fdctrl_connect_drives(fdctrl);
> +
>      fdctrl_external_reset(fdctrl);
>      vmstate_register(-1, &vmstate_fdc, fdctrl);
>      qemu_register_reset(fdctrl_external_reset, fdctrl);
> @@ -1949,6 +1937,7 @@ static int isabus_fdc_init1(ISADevice *dev)
>      fdctrl_t *fdctrl = &isa->state;
>      int iobase = 0x3f0;
>      int isairq = 6;
> +    int dma_chann = 2;
>  
>      register_ioport_read(iobase + 0x01, 5, 1,
>                           &fdctrl_read_port, fdctrl);
> @@ -1959,6 +1948,7 @@ static int isabus_fdc_init1(ISADevice *dev)
>      register_ioport_write(iobase + 0x07, 1, 1,
>                            &fdctrl_write_port, fdctrl);
>      isa_init_irq(&isa->busdev, &fdctrl->irq, isairq);
> +    fdctrl->dma_chann = dma_chann;
>  
>      return fdctrl_init_common(fdctrl);
>  }
> @@ -1972,6 +1962,7 @@ static int sysbus_fdc_init1(SysBusDevice *dev)
>      sysbus_init_mmio(dev, 0x08, io);
>      sysbus_init_irq(dev, &fdctrl->irq);
>      qdev_init_gpio_in(&dev->qdev, fdctrl_handle_tc, 1);
> +    fdctrl->dma_chann = -1;
>  
>      return fdctrl_init_common(fdctrl);
>  }

Hmm, as far as I can see, initialization of fdctrl->dma_chann moved to
the qdev init() method for ISA and Sun4m, but not for sysbus.
Intentional?  If yes, what about explaining it in the code, or perhaps
the commit message?
Gerd Hoffmann - Sept. 18, 2009, 2:58 p.m.
>>   fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>> @@ -1870,19 +1861,16 @@ fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>>       fdctrl_sysbus_t *sys;
>>
>>       dev = qdev_create(NULL, "sysbus-fdc");
>> +    sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
>> +    fdctrl =&sys->state;
>> +    fdctrl->dma_chann = dma_chann; /* FIXME */
>
> What needs to be fixed here?  Could that be explained in the comment?

See below.

>> -    fdctrl_connect_drives(fdctrl);
>> +    *fdc_tc = qdev_get_gpio_in(dev, 0); /* FIXME */
>
> Same question.

fdctrl_init_$kind() should just be convinience wrappers.  Creating the 
devices via -device should work too.  Which means the convinience 
wrappers must do nothing but qdev_create + set properties + qdev_init.

> Hmm, as far as I can see, initialization of fdctrl->dma_chann moved to
> the qdev init() method for ISA and Sun4m, but not for sysbus.
> Intentional?  If yes, what about explaining it in the code, or perhaps
> the commit message?

Yes, intentional.  On the ISA bus the dma channel is fixed: #2.  sun4m 
doesn't use dma.  The third variant has one user which uses dma channel 
#0.  So we could either hard-code that one too (like we do for isa).  Or 
we could make it a property so it can be configured as needed.  Dunno 
which of the two variants would be the correct one.

cheers,
   Gerd

Patch

diff --git a/hw/fdc.c b/hw/fdc.c
index ea3b8ac..537db66 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1842,23 +1842,14 @@  static void fdctrl_connect_drives(fdctrl_t *fdctrl)
 
 fdctrl_t *fdctrl_init_isa(DriveInfo **fds)
 {
-    fdctrl_t *fdctrl;
     ISADevice *dev;
-    int dma_chann = 2;
 
     dev = isa_create("isa-fdc");
     qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]);
     qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]);
     if (qdev_init(&dev->qdev) != 0)
         return NULL;
-    fdctrl = &(DO_UPCAST(fdctrl_isabus_t, busdev, dev)->state);
-
-    fdctrl->dma_chann = dma_chann;
-    DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
-
-    fdctrl_connect_drives(fdctrl);
-
-    return fdctrl;
+    return &(DO_UPCAST(fdctrl_isabus_t, busdev, dev)->state);
 }
 
 fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
@@ -1870,19 +1861,16 @@  fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
     fdctrl_sysbus_t *sys;
 
     dev = qdev_create(NULL, "sysbus-fdc");
+    sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
+    fdctrl = &sys->state;
+    fdctrl->dma_chann = dma_chann; /* FIXME */
     qdev_prop_set_drive(dev, "driveA", fds[0]);
     qdev_prop_set_drive(dev, "driveB", fds[1]);
     if (qdev_init(dev) != 0)
         return NULL;
-    sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
-    fdctrl = &sys->state;
     sysbus_connect_irq(&sys->busdev, 0, irq);
     sysbus_mmio_map(&sys->busdev, 0, mmio_base);
 
-    fdctrl->dma_chann = dma_chann;
-    DMA_register_channel(dma_chann, &fdctrl_transfer_handler, fdctrl);
-    fdctrl_connect_drives(fdctrl);
-
     return fdctrl;
 }
 
@@ -1901,11 +1889,7 @@  fdctrl_t *sun4m_fdctrl_init (qemu_irq irq, target_phys_addr_t io_base,
     fdctrl = &sys->state;
     sysbus_connect_irq(&sys->busdev, 0, irq);
     sysbus_mmio_map(&sys->busdev, 0, io_base);
-    *fdc_tc = qdev_get_gpio_in(dev, 0);
-
-    fdctrl->dma_chann = -1;
-
-    fdctrl_connect_drives(fdctrl);
+    *fdc_tc = qdev_get_gpio_in(dev, 0); /* FIXME */
 
     return fdctrl;
 }
@@ -1937,6 +1921,10 @@  static int fdctrl_init_common(fdctrl_t *fdctrl)
     fdctrl->config = FD_CONFIG_EIS | FD_CONFIG_EFIFO; /* Implicit seek, polling & FIFO enabled */
     fdctrl->num_floppies = MAX_FD;
 
+    if (fdctrl->dma_chann != -1)
+        DMA_register_channel(fdctrl->dma_chann, &fdctrl_transfer_handler, fdctrl);
+    fdctrl_connect_drives(fdctrl);
+
     fdctrl_external_reset(fdctrl);
     vmstate_register(-1, &vmstate_fdc, fdctrl);
     qemu_register_reset(fdctrl_external_reset, fdctrl);
@@ -1949,6 +1937,7 @@  static int isabus_fdc_init1(ISADevice *dev)
     fdctrl_t *fdctrl = &isa->state;
     int iobase = 0x3f0;
     int isairq = 6;
+    int dma_chann = 2;
 
     register_ioport_read(iobase + 0x01, 5, 1,
                          &fdctrl_read_port, fdctrl);
@@ -1959,6 +1948,7 @@  static int isabus_fdc_init1(ISADevice *dev)
     register_ioport_write(iobase + 0x07, 1, 1,
                           &fdctrl_write_port, fdctrl);
     isa_init_irq(&isa->busdev, &fdctrl->irq, isairq);
+    fdctrl->dma_chann = dma_chann;
 
     return fdctrl_init_common(fdctrl);
 }
@@ -1972,6 +1962,7 @@  static int sysbus_fdc_init1(SysBusDevice *dev)
     sysbus_init_mmio(dev, 0x08, io);
     sysbus_init_irq(dev, &fdctrl->irq);
     qdev_init_gpio_in(&dev->qdev, fdctrl_handle_tc, 1);
+    fdctrl->dma_chann = -1;
 
     return fdctrl_init_common(fdctrl);
 }