diff mbox series

[v3,16/33] serial-mm: use sysbus facilities

Message ID 20191023173154.30051-17-marcandre.lureau@redhat.com
State New
Headers show
Series Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand

Commit Message

Marc-André Lureau Oct. 23, 2019, 5:31 p.m. UTC 
Make SerialMM a regular sysbus device, by registering the irq, and the
mmio region. Reexport the internal serial properties.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/char/serial.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

Comments

Peter Maydell Nov. 18, 2019, 3:09 p.m. UTC | #1 
On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Make SerialMM a regular sysbus device, by registering the irq, and the
> mmio region. Reexport the internal serial properties.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/char/serial.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 2f7667c30c..a40bc3ab81 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1074,21 +1074,18 @@ SerialMM *serial_mm_init(MemoryRegion *address_space,
>                              Chardev *chr, enum device_endian end)
>  {
>      SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
> -    SerialState *s = &self->serial;
> +    MemoryRegion *mr;
>
>      qdev_prop_set_uint8(DEVICE(self), "regshift", regshift);
> -    s->irq = irq;
> -    qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
> -    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
> -    qdev_prop_set_int32(DEVICE(s), "instance-id", base);
> -    qdev_prop_set_uint8(DEVICE(s), "endianness", end);
> -
> -    qdev_init_nofail(DEVICE(s));
> +    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_prop_set_uint8(DEVICE(self), "endianness", end);

(this last line should be in patch 15)

>      qdev_init_nofail(DEVICE(self));
>
> -    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self,
> -                          "serial", 8 << regshift);
> -    memory_region_add_subregion(address_space, base, &s->io);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(self), 0);
> +    memory_region_add_subregion(address_space, base, mr);
>
>      return self;
>  }
> @@ -1099,6 +1096,8 @@ static void serial_mm_instance_init(Object *o)
>
>      object_initialize_child(o, "serial", &self->serial, sizeof(self->serial),
>                              TYPE_SERIAL, &error_abort, NULL);
> +
> +    qdev_alias_all_properties(DEVICE(&self->serial), o);
>  }
>
>  static Property serial_mm_properties[] = {
> @@ -1107,11 +1106,25 @@ static Property serial_mm_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static void serial_mm_realize(DeviceState *dev, Error **errp)
> +{
> +    SerialMM *self = SERIAL_MM(dev);
> +    SerialState *s = &self->serial;

Again, 'self' isn't idiomatic in QOM methods.

> +
> +    qdev_init_nofail(DEVICE(s));
> +
> +    memory_region_init_io(&s->io, NULL, &serial_mm_ops[self->endianness], self,
> +                          "serial", 8 << self->regshift);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(self), &s->io);
> +    sysbus_init_irq(SYS_BUS_DEVICE(self), &self->serial.irq);
> +}
> +
>  static void serial_mm_class_init(ObjectClass *klass, void* data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      dc->props = serial_mm_properties;
> +    dc->realize = serial_mm_realize;
>  }
>
>  static const TypeInfo serial_mm_info = {
> --
> 2.23.0.606.g08da6496b6

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Marc-André Lureau Nov. 20, 2019, 8:30 a.m. UTC | #2 
Hi

On Mon, Nov 18, 2019 at 7:09 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
> >
> > Make SerialMM a regular sysbus device, by registering the irq, and the
> > mmio region. Reexport the internal serial properties.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/char/serial.c | 35 ++++++++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 2f7667c30c..a40bc3ab81 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -1074,21 +1074,18 @@ SerialMM *serial_mm_init(MemoryRegion *address_space,
> >                              Chardev *chr, enum device_endian end)
> >  {
> >      SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
> > -    SerialState *s = &self->serial;
> > +    MemoryRegion *mr;
> >
> >      qdev_prop_set_uint8(DEVICE(self), "regshift", regshift);
> > -    s->irq = irq;
> > -    qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
> > -    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
> > -    qdev_prop_set_int32(DEVICE(s), "instance-id", base);
> > -    qdev_prop_set_uint8(DEVICE(s), "endianness", end);
> > -
> > -    qdev_init_nofail(DEVICE(s));
> > +    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_prop_set_uint8(DEVICE(self), "endianness", end);
>
> (this last line should be in patch 15)
>
> >      qdev_init_nofail(DEVICE(self));
> >
> > -    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self,
> > -                          "serial", 8 << regshift);
> > -    memory_region_add_subregion(address_space, base, &s->io);
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq);
> > +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(self), 0);
> > +    memory_region_add_subregion(address_space, base, mr);
> >
> >      return self;
> >  }
> > @@ -1099,6 +1096,8 @@ static void serial_mm_instance_init(Object *o)
> >
> >      object_initialize_child(o, "serial", &self->serial, sizeof(self->serial),
> >                              TYPE_SERIAL, &error_abort, NULL);
> > +
> > +    qdev_alias_all_properties(DEVICE(&self->serial), o);
> >  }
> >
> >  static Property serial_mm_properties[] = {
> > @@ -1107,11 +1106,25 @@ static Property serial_mm_properties[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > +static void serial_mm_realize(DeviceState *dev, Error **errp)
> > +{
> > +    SerialMM *self = SERIAL_MM(dev);
> > +    SerialState *s = &self->serial;
>
> Again, 'self' isn't idiomatic in QOM methods.

I made my argument earlier about why I prefer "self" even though it's
not represented today in hw/

> > +
> > +    qdev_init_nofail(DEVICE(s));
> > +
> > +    memory_region_init_io(&s->io, NULL, &serial_mm_ops[self->endianness], self,
> > +                          "serial", 8 << self->regshift);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(self), &s->io);
> > +    sysbus_init_irq(SYS_BUS_DEVICE(self), &self->serial.irq);
> > +}
> > +
> >  static void serial_mm_class_init(ObjectClass *klass, void* data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >
> >      dc->props = serial_mm_properties;
> > +    dc->realize = serial_mm_realize;
> >  }
> >
> >  static const TypeInfo serial_mm_info = {
> > --
> > 2.23.0.606.g08da6496b6
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Does your r-b stands if I keep "self"? Or do you feel strongly about it?

thanks
Peter Maydell Nov. 20, 2019, 10:46 a.m. UTC | #3 
On Wed, 20 Nov 2019 at 08:30, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> On Mon, Nov 18, 2019 at 7:09 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > Otherwise
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Does your r-b stands if I keep "self"? Or do you feel strongly about it?

Yes, I do strongly think you need to change that (see argument in other thread).

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 2f7667c30c..a40bc3ab81 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1074,21 +1074,18 @@  SerialMM *serial_mm_init(MemoryRegion *address_space,
                             Chardev *chr, enum device_endian end)
 {
     SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM));
-    SerialState *s = &self->serial;
+    MemoryRegion *mr;
 
     qdev_prop_set_uint8(DEVICE(self), "regshift", regshift);
-    s->irq = irq;
-    qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase);
-    qdev_prop_set_chr(DEVICE(s), "chardev", chr);
-    qdev_prop_set_int32(DEVICE(s), "instance-id", base);
-    qdev_prop_set_uint8(DEVICE(s), "endianness", end);
-
-    qdev_init_nofail(DEVICE(s));
+    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_prop_set_uint8(DEVICE(self), "endianness", end);
     qdev_init_nofail(DEVICE(self));
 
-    memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self,
-                          "serial", 8 << regshift);
-    memory_region_add_subregion(address_space, base, &s->io);
+    sysbus_connect_irq(SYS_BUS_DEVICE(self), 0, irq);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(self), 0);
+    memory_region_add_subregion(address_space, base, mr);
 
     return self;
 }
@@ -1099,6 +1096,8 @@  static void serial_mm_instance_init(Object *o)
 
     object_initialize_child(o, "serial", &self->serial, sizeof(self->serial),
                             TYPE_SERIAL, &error_abort, NULL);
+
+    qdev_alias_all_properties(DEVICE(&self->serial), o);
 }
 
 static Property serial_mm_properties[] = {
@@ -1107,11 +1106,25 @@  static Property serial_mm_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void serial_mm_realize(DeviceState *dev, Error **errp)
+{
+    SerialMM *self = SERIAL_MM(dev);
+    SerialState *s = &self->serial;
+
+    qdev_init_nofail(DEVICE(s));
+
+    memory_region_init_io(&s->io, NULL, &serial_mm_ops[self->endianness], self,
+                          "serial", 8 << self->regshift);
+    sysbus_init_mmio(SYS_BUS_DEVICE(self), &s->io);
+    sysbus_init_irq(SYS_BUS_DEVICE(self), &self->serial.irq);
+}
+
 static void serial_mm_class_init(ObjectClass *klass, void* data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->props = serial_mm_properties;
+    dc->realize = serial_mm_realize;
 }
 
 static const TypeInfo serial_mm_info = {