| Message ID | 20191120152442.26657-18-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On 11/20/19 4:24 PM, Marc-André Lureau 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 | 15 ++++++++++++--- > include/hw/char/serial.h | 2 -- > 3 files changed, 12 insertions(+), 21 deletions(-) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > - > - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); > - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); > - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); > - qdev_init_nofail(DEVICE(sio)); > - > - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); > - memory_region_add_subregion(system_io, base, &sio->serial.io); > - > - return sio; > -} > - > 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..bfafa4d7e9 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,17 @@ 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_uint32(DEVICE(dev), "baudbase", 115200); > + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > + qdev_set_legacy_instance_id(dev, 0x3f8, 2); > + qdev_init_nofail(dev); > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > + memory_region_add_subregion(get_system_io(), 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 d356ba838c..535fa23a2b 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -108,8 +108,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, > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Wednesday, November 20, 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 | 15 ++++++++++++--- > include/hw/char/serial.h | 2 -- > 3 files changed, 12 insertions(+), 21 deletions(-) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > - > - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); > - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); > - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); > - qdev_init_nofail(DEVICE(sio)); > - > - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); > - memory_region_add_subregion(system_io, base, &sio->serial.io); > - > - return sio; > -} > - > 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..bfafa4d7e9 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,17 @@ 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_uint32(DEVICE(dev), "baudbase", 115200); > + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > + qdev_set_legacy_instance_id(dev, 0x3f8, 2); > + qdev_init_nofail(dev); > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > + memory_region_add_subregion(get_system_io(), 0x3f8, > + &SERIAL_IO(dev)->serial.io); > + } Please explain why the code in the deleted function and the new function are not identical. Why is the new code better? Thanks, Aleksandar > > 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 d356ba838c..535fa23a2b 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -108,8 +108,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.24.0 > > >
Hi On Mon, Nov 25, 2019 at 2:12 PM Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote: > > > > On Wednesday, November 20, 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 | 15 ++++++++++++--- >> include/hw/char/serial.h | 2 -- >> 3 files changed, 12 insertions(+), 21 deletions(-) >> >> diff --git a/hw/char/serial.c b/hw/char/serial.c >> index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); >> - >> - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); >> - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); >> - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); >> - qdev_init_nofail(DEVICE(sio)); >> - >> - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); >> - memory_region_add_subregion(system_io, base, &sio->serial.io); >> - >> - return sio; >> -} >> - >> 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..bfafa4d7e9 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,17 @@ 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)) a >> - 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_uint32(DEVICE(dev), "baudbase", 115200); >> + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); >> + qdev_set_legacy_instance_id(dev, 0x3f8, 2); >> + qdev_init_nofail(dev); >> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); >> + memory_region_add_subregion(get_system_io(), 0x3f8, >> + &SERIAL_IO(dev)->serial.io); >> + } > > > > Please explain why the code in the deleted function and the new function are not identical. Why is the new code better? What differences do you spot? serial_init() looks very generic, but it is not, and this is confusing. QDev users should call qdev_create(). If there is a single user, it is better to inline the code where it is being used, or move the function there.
On Monday, November 25, 2019, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > Hi > > On Mon, Nov 25, 2019 at 2:12 PM Aleksandar Markovic > <aleksandar.m.mail@gmail.com> wrote: > > > > > > > > On Wednesday, November 20, 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 | 15 ++++++++++++--- > >> include/hw/char/serial.h | 2 -- > >> 3 files changed, 12 insertions(+), 21 deletions(-) > >> > >> diff --git a/hw/char/serial.c b/hw/char/serial.c > >> index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > >> - > >> - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); > >> - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); > >> - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); > >> - qdev_init_nofail(DEVICE(sio)); > >> - > >> - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); > >> - memory_region_add_subregion(system_io, base, &sio->serial.io); > >> - > >> - return sio; > >> -} > >> - > >> 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..bfafa4d7e9 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,17 @@ 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)) a > >> - 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_uint32(DEVICE(dev), "baudbase", 115200); > >> + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > >> + qdev_set_legacy_instance_id(dev, 0x3f8, 2); > >> + qdev_init_nofail(dev); > >> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > >> + memory_region_add_subregion(get_system_io(), 0x3f8, > >> + &SERIAL_IO(dev)->serial.io); > >> + } > > > > > > > > Please explain why the code in the deleted function and the new function > are not identical. Why is the new code better? > > What differences do you spot? > > serial_init() looks very generic, but it is not, and this is > confusing. QDev users should call qdev_create(). > > If there is a single user, it is better to inline the code where it is > being used, or move the function there. > > Marc-Andre, you misunderstood me: I am not complaining about inlining at all, I would do the same if I were you. I just see you expressed the same thing from former function in a slightly different way in the inlined version, and they look functionally equivalent to me. But I suppose the new way is better, and I want to know why. Is it a more standard way, or something even more? Please understand that I don't come from device emulation background - I want to learn a little bit about device-related QEMU customs and conventions, that is all. :) For example, in old version: >> -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, >> - Chardev *chr, MemoryRegion *system_io) >> -{ >> - SerialIO *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); >> - >> - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); >> - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); >> - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); >> - qdev_init_nofail(DEVICE(sio)); >> - >> - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); >> - memory_region_add_subregion(system_io, base, &sio->serial.io); >> - >> - return sio; >> -} in new version: >> + if (serial_hd(0)) { >> + DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO); >> + >> + qdev_prop_set_uint32(DEVICE(dev), "baudbase", 115200); >> + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); <-- you didn't use here DEVICE(), while it is used in the previous line, and also in the old version, and similar questions >> + qdev_set_legacy_instance_id(dev, 0x3f8, 2); >> + qdev_init_nofail(dev); >> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); >> + memory_region_add_subregion(get_system_io(), 0x3f8, >> + &SERIAL_IO(dev)->serial.io); >> + } For you all these details are probably perfectly natural and obvois, but not for me. Yours, Aleksandar
Hi On Mon, Nov 25, 2019 at 5:33 PM Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote: > > > > On Monday, November 25, 2019, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: >> >> Hi >> >> On Mon, Nov 25, 2019 at 2:12 PM Aleksandar Markovic >> <aleksandar.m.mail@gmail.com> wrote: >> > >> > >> > >> > On Wednesday, November 20, 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 | 15 ++++++++++++--- >> >> include/hw/char/serial.h | 2 -- >> >> 3 files changed, 12 insertions(+), 21 deletions(-) >> >> >> >> diff --git a/hw/char/serial.c b/hw/char/serial.c >> >> index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); >> >> - >> >> - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); >> >> - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); >> >> - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); >> >> - qdev_init_nofail(DEVICE(sio)); >> >> - >> >> - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); >> >> - memory_region_add_subregion(system_io, base, &sio->serial.io); >> >> - >> >> - return sio; >> >> -} >> >> - >> >> 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..bfafa4d7e9 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,17 @@ 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)) a >> >> - 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_uint32(DEVICE(dev), "baudbase", 115200); >> >> + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); >> >> + qdev_set_legacy_instance_id(dev, 0x3f8, 2); >> >> + qdev_init_nofail(dev); >> >> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); >> >> + memory_region_add_subregion(get_system_io(), 0x3f8, >> >> + &SERIAL_IO(dev)->serial.io); >> >> + } >> > >> > >> > >> > Please explain why the code in the deleted function and the new function are not identical. Why is the new code better? >> >> What differences do you spot? >> >> serial_init() looks very generic, but it is not, and this is >> confusing. QDev users should call qdev_create(). >> >> If there is a single user, it is better to inline the code where it is >> being used, or move the function there. >> > > Marc-Andre, you misunderstood me: I am not complaining about inlining at all, I would do the same if I were you. > > I just see you expressed the same thing from former function in a slightly different way in the inlined version, and they look functionally equivalent to me. But I suppose the new way is better, and I want to know why. Is it a more standard way, or something even more? Please understand that I don't come from device emulation background - I want to learn a little bit about device-related QEMU customs and conventions, that is all. :) > > For example, > > in old version: > > >> -SerialIO *serial_init(int base, qemu_irq irq, int baudbase, > >> - Chardev *chr, MemoryRegion *system_io) > >> -{ > >> - SerialIO *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > >> - > >> - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); > >> - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); > >> - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); > >> - qdev_init_nofail(DEVICE(sio)); > >> - > >> - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); > >> - memory_region_add_subregion(system_io, base, &sio->serial.io); > >> - > >> - return sio; > >> -} > > in new version: > > >> + if (serial_hd(0)) { > >> + DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO); > >> + > >> + qdev_prop_set_uint32(DEVICE(dev), "baudbase", 115200); > >> + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); <-- you didn't use here DEVICE(), while it is used in the previous line, and also in the old version, and similar questions > >> + qdev_set_legacy_instance_id(dev, 0x3f8, 2); > >> + qdev_init_nofail(dev); > >> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > >> + memory_region_add_subregion(get_system_io(), 0x3f8, > >> + &SERIAL_IO(dev)->serial.io); > >> + } > > For you all these details are probably perfectly natural and obvois, but not for me. Oh, there is no big difference between a variable using the base type DeviceState * or the instance type SerialIO *. Basically, you need a variable to hold the instance, and then you cast it with DEVICE() or SERIAL_IO() or event SYS_BUS_DEVICE() as necessary, depending on what type the function you call expect. I probably picked DeviceState* because it is the most commonly used, and it avoids having plenty of DEVICE() casts.
On Wednesday, November 20, 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 | 15 ++++++++++++--- > include/hw/char/serial.h | 2 -- > 3 files changed, 12 insertions(+), 21 deletions(-) > > Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com> > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); > - > - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); > - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); > - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); > - qdev_init_nofail(DEVICE(sio)); > - > - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); > - memory_region_add_subregion(system_io, base, &sio->serial.io); > - > - return sio; > -} > - > 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..bfafa4d7e9 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,17 @@ 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_uint32(DEVICE(dev), "baudbase", 115200); > + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); > + qdev_set_legacy_instance_id(dev, 0x3f8, 2); > + qdev_init_nofail(dev); > + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); > + memory_region_add_subregion(get_system_io(), 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 d356ba838c..535fa23a2b 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -108,8 +108,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.24.0 > > >
diff --git a/hw/char/serial.c b/hw/char/serial.c index 164146ede8..23f0b02516 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 *sio = SERIAL_IO(qdev_create(NULL, TYPE_SERIAL_IO)); - - qdev_prop_set_uint32(DEVICE(sio), "baudbase", baudbase); - qdev_prop_set_chr(DEVICE(sio), "chardev", chr); - qdev_set_legacy_instance_id(DEVICE(sio), base, 2); - qdev_init_nofail(DEVICE(sio)); - - sysbus_connect_irq(SYS_BUS_DEVICE(sio), 0, irq); - memory_region_add_subregion(system_io, base, &sio->serial.io); - - return sio; -} - 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..bfafa4d7e9 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,17 @@ 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_uint32(DEVICE(dev), "baudbase", 115200); + qdev_prop_set_chr(dev, "chardev", serial_hd(0)); + qdev_set_legacy_instance_id(dev, 0x3f8, 2); + qdev_init_nofail(dev); + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, env->irq[4]); + memory_region_add_subregion(get_system_io(), 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 d356ba838c..535fa23a2b 100644 --- a/include/hw/char/serial.h +++ b/include/hw/char/serial.h @@ -108,8 +108,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 | 15 ++++++++++++--- include/hw/char/serial.h | 2 -- 3 files changed, 12 insertions(+), 21 deletions(-)