| Message ID | 20191022162137.27161-11-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Tue, 22 Oct 2019 at 17:23, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 3 +++ > include/hw/char/serial.h | 1 + > 2 files changed, 4 insertions(+) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index 0ae52ec60c..4748a2b023 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -990,6 +990,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, > qdev_connect_gpio_out_named(dev, "serial-irq", 0, irq); > qdev_prop_set_uint32(dev, "baudbase", baudbase); > qdev_prop_set_chr(dev, "chardev", chr); > + qdev_prop_set_uint64(dev, "base", base); > serial_realize_core(s, &error_fatal); > qdev_set_legacy_instance_id(dev, base, 2); > qdev_init_nofail(dev); > @@ -1003,6 +1004,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, > static Property serial_properties[] = { > DEFINE_PROP_CHR("chardev", SerialState, chr), > DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), > + DEFINE_PROP_UINT64("base", SerialState, base, 0), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -1083,6 +1085,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, > qdev_prop_set_uint32(dev, "baudbase", baudbase); > qdev_prop_set_chr(dev, "chardev", chr); > qdev_prop_set_uint8(dev, "regshift", regshift); > + qdev_prop_set_uint64(dev, "base", base); > > serial_realize_core(s, &error_fatal); > qdev_set_legacy_instance_id(DEVICE(s), base, 2); > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > index ecbd3f1b40..6e9c9768ed 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -77,6 +77,7 @@ typedef struct SerialState { > > QEMUTimer *modem_status_poll; > MemoryRegion io; > + uint64_t base; > } SerialState; Devices shouldn't have properties to set their MMIO base address -- instead the memory-mapped serial device should be a child of TYPE_SYSBUS_DEVICE, and should provide a sysbus mmio region, which users of the device can map at the address they want to map the registers. thanks -- PMM
Hi On Tue, Oct 22, 2019 at 6:32 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 22 Oct 2019 at 17:23, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/char/serial.c | 3 +++ > > include/hw/char/serial.h | 1 + > > 2 files changed, 4 insertions(+) > > > > diff --git a/hw/char/serial.c b/hw/char/serial.c > > index 0ae52ec60c..4748a2b023 100644 > > --- a/hw/char/serial.c > > +++ b/hw/char/serial.c > > @@ -990,6 +990,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, > > qdev_connect_gpio_out_named(dev, "serial-irq", 0, irq); > > qdev_prop_set_uint32(dev, "baudbase", baudbase); > > qdev_prop_set_chr(dev, "chardev", chr); > > + qdev_prop_set_uint64(dev, "base", base); > > serial_realize_core(s, &error_fatal); > > qdev_set_legacy_instance_id(dev, base, 2); > > qdev_init_nofail(dev); > > @@ -1003,6 +1004,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, > > static Property serial_properties[] = { > > DEFINE_PROP_CHR("chardev", SerialState, chr), > > DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), > > + DEFINE_PROP_UINT64("base", SerialState, base, 0), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > @@ -1083,6 +1085,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, > > qdev_prop_set_uint32(dev, "baudbase", baudbase); > > qdev_prop_set_chr(dev, "chardev", chr); > > qdev_prop_set_uint8(dev, "regshift", regshift); > > + qdev_prop_set_uint64(dev, "base", base); > > > > serial_realize_core(s, &error_fatal); > > qdev_set_legacy_instance_id(DEVICE(s), base, 2); > > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > > index ecbd3f1b40..6e9c9768ed 100644 > > --- a/include/hw/char/serial.h > > +++ b/include/hw/char/serial.h > > @@ -77,6 +77,7 @@ typedef struct SerialState { > > > > QEMUTimer *modem_status_poll; > > MemoryRegion io; > > + uint64_t base; > > } SerialState; > > Devices shouldn't have properties to set their MMIO base > address -- instead the memory-mapped serial device should > be a child of TYPE_SYSBUS_DEVICE, and should provide > a sysbus mmio region, which users of the device can > map at the address they want to map the registers. Can we make serial a sysbus device? It seems to be embedded from various places, in various buses. Not sure that makes sense, please advise me :) "base" is mostly needed to set qdev_set_legacy_instance_id(), I should have added a commit comment. Otherwise, it can be passed to serial_mm_connect() directly.
On Tue, 22 Oct 2019 at 17:42, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Hi > > On Tue, Oct 22, 2019 at 6:32 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 22 Oct 2019 at 17:23, Marc-André Lureau > > <marcandre.lureau@redhat.com> wrote: > > Devices shouldn't have properties to set their MMIO base > > address -- instead the memory-mapped serial device should > > be a child of TYPE_SYSBUS_DEVICE, and should provide > > a sysbus mmio region, which users of the device can > > map at the address they want to map the registers. > > > Can we make serial a sysbus device? It seems to be embedded from > various places, in various buses. Not sure that makes sense, please > advise me :) You want the SerialMMState to be is-a sysbus-device, and to has-a SerialState, I think. > "base" is mostly needed to set qdev_set_legacy_instance_id(), I should > have added a commit comment. Otherwise, it can be passed to > serial_mm_connect() directly. serial_mm_connect() shouldn't exist at all, though -- we want to end up with a normal sysbus device whose users create it and wire up its MMIO and IRQ in the same way they do any other sysbus device. thanks -- PMM
diff --git a/hw/char/serial.c b/hw/char/serial.c index 0ae52ec60c..4748a2b023 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -990,6 +990,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, qdev_connect_gpio_out_named(dev, "serial-irq", 0, irq); qdev_prop_set_uint32(dev, "baudbase", baudbase); qdev_prop_set_chr(dev, "chardev", chr); + qdev_prop_set_uint64(dev, "base", base); serial_realize_core(s, &error_fatal); qdev_set_legacy_instance_id(dev, base, 2); qdev_init_nofail(dev); @@ -1003,6 +1004,7 @@ SerialState *serial_init(int base, qemu_irq irq, int baudbase, static Property serial_properties[] = { DEFINE_PROP_CHR("chardev", SerialState, chr), DEFINE_PROP_UINT32("baudbase", SerialState, baudbase, 115200), + DEFINE_PROP_UINT64("base", SerialState, base, 0), DEFINE_PROP_END_OF_LIST(), }; @@ -1083,6 +1085,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space, qdev_prop_set_uint32(dev, "baudbase", baudbase); qdev_prop_set_chr(dev, "chardev", chr); qdev_prop_set_uint8(dev, "regshift", regshift); + qdev_prop_set_uint64(dev, "base", base); serial_realize_core(s, &error_fatal); qdev_set_legacy_instance_id(DEVICE(s), base, 2); diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h index ecbd3f1b40..6e9c9768ed 100644 --- a/include/hw/char/serial.h +++ b/include/hw/char/serial.h @@ -77,6 +77,7 @@ typedef struct SerialState { QEMUTimer *modem_status_poll; MemoryRegion io; + uint64_t base; } SerialState; typedef struct SerialMMState {
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/char/serial.c | 3 +++ include/hw/char/serial.h | 1 + 2 files changed, 4 insertions(+)