[PATCH 0/2] Introduce PCI mbox module for Nuvoton SoC

[PATCH 0/2] Introduce PCI mbox module for Nuvoton SoC

[Patrick Venture]

The PCI mbox module connects to a host such that the host can interact with the BMC SoC as a PCI device using a chardev.  This chardev portion will be in a follow-on CL with the corresponding documentation.

Hao Wu (2):
  hw/misc: Add Nuvoton's PCI Mailbox Module
  hw/arm: Add PCI mailbox module to Nuvoton SoC

 docs/system/arm/nuvoton.rst        |   1 +
 hw/arm/npcm7xx.c                   |  15 ++-
 hw/misc/meson.build                |   1 +
 hw/misc/npcm7xx_pci_mbox.c         | 178 +++++++++++++++++++++++++++++
 hw/misc/trace-events               |   5 +
 include/hw/arm/npcm7xx.h           |   2 +
 include/hw/misc/npcm7xx_pci_mbox.h |  63 ++++++++++
 7 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 hw/misc/npcm7xx_pci_mbox.c
 create mode 100644 include/hw/misc/npcm7xx_pci_mbox.h

-- 
2.34.1.575.g55b058a8bb-goog

[PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module

[Patrick Venture]

From: Hao Wu <wuhaotsh@google.com>

The PCI Mailbox Module is a high-bandwidth communcation module
between a Nuvoton BMC and CPU. It features 16KB RAM that are both
accessible by the BMC and core CPU. and supports interrupt for
both sides.

This patch implements the BMC side of the PCI mailbox module.
Communication with the core CPU is emulated via a chardev and
will be in a follow-up patch.

Reviewed-by: Patrick Venture <venture@google.com>
Reviewed-by: Joe Komlodi <komlodi@google.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 hw/misc/meson.build                |   1 +
 hw/misc/npcm7xx_pci_mbox.c         | 178 +++++++++++++++++++++++++++++
 hw/misc/trace-events               |   5 +
 include/hw/misc/npcm7xx_pci_mbox.h |  63 ++++++++++
 4 files changed, 247 insertions(+)
 create mode 100644 hw/misc/npcm7xx_pci_mbox.c
 create mode 100644 include/hw/misc/npcm7xx_pci_mbox.h

diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 3f41a3a5b2..da8d6acaa3 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -63,6 +63,7 @@ softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files(
   'npcm7xx_clk.c',
   'npcm7xx_gcr.c',
   'npcm7xx_mft.c',
+  'npcm7xx_pci_mbox.c',
   'npcm7xx_pwm.c',
   'npcm7xx_rng.c',
 ))
diff --git a/hw/misc/npcm7xx_pci_mbox.c b/hw/misc/npcm7xx_pci_mbox.c
new file mode 100644
index 0000000000..604b3c5fb1
--- /dev/null
+++ b/hw/misc/npcm7xx_pci_mbox.c
@@ -0,0 +1,178 @@
+/*
+ * Nuvoton NPCM7xx PCI Mailbox Module
+ *
+ * Copyright 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-properties-system.h"
+#include "hw/misc/npcm7xx_pci_mbox.h"
+#include "hw/registerfields.h"
+#include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+#include "qemu/bitops.h"
+#include "qemu/error-report.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/timer.h"
+#include "qemu/units.h"
+#include "trace.h"
+
+REG32(NPCM7XX_PCI_MBOX_BMBXSTAT, 0x00);
+REG32(NPCM7XX_PCI_MBOX_BMBXCTL, 0x04);
+REG32(NPCM7XX_PCI_MBOX_BMBXCMD, 0x08);
+
+
+#define NPCM7XX_PCI_MBOX_NR_CI 8
+#define NPCM7XX_PCI_MBOX_CI_MASK MAKE_64BIT_MASK(0, NPCM7XX_PCI_MBOX_NR_CI)
+
+static void npcm7xx_pci_mbox_update_irq(NPCM7xxPCIMBoxState *s)
+{
+    /* We should send an interrupt when one of the CIE and CIF are both 1. */
+    if (s->regs[R_NPCM7XX_PCI_MBOX_BMBXSTAT] &
+        s->regs[R_NPCM7XX_PCI_MBOX_BMBXCTL] &
+        NPCM7XX_PCI_MBOX_CI_MASK) {
+        qemu_irq_raise(s->irq);
+        trace_npcm7xx_pci_mbox_irq(1);
+    } else {
+        qemu_irq_lower(s->irq);
+        trace_npcm7xx_pci_mbox_irq(0);
+    }
+}
+
+static uint64_t npcm7xx_pci_mbox_read(void *opaque, hwaddr offset,
+                                      unsigned size)
+{
+    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(opaque);
+    uint16_t value = 0;
+
+    if (offset / sizeof(uint32_t) >= NPCM7XX_PCI_MBOX_NR_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
+                      __func__, offset);
+        return 0;
+    }
+
+    value = s->regs[offset / sizeof(uint32_t)];
+    trace_npcm7xx_pci_mbox_read(DEVICE(s)->canonical_path, offset, value,
+                                size);
+    return value;
+}
+
+static void npcm7xx_pci_mbox_write(void *opaque, hwaddr offset,
+                              uint64_t v, unsigned size)
+{
+    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(opaque);
+
+    trace_npcm7xx_pci_mbox_write(DEVICE(s)->canonical_path, offset, v, size);
+    switch (offset) {
+    case A_NPCM7XX_PCI_MBOX_BMBXSTAT:
+        /* Clear bits that are 1. */
+        s->regs[R_NPCM7XX_PCI_MBOX_BMBXSTAT] &= ~v;
+        break;
+
+    case A_NPCM7XX_PCI_MBOX_BMBXCTL:
+        s->regs[R_NPCM7XX_PCI_MBOX_BMBXCTL] = v;
+        break;
+
+    case A_NPCM7XX_PCI_MBOX_BMBXCMD:
+        /* Set the bits that are 1. */
+        s->regs[R_NPCM7XX_PCI_MBOX_BMBXCMD] |= v;
+        /* TODO: Set interrupt to host. */
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
+                      __func__, offset);
+    }
+    npcm7xx_pci_mbox_update_irq(s);
+}
+
+static const struct MemoryRegionOps npcm7xx_pci_mbox_ops = {
+    .read       = npcm7xx_pci_mbox_read,
+    .write      = npcm7xx_pci_mbox_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid      = {
+        .min_access_size        = 4,
+        .max_access_size        = 4,
+        .unaligned              = false,
+    },
+};
+
+static void npcm7xx_pci_mbox_enter_reset(Object *obj, ResetType type)
+{
+    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
+
+    memset(s->regs, 0, 4 * NPCM7XX_PCI_MBOX_NR_REGS);
+}
+
+static void npcm7xx_pci_mbox_hold_reset(Object *obj)
+{
+    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
+
+    qemu_irq_lower(s->irq);
+}
+
+static void npcm7xx_pci_mbox_init(Object *obj)
+{
+    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_ram_device_ptr(&s->ram, obj, "pci-mbox-ram",
+                                      NPCM7XX_PCI_MBOX_RAM_SIZE, s->content);
+    memory_region_init_io(&s->iomem, obj, &npcm7xx_pci_mbox_ops, s,
+                          "pci-mbox-iomem", 4 * KiB);
+    sysbus_init_mmio(sbd, &s->ram);
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+}
+
+static const VMStateDescription vmstate_npcm7xx_pci_mbox = {
+    .name = "npcm7xx-pci-mbox-module",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, NPCM7xxPCIMBoxState,
+                             NPCM7XX_PCI_MBOX_NR_REGS),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static void npcm7xx_pci_mbox_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "NPCM7xx PCI Mailbox Controller";
+    dc->vmsd = &vmstate_npcm7xx_pci_mbox;
+    rc->phases.enter = npcm7xx_pci_mbox_enter_reset;
+    rc->phases.hold = npcm7xx_pci_mbox_hold_reset;
+}
+
+static const TypeInfo npcm7xx_pci_mbox_info = {
+    .name               = TYPE_NPCM7XX_PCI_MBOX,
+    .parent             = TYPE_SYS_BUS_DEVICE,
+    .instance_size      = sizeof(NPCM7xxPCIMBoxState),
+    .class_init         = npcm7xx_pci_mbox_class_init,
+    .instance_init      = npcm7xx_pci_mbox_init,
+};
+
+static void npcm7xx_pci_mbox_register_type(void)
+{
+    type_register_static(&npcm7xx_pci_mbox_info);
+}
+type_init(npcm7xx_pci_mbox_register_type);
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 2da96d167a..61d3dd5524 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -124,6 +124,11 @@ npcm7xx_pwm_write(const char *id, uint64_t offset, uint32_t value) "%s offset: 0
 npcm7xx_pwm_update_freq(const char *id, uint8_t index, uint32_t old_value, uint32_t new_value) "%s pwm[%u] Update Freq: old_freq: %u, new_freq: %u"
 npcm7xx_pwm_update_duty(const char *id, uint8_t index, uint32_t old_value, uint32_t new_value) "%s pwm[%u] Update Duty: old_duty: %u, new_duty: %u"
 
+# npcm7xx_pci_mbox.c
+npcm7xx_pci_mbox_read(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
+npcm7xx_pci_mbox_write(const char *id, uint64_t offset, uint64_t value, unsigned size) "%s offset: 0x%04" PRIx64 " value: 0x%02" PRIx64 " size: %u"
+npcm7xx_pci_mbox_irq(int irq_level) "irq level: %d"
+
 # stm32f4xx_syscfg.c
 stm32f4xx_syscfg_set_irq(int gpio, int line, int level) "Interrupt: GPIO: %d, Line: %d; Level: %d"
 stm32f4xx_pulse_exti(int irq) "Pulse EXTI: %d"
diff --git a/include/hw/misc/npcm7xx_pci_mbox.h b/include/hw/misc/npcm7xx_pci_mbox.h
new file mode 100644
index 0000000000..0f8fda0db1
--- /dev/null
+++ b/include/hw/misc/npcm7xx_pci_mbox.h
@@ -0,0 +1,63 @@
+/*
+ * Nuvoton NPCM7xx PCI Mailbox Module
+ *
+ * Copyright 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+#ifndef NPCM7XX_PCI_MBOX_H
+#define NPCM7XX_PCI_MBOX_H
+
+#include "chardev/char-fe.h"
+#include "exec/memory.h"
+#include "hw/clock.h"
+#include "hw/irq.h"
+#include "hw/pci/pci.h"
+#include "hw/sysbus.h"
+#include "qom/object.h"
+
+#define NPCM7XX_PCI_MBOX_RAM_SIZE 0x4000
+
+#define NPCM7XX_PCI_VENDOR_ID   0x1050
+#define NPCM7XX_PCI_DEVICE_ID   0x0750
+#define NPCM7XX_PCI_REVISION    0
+#define NPCM7XX_PCI_CLASS_CODE  0xff
+
+/*
+ * Maximum amount of control registers in PCI Mailbox module. Do not increase
+ * this value without bumping vm version.
+ */
+#define NPCM7XX_PCI_MBOX_NR_REGS 3
+
+/**
+ * struct NPCM7xxPciMboxState - PCI Mailbox Device
+ * @parent: System bus device.
+ * @ram: the mailbox RAM memory space
+ * @iomem: Memory region through which registers are accessed.
+ * @content: The content of the PCI mailbox, initialized to 0.
+ * @regs: The MMIO registers.
+ */
+typedef struct NPCM7xxPCIMBoxState {
+    SysBusDevice parent;
+
+    MemoryRegion ram;
+    MemoryRegion iomem;
+
+    qemu_irq irq;
+    uint8_t content[NPCM7XX_PCI_MBOX_RAM_SIZE];
+    uint32_t regs[NPCM7XX_PCI_MBOX_NR_REGS];
+} NPCM7xxPCIMBoxState;
+
+#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox"
+#define NPCM7XX_PCI_MBOX(obj) \
+    OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX)
+
+#endif /* NPCM7XX_PCI_MBOX_H */
-- 
2.34.1.575.g55b058a8bb-goog

[PATCH 2/2] hw/arm: Add PCI mailbox module to Nuvoton SoC

[Patrick Venture]

From: Hao Wu <wuhaotsh@google.com>

This patch wires the PCI mailbox module to Nuvoton SoC.

Reviewed-by: Patrick Venture <venture@google.com>
Reviewed-by: Joe Komlodi <komlodi@google.com>
Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 docs/system/arm/nuvoton.rst |  1 +
 hw/arm/npcm7xx.c            | 15 ++++++++++++++-
 include/hw/arm/npcm7xx.h    |  2 ++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index adf497e679..706c6f61c1 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -48,6 +48,7 @@ Supported devices
  * SMBus controller (SMBF)
  * Ethernet controller (EMC)
  * Tachometer
+ * PCI Mailbox
 
 Missing devices
 ---------------
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 878c2208e0..ef8c9129ca 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -53,6 +53,9 @@
 /* ADC Module */
 #define NPCM7XX_ADC_BA          (0xf000c000)
 
+/* PCI Mailbox Module */
+#define NPCM7XX_PCI_MBOX_BA     (0xf0848000)
+
 /* Internal AHB SRAM */
 #define NPCM7XX_RAM3_BA         (0xc0008000)
 #define NPCM7XX_RAM3_SZ         (4 * KiB)
@@ -83,6 +86,7 @@ enum NPCM7xxInterrupt {
     NPCM7XX_UART1_IRQ,
     NPCM7XX_UART2_IRQ,
     NPCM7XX_UART3_IRQ,
+    NPCM7XX_PCI_MBOX_IRQ        = 8,
     NPCM7XX_EMC1RX_IRQ          = 15,
     NPCM7XX_EMC1TX_IRQ,
     NPCM7XX_MMC_IRQ             = 26,
@@ -447,6 +451,8 @@ static void npcm7xx_init(Object *obj)
         object_initialize_child(obj, "emc[*]", &s->emc[i], TYPE_NPCM7XX_EMC);
     }
 
+    object_initialize_child(obj, "pci-mbox", &s->pci_mbox,
+                            TYPE_NPCM7XX_PCI_MBOX);
     object_initialize_child(obj, "mmc", &s->mmc, TYPE_NPCM7XX_SDHCI);
 }
 
@@ -697,6 +703,14 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    /* PCI Mailbox. Cannot fail */
+    sysbus_realize(SYS_BUS_DEVICE(&s->pci_mbox), &error_abort);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->pci_mbox), 0, NPCM7XX_PCI_MBOX_BA);
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->pci_mbox), 1,
+        NPCM7XX_PCI_MBOX_BA + NPCM7XX_PCI_MBOX_RAM_SIZE);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pci_mbox), 0,
+                       npcm7xx_irq(s, NPCM7XX_PCI_MBOX_IRQ));
+
     /* RAM2 (SRAM) */
     memory_region_init_ram(&s->sram, OBJECT(dev), "ram2",
                            NPCM7XX_RAM2_SZ, &error_abort);
@@ -747,7 +761,6 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
     create_unimplemented_device("npcm7xx.usbd[8]",      0xf0838000,   4 * KiB);
     create_unimplemented_device("npcm7xx.usbd[9]",      0xf0839000,   4 * KiB);
     create_unimplemented_device("npcm7xx.sd",           0xf0840000,   8 * KiB);
-    create_unimplemented_device("npcm7xx.pcimbx",       0xf0848000, 512 * KiB);
     create_unimplemented_device("npcm7xx.aes",          0xf0858000,   4 * KiB);
     create_unimplemented_device("npcm7xx.des",          0xf0859000,   4 * KiB);
     create_unimplemented_device("npcm7xx.sha",          0xf085a000,   4 * KiB);
diff --git a/include/hw/arm/npcm7xx.h b/include/hw/arm/npcm7xx.h
index ce593235d9..0bc27a28d6 100644
--- a/include/hw/arm/npcm7xx.h
+++ b/include/hw/arm/npcm7xx.h
@@ -26,6 +26,7 @@
 #include "hw/misc/npcm7xx_clk.h"
 #include "hw/misc/npcm7xx_gcr.h"
 #include "hw/misc/npcm7xx_mft.h"
+#include "hw/misc/npcm7xx_pci_mbox.h"
 #include "hw/misc/npcm7xx_pwm.h"
 #include "hw/misc/npcm7xx_rng.h"
 #include "hw/net/npcm7xx_emc.h"
@@ -104,6 +105,7 @@ typedef struct NPCM7xxState {
     OHCISysBusState     ohci;
     NPCM7xxFIUState     fiu[2];
     NPCM7xxEMCState     emc[2];
+    NPCM7xxPCIMBoxState pci_mbox;
     NPCM7xxSDHCIState   mmc;
 } NPCM7xxState;
 
-- 
2.34.1.575.g55b058a8bb-goog

Re: [PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module

[Peter Maydell]

On Mon, 10 Jan 2022 at 17:56, Patrick Venture <venture@google.com> wrote:
>
> From: Hao Wu <wuhaotsh@google.com>
>
> The PCI Mailbox Module is a high-bandwidth communcation module
> between a Nuvoton BMC and CPU. It features 16KB RAM that are both
> accessible by the BMC and core CPU. and supports interrupt for
> both sides.
>
> This patch implements the BMC side of the PCI mailbox module.
> Communication with the core CPU is emulated via a chardev and
> will be in a follow-up patch.
>
> Reviewed-by: Patrick Venture <venture@google.com>
> Reviewed-by: Joe Komlodi <komlodi@google.com>

Hi; this mostly looks good, but I have a question about s->content.

> +static void npcm7xx_pci_mbox_init(Object *obj)
> +{
> +    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_ram_device_ptr(&s->ram, obj, "pci-mbox-ram",
> +                                      NPCM7XX_PCI_MBOX_RAM_SIZE, s->content);

What's s->content for? Nothing in the rest of the device emulation
touches it, which seems odd.

memory_region_init_ram_device_ptr() is intended primarily
for "create a MemoryRegion corresponding to something like
a bit of a host device (eg a host PCI MMIO BAR). That doesn't
seem like what you're doing here. In particular, using it
means that you take on responsibility for ensuring that the
underlying memory gets migrated, which you're not doing.

If you just want a MemoryRegion that's backed by a bit of
host memory, use memory_region_init_ram().

> +#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox"
> +#define NPCM7XX_PCI_MBOX(obj) \
> +    OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX)

We prefer the OBJECT_DECLARE_SIMPLE_TYPE() macro rather than
hand-defining a cast macro these days.

thanks
-- PMM

Re: [PATCH 2/2] hw/arm: Add PCI mailbox module to Nuvoton SoC

[Peter Maydell]

On Mon, 10 Jan 2022 at 17:56, Patrick Venture <venture@google.com> wrote:
>
> From: Hao Wu <wuhaotsh@google.com>
>
> This patch wires the PCI mailbox module to Nuvoton SoC.
>
> Reviewed-by: Patrick Venture <venture@google.com>
> Reviewed-by: Joe Komlodi <komlodi@google.com>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---

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

thanks
-- PMM

Re: [PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

On Thu, Jan 27, 2022 at 10:37 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 10 Jan 2022 at 17:56, Patrick Venture <venture@google.com> wrote:
> >
> > From: Hao Wu <wuhaotsh@google.com>
> >
> > The PCI Mailbox Module is a high-bandwidth communcation module
> > between a Nuvoton BMC and CPU. It features 16KB RAM that are both
> > accessible by the BMC and core CPU. and supports interrupt for
> > both sides.
> >
> > This patch implements the BMC side of the PCI mailbox module.
> > Communication with the core CPU is emulated via a chardev and
> > will be in a follow-up patch.
> >
> > Reviewed-by: Patrick Venture <venture@google.com>
> > Reviewed-by: Joe Komlodi <komlodi@google.com>
>
> Hi; this mostly looks good, but I have a question about s->content.
>
> > +static void npcm7xx_pci_mbox_init(Object *obj)
> > +{
> > +    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +
> > +    memory_region_init_ram_device_ptr(&s->ram, obj, "pci-mbox-ram",
> > +                                      NPCM7XX_PCI_MBOX_RAM_SIZE,
> s->content);
>
> What's s->content for? Nothing in the rest of the device emulation
> touches it, which seems odd.
>

Hao informed me that we can drop the content bit here, since it's not used
until a later patch that we'll be sending up with a bit more detail when we
get a chance. I sent this series up to land some of the groundwork.

I can send out a v2 with that bit removed.


>
> memory_region_init_ram_device_ptr() is intended primarily
> for "create a MemoryRegion corresponding to something like
> a bit of a host device (eg a host PCI MMIO BAR). That doesn't
> seem like what you're doing here. In particular, using it
> means that you take on responsibility for ensuring that the
> underlying memory gets migrated, which you're not doing.
>
> If you just want a MemoryRegion that's backed by a bit of
> host memory, use memory_region_init_ram().
>

I think this is what we use it for in the future patches, when we add it
back, it'll come with the context.


>
> > +#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox"
> > +#define NPCM7XX_PCI_MBOX(obj) \
> > +    OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX)
>
> We prefer the OBJECT_DECLARE_SIMPLE_TYPE() macro rather than
> hand-defining a cast macro these days.
>

Ack.


>
> thanks
> -- PMM
>

Thanks!

[-- Attachment #2: Type: text/html, Size: 3808 bytes --]

Re: [PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 2733 bytes --]

On Thu, Jan 27, 2022 at 1:27 PM Patrick Venture <venture@google.com> wrote:

>
>
> On Thu, Jan 27, 2022 at 10:37 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On Mon, 10 Jan 2022 at 17:56, Patrick Venture <venture@google.com> wrote:
>> >
>> > From: Hao Wu <wuhaotsh@google.com>
>> >
>> > The PCI Mailbox Module is a high-bandwidth communcation module
>> > between a Nuvoton BMC and CPU. It features 16KB RAM that are both
>> > accessible by the BMC and core CPU. and supports interrupt for
>> > both sides.
>> >
>> > This patch implements the BMC side of the PCI mailbox module.
>> > Communication with the core CPU is emulated via a chardev and
>> > will be in a follow-up patch.
>> >
>> > Reviewed-by: Patrick Venture <venture@google.com>
>> > Reviewed-by: Joe Komlodi <komlodi@google.com>
>>
>> Hi; this mostly looks good, but I have a question about s->content.
>>
>> > +static void npcm7xx_pci_mbox_init(Object *obj)
>> > +{
>> > +    NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
>> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> > +
>> > +    memory_region_init_ram_device_ptr(&s->ram, obj, "pci-mbox-ram",
>> > +                                      NPCM7XX_PCI_MBOX_RAM_SIZE,
>> s->content);
>>
>> What's s->content for? Nothing in the rest of the device emulation
>> touches it, which seems odd.
>>
>
> Hao informed me that we can drop the content bit here, since it's not used
> until a later patch that we'll be sending up with a bit more detail when we
> get a chance. I sent this series up to land some of the groundwork.
>
> I can send out a v2 with that bit removed.
>
>
>>
>> memory_region_init_ram_device_ptr() is intended primarily
>> for "create a MemoryRegion corresponding to something like
>> a bit of a host device (eg a host PCI MMIO BAR). That doesn't
>> seem like what you're doing here. In particular, using it
>> means that you take on responsibility for ensuring that the
>> underlying memory gets migrated, which you're not doing.
>>
>> If you just want a MemoryRegion that's backed by a bit of
>> host memory, use memory_region_init_ram().
>>
>
> I think this is what we use it for in the future patches, when we add it
> back, it'll come with the context.
>
>
>>
>> > +#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox"
>> > +#define NPCM7XX_PCI_MBOX(obj) \
>> > +    OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX)
>>
>> We prefer the OBJECT_DECLARE_SIMPLE_TYPE() macro rather than
>> hand-defining a cast macro these days.
>>
>
> Ack.
>
>
>>
>> thanks
>> -- PMM
>>
>
Peter, just an FYI, this fell off my radar, but I will be circling back
this week to revisit any patches I've sent that are in limbo or waiting on
me, etc.  Thanks for your patience.


>
> Thanks!
>

[-- Attachment #2: Type: text/html, Size: 4752 bytes --]