| Message ID | 20191120152442.26657-13-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Memory mapped serial device is in fact a sysbus device. The following > patches will make use of sysbus facilities for resource and > registration. In particular, "serial-mm: use sysbus facilities" will > move internal serial realization to serial_mm_realize callback to > follow qdev best practices. What goes wrong if you just put the realize of smm->serial in the right place to start with ? thanks -- PMM
On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > Memory mapped serial device is in fact a sysbus device. The following > > patches will make use of sysbus facilities for resource and > > registration. In particular, "serial-mm: use sysbus facilities" will > > move internal serial realization to serial_mm_realize callback to > > follow qdev best practices. > > What goes wrong if you just put the realize of smm->serial in > the right place to start with ? You mean squash the following patches? Sometime I go too fast, sometime it's too slow. Decide what you prefer, this doesnt' matter much to me.
On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau > > <marcandre.lureau@redhat.com> wrote: > > > > > > Memory mapped serial device is in fact a sysbus device. The following > > > patches will make use of sysbus facilities for resource and > > > registration. In particular, "serial-mm: use sysbus facilities" will > > > move internal serial realization to serial_mm_realize callback to > > > follow qdev best practices. > > > > What goes wrong if you just put the realize of smm->serial in > > the right place to start with ? > > You mean squash the following patches? No, I meant just having this patch have static void serial_mm_realize(DeviceState *dev, Error **errp) { SerialMM *smm = SERIAL_MM(dev); SerialState *s = &smm->serial; object_property_set_bool(OBJECT(dev), true, "realized", &err); if (err) { error_propagate(errp, err); return; } } and + dc->realize = serial_mm_realize; rather than manually doing the qdev_init_nofail() in serial_mm_init(). This seems to me like an integral part of the change to doing the init of the subdevice in the init method, so it would be better unless there's a reason why it breaks something. The rest of patch 15 (which is what currently makes the equivalent change to realize) is all about passing through the properties and exposing the sysbus MMIO/irq regions and should stay a separate patch. (setting the 'realized' property is better in a realize method than using qdev_init_nofail() because it means we can propagate any error outward rather than killing qemu.) thanks -- PMM
Hi On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau > > > <marcandre.lureau@redhat.com> wrote: > > > > > > > > Memory mapped serial device is in fact a sysbus device. The following > > > > patches will make use of sysbus facilities for resource and > > > > registration. In particular, "serial-mm: use sysbus facilities" will > > > > move internal serial realization to serial_mm_realize callback to > > > > follow qdev best practices. > > > > > > What goes wrong if you just put the realize of smm->serial in > > > the right place to start with ? > > > > You mean squash the following patches? > > No, I meant just having this patch have > > static void serial_mm_realize(DeviceState *dev, Error **errp) > { > SerialMM *smm = SERIAL_MM(dev); > SerialState *s = &smm->serial; > > object_property_set_bool(OBJECT(dev), true, "realized", &err); > if (err) { > error_propagate(errp, err); > return; > } > } > > and > > + dc->realize = serial_mm_realize; > > rather than manually doing the qdev_init_nofail() > in serial_mm_init(). This seems to me like an integral > part of the change to doing the init of the subdevice in the > init method, so it would be better unless there's a reason > why it breaks something. The rest of patch 15 (which is > what currently makes the equivalent change to realize) is all > about passing through the properties and exposing the > sysbus MMIO/irq regions and should stay a separate patch. > > (setting the 'realized' property is better in a realize method > than using qdev_init_nofail() because it means we can propagate > any error outward rather than killing qemu.) Ok, but I implemented realize and moved serial init in "serial: make SerialIO a sysbus device". I propose to add another patch to follow your suggestion to use set_boot("realize", true, errp) on top.
On Thu, 21 Nov 2019 at 18:51, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau > > <marcandre.lureau@gmail.com> wrote: > > > > > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau > > > > <marcandre.lureau@redhat.com> wrote: > > > > > > > > > > Memory mapped serial device is in fact a sysbus device. The following > > > > > patches will make use of sysbus facilities for resource and > > > > > registration. In particular, "serial-mm: use sysbus facilities" will > > > > > move internal serial realization to serial_mm_realize callback to > > > > > follow qdev best practices. > > > > > > > > What goes wrong if you just put the realize of smm->serial in > > > > the right place to start with ? > > > > > > You mean squash the following patches? > > > > No, I meant just having this patch have > > > > static void serial_mm_realize(DeviceState *dev, Error **errp) > > { > > SerialMM *smm = SERIAL_MM(dev); > > SerialState *s = &smm->serial; > > > > object_property_set_bool(OBJECT(dev), true, "realized", &err); > > if (err) { > > error_propagate(errp, err); > > return; > > } > > } > > > > and > > > > + dc->realize = serial_mm_realize; > > > > rather than manually doing the qdev_init_nofail() > > in serial_mm_init(). This seems to me like an integral > > part of the change to doing the init of the subdevice in the > > init method, so it would be better unless there's a reason > > why it breaks something. The rest of patch 15 (which is > > what currently makes the equivalent change to realize) is all > > about passing through the properties and exposing the > > sysbus MMIO/irq regions and should stay a separate patch. > > > > (setting the 'realized' property is better in a realize method > > than using qdev_init_nofail() because it means we can propagate > > any error outward rather than killing qemu.) > > Ok, but I implemented realize and moved serial init in "serial: make > SerialIO a sysbus device". That patch is for the TYPE_SERIAL_IO device, isn't it? It changes both serial_io_instance_init and serial_io_realize to do the instance init and realize of the TYPE_SERIAL device, so it doesn't have the same "only doing half a change" issue that this patch has for TYPE_SERIAL_MM. thanks -- PMM
On Fri, Nov 22, 2019 at 2:11 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 21 Nov 2019 at 18:51, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > > > Hi > > > > On Thu, Nov 21, 2019 at 10:24 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > On Thu, 21 Nov 2019 at 18:15, Marc-André Lureau > > > <marcandre.lureau@gmail.com> wrote: > > > > > > > > On Thu, Nov 21, 2019 at 5:47 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > > > > > > > On Wed, 20 Nov 2019 at 15:27, Marc-André Lureau > > > > > <marcandre.lureau@redhat.com> wrote: > > > > > > > > > > > > Memory mapped serial device is in fact a sysbus device. The following > > > > > > patches will make use of sysbus facilities for resource and > > > > > > registration. In particular, "serial-mm: use sysbus facilities" will > > > > > > move internal serial realization to serial_mm_realize callback to > > > > > > follow qdev best practices. > > > > > > > > > > What goes wrong if you just put the realize of smm->serial in > > > > > the right place to start with ? > > > > > > > > You mean squash the following patches? > > > > > > No, I meant just having this patch have > > > > > > static void serial_mm_realize(DeviceState *dev, Error **errp) > > > { > > > SerialMM *smm = SERIAL_MM(dev); > > > SerialState *s = &smm->serial; > > > > > > object_property_set_bool(OBJECT(dev), true, "realized", &err); > > > if (err) { > > > error_propagate(errp, err); > > > return; > > > } > > > } > > > > > > and > > > > > > + dc->realize = serial_mm_realize; > > > > > > rather than manually doing the qdev_init_nofail() > > > in serial_mm_init(). This seems to me like an integral > > > part of the change to doing the init of the subdevice in the > > > init method, so it would be better unless there's a reason > > > why it breaks something. The rest of patch 15 (which is > > > what currently makes the equivalent change to realize) is all > > > about passing through the properties and exposing the > > > sysbus MMIO/irq regions and should stay a separate patch. > > > > > > (setting the 'realized' property is better in a realize method > > > than using qdev_init_nofail() because it means we can propagate > > > any error outward rather than killing qemu.) > > > > Ok, but I implemented realize and moved serial init in "serial: make > > SerialIO a sysbus device". > > That patch is for the TYPE_SERIAL_IO device, isn't it? It > changes both serial_io_instance_init and serial_io_realize > to do the instance init and realize of the TYPE_SERIAL device, > so it doesn't have the same "only doing half a change" issue > that this patch has for TYPE_SERIAL_MM. > Ok, I got it, thanks.
diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c index 13e4f43c4c..e8da933378 100644 --- a/hw/char/omap_uart.c +++ b/hw/char/omap_uart.c @@ -27,7 +27,7 @@ struct omap_uart_s { MemoryRegion iomem; hwaddr base; - SerialState *serial; /* TODO */ + SerialMM *serial; /* TODO */ struct omap_target_agent_s *ta; omap_clk fclk; qemu_irq irq; diff --git a/hw/char/serial.c b/hw/char/serial.c index 2d8471dbbb..cb9cbd5146 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -1031,16 +1031,16 @@ static const TypeInfo serial_info = { static uint64_t serial_mm_read(void *opaque, hwaddr addr, unsigned size) { - SerialState *s = opaque; - return serial_ioport_read(s, addr >> s->it_shift, 1); + SerialMM *s = SERIAL_MM(opaque); + return serial_ioport_read(&s->serial, addr >> s->it_shift, 1); } static void serial_mm_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { - SerialState *s = opaque; + SerialMM *s = SERIAL_MM(opaque); value &= 255; - serial_ioport_write(s, addr >> s->it_shift, value, 1); + serial_ioport_write(&s->serial, addr >> s->it_shift, value, 1); } static const MemoryRegionOps serial_mm_ops[3] = { @@ -1067,30 +1067,49 @@ static const MemoryRegionOps serial_mm_ops[3] = { }, }; -SerialState *serial_mm_init(MemoryRegion *address_space, +SerialMM *serial_mm_init(MemoryRegion *address_space, hwaddr base, int it_shift, qemu_irq irq, int baudbase, Chardev *chr, enum device_endian end) { - DeviceState *dev = DEVICE(object_new(TYPE_SERIAL)); - SerialState *s = SERIAL(dev); + SerialMM *smm = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM)); + SerialState *s = &smm->serial; - s->it_shift = it_shift; + smm->it_shift = it_shift; s->irq = irq; - qdev_prop_set_uint32(dev, "baudbase", baudbase); - qdev_prop_set_chr(dev, "chardev", chr); - qdev_set_legacy_instance_id(dev, base, 2); - qdev_init_nofail(dev); + qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase); + qdev_prop_set_chr(DEVICE(s), "chardev", chr); + qdev_set_legacy_instance_id(DEVICE(s), base, 2); + + qdev_init_nofail(DEVICE(s)); + qdev_init_nofail(DEVICE(smm)); - memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s, + memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], smm, "serial", 8 << it_shift); memory_region_add_subregion(address_space, base, &s->io); - return s; + + return smm; +} + +static void serial_mm_instance_init(Object *o) +{ + SerialMM *smm = SERIAL_MM(o); + + object_initialize_child(o, "serial", &smm->serial, sizeof(smm->serial), + TYPE_SERIAL, &error_abort, NULL); } +static const TypeInfo serial_mm_info = { + .name = TYPE_SERIAL_MM, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_init = serial_mm_instance_init, + .instance_size = sizeof(SerialMM), +}; + static void serial_register_types(void) { type_register_static(&serial_info); + type_register_static(&serial_mm_info); } type_init(serial_register_types) diff --git a/hw/mips/boston.c b/hw/mips/boston.c index ca7d813a52..23fdd5ec6a 100644 --- a/hw/mips/boston.c +++ b/hw/mips/boston.c @@ -50,7 +50,7 @@ typedef struct { MachineState *mach; MIPSCPSState cps; - SerialState *uart; + SerialMM *uart; CharBackend lcd_display; char lcd_content[8]; diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c index 92e9ca5bfa..89b5e3123a 100644 --- a/hw/mips/mips_malta.c +++ b/hw/mips/mips_malta.c @@ -83,7 +83,7 @@ typedef struct { uint32_t i2csel; CharBackend display; char display_text[9]; - SerialState *uart; + SerialMM *uart; bool display_inited; } MaltaFPGAState; diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h index 548944b06a..730165347c 100644 --- a/include/hw/char/serial.h +++ b/include/hw/char/serial.h @@ -57,7 +57,6 @@ typedef struct SerialState { qemu_irq irq; CharBackend chr; int last_break_enable; - int it_shift; uint32_t baudbase; uint32_t tsr_retry; guint watch_tag; @@ -80,6 +79,14 @@ typedef struct SerialState { MemoryRegion io; } SerialState; +typedef struct SerialMM { + SysBusDevice parent; + + SerialState serial; + + int it_shift; +} SerialMM; + extern const VMStateDescription vmstate_serial; extern const MemoryRegionOps serial_io_ops; @@ -88,12 +95,15 @@ void serial_set_frequency(SerialState *s, uint32_t frequency); #define TYPE_SERIAL "serial" #define SERIAL(s) OBJECT_CHECK(SerialState, (s), TYPE_SERIAL) +#define TYPE_SERIAL_MM "serial-mm" +#define SERIAL_MM(s) OBJECT_CHECK(SerialMM, (s), TYPE_SERIAL_MM) + SerialState *serial_init(int base, qemu_irq irq, int baudbase, Chardev *chr, MemoryRegion *system_io); -SerialState *serial_mm_init(MemoryRegion *address_space, - hwaddr base, int it_shift, - qemu_irq irq, int baudbase, - Chardev *chr, enum device_endian end); +SerialMM *serial_mm_init(MemoryRegion *address_space, + hwaddr base, int it_shift, + qemu_irq irq, int baudbase, + Chardev *chr, enum device_endian end); /* serial-isa.c */
Memory mapped serial device is in fact a sysbus device. The following patches will make use of sysbus facilities for resource and registration. In particular, "serial-mm: use sysbus facilities" will move internal serial realization to serial_mm_realize callback to follow qdev best practices. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/char/omap_uart.c | 2 +- hw/char/serial.c | 47 ++++++++++++++++++++++++++++------------ hw/mips/boston.c | 2 +- hw/mips/mips_malta.c | 2 +- include/hw/char/serial.h | 20 ++++++++++++----- 5 files changed, 51 insertions(+), 22 deletions(-)