| Message ID | 20191023173154.30051-19-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > The function is specific to mipssim, let's inline it. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 16 ---------------- > hw/mips/mips_mipssim.c | 13 ++++++++++--- > include/hw/char/serial.h | 2 -- > 3 files changed, 10 insertions(+), 21 deletions(-) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 84de2740a7..ca95e09ec9 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -1023,22 +1023,6 @@ static const TypeInfo serial_io_info = { > .class_init = serial_io_class_init, > }; > > -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, > - Chardev *chr, MemoryRegion *system_io) > -{ > - SerialIO *self = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > - > - qdev_prop_set_uint32(DEVICE(self), "baudbase", baudbase); > - qdev_prop_set_chr(DEVICE(self), "chardev", chr); > - qdev_prop_set_int32(DEVICE(self), "instance-id", base); > - qdev_init_nofail(DEVICE(self)); > - > - sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq); > - memory_region_add_subregion(system_io, base, &self->serial.io); > - > - return self; > -} > - > static Property serial_properties[] = { > DEFINE_PROP_CHR("chardev", SerialState, chr), > DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), > diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c > index 282bbecb24..26fb8fa4de 100644 > --- a/hw/mips/mips_mipssim.c > +++ b/hw/mips/mips_mipssim.c > @@ -40,6 +40,7 @@ > #include "hw/loader.h" > #include "elf.h" > #include "hw/sysbus.h" > +#include "hw/qdev-properties.h" > #include "exec/address-spaces.h" > #include "qemu/error-report.h" > #include "sysemu/qtest.h" > @@ -219,9 +220,15 @@ mips_mipssim_init(MachineState *machine) > * A single 16450 sits at offset 0x3f8. It is attached to > * MIPS CPU INT2, which is interrupt 4. > */ > - if (serial_hd(0)) > - serial_init(0x3f8, env->irq[4], 115200, serial_hd(0), > - get_system_io()); > + if (serial_hd(0)) { > + DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO); > + > + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > + qdev_prop_set_int32(dev, "instance-id", 0x3f8); > + qdev_init_nofail(dev); > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > + sysbus_add_io(SYS_BUS_DEVICE(dev), 0x3f8, &SERIAL_IO(dev)->serial.io); > + } The code being inlined here doesn't seem to be identical to the code in the function above (it uses sysbus_add_io() rather than memory_region_add_subregion()). It's also a bit ugly that the code has to reach into the internals of the device to get the MemoryRegion pointer. Would it work to have the device use sysbus_init_mmio() in its init or realize method to register the IO port MemoryRegion, and then we could use sysbus_mmio_get_region() to get the MemoryRegion pointer ? (If that does work, we should someday refactor sysbus_add_io() so that all the devices it's used with register the MemoryRegions that way, and then sysbus_add_io() can just take an 'int n' the way sysbus_mmio_map() does. But not today.) thanks -- PMM
On Wednesday, October 23, 2019, Marc-André Lureau < marcandre.lureau@redhat.com> wrote: > The function is specific to mipssim, let's inline it. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 16 ---------------- > hw/mips/mips_mipssim.c | 13 ++++++++++--- > include/hw/char/serial.h | 2 -- > 3 files changed, 10 insertions(+), 21 deletions(-) Marc-André, hi! Mismatch between the commit message and the patch content. Not acceptable. Yours, Aleksandar > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 84de2740a7..ca95e09ec9 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -1023,22 +1023,6 @@ static const TypeInfo serial_io_info = { > .class_init = serial_io_class_init, > }; > > -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, > - Chardev *chr, MemoryRegion *system_io) > -{ > - SerialIO *self = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > - > - qdev_prop_set_uint32(DEVICE(self), "baudbase", baudbase); > - qdev_prop_set_chr(DEVICE(self), "chardev", chr); > - qdev_prop_set_int32(DEVICE(self), "instance-id", base); > - qdev_init_nofail(DEVICE(self)); > - > - sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq); > - memory_region_add_subregion(system_io, base, &self->serial.io); > - > - return self; > -} > - > static Property serial_properties[] = { > DEFINE_PROP_CHR("chardev", SerialState, chr), > DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), > diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c > index 282bbecb24..26fb8fa4de 100644 > --- a/hw/mips/mips_mipssim.c > +++ b/hw/mips/mips_mipssim.c > @@ -40,6 +40,7 @@ > #include "hw/loader.h" > #include "elf.h" > #include "hw/sysbus.h" > +#include "hw/qdev-properties.h" > #include "exec/address-spaces.h" > #include "qemu/error-report.h" > #include "sysemu/qtest.h" > @@ -219,9 +220,15 @@ mips_mipssim_init(MachineState *machine) > * A single 16450 sits at offset 0x3f8. It is attached to > * MIPS CPU INT2, which is interrupt 4. > */ > - if (serial_hd(0)) > - serial_init(0x3f8, env->irq[4], 115200, serial_hd(0), > - get_system_io()); > + if (serial_hd(0)) { > + DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO); > + > + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > + qdev_prop_set_int32(dev, "instance-id", 0x3f8); > + qdev_init_nofail(dev); > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > + sysbus_add_io(SYS_BUS_DEVICE(dev), 0x3f8, &SERIAL_IO(dev)-> > serial.io); > + } > > if (nd_table[0].used) > /* MIPSnet uses the MIPS CPU INT0, which is interrupt 2. */ > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > index cf9cdafaee..40e35e6fff 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -109,8 +109,6 @@ void serial_set_frequency(SerialState *s, uint32_t > frequency); > #define TYPE_SERIAL_IO "serial-io" > #define SERIAL_IO(s) OBJECT_CHECK(SerialIO, (s), TYPE_SERIAL_IO) > > -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, > - Chardev *chr, MemoryRegion *system_io); > SerialMM *serial_mm_init(MemoryRegion *address_space, > hwaddr base, int regshift, > qemu_irq irq, int baudbase, > -- > 2.23.0.606.g08da6496b6 > > >
Hi On Mon, Nov 18, 2019 at 7:25 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > The function is specific to mipssim, let's inline it. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/char/serial.c | 16 ---------------- > > hw/mips/mips_mipssim.c | 13 ++++++++++--- > > include/hw/char/serial.h | 2 -- > > 3 files changed, 10 insertions(+), 21 deletions(-) > > > > diff --git a/hw/char/serial.c b/hw/char/serial.c > > index 84de2740a7..ca95e09ec9 100644 > > --- a/hw/char/serial.c > > +++ b/hw/char/serial.c > > @@ -1023,22 +1023,6 @@ static const TypeInfo serial_io_info = { > > .class_init = serial_io_class_init, > > }; > > > > -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, > > - Chardev *chr, MemoryRegion *system_io) > > -{ > > - SerialIO *self = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > > - > > - qdev_prop_set_uint32(DEVICE(self), "baudbase", baudbase); > > - qdev_prop_set_chr(DEVICE(self), "chardev", chr); > > - qdev_prop_set_int32(DEVICE(self), "instance-id", base); > > - qdev_init_nofail(DEVICE(self)); > > - > > - sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq); > > - memory_region_add_subregion(system_io, base, &self->serial.io); > > - > > - return self; > > -} > > - > > static Property serial_properties[] = { > > DEFINE_PROP_CHR("chardev", SerialState, chr), > > DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), > > diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c > > index 282bbecb24..26fb8fa4de 100644 > > --- a/hw/mips/mips_mipssim.c > > +++ b/hw/mips/mips_mipssim.c > > @@ -40,6 +40,7 @@ > > #include "hw/loader.h" > > #include "elf.h" > > #include "hw/sysbus.h" > > +#include "hw/qdev-properties.h" > > #include "exec/address-spaces.h" > > #include "qemu/error-report.h" > > #include "sysemu/qtest.h" > > @@ -219,9 +220,15 @@ mips_mipssim_init(MachineState *machine) > > * A single 16450 sits at offset 0x3f8. It is attached to > > * MIPS CPU INT2, which is interrupt 4. > > */ > > - if (serial_hd(0)) > > - serial_init(0x3f8, env->irq[4], 115200, serial_hd(0), > > - get_system_io()); > > + if (serial_hd(0)) { > > + DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO); > > + > > + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > > + qdev_prop_set_int32(dev, "instance-id", 0x3f8); > > + qdev_init_nofail(dev); > > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > > + sysbus_add_io(SYS_BUS_DEVICE(dev), 0x3f8, &SERIAL_IO(dev)->serial.io); > > + } > > The code being inlined here doesn't seem to be identical to the > code in the function above (it uses sysbus_add_io() rather than > memory_region_add_subregion()). sysbus_add_io() is memory_region_add_subregion(get_system_io(), ...). I went a step too fast, let's make it a seperate patch to clarify it then. > > It's also a bit ugly that the code has to reach into the > internals of the device to get the MemoryRegion pointer. > Would it work to have the device use sysbus_init_mmio() > in its init or realize method to register the IO port MemoryRegion, > and then we could use sysbus_mmio_get_region() to get the > MemoryRegion pointer ? Sure, seems fine to me. The caller must have extra knowledge of the memory region number though. This seems a bit fragile to me, compared to static compilation checking. > > (If that does work, we should someday refactor > sysbus_add_io() so that all the devices it's used with > register the MemoryRegions that way, and then sysbus_add_io() > can just take an 'int n' the way sysbus_mmio_map() does. > But not today.) I see, left for another day
diff --git a/hw/char/serial.c b/hw/char/serial.c index 84de2740a7..ca95e09ec9 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -1023,22 +1023,6 @@ static const TypeInfo serial_io_info = { .class_init = serial_io_class_init, }; -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, - Chardev *chr, MemoryRegion *system_io) -{ - SerialIO *self = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); - - qdev_prop_set_uint32(DEVICE(self), "baudbase", baudbase); - qdev_prop_set_chr(DEVICE(self), "chardev", chr); - qdev_prop_set_int32(DEVICE(self), "instance-id", base); - qdev_init_nofail(DEVICE(self)); - - sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq); - memory_region_add_subregion(system_io, base, &self->serial.io); - - return self; -} - static Property serial_properties[] = { DEFINE_PROP_CHR("chardev", SerialState, chr), DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c index 282bbecb24..26fb8fa4de 100644 --- a/hw/mips/mips_mipssim.c +++ b/hw/mips/mips_mipssim.c @@ -40,6 +40,7 @@ #include "hw/loader.h" #include "elf.h" #include "hw/sysbus.h" +#include "hw/qdev-properties.h" #include "exec/address-spaces.h" #include "qemu/error-report.h" #include "sysemu/qtest.h" @@ -219,9 +220,15 @@ mips_mipssim_init(MachineState *machine) * A single 16450 sits at offset 0x3f8. It is attached to * MIPS CPU INT2, which is interrupt 4. */ - if (serial_hd(0)) - serial_init(0x3f8, env->irq[4], 115200, serial_hd(0), - get_system_io()); + if (serial_hd(0)) { + DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO); + + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); + qdev_prop_set_int32(dev, "instance-id", 0x3f8); + qdev_init_nofail(dev); + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); + sysbus_add_io(SYS_BUS_DEVICE(dev), 0x3f8, &SERIAL_IO(dev)->serial.io); + } if (nd_table[0].used) /* MIPSnet uses the MIPS CPU INT0, which is interrupt 2. */ diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h index cf9cdafaee..40e35e6fff 100644 --- a/include/hw/char/serial.h +++ b/include/hw/char/serial.h @@ -109,8 +109,6 @@ void serial_set_frequency(SerialState *s, uint32_t frequency); #define TYPE_SERIAL_IO "serial-io" #define SERIAL_IO(s) OBJECT_CHECK(SerialIO, (s), TYPE_SERIAL_IO) -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, - Chardev *chr, MemoryRegion *system_io); SerialMM *serial_mm_init(MemoryRegion *address_space, hwaddr base, int regshift, qemu_irq irq, int baudbase,
The function is specific to mipssim, let's inline it. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/char/serial.c | 16 ---------------- hw/mips/mips_mipssim.c | 13 ++++++++++--- include/hw/char/serial.h | 2 -- 3 files changed, 10 insertions(+), 21 deletions(-)