| Message ID | 20191023173154.30051-16-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Add a qdev property for endianness, so memory region setup can be done > in realize. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 2 ++ > include/hw/char/serial.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index c28cfc94fd..2f7667c30c 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -1081,6 +1081,7 @@ SerialMM *serial_mm_init(MemoryRegion *address_space, > 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_init_nofail(DEVICE(self)); > @@ -1102,6 +1103,7 @@ static void serial_mm_instance_init(Object *o) > > static Property serial_mm_properties[] = { > DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0), > + DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN), Again, a brief comment documenting the property here would be nice. > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > index 759c85976d..2d0802a909 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -86,6 +86,7 @@ typedef struct SerialMM { > SerialState serial; > > uint8_t regshift; > + uint8_t endianness; > } SerialMM; (The type-checking on the property-setting macros doesn't let us define this as 'enum device_endian'.) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Add a qdev property for endianness, so memory region setup can be done > in realize. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 2 ++ > include/hw/char/serial.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index c28cfc94fd..2f7667c30c 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -1081,6 +1081,7 @@ SerialMM *serial_mm_init(MemoryRegion *address_space, > 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_init_nofail(DEVICE(self)); > @@ -1102,6 +1103,7 @@ static void serial_mm_instance_init(Object *o) > > static Property serial_mm_properties[] = { > DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0), > + DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN), > DEFINE_PROP_END_OF_LIST(), > }; ...on reading patch 16, I just noticed that here in patch 15 you define the 'endianness' property on the SerialMM object, but you're trying to set it on the SerialState object. This bug then gets fixed in passing in patch 16, but we should just be setting it on the right object to start with. thanks -- PMM
On Mon, Nov 18, 2019 at 7:07 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 23 Oct 2019 at 18:34, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > Add a qdev property for endianness, so memory region setup can be done > > in realize. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/char/serial.c | 2 ++ > > include/hw/char/serial.h | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/hw/char/serial.c b/hw/char/serial.c > > index c28cfc94fd..2f7667c30c 100644 > > --- a/hw/char/serial.c > > +++ b/hw/char/serial.c > > @@ -1081,6 +1081,7 @@ SerialMM *serial_mm_init(MemoryRegion *address_space, > > 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_init_nofail(DEVICE(self)); > > @@ -1102,6 +1103,7 @@ static void serial_mm_instance_init(Object *o) > > > > static Property serial_mm_properties[] = { > > DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0), > > + DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN), > > DEFINE_PROP_END_OF_LIST(), > > }; > > ...on reading patch 16, I just noticed that here in patch 15 > you define the 'endianness' property on the SerialMM object, but > you're trying to set it on the SerialState object. This bug then > gets fixed in passing in patch 16, but we should just be > setting it on the right object to start with. Correct! fixed. thanks
On 10/23/19 7:31 PM, Marc-André Lureau wrote: > Add a qdev property for endianness, so memory region setup can be done > in realize. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/serial.c | 2 ++ > include/hw/char/serial.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/hw/char/serial.c b/hw/char/serial.c > index c28cfc94fd..2f7667c30c 100644 > --- a/hw/char/serial.c > +++ b/hw/char/serial.c > @@ -1081,6 +1081,7 @@ SerialMM *serial_mm_init(MemoryRegion *address_space, > 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_init_nofail(DEVICE(self)); > @@ -1102,6 +1103,7 @@ static void serial_mm_instance_init(Object *o) > > static Property serial_mm_properties[] = { > DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0), > + DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN), The endianess is related to how the chipset is wired, but is not an intrinsic part of it. Previously, serial_mm_init() was taking care of doing the correct wiring. Now it seems the endianess is a property from the hardware. Anyway: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h > index 759c85976d..2d0802a909 100644 > --- a/include/hw/char/serial.h > +++ b/include/hw/char/serial.h > @@ -86,6 +86,7 @@ typedef struct SerialMM { > SerialState serial; > > uint8_t regshift; > + uint8_t endianness; > } SerialMM; > > extern const VMStateDescription vmstate_serial; >
diff --git a/hw/char/serial.c b/hw/char/serial.c index c28cfc94fd..2f7667c30c 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -1081,6 +1081,7 @@ SerialMM *serial_mm_init(MemoryRegion *address_space, 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_init_nofail(DEVICE(self)); @@ -1102,6 +1103,7 @@ static void serial_mm_instance_init(Object *o) static Property serial_mm_properties[] = { DEFINE_PROP_UINT8("regshift", SerialMM, regshift, 0), + DEFINE_PROP_UINT8("endianness", SerialMM, endianness, DEVICE_NATIVE_ENDIAN), DEFINE_PROP_END_OF_LIST(), }; diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h index 759c85976d..2d0802a909 100644 --- a/include/hw/char/serial.h +++ b/include/hw/char/serial.h @@ -86,6 +86,7 @@ typedef struct SerialMM { SerialState serial; uint8_t regshift; + uint8_t endianness; } SerialMM; extern const VMStateDescription vmstate_serial;
Add a qdev property for endianness, so memory region setup can be done in realize. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/char/serial.c | 2 ++ include/hw/char/serial.h | 1 + 2 files changed, 3 insertions(+)