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(),
 };
 
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;
-- 
2.23.0.606.g08da6496b6

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;
>