Re: [PATCH v2 10/28] serial: add "base" property

[Marc-André Lureau]

Hi

On Tue, Oct 22, 2019 at 6:32 PM Peter Maydell <address@hidden> wrote:
>
> On Tue, 22 Oct 2019 at 17:23, Marc-André Lureau
> <address@hidden> wrote:
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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.