| Message ID | 1253132744-10492-3-git-send-email-kraxel@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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?
>> 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
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); }
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/fdc.c | 33 ++++++++++++--------------------- 1 files changed, 12 insertions(+), 21 deletions(-)