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