[PATCH v2 0/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[PATCH v2 0/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

Hi,

This series QOM'ify the PolarFire UART (the project
would get ride of non-QOM devices).

Since v1:
- Simplify MCHP_PFSOC_MMUART_REG_SIZE
- Use MemoryRegion container
- Mention qdev_set_legacy_instance_id() is not needed (Bin)
- Properly map the 16550 (Bin)
- Add DeviceReset() method (Peter)
- Add migration state (Peter)

Tested with U-Boot v2021.07 following documentation in
docs/system/riscv/microchip-icicle-kit.rst.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition
  hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container
  hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

 include/hw/char/mchp_pfsoc_mmuart.h |  17 ++--
 hw/char/mchp_pfsoc_mmuart.c         | 126 ++++++++++++++++++++++------
 2 files changed, 114 insertions(+), 29 deletions(-)

-- 
2.31.1

[PATCH v2 0/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

Hi,

This series QOM'ify the PolarFire UART (the project
would get ride of non-QOM devices).

Since v1:
- Simplify MCHP_PFSOC_MMUART_REG_SIZE
- Use MemoryRegion container
- Mention qdev_set_legacy_instance_id() is not needed (Bin)
- Properly map the 16550 (Bin)
- Add DeviceReset() method (Peter)
- Add migration state (Peter)

Tested with U-Boot v2021.07 following documentation in
docs/system/riscv/microchip-icicle-kit.rst.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition
  hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container
  hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

 include/hw/char/mchp_pfsoc_mmuart.h |  17 ++--
 hw/char/mchp_pfsoc_mmuart.c         | 126 ++++++++++++++++++++++------
 2 files changed, 114 insertions(+), 29 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Philippe Mathieu-Daudé]

The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
size occupied by all the registers. However all registers are
32-bit wide, and the MemoryRegionOps handlers are restricted to
32-bit:

  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
      .read = mchp_pfsoc_mmuart_read,
      .write = mchp_pfsoc_mmuart_write,
      .impl = {
          .min_access_size = 4,
          .max_access_size = 4,
      },

Avoid being triskaidekaphobic, simplify by using the number of
registers.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
 hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
index f61990215f0..9c012e6c977 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -30,7 +30,7 @@
 
 #include "hw/char/serial.h"
 
-#define MCHP_PFSOC_MMUART_REG_SIZE  52
+#define MCHP_PFSOC_MMUART_REG_COUNT 13
 
 typedef struct MchpPfSoCMMUartState {
     MemoryRegion iomem;
@@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
 
     SerialMM *serial;
 
-    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
+    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
 } MchpPfSoCMMUartState;
 
 /**
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index 2facf85c2d8..584e7fec17c 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
 {
     MchpPfSoCMMUartState *s = opaque;
 
-    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
+    addr >>= 2;
+    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
-                      __func__, addr);
+                      __func__, addr << 2);
         return 0;
     }
 
-    return s->reg[addr / sizeof(uint32_t)];
+    return s->reg[addr];
 }
 
 static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
@@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
     MchpPfSoCMMUartState *s = opaque;
     uint32_t val32 = (uint32_t)value;
 
-    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
+    addr >>= 2;
+    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
-                      " v=0x%x\n", __func__, addr, val32);
+                      " v=0x%x\n", __func__, addr << 2, val32);
         return;
     }
 
-    s->reg[addr / sizeof(uint32_t)] = val32;
+    s->reg[addr] = val32;
 }
 
 static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
-- 
2.31.1

[PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Philippe Mathieu-Daudé]

The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
size occupied by all the registers. However all registers are
32-bit wide, and the MemoryRegionOps handlers are restricted to
32-bit:

  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
      .read = mchp_pfsoc_mmuart_read,
      .write = mchp_pfsoc_mmuart_write,
      .impl = {
          .min_access_size = 4,
          .max_access_size = 4,
      },

Avoid being triskaidekaphobic, simplify by using the number of
registers.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
 hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
index f61990215f0..9c012e6c977 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -30,7 +30,7 @@
 
 #include "hw/char/serial.h"
 
-#define MCHP_PFSOC_MMUART_REG_SIZE  52
+#define MCHP_PFSOC_MMUART_REG_COUNT 13
 
 typedef struct MchpPfSoCMMUartState {
     MemoryRegion iomem;
@@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
 
     SerialMM *serial;
 
-    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
+    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
 } MchpPfSoCMMUartState;
 
 /**
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index 2facf85c2d8..584e7fec17c 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
 {
     MchpPfSoCMMUartState *s = opaque;
 
-    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
+    addr >>= 2;
+    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
-                      __func__, addr);
+                      __func__, addr << 2);
         return 0;
     }
 
-    return s->reg[addr / sizeof(uint32_t)];
+    return s->reg[addr];
 }
 
 static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
@@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
     MchpPfSoCMMUartState *s = opaque;
     uint32_t val32 = (uint32_t)value;
 
-    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
+    addr >>= 2;
+    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
-                      " v=0x%x\n", __func__, addr, val32);
+                      " v=0x%x\n", __func__, addr << 2, val32);
         return;
     }
 
-    s->reg[addr / sizeof(uint32_t)] = val32;
+    s->reg[addr] = val32;
 }
 
 static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
-- 
2.31.1

[PATCH v2 2/3] hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container

[Philippe Mathieu-Daudé]

Our device have 2 different I/O regions:
- a 16550 UART mapped for 32-bit accesses
- 13 extra registers

Instead of mapping each region on the main bus, introduce
a container, map the 2 devices regions on the container,
and map the container on the main bus.

Before:

  (qemu) info mtree
    ...
    0000000020100000-000000002010001f (prio 0, i/o): serial
    0000000020100020-000000002010101f (prio 0, i/o): mchp.pfsoc.mmuart
    0000000020102000-000000002010201f (prio 0, i/o): serial
    0000000020102020-000000002010301f (prio 0, i/o): mchp.pfsoc.mmuart
    0000000020104000-000000002010401f (prio 0, i/o): serial
    0000000020104020-000000002010501f (prio 0, i/o): mchp.pfsoc.mmuart
    0000000020106000-000000002010601f (prio 0, i/o): serial
    0000000020106020-000000002010701f (prio 0, i/o): mchp.pfsoc.mmuart

After:

  (qemu) info mtree
    ...
    0000000020100000-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020100000-000000002010001f (prio 0, i/o): serial
      0000000020100020-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
    0000000020102000-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020102000-000000002010201f (prio 0, i/o): serial
      0000000020102020-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
    0000000020104000-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020104000-000000002010401f (prio 0, i/o): serial
      0000000020104020-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
    0000000020106000-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020106000-000000002010601f (prio 0, i/o): serial
      0000000020106020-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart.regs

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/mchp_pfsoc_mmuart.h |  1 +
 hw/char/mchp_pfsoc_mmuart.c         | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
index 9c012e6c977..864ac1a36b5 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -33,6 +33,7 @@
 #define MCHP_PFSOC_MMUART_REG_COUNT 13
 
 typedef struct MchpPfSoCMMUartState {
+    MemoryRegion container;
     MemoryRegion iomem;
     hwaddr base;
     qemu_irq irq;
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index 584e7fec17c..ea586559761 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -25,6 +25,8 @@
 #include "chardev/char.h"
 #include "hw/char/mchp_pfsoc_mmuart.h"
 
+#define REGS_OFFSET 0x20
+
 static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
 {
     MchpPfSoCMMUartState *s = opaque;
@@ -72,16 +74,19 @@ MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
 
     s = g_new0(MchpPfSoCMMUartState, 1);
 
+    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
+
     memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
-                          "mchp.pfsoc.mmuart", 0x1000);
+                          "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
+    memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
 
     s->base = base;
     s->irq = irq;
 
-    s->serial = serial_mm_init(sysmem, base, 2, irq, 399193, chr,
+    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
                                DEVICE_LITTLE_ENDIAN);
 
-    memory_region_add_subregion(sysmem, base + 0x20, &s->iomem);
+    memory_region_add_subregion(sysmem, base, &s->container);
 
     return s;
 }
-- 
2.31.1

[PATCH v2 2/3] hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container

[Philippe Mathieu-Daudé]

Our device have 2 different I/O regions:
- a 16550 UART mapped for 32-bit accesses
- 13 extra registers

Instead of mapping each region on the main bus, introduce
a container, map the 2 devices regions on the container,
and map the container on the main bus.

Before:

  (qemu) info mtree
    ...
    0000000020100000-000000002010001f (prio 0, i/o): serial
    0000000020100020-000000002010101f (prio 0, i/o): mchp.pfsoc.mmuart
    0000000020102000-000000002010201f (prio 0, i/o): serial
    0000000020102020-000000002010301f (prio 0, i/o): mchp.pfsoc.mmuart
    0000000020104000-000000002010401f (prio 0, i/o): serial
    0000000020104020-000000002010501f (prio 0, i/o): mchp.pfsoc.mmuart
    0000000020106000-000000002010601f (prio 0, i/o): serial
    0000000020106020-000000002010701f (prio 0, i/o): mchp.pfsoc.mmuart

After:

  (qemu) info mtree
    ...
    0000000020100000-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020100000-000000002010001f (prio 0, i/o): serial
      0000000020100020-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
    0000000020102000-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020102000-000000002010201f (prio 0, i/o): serial
      0000000020102020-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
    0000000020104000-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020104000-000000002010401f (prio 0, i/o): serial
      0000000020104020-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
    0000000020106000-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart
      0000000020106000-000000002010601f (prio 0, i/o): serial
      0000000020106020-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart.regs

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/mchp_pfsoc_mmuart.h |  1 +
 hw/char/mchp_pfsoc_mmuart.c         | 11 ++++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
index 9c012e6c977..864ac1a36b5 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -33,6 +33,7 @@
 #define MCHP_PFSOC_MMUART_REG_COUNT 13
 
 typedef struct MchpPfSoCMMUartState {
+    MemoryRegion container;
     MemoryRegion iomem;
     hwaddr base;
     qemu_irq irq;
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index 584e7fec17c..ea586559761 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -25,6 +25,8 @@
 #include "chardev/char.h"
 #include "hw/char/mchp_pfsoc_mmuart.h"
 
+#define REGS_OFFSET 0x20
+
 static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
 {
     MchpPfSoCMMUartState *s = opaque;
@@ -72,16 +74,19 @@ MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
 
     s = g_new0(MchpPfSoCMMUartState, 1);
 
+    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
+
     memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
-                          "mchp.pfsoc.mmuart", 0x1000);
+                          "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
+    memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
 
     s->base = base;
     s->irq = irq;
 
-    s->serial = serial_mm_init(sysmem, base, 2, irq, 399193, chr,
+    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
                                DEVICE_LITTLE_ENDIAN);
 
-    memory_region_add_subregion(sysmem, base + 0x20, &s->iomem);
+    memory_region_add_subregion(sysmem, base, &s->container);
 
     return s;
 }
-- 
2.31.1

[PATCH v2 3/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

- Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
- Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
- Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
- Add DeviceReset() method
- Add vmstate structure for migration
- Register device in 'input' category
- Keep mchp_pfsoc_mmuart_create() behavior

Note, serial_mm_init() calls qdev_set_legacy_instance_id().
This call is only needed for backwards-compatibility of incoming
migration data with old versions of QEMU which implemented migration
of devices with hand-rolled code. Since this device didn't previously
handle migration at all, then it doesn't need to set the legacy
instance ID.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Peter Maydell <peter.maydell@linaro.org>

I haven't kept Alistair R-b tag from v1.
---
 include/hw/char/mchp_pfsoc_mmuart.h |  12 +++-
 hw/char/mchp_pfsoc_mmuart.c         | 105 +++++++++++++++++++++++-----
 2 files changed, 97 insertions(+), 20 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
index 864ac1a36b5..b0e14ca3554 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -28,17 +28,23 @@
 #ifndef HW_MCHP_PFSOC_MMUART_H
 #define HW_MCHP_PFSOC_MMUART_H
 
+#include "hw/sysbus.h"
 #include "hw/char/serial.h"
 
 #define MCHP_PFSOC_MMUART_REG_COUNT 13
 
+#define TYPE_MCHP_PFSOC_UART "mchp.pfsoc.uart"
+OBJECT_DECLARE_SIMPLE_TYPE(MchpPfSoCMMUartState, MCHP_PFSOC_UART)
+
 typedef struct MchpPfSoCMMUartState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
     MemoryRegion container;
     MemoryRegion iomem;
-    hwaddr base;
-    qemu_irq irq;
 
-    SerialMM *serial;
+    SerialMM serial_mm;
 
     uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
 } MchpPfSoCMMUartState;
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index ea586559761..22f3e78eb9e 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -22,8 +22,10 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
-#include "chardev/char.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
 #include "hw/char/mchp_pfsoc_mmuart.h"
+#include "hw/qdev-properties.h"
 
 #define REGS_OFFSET 0x20
 
@@ -67,26 +69,95 @@ static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
     },
 };
 
-MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
-    hwaddr base, qemu_irq irq, Chardev *chr)
+static void mchp_pfsoc_mmuart_reset(DeviceState *dev)
 {
-    MchpPfSoCMMUartState *s;
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
 
-    s = g_new0(MchpPfSoCMMUartState, 1);
+    memset(s->reg, 0, sizeof(s->reg));
+    device_cold_reset(DEVICE(&s->serial_mm));
+}
 
-    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
+static void mchp_pfsoc_mmuart_init(Object *obj)
+{
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(obj);
 
-    memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
+    object_initialize_child(obj, "serial-mm", &s->serial_mm, TYPE_SERIAL_MM);
+    object_property_add_alias(obj, "chardev", OBJECT(&s->serial_mm), "chardev");
+}
+
+static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
+{
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
+
+    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "regshift", 2);
+    qdev_prop_set_uint32(DEVICE(&s->serial_mm), "baudbase", 399193);
+    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "endianness",
+                        DEVICE_LITTLE_ENDIAN);
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->serial_mm), errp)) {
+        return;
+    }
+
+    sysbus_pass_irq(SYS_BUS_DEVICE(dev), SYS_BUS_DEVICE(&s->serial_mm));
+
+    memory_region_init(&s->container, OBJECT(s), "mchp.pfsoc.mmuart", 0x1000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
+
+    memory_region_add_subregion(&s->container, 0,
+                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->serial_mm), 0));
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &mchp_pfsoc_mmuart_ops, s,
                           "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
     memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
-
-    s->base = base;
-    s->irq = irq;
-
-    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
-                               DEVICE_LITTLE_ENDIAN);
-
-    memory_region_add_subregion(sysmem, base, &s->container);
-
-    return s;
+}
+
+static const VMStateDescription mchp_pfsoc_mmuart_vmstate = {
+    .name = "mchp.pfsoc.uart",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(reg, MchpPfSoCMMUartState,
+                             MCHP_PFSOC_MMUART_REG_COUNT),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void mchp_pfsoc_mmuart_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = mchp_pfsoc_mmuart_realize;
+    dc->reset = mchp_pfsoc_mmuart_reset;
+    dc->vmsd = &mchp_pfsoc_mmuart_vmstate;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
+static const TypeInfo mchp_pfsoc_mmuart_info = {
+    .name          = TYPE_MCHP_PFSOC_UART,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(MchpPfSoCMMUartState),
+    .instance_init = mchp_pfsoc_mmuart_init,
+    .class_init    = mchp_pfsoc_mmuart_class_init,
+};
+
+static void mchp_pfsoc_mmuart_register_types(void)
+{
+    type_register_static(&mchp_pfsoc_mmuart_info);
+}
+
+type_init(mchp_pfsoc_mmuart_register_types)
+
+MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
+                                               hwaddr base,
+                                               qemu_irq irq, Chardev *chr)
+{
+    DeviceState *dev = qdev_new(TYPE_MCHP_PFSOC_UART);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    qdev_prop_set_chr(dev, "chardev", chr);
+    sysbus_realize(sbd, &error_fatal);
+
+    memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(sbd, 0));
+    sysbus_connect_irq(sbd, 0, irq);
+
+    return MCHP_PFSOC_UART(dev);
 }
-- 
2.31.1

[PATCH v2 3/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Philippe Mathieu-Daudé]

- Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
- Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
- Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
- Add DeviceReset() method
- Add vmstate structure for migration
- Register device in 'input' category
- Keep mchp_pfsoc_mmuart_create() behavior

Note, serial_mm_init() calls qdev_set_legacy_instance_id().
This call is only needed for backwards-compatibility of incoming
migration data with old versions of QEMU which implemented migration
of devices with hand-rolled code. Since this device didn't previously
handle migration at all, then it doesn't need to set the legacy
instance ID.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Peter Maydell <peter.maydell@linaro.org>

I haven't kept Alistair R-b tag from v1.
---
 include/hw/char/mchp_pfsoc_mmuart.h |  12 +++-
 hw/char/mchp_pfsoc_mmuart.c         | 105 +++++++++++++++++++++++-----
 2 files changed, 97 insertions(+), 20 deletions(-)

diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
index 864ac1a36b5..b0e14ca3554 100644
--- a/include/hw/char/mchp_pfsoc_mmuart.h
+++ b/include/hw/char/mchp_pfsoc_mmuart.h
@@ -28,17 +28,23 @@
 #ifndef HW_MCHP_PFSOC_MMUART_H
 #define HW_MCHP_PFSOC_MMUART_H
 
+#include "hw/sysbus.h"
 #include "hw/char/serial.h"
 
 #define MCHP_PFSOC_MMUART_REG_COUNT 13
 
+#define TYPE_MCHP_PFSOC_UART "mchp.pfsoc.uart"
+OBJECT_DECLARE_SIMPLE_TYPE(MchpPfSoCMMUartState, MCHP_PFSOC_UART)
+
 typedef struct MchpPfSoCMMUartState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
     MemoryRegion container;
     MemoryRegion iomem;
-    hwaddr base;
-    qemu_irq irq;
 
-    SerialMM *serial;
+    SerialMM serial_mm;
 
     uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
 } MchpPfSoCMMUartState;
diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
index ea586559761..22f3e78eb9e 100644
--- a/hw/char/mchp_pfsoc_mmuart.c
+++ b/hw/char/mchp_pfsoc_mmuart.c
@@ -22,8 +22,10 @@
 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
-#include "chardev/char.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
 #include "hw/char/mchp_pfsoc_mmuart.h"
+#include "hw/qdev-properties.h"
 
 #define REGS_OFFSET 0x20
 
@@ -67,26 +69,95 @@ static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
     },
 };
 
-MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
-    hwaddr base, qemu_irq irq, Chardev *chr)
+static void mchp_pfsoc_mmuart_reset(DeviceState *dev)
 {
-    MchpPfSoCMMUartState *s;
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
 
-    s = g_new0(MchpPfSoCMMUartState, 1);
+    memset(s->reg, 0, sizeof(s->reg));
+    device_cold_reset(DEVICE(&s->serial_mm));
+}
 
-    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
+static void mchp_pfsoc_mmuart_init(Object *obj)
+{
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(obj);
 
-    memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
+    object_initialize_child(obj, "serial-mm", &s->serial_mm, TYPE_SERIAL_MM);
+    object_property_add_alias(obj, "chardev", OBJECT(&s->serial_mm), "chardev");
+}
+
+static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
+{
+    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
+
+    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "regshift", 2);
+    qdev_prop_set_uint32(DEVICE(&s->serial_mm), "baudbase", 399193);
+    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "endianness",
+                        DEVICE_LITTLE_ENDIAN);
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->serial_mm), errp)) {
+        return;
+    }
+
+    sysbus_pass_irq(SYS_BUS_DEVICE(dev), SYS_BUS_DEVICE(&s->serial_mm));
+
+    memory_region_init(&s->container, OBJECT(s), "mchp.pfsoc.mmuart", 0x1000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
+
+    memory_region_add_subregion(&s->container, 0,
+                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->serial_mm), 0));
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &mchp_pfsoc_mmuart_ops, s,
                           "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
     memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
-
-    s->base = base;
-    s->irq = irq;
-
-    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
-                               DEVICE_LITTLE_ENDIAN);
-
-    memory_region_add_subregion(sysmem, base, &s->container);
-
-    return s;
+}
+
+static const VMStateDescription mchp_pfsoc_mmuart_vmstate = {
+    .name = "mchp.pfsoc.uart",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(reg, MchpPfSoCMMUartState,
+                             MCHP_PFSOC_MMUART_REG_COUNT),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void mchp_pfsoc_mmuart_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = mchp_pfsoc_mmuart_realize;
+    dc->reset = mchp_pfsoc_mmuart_reset;
+    dc->vmsd = &mchp_pfsoc_mmuart_vmstate;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
+static const TypeInfo mchp_pfsoc_mmuart_info = {
+    .name          = TYPE_MCHP_PFSOC_UART,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(MchpPfSoCMMUartState),
+    .instance_init = mchp_pfsoc_mmuart_init,
+    .class_init    = mchp_pfsoc_mmuart_class_init,
+};
+
+static void mchp_pfsoc_mmuart_register_types(void)
+{
+    type_register_static(&mchp_pfsoc_mmuart_info);
+}
+
+type_init(mchp_pfsoc_mmuart_register_types)
+
+MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
+                                               hwaddr base,
+                                               qemu_irq irq, Chardev *chr)
+{
+    DeviceState *dev = qdev_new(TYPE_MCHP_PFSOC_UART);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    qdev_prop_set_chr(dev, "chardev", chr);
+    sysbus_realize(sbd, &error_fatal);
+
+    memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(sbd, 0));
+    sysbus_connect_irq(sbd, 0, irq);
+
+    return MCHP_PFSOC_UART(dev);
 }
-- 
2.31.1

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Bin Meng]

On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of

typo? See https://www.dictionary.com/browse/triskaidekaphobia

Learned a new word today but I have to say this word is too hard for a
non-native speaker :)

> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Bin Meng]

On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of

typo? See https://www.dictionary.com/browse/triskaidekaphobia

Learned a new word today but I have to say this word is too hard for a
non-native speaker :)

> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH v2 2/3] hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container

[Bin Meng]

On Sat, Sep 25, 2021 at 9:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Our device have 2 different I/O regions:
> - a 16550 UART mapped for 32-bit accesses
> - 13 extra registers
>
> Instead of mapping each region on the main bus, introduce
> a container, map the 2 devices regions on the container,
> and map the container on the main bus.
>
> Before:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-000000002010001f (prio 0, i/o): serial
>     0000000020100020-000000002010101f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020102000-000000002010201f (prio 0, i/o): serial
>     0000000020102020-000000002010301f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020104000-000000002010401f (prio 0, i/o): serial
>     0000000020104020-000000002010501f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020106000-000000002010601f (prio 0, i/o): serial
>     0000000020106020-000000002010701f (prio 0, i/o): mchp.pfsoc.mmuart
>
> After:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020100000-000000002010001f (prio 0, i/o): serial
>       0000000020100020-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020102000-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020102000-000000002010201f (prio 0, i/o): serial
>       0000000020102020-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020104000-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020104000-000000002010401f (prio 0, i/o): serial
>       0000000020104020-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020106000-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020106000-000000002010601f (prio 0, i/o): serial
>       0000000020106020-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  1 +
>  hw/char/mchp_pfsoc_mmuart.c         | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH v2 2/3] hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container

[Bin Meng]

On Sat, Sep 25, 2021 at 9:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Our device have 2 different I/O regions:
> - a 16550 UART mapped for 32-bit accesses
> - 13 extra registers
>
> Instead of mapping each region on the main bus, introduce
> a container, map the 2 devices regions on the container,
> and map the container on the main bus.
>
> Before:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-000000002010001f (prio 0, i/o): serial
>     0000000020100020-000000002010101f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020102000-000000002010201f (prio 0, i/o): serial
>     0000000020102020-000000002010301f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020104000-000000002010401f (prio 0, i/o): serial
>     0000000020104020-000000002010501f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020106000-000000002010601f (prio 0, i/o): serial
>     0000000020106020-000000002010701f (prio 0, i/o): mchp.pfsoc.mmuart
>
> After:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020100000-000000002010001f (prio 0, i/o): serial
>       0000000020100020-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020102000-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020102000-000000002010201f (prio 0, i/o): serial
>       0000000020102020-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020104000-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020104000-000000002010401f (prio 0, i/o): serial
>       0000000020104020-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020106000-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020106000-000000002010601f (prio 0, i/o): serial
>       0000000020106020-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  1 +
>  hw/char/mchp_pfsoc_mmuart.c         | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH v2 3/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Bin Meng]

On Sat, Sep 25, 2021 at 9:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> - Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
> - Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
> - Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
> - Add DeviceReset() method
> - Add vmstate structure for migration
> - Register device in 'input' category
> - Keep mchp_pfsoc_mmuart_create() behavior
>
> Note, serial_mm_init() calls qdev_set_legacy_instance_id().
> This call is only needed for backwards-compatibility of incoming
> migration data with old versions of QEMU which implemented migration
> of devices with hand-rolled code. Since this device didn't previously
> handle migration at all, then it doesn't need to set the legacy
> instance ID.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> I haven't kept Alistair R-b tag from v1.
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  12 +++-
>  hw/char/mchp_pfsoc_mmuart.c         | 105 +++++++++++++++++++++++-----
>  2 files changed, 97 insertions(+), 20 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH v2 3/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Bin Meng]

On Sat, Sep 25, 2021 at 9:37 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> - Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
> - Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
> - Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
> - Add DeviceReset() method
> - Add vmstate structure for migration
> - Register device in 'input' category
> - Keep mchp_pfsoc_mmuart_create() behavior
>
> Note, serial_mm_init() calls qdev_set_legacy_instance_id().
> This call is only needed for backwards-compatibility of incoming
> migration data with old versions of QEMU which implemented migration
> of devices with hand-rolled code. Since this device didn't previously
> handle migration at all, then it doesn't need to set the legacy
> instance ID.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> I haven't kept Alistair R-b tag from v1.
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  12 +++-
>  hw/char/mchp_pfsoc_mmuart.c         | 105 +++++++++++++++++++++++-----
>  2 files changed, 97 insertions(+), 20 deletions(-)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>
Tested-by: Bin Meng <bin.meng@windriver.com>

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Bin Meng]

On Sun, Sep 26, 2021 at 4:31 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> > size occupied by all the registers. However all registers are
> > 32-bit wide, and the MemoryRegionOps handlers are restricted to
> > 32-bit:
> >
> >   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> >       .read = mchp_pfsoc_mmuart_read,
> >       .write = mchp_pfsoc_mmuart_write,
> >       .impl = {
> >           .min_access_size = 4,
> >           .max_access_size = 4,
> >       },
> >
> > Avoid being triskaidekaphobic, simplify by using the number of
>
> typo? See https://www.dictionary.com/browse/triskaidekaphobia
>
> Learned a new word today but I have to say this word is too hard for a
> non-native speaker :)
>

Never mind, triskaidekaphobia is a noun, and triskaidekaphobic is the
adjective which is grammarly correct :)

> > registers.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
> >  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> Tested-by: Bin Meng <bin.meng@windriver.com>

Regards,
Bin

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Bin Meng]

On Sun, Sep 26, 2021 at 4:31 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> > size occupied by all the registers. However all registers are
> > 32-bit wide, and the MemoryRegionOps handlers are restricted to
> > 32-bit:
> >
> >   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> >       .read = mchp_pfsoc_mmuart_read,
> >       .write = mchp_pfsoc_mmuart_write,
> >       .impl = {
> >           .min_access_size = 4,
> >           .max_access_size = 4,
> >       },
> >
> > Avoid being triskaidekaphobic, simplify by using the number of
>
> typo? See https://www.dictionary.com/browse/triskaidekaphobia
>
> Learned a new word today but I have to say this word is too hard for a
> non-native speaker :)
>

Never mind, triskaidekaphobia is a noun, and triskaidekaphobic is the
adjective which is grammarly correct :)

> > registers.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
> >  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bin.meng@windriver.com>
> Tested-by: Bin Meng <bin.meng@windriver.com>

Regards,
Bin

Re: [PATCH v2 2/3] hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Our device have 2 different I/O regions:
> - a 16550 UART mapped for 32-bit accesses
> - 13 extra registers
>
> Instead of mapping each region on the main bus, introduce
> a container, map the 2 devices regions on the container,
> and map the container on the main bus.
>
> Before:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-000000002010001f (prio 0, i/o): serial
>     0000000020100020-000000002010101f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020102000-000000002010201f (prio 0, i/o): serial
>     0000000020102020-000000002010301f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020104000-000000002010401f (prio 0, i/o): serial
>     0000000020104020-000000002010501f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020106000-000000002010601f (prio 0, i/o): serial
>     0000000020106020-000000002010701f (prio 0, i/o): mchp.pfsoc.mmuart
>
> After:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020100000-000000002010001f (prio 0, i/o): serial
>       0000000020100020-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020102000-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020102000-000000002010201f (prio 0, i/o): serial
>       0000000020102020-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020104000-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020104000-000000002010401f (prio 0, i/o): serial
>       0000000020104020-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020106000-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020106000-000000002010601f (prio 0, i/o): serial
>       0000000020106020-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  1 +
>  hw/char/mchp_pfsoc_mmuart.c         | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index 9c012e6c977..864ac1a36b5 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -33,6 +33,7 @@
>  #define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
> +    MemoryRegion container;
>      MemoryRegion iomem;
>      hwaddr base;
>      qemu_irq irq;
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 584e7fec17c..ea586559761 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -25,6 +25,8 @@
>  #include "chardev/char.h"
>  #include "hw/char/mchp_pfsoc_mmuart.h"
>
> +#define REGS_OFFSET 0x20
> +
>  static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
> @@ -72,16 +74,19 @@ MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
>
>      s = g_new0(MchpPfSoCMMUartState, 1);
>
> +    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
> +
>      memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
> -                          "mchp.pfsoc.mmuart", 0x1000);
> +                          "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
> +    memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
>
>      s->base = base;
>      s->irq = irq;
>
> -    s->serial = serial_mm_init(sysmem, base, 2, irq, 399193, chr,
> +    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
>                                 DEVICE_LITTLE_ENDIAN);
>
> -    memory_region_add_subregion(sysmem, base + 0x20, &s->iomem);
> +    memory_region_add_subregion(sysmem, base, &s->container);
>
>      return s;
>  }
> --
> 2.31.1
>

Re: [PATCH v2 2/3] hw/char/mchp_pfsoc_mmuart: Use a MemoryRegion container

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Our device have 2 different I/O regions:
> - a 16550 UART mapped for 32-bit accesses
> - 13 extra registers
>
> Instead of mapping each region on the main bus, introduce
> a container, map the 2 devices regions on the container,
> and map the container on the main bus.
>
> Before:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-000000002010001f (prio 0, i/o): serial
>     0000000020100020-000000002010101f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020102000-000000002010201f (prio 0, i/o): serial
>     0000000020102020-000000002010301f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020104000-000000002010401f (prio 0, i/o): serial
>     0000000020104020-000000002010501f (prio 0, i/o): mchp.pfsoc.mmuart
>     0000000020106000-000000002010601f (prio 0, i/o): serial
>     0000000020106020-000000002010701f (prio 0, i/o): mchp.pfsoc.mmuart
>
> After:
>
>   (qemu) info mtree
>     ...
>     0000000020100000-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020100000-000000002010001f (prio 0, i/o): serial
>       0000000020100020-0000000020100fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020102000-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020102000-000000002010201f (prio 0, i/o): serial
>       0000000020102020-0000000020102fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020104000-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020104000-000000002010401f (prio 0, i/o): serial
>       0000000020104020-0000000020104fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>     0000000020106000-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart
>       0000000020106000-000000002010601f (prio 0, i/o): serial
>       0000000020106020-0000000020106fff (prio 0, i/o): mchp.pfsoc.mmuart.regs
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  1 +
>  hw/char/mchp_pfsoc_mmuart.c         | 11 ++++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index 9c012e6c977..864ac1a36b5 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -33,6 +33,7 @@
>  #define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
> +    MemoryRegion container;
>      MemoryRegion iomem;
>      hwaddr base;
>      qemu_irq irq;
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 584e7fec17c..ea586559761 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -25,6 +25,8 @@
>  #include "chardev/char.h"
>  #include "hw/char/mchp_pfsoc_mmuart.h"
>
> +#define REGS_OFFSET 0x20
> +
>  static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
> @@ -72,16 +74,19 @@ MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
>
>      s = g_new0(MchpPfSoCMMUartState, 1);
>
> +    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
> +
>      memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
> -                          "mchp.pfsoc.mmuart", 0x1000);
> +                          "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
> +    memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
>
>      s->base = base;
>      s->irq = irq;
>
> -    s->serial = serial_mm_init(sysmem, base, 2, irq, 399193, chr,
> +    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
>                                 DEVICE_LITTLE_ENDIAN);
>
> -    memory_region_add_subregion(sysmem, base + 0x20, &s->iomem);
> +    memory_region_add_subregion(sysmem, base, &s->container);
>
>      return s;
>  }
> --
> 2.31.1
>

Re: [PATCH v2 3/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> - Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
> - Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
> - Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
> - Add DeviceReset() method
> - Add vmstate structure for migration
> - Register device in 'input' category
> - Keep mchp_pfsoc_mmuart_create() behavior
>
> Note, serial_mm_init() calls qdev_set_legacy_instance_id().
> This call is only needed for backwards-compatibility of incoming
> migration data with old versions of QEMU which implemented migration
> of devices with hand-rolled code. Since this device didn't previously
> handle migration at all, then it doesn't need to set the legacy
> instance ID.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> I haven't kept Alistair R-b tag from v1.
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  12 +++-
>  hw/char/mchp_pfsoc_mmuart.c         | 105 +++++++++++++++++++++++-----
>  2 files changed, 97 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index 864ac1a36b5..b0e14ca3554 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -28,17 +28,23 @@
>  #ifndef HW_MCHP_PFSOC_MMUART_H
>  #define HW_MCHP_PFSOC_MMUART_H
>
> +#include "hw/sysbus.h"
>  #include "hw/char/serial.h"
>
>  #define MCHP_PFSOC_MMUART_REG_COUNT 13
>
> +#define TYPE_MCHP_PFSOC_UART "mchp.pfsoc.uart"
> +OBJECT_DECLARE_SIMPLE_TYPE(MchpPfSoCMMUartState, MCHP_PFSOC_UART)
> +
>  typedef struct MchpPfSoCMMUartState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
>      MemoryRegion container;
>      MemoryRegion iomem;
> -    hwaddr base;
> -    qemu_irq irq;
>
> -    SerialMM *serial;
> +    SerialMM serial_mm;
>
>      uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index ea586559761..22f3e78eb9e 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -22,8 +22,10 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> -#include "chardev/char.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
>  #include "hw/char/mchp_pfsoc_mmuart.h"
> +#include "hw/qdev-properties.h"
>
>  #define REGS_OFFSET 0x20
>
> @@ -67,26 +69,95 @@ static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>      },
>  };
>
> -MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
> -    hwaddr base, qemu_irq irq, Chardev *chr)
> +static void mchp_pfsoc_mmuart_reset(DeviceState *dev)
>  {
> -    MchpPfSoCMMUartState *s;
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
>
> -    s = g_new0(MchpPfSoCMMUartState, 1);
> +    memset(s->reg, 0, sizeof(s->reg));
> +    device_cold_reset(DEVICE(&s->serial_mm));
> +}
>
> -    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
> +static void mchp_pfsoc_mmuart_init(Object *obj)
> +{
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(obj);
>
> -    memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
> +    object_initialize_child(obj, "serial-mm", &s->serial_mm, TYPE_SERIAL_MM);
> +    object_property_add_alias(obj, "chardev", OBJECT(&s->serial_mm), "chardev");
> +}
> +
> +static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
> +{
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
> +
> +    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "regshift", 2);
> +    qdev_prop_set_uint32(DEVICE(&s->serial_mm), "baudbase", 399193);
> +    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "endianness",
> +                        DEVICE_LITTLE_ENDIAN);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->serial_mm), errp)) {
> +        return;
> +    }
> +
> +    sysbus_pass_irq(SYS_BUS_DEVICE(dev), SYS_BUS_DEVICE(&s->serial_mm));
> +
> +    memory_region_init(&s->container, OBJECT(s), "mchp.pfsoc.mmuart", 0x1000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
> +
> +    memory_region_add_subregion(&s->container, 0,
> +                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->serial_mm), 0));
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &mchp_pfsoc_mmuart_ops, s,
>                            "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
>      memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
> -
> -    s->base = base;
> -    s->irq = irq;
> -
> -    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
> -                               DEVICE_LITTLE_ENDIAN);
> -
> -    memory_region_add_subregion(sysmem, base, &s->container);
> -
> -    return s;
> +}
> +
> +static const VMStateDescription mchp_pfsoc_mmuart_vmstate = {
> +    .name = "mchp.pfsoc.uart",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(reg, MchpPfSoCMMUartState,
> +                             MCHP_PFSOC_MMUART_REG_COUNT),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void mchp_pfsoc_mmuart_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = mchp_pfsoc_mmuart_realize;
> +    dc->reset = mchp_pfsoc_mmuart_reset;
> +    dc->vmsd = &mchp_pfsoc_mmuart_vmstate;
> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> +}
> +
> +static const TypeInfo mchp_pfsoc_mmuart_info = {
> +    .name          = TYPE_MCHP_PFSOC_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MchpPfSoCMMUartState),
> +    .instance_init = mchp_pfsoc_mmuart_init,
> +    .class_init    = mchp_pfsoc_mmuart_class_init,
> +};
> +
> +static void mchp_pfsoc_mmuart_register_types(void)
> +{
> +    type_register_static(&mchp_pfsoc_mmuart_info);
> +}
> +
> +type_init(mchp_pfsoc_mmuart_register_types)
> +
> +MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
> +                                               hwaddr base,
> +                                               qemu_irq irq, Chardev *chr)
> +{
> +    DeviceState *dev = qdev_new(TYPE_MCHP_PFSOC_UART);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +    sysbus_realize(sbd, &error_fatal);
> +
> +    memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(sbd, 0));
> +    sysbus_connect_irq(sbd, 0, irq);
> +
> +    return MCHP_PFSOC_UART(dev);
>  }
> --
> 2.31.1
>

Re: [PATCH v2 3/3] hw/char/mchp_pfsoc_mmuart: QOM'ify PolarFire MMUART

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> - Embed SerialMM in MchpPfSoCMMUartState and QOM-initialize it
> - Alias SERIAL_MM 'chardev' property on MCHP_PFSOC_UART
> - Forward SerialMM sysbus IRQ in mchp_pfsoc_mmuart_realize()
> - Add DeviceReset() method
> - Add vmstate structure for migration
> - Register device in 'input' category
> - Keep mchp_pfsoc_mmuart_create() behavior
>
> Note, serial_mm_init() calls qdev_set_legacy_instance_id().
> This call is only needed for backwards-compatibility of incoming
> migration data with old versions of QEMU which implemented migration
> of devices with hand-rolled code. Since this device didn't previously
> handle migration at all, then it doesn't need to set the legacy
> instance ID.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: Peter Maydell <peter.maydell@linaro.org>
>
> I haven't kept Alistair R-b tag from v1.
> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  12 +++-
>  hw/char/mchp_pfsoc_mmuart.c         | 105 +++++++++++++++++++++++-----
>  2 files changed, 97 insertions(+), 20 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index 864ac1a36b5..b0e14ca3554 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -28,17 +28,23 @@
>  #ifndef HW_MCHP_PFSOC_MMUART_H
>  #define HW_MCHP_PFSOC_MMUART_H
>
> +#include "hw/sysbus.h"
>  #include "hw/char/serial.h"
>
>  #define MCHP_PFSOC_MMUART_REG_COUNT 13
>
> +#define TYPE_MCHP_PFSOC_UART "mchp.pfsoc.uart"
> +OBJECT_DECLARE_SIMPLE_TYPE(MchpPfSoCMMUartState, MCHP_PFSOC_UART)
> +
>  typedef struct MchpPfSoCMMUartState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
>      MemoryRegion container;
>      MemoryRegion iomem;
> -    hwaddr base;
> -    qemu_irq irq;
>
> -    SerialMM *serial;
> +    SerialMM serial_mm;
>
>      uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index ea586559761..22f3e78eb9e 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -22,8 +22,10 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> -#include "chardev/char.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
>  #include "hw/char/mchp_pfsoc_mmuart.h"
> +#include "hw/qdev-properties.h"
>
>  #define REGS_OFFSET 0x20
>
> @@ -67,26 +69,95 @@ static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>      },
>  };
>
> -MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
> -    hwaddr base, qemu_irq irq, Chardev *chr)
> +static void mchp_pfsoc_mmuart_reset(DeviceState *dev)
>  {
> -    MchpPfSoCMMUartState *s;
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
>
> -    s = g_new0(MchpPfSoCMMUartState, 1);
> +    memset(s->reg, 0, sizeof(s->reg));
> +    device_cold_reset(DEVICE(&s->serial_mm));
> +}
>
> -    memory_region_init(&s->container, NULL, "mchp.pfsoc.mmuart", 0x1000);
> +static void mchp_pfsoc_mmuart_init(Object *obj)
> +{
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(obj);
>
> -    memory_region_init_io(&s->iomem, NULL, &mchp_pfsoc_mmuart_ops, s,
> +    object_initialize_child(obj, "serial-mm", &s->serial_mm, TYPE_SERIAL_MM);
> +    object_property_add_alias(obj, "chardev", OBJECT(&s->serial_mm), "chardev");
> +}
> +
> +static void mchp_pfsoc_mmuart_realize(DeviceState *dev, Error **errp)
> +{
> +    MchpPfSoCMMUartState *s = MCHP_PFSOC_UART(dev);
> +
> +    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "regshift", 2);
> +    qdev_prop_set_uint32(DEVICE(&s->serial_mm), "baudbase", 399193);
> +    qdev_prop_set_uint8(DEVICE(&s->serial_mm), "endianness",
> +                        DEVICE_LITTLE_ENDIAN);
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->serial_mm), errp)) {
> +        return;
> +    }
> +
> +    sysbus_pass_irq(SYS_BUS_DEVICE(dev), SYS_BUS_DEVICE(&s->serial_mm));
> +
> +    memory_region_init(&s->container, OBJECT(s), "mchp.pfsoc.mmuart", 0x1000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
> +
> +    memory_region_add_subregion(&s->container, 0,
> +                    sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->serial_mm), 0));
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &mchp_pfsoc_mmuart_ops, s,
>                            "mchp.pfsoc.mmuart.regs", 0x1000 - REGS_OFFSET);
>      memory_region_add_subregion(&s->container, REGS_OFFSET, &s->iomem);
> -
> -    s->base = base;
> -    s->irq = irq;
> -
> -    s->serial = serial_mm_init(&s->container, 0, 2, irq, 399193, chr,
> -                               DEVICE_LITTLE_ENDIAN);
> -
> -    memory_region_add_subregion(sysmem, base, &s->container);
> -
> -    return s;
> +}
> +
> +static const VMStateDescription mchp_pfsoc_mmuart_vmstate = {
> +    .name = "mchp.pfsoc.uart",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(reg, MchpPfSoCMMUartState,
> +                             MCHP_PFSOC_MMUART_REG_COUNT),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void mchp_pfsoc_mmuart_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = mchp_pfsoc_mmuart_realize;
> +    dc->reset = mchp_pfsoc_mmuart_reset;
> +    dc->vmsd = &mchp_pfsoc_mmuart_vmstate;
> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> +}
> +
> +static const TypeInfo mchp_pfsoc_mmuart_info = {
> +    .name          = TYPE_MCHP_PFSOC_UART,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MchpPfSoCMMUartState),
> +    .instance_init = mchp_pfsoc_mmuart_init,
> +    .class_init    = mchp_pfsoc_mmuart_class_init,
> +};
> +
> +static void mchp_pfsoc_mmuart_register_types(void)
> +{
> +    type_register_static(&mchp_pfsoc_mmuart_info);
> +}
> +
> +type_init(mchp_pfsoc_mmuart_register_types)
> +
> +MchpPfSoCMMUartState *mchp_pfsoc_mmuart_create(MemoryRegion *sysmem,
> +                                               hwaddr base,
> +                                               qemu_irq irq, Chardev *chr)
> +{
> +    DeviceState *dev = qdev_new(TYPE_MCHP_PFSOC_UART);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    qdev_prop_set_chr(dev, "chardev", chr);
> +    sysbus_realize(sbd, &error_fatal);
> +
> +    memory_region_add_subregion(sysmem, base, sysbus_mmio_get_region(sbd, 0));
> +    sysbus_connect_irq(sbd, 0, irq);
> +
> +    return MCHP_PFSOC_UART(dev);
>  }
> --
> 2.31.1
>

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of
> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index f61990215f0..9c012e6c977 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -30,7 +30,7 @@
>
>  #include "hw/char/serial.h"
>
> -#define MCHP_PFSOC_MMUART_REG_SIZE  52
> +#define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
>      MemoryRegion iomem;
> @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
>
>      SerialMM *serial;
>
> -    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
> +    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
>
>  /**
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 2facf85c2d8..584e7fec17c 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr << 2);
>          return 0;
>      }
>
> -    return s->reg[addr / sizeof(uint32_t)];
> +    return s->reg[addr];
>  }
>
>  static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
> @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
>      MchpPfSoCMMUartState *s = opaque;
>      uint32_t val32 = (uint32_t)value;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
> -                      " v=0x%x\n", __func__, addr, val32);
> +                      " v=0x%x\n", __func__, addr << 2, val32);
>          return;
>      }
>
> -    s->reg[addr / sizeof(uint32_t)] = val32;
> +    s->reg[addr] = val32;
>  }
>
>  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> --
> 2.31.1
>

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of
> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index f61990215f0..9c012e6c977 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -30,7 +30,7 @@
>
>  #include "hw/char/serial.h"
>
> -#define MCHP_PFSOC_MMUART_REG_SIZE  52
> +#define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
>      MemoryRegion iomem;
> @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
>
>      SerialMM *serial;
>
> -    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
> +    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
>
>  /**
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 2facf85c2d8..584e7fec17c 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr << 2);
>          return 0;
>      }
>
> -    return s->reg[addr / sizeof(uint32_t)];
> +    return s->reg[addr];
>  }
>
>  static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
> @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
>      MchpPfSoCMMUartState *s = opaque;
>      uint32_t val32 = (uint32_t)value;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
> -                      " v=0x%x\n", __func__, addr, val32);
> +                      " v=0x%x\n", __func__, addr << 2, val32);
>          return;
>      }
>
> -    s->reg[addr / sizeof(uint32_t)] = val32;
> +    s->reg[addr] = val32;
>  }
>
>  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> --
> 2.31.1
>

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sun, Sep 26, 2021 at 6:31 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> > size occupied by all the registers. However all registers are
> > 32-bit wide, and the MemoryRegionOps handlers are restricted to
> > 32-bit:
> >
> >   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> >       .read = mchp_pfsoc_mmuart_read,
> >       .write = mchp_pfsoc_mmuart_write,
> >       .impl = {
> >           .min_access_size = 4,
> >           .max_access_size = 4,
> >       },
> >
> > Avoid being triskaidekaphobic, simplify by using the number of
>
> typo? See https://www.dictionary.com/browse/triskaidekaphobia
>
> Learned a new word today but I have to say this word is too hard for a
> non-native speaker :)

Ha! Even as a native English speaker I had to look it up :)

Alistair

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sun, Sep 26, 2021 at 6:31 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 25, 2021 at 9:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> > size occupied by all the registers. However all registers are
> > 32-bit wide, and the MemoryRegionOps handlers are restricted to
> > 32-bit:
> >
> >   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> >       .read = mchp_pfsoc_mmuart_read,
> >       .write = mchp_pfsoc_mmuart_write,
> >       .impl = {
> >           .min_access_size = 4,
> >           .max_access_size = 4,
> >       },
> >
> > Avoid being triskaidekaphobic, simplify by using the number of
>
> typo? See https://www.dictionary.com/browse/triskaidekaphobia
>
> Learned a new word today but I have to say this word is too hard for a
> non-native speaker :)

Ha! Even as a native English speaker I had to look it up :)

Alistair

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of
> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index f61990215f0..9c012e6c977 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -30,7 +30,7 @@
>
>  #include "hw/char/serial.h"
>
> -#define MCHP_PFSOC_MMUART_REG_SIZE  52
> +#define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
>      MemoryRegion iomem;
> @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
>
>      SerialMM *serial;
>
> -    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
> +    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
>
>  /**
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 2facf85c2d8..584e7fec17c 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr << 2);
>          return 0;
>      }
>
> -    return s->reg[addr / sizeof(uint32_t)];
> +    return s->reg[addr];
>  }
>
>  static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
> @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
>      MchpPfSoCMMUartState *s = opaque;
>      uint32_t val32 = (uint32_t)value;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
> -                      " v=0x%x\n", __func__, addr, val32);
> +                      " v=0x%x\n", __func__, addr << 2, val32);
>          return;
>      }
>
> -    s->reg[addr / sizeof(uint32_t)] = val32;
> +    s->reg[addr] = val32;
>  }
>
>  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> --
> 2.31.1
>

Re: [PATCH v2 1/3] hw/char/mchp_pfsoc_mmuart: Simplify MCHP_PFSOC_MMUART_REG definition

[Alistair Francis]

On Sat, Sep 25, 2021 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The current MCHP_PFSOC_MMUART_REG_SIZE definition represent the
> size occupied by all the registers. However all registers are
> 32-bit wide, and the MemoryRegionOps handlers are restricted to
> 32-bit:
>
>   static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
>       .read = mchp_pfsoc_mmuart_read,
>       .write = mchp_pfsoc_mmuart_write,
>       .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
>
> Avoid being triskaidekaphobic, simplify by using the number of
> registers.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  include/hw/char/mchp_pfsoc_mmuart.h |  4 ++--
>  hw/char/mchp_pfsoc_mmuart.c         | 14 ++++++++------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/include/hw/char/mchp_pfsoc_mmuart.h b/include/hw/char/mchp_pfsoc_mmuart.h
> index f61990215f0..9c012e6c977 100644
> --- a/include/hw/char/mchp_pfsoc_mmuart.h
> +++ b/include/hw/char/mchp_pfsoc_mmuart.h
> @@ -30,7 +30,7 @@
>
>  #include "hw/char/serial.h"
>
> -#define MCHP_PFSOC_MMUART_REG_SIZE  52
> +#define MCHP_PFSOC_MMUART_REG_COUNT 13
>
>  typedef struct MchpPfSoCMMUartState {
>      MemoryRegion iomem;
> @@ -39,7 +39,7 @@ typedef struct MchpPfSoCMMUartState {
>
>      SerialMM *serial;
>
> -    uint32_t reg[MCHP_PFSOC_MMUART_REG_SIZE / sizeof(uint32_t)];
> +    uint32_t reg[MCHP_PFSOC_MMUART_REG_COUNT];
>  } MchpPfSoCMMUartState;
>
>  /**
> diff --git a/hw/char/mchp_pfsoc_mmuart.c b/hw/char/mchp_pfsoc_mmuart.c
> index 2facf85c2d8..584e7fec17c 100644
> --- a/hw/char/mchp_pfsoc_mmuart.c
> +++ b/hw/char/mchp_pfsoc_mmuart.c
> @@ -29,13 +29,14 @@ static uint64_t mchp_pfsoc_mmuart_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MchpPfSoCMMUartState *s = opaque;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: read: addr=0x%" HWADDR_PRIx "\n",
> -                      __func__, addr);
> +                      __func__, addr << 2);
>          return 0;
>      }
>
> -    return s->reg[addr / sizeof(uint32_t)];
> +    return s->reg[addr];
>  }
>
>  static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
> @@ -44,13 +45,14 @@ static void mchp_pfsoc_mmuart_write(void *opaque, hwaddr addr,
>      MchpPfSoCMMUartState *s = opaque;
>      uint32_t val32 = (uint32_t)value;
>
> -    if (addr >= MCHP_PFSOC_MMUART_REG_SIZE) {
> +    addr >>= 2;
> +    if (addr >= MCHP_PFSOC_MMUART_REG_COUNT) {
>          qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write: addr=0x%" HWADDR_PRIx
> -                      " v=0x%x\n", __func__, addr, val32);
> +                      " v=0x%x\n", __func__, addr << 2, val32);
>          return;
>      }
>
> -    s->reg[addr / sizeof(uint32_t)] = val32;
> +    s->reg[addr] = val32;
>  }
>
>  static const MemoryRegionOps mchp_pfsoc_mmuart_ops = {
> --
> 2.31.1
>