[2/3] hw/pci-host: Add emulation of Mai Logic Articia S

The Articia S is a generic chipset supporting several different CPUs
that were used on some PPC boards. This is a minimal emulation of the
parts needed for emulating the AmigaOne board.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/Kconfig           |   5 +
 hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
 hw/pci-host/meson.build       |   2 +
 include/hw/pci-host/articia.h |  17 +++
 4 files changed, 290 insertions(+)
 create mode 100644 hw/pci-host/articia.c
 create mode 100644 include/hw/pci-host/articia.h

Am 06.10.23 um 00:13 schrieb BALATON Zoltan:
> The Articia S is a generic chipset supporting several different CPUs
> that were used on some PPC boards. This is a minimal emulation of the
> parts needed for emulating the AmigaOne board.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/pci-host/Kconfig           |   5 +
>  hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>  hw/pci-host/meson.build       |   2 +
>  include/hw/pci-host/articia.h |  17 +++
>  4 files changed, 290 insertions(+)
>  create mode 100644 hw/pci-host/articia.c
>  create mode 100644 include/hw/pci-host/articia.h
> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
> new file mode 100644
> index 0000000000..80558e1c47
> --- /dev/null
> +++ b/hw/pci-host/articia.c
> @@ -0,0 +1,266 @@
> +/*
> + * Mai Logic Articia S emulation
> + *
> + * Copyright (c) 2023 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/pci/pci_device.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/irq.h"
> +#include "hw/i2c/bitbang_i2c.h"
> +#include "hw/intc/i8259.h"
> +#include "hw/pci-host/articia.h"
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
> +struct ArticiaHostState {
> +    PCIDevice parent_obj;
> +
> +    ArticiaState *as;
> +};
> +
> +/* TYPE_ARTICIA */
> +
> +struct ArticiaState {
> +    PCIHostState parent_obj;
> +
> +    qemu_irq irq[PCI_NUM_PINS];
> +    MemoryRegion io;
> +    MemoryRegion mem;
> +    MemoryRegion reg;
> +
> +    bitbang_i2c_interface smbus;
> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
> +    hwaddr gpio_base;
> +    MemoryRegion gpio_reg;
> +};
> +
> +static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +
> +    return (s->gpio >> (addr * 8)) & 0xff;
> +}
> +
> +static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
> +                               unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +    uint32_t sh = addr * 8;
> +
> +    if (addr == 0) {
> +        /* in bits read only? */
> +        return;
> +    }
> +
> +    if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
> +        s->gpio &= ~(0xff << sh | 0xff);
> +        s->gpio |= (val & 0xff) << sh;
> +        s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
> +                                   s->gpio & BIT(16) ?
> +                                   !!(s->gpio & BIT(8)) : 1);
> +        if ((s->gpio & BIT(17))) {
> +            s->gpio &= ~BIT(0);
> +            s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
> +                                       !!(s->gpio & BIT(9)));
> +        }
> +    }
> +}
> +
> +static const MemoryRegionOps articia_gpio_ops = {
> +    .read = articia_gpio_read,
> +    .write = articia_gpio_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 1,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +    uint64_t ret = UINT_MAX;
> +
> +    switch (addr) {
> +    case 0xc00cf8:
> +        ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
> +        break;
> +    case 0xe00cfc ... 0xe00cff:
> +        ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
> +        break;
> +    case 0xf00000:
> +        ret = pic_read_irq(isa_pic);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
> +                      HWADDR_PRIx " %d\n", __func__, addr, size);
> +        break;
> +    }
> +    return ret;
> +}
> +
> +static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
> +                              unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +
> +    switch (addr) {
> +    case 0xc00cf8:
> +        pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
> +        break;
> +    case 0xe00cfc ... 0xe00cff:
> +        pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
> +                      HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps articia_reg_ops = {
> +    .read = articia_reg_read,
> +    .write = articia_reg_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void articia_pcihost_set_irq(void *opaque, int n, int level)
> +{
> +    ArticiaState *s = opaque;
> +    qemu_set_irq(s->irq[n], level);
> +}
> +
> +static void articia_realize(DeviceState *dev, Error **errp)
> +{
> +    ArticiaState *s = ARTICIA(dev);
> +    PCIHostState *h = PCI_HOST_BRIDGE(dev);
> +    PCIDevice *pdev;
> +
> +    bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
> +    memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
> +                          TYPE_ARTICIA, 4);
> +
> +    memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
> +    memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
> +    memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
> +                          TYPE_ARTICIA, 0x1000000);
> +    memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
> +
> +    /* devfn_min is 8 that matches first PCI slot in AmigaOne */
> +    h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
> +                                   pci_swizzle_map_irq_fn, dev, &s->mem,
> +                                   &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);

Hi,

pci_swizzle_map_irq_fn() doesn't correctly map all AmigaOne PCI
interrupt pin on device to interrupt pin on connector connections.

I found this mapping and I think it's correct. For the VIA Southbridge
at 00:07.0 it's the only possible correct mapping. All PCI functions
interrupts are connected internally.

diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
index d9f052e3f0..2ad011af9c 100644
--- a/hw/pci-host/articia.c
+++ b/hw/pci-host/articia.c
@@ -138,6 +138,29 @@ static void articia_pcihost_set_irq(void *opaque,
int n, int level)
     qemu_set_irq(s->irq[n], level);
 }
 
+/*
+ * AmigaOne SE PCI slot to IRQ routing
+ *
+ * repository: https://source.denx.de/u-boot/custodians/u-boot-avr32.git
+ * refspec: v2010.06
+ * file: board/MAI/AmigaOneG3SE/articiaS_pci.c
+ */
+static int amigaone_pcihost_bus0_map_irq(PCIDevice *pdev, int pin)
+{
+    switch(PCI_SLOT(pdev->devfn))
+    {
+    case 6:     /* On board ethernet */
+        return 3;
+    case 7:     /* Southbridge */
+        return pin;
+    default:
+        break;
+    }
+
+    /* Slot 1 Device 8, Slot 2 Device 9, Slot 3 Device 10 */
+    return pci_swizzle(PCI_SLOT(pdev->devfn), pin);
+}
+
 static void articia_realize(DeviceState *dev, Error **errp)
 {
     ArticiaState *s = ARTICIA(dev);
@@ -155,7 +178,7 @@ static void articia_realize(DeviceState *dev, Error
**errp)
     memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
 
     h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
-                                   pci_swizzle_map_irq_fn, dev,
+                                   amigaone_pcihost_bus0_map_irq, dev,
                                    &s->mem, &s->io, 0, 4, TYPE_PCI_BUS);
     pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
                                            TYPE_ARTICIA_PCI_HOST);

With best regards,
Volker


> +    pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
> +                                           TYPE_ARTICIA_PCI_HOST);
> +    ARTICIA_PCI_HOST(pdev)->as = s;
> +    pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
> +
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
> +    qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
> +}
> +
> +static void articia_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = articia_realize;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +}
> +
> +/* TYPE_ARTICIA_PCI_HOST */
> +
> +static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
> +                                       uint32_t val, int len)
> +{
> +    ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
> +
> +    pci_default_write_config(d, addr, val, len);
> +    switch (addr) {
> +    case 0x40:
> +        s->gpio_base = val;
> +        break;
> +    case 0x44:
> +        if (val != 0x11) {
> +            /* FIXME what do the bits actually mean? */
> +            break;
> +        }
> +        if (memory_region_is_mapped(&s->gpio_reg)) {
> +            memory_region_del_subregion(&s->io, &s->gpio_reg);
> +        }
> +        memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
> +        break;
> +    }
> +}
> +
> +static void articia_pci_host_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->config_write = articia_pci_host_cfg_write;
> +    k->vendor_id = 0x10cc;
> +    k->device_id = 0x0660;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge,
> +     * not usable without the host-facing part
> +     */
> +    dc->user_creatable = false;
> +}
> +
> +/* TYPE_ARTICIA_PCI_BRIDGE */
> +
> +static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->vendor_id = 0x10cc;
> +    k->device_id = 0x0661;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge,
> +     * not usable without the host-facing part
> +     */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo articia_types[] = {
> +    {
> +        .name          = TYPE_ARTICIA,
> +        .parent        = TYPE_PCI_HOST_BRIDGE,
> +        .instance_size = sizeof(ArticiaState),
> +        .class_init    = articia_class_init,
> +    },
> +    {
> +        .name          = TYPE_ARTICIA_PCI_HOST,
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(ArticiaHostState),
> +        .class_init    = articia_pci_host_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +              { },
> +        },
> +    },
> +    {
> +        .name          = TYPE_ARTICIA_PCI_BRIDGE,
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(PCIDevice),
> +        .class_init    = articia_pci_bridge_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +              { },
> +        },
> +    },
> +};
> +
> +DEFINE_TYPES(articia_types)
>

On Fri, 6 Oct 2023, Volker Rümelin wrote:
> Am 06.10.23 um 00:13 schrieb BALATON Zoltan:
>> The Articia S is a generic chipset supporting several different CPUs
>> that were used on some PPC boards. This is a minimal emulation of the
>> parts needed for emulating the AmigaOne board.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/pci-host/Kconfig           |   5 +
>>  hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>>  hw/pci-host/meson.build       |   2 +
>>  include/hw/pci-host/articia.h |  17 +++
>>  4 files changed, 290 insertions(+)
>>  create mode 100644 hw/pci-host/articia.c
>>  create mode 100644 include/hw/pci-host/articia.h
>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>> new file mode 100644
>> index 0000000000..80558e1c47
>> --- /dev/null
>> +++ b/hw/pci-host/articia.c
>> @@ -0,0 +1,266 @@
>> +/*
>> + * Mai Logic Articia S emulation
>> + *
>> + * Copyright (c) 2023 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qapi/error.h"
>> +#include "hw/pci/pci_device.h"
>> +#include "hw/pci/pci_host.h"
>> +#include "hw/irq.h"
>> +#include "hw/i2c/bitbang_i2c.h"
>> +#include "hw/intc/i8259.h"
>> +#include "hw/pci-host/articia.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>> +struct ArticiaHostState {
>> +    PCIDevice parent_obj;
>> +
>> +    ArticiaState *as;
>> +};
>> +
>> +/* TYPE_ARTICIA */
>> +
>> +struct ArticiaState {
>> +    PCIHostState parent_obj;
>> +
>> +    qemu_irq irq[PCI_NUM_PINS];
>> +    MemoryRegion io;
>> +    MemoryRegion mem;
>> +    MemoryRegion reg;
>> +
>> +    bitbang_i2c_interface smbus;
>> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
>> +    hwaddr gpio_base;
>> +    MemoryRegion gpio_reg;
>> +};
>> +
>> +static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    ArticiaState *s = opaque;
>> +
>> +    return (s->gpio >> (addr * 8)) & 0xff;
>> +}
>> +
>> +static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
>> +                               unsigned int size)
>> +{
>> +    ArticiaState *s = opaque;
>> +    uint32_t sh = addr * 8;
>> +
>> +    if (addr == 0) {
>> +        /* in bits read only? */
>> +        return;
>> +    }
>> +
>> +    if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
>> +        s->gpio &= ~(0xff << sh | 0xff);
>> +        s->gpio |= (val & 0xff) << sh;
>> +        s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
>> +                                   s->gpio & BIT(16) ?
>> +                                   !!(s->gpio & BIT(8)) : 1);
>> +        if ((s->gpio & BIT(17))) {
>> +            s->gpio &= ~BIT(0);
>> +            s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
>> +                                       !!(s->gpio & BIT(9)));
>> +        }
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps articia_gpio_ops = {
>> +    .read = articia_gpio_read,
>> +    .write = articia_gpio_write,
>> +    .valid.min_access_size = 1,
>> +    .valid.max_access_size = 1,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    ArticiaState *s = opaque;
>> +    uint64_t ret = UINT_MAX;
>> +
>> +    switch (addr) {
>> +    case 0xc00cf8:
>> +        ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
>> +        break;
>> +    case 0xe00cfc ... 0xe00cff:
>> +        ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
>> +        break;
>> +    case 0xf00000:
>> +        ret = pic_read_irq(isa_pic);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
>> +                      HWADDR_PRIx " %d\n", __func__, addr, size);
>> +        break;
>> +    }
>> +    return ret;
>> +}
>> +
>> +static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
>> +                              unsigned int size)
>> +{
>> +    ArticiaState *s = opaque;
>> +
>> +    switch (addr) {
>> +    case 0xc00cf8:
>> +        pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
>> +        break;
>> +    case 0xe00cfc ... 0xe00cff:
>> +        pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
>> +                      HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps articia_reg_ops = {
>> +    .read = articia_reg_read,
>> +    .write = articia_reg_write,
>> +    .valid.min_access_size = 1,
>> +    .valid.max_access_size = 4,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static void articia_pcihost_set_irq(void *opaque, int n, int level)
>> +{
>> +    ArticiaState *s = opaque;
>> +    qemu_set_irq(s->irq[n], level);
>> +}
>> +
>> +static void articia_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ArticiaState *s = ARTICIA(dev);
>> +    PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> +    PCIDevice *pdev;
>> +
>> +    bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
>> +    memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
>> +                          TYPE_ARTICIA, 4);
>> +
>> +    memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
>> +    memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
>> +    memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
>> +                          TYPE_ARTICIA, 0x1000000);
>> +    memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>> +
>> +    /* devfn_min is 8 that matches first PCI slot in AmigaOne */
>> +    h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
>> +                                   pci_swizzle_map_irq_fn, dev, &s->mem,
>> +                                   &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
>
> Hi,
>
> pci_swizzle_map_irq_fn() doesn't correctly map all AmigaOne PCI
> interrupt pin on device to interrupt pin on connector connections.
>
> I found this mapping and I think it's correct. For the VIA Southbridge
> at 00:07.0 it's the only possible correct mapping. All PCI functions
> interrupts are connected internally.

I think you're right, thanks a lot for finding this, I'll include this 
change in v2 but wait a bit more with that if review comes up with 
something else or R-b tags I can add then.

Regards,
BALATON Zoltan

> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
> index d9f052e3f0..2ad011af9c 100644
> --- a/hw/pci-host/articia.c
> +++ b/hw/pci-host/articia.c
> @@ -138,6 +138,29 @@ static void articia_pcihost_set_irq(void *opaque,
> int n, int level)
>      qemu_set_irq(s->irq[n], level);
>  }
>  
> +/*
> + * AmigaOne SE PCI slot to IRQ routing
> + *
> + * repository: https://source.denx.de/u-boot/custodians/u-boot-avr32.git
> + * refspec: v2010.06
> + * file: board/MAI/AmigaOneG3SE/articiaS_pci.c
> + */
> +static int amigaone_pcihost_bus0_map_irq(PCIDevice *pdev, int pin)
> +{
> +    switch(PCI_SLOT(pdev->devfn))
> +    {
> +    case 6:     /* On board ethernet */
> +        return 3;
> +    case 7:     /* Southbridge */
> +        return pin;
> +    default:
> +        break;
> +    }
> +
> +    /* Slot 1 Device 8, Slot 2 Device 9, Slot 3 Device 10 */
> +    return pci_swizzle(PCI_SLOT(pdev->devfn), pin);
> +}
> +
>  static void articia_realize(DeviceState *dev, Error **errp)
>  {
>      ArticiaState *s = ARTICIA(dev);
> @@ -155,7 +178,7 @@ static void articia_realize(DeviceState *dev, Error
> **errp)
>      memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
>  
>      h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
> -                                   pci_swizzle_map_irq_fn, dev,
> +                                   amigaone_pcihost_bus0_map_irq, dev,
>                                     &s->mem, &s->io, 0, 4, TYPE_PCI_BUS);
>      pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
>                                             TYPE_ARTICIA_PCI_HOST);
>
> With best regards,
> Volker
>
>
>> +    pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
>> +                                           TYPE_ARTICIA_PCI_HOST);
>> +    ARTICIA_PCI_HOST(pdev)->as = s;
>> +    pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
>> +
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
>> +    qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
>> +}
>> +
>> +static void articia_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = articia_realize;
>> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>> +}
>> +
>> +/* TYPE_ARTICIA_PCI_HOST */
>> +
>> +static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
>> +                                       uint32_t val, int len)
>> +{
>> +    ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
>> +
>> +    pci_default_write_config(d, addr, val, len);
>> +    switch (addr) {
>> +    case 0x40:
>> +        s->gpio_base = val;
>> +        break;
>> +    case 0x44:
>> +        if (val != 0x11) {
>> +            /* FIXME what do the bits actually mean? */
>> +            break;
>> +        }
>> +        if (memory_region_is_mapped(&s->gpio_reg)) {
>> +            memory_region_del_subregion(&s->io, &s->gpio_reg);
>> +        }
>> +        memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
>> +        break;
>> +    }
>> +}
>> +
>> +static void articia_pci_host_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +    k->config_write = articia_pci_host_cfg_write;
>> +    k->vendor_id = 0x10cc;
>> +    k->device_id = 0x0660;
>> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
>> +    /*
>> +     * PCI-facing part of the host bridge,
>> +     * not usable without the host-facing part
>> +     */
>> +    dc->user_creatable = false;
>> +}
>> +
>> +/* TYPE_ARTICIA_PCI_BRIDGE */
>> +
>> +static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +    k->vendor_id = 0x10cc;
>> +    k->device_id = 0x0661;
>> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
>> +    /*
>> +     * PCI-facing part of the host bridge,
>> +     * not usable without the host-facing part
>> +     */
>> +    dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo articia_types[] = {
>> +    {
>> +        .name          = TYPE_ARTICIA,
>> +        .parent        = TYPE_PCI_HOST_BRIDGE,
>> +        .instance_size = sizeof(ArticiaState),
>> +        .class_init    = articia_class_init,
>> +    },
>> +    {
>> +        .name          = TYPE_ARTICIA_PCI_HOST,
>> +        .parent        = TYPE_PCI_DEVICE,
>> +        .instance_size = sizeof(ArticiaHostState),
>> +        .class_init    = articia_pci_host_class_init,
>> +        .interfaces = (InterfaceInfo[]) {
>> +              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> +              { },
>> +        },
>> +    },
>> +    {
>> +        .name          = TYPE_ARTICIA_PCI_BRIDGE,
>> +        .parent        = TYPE_PCI_DEVICE,
>> +        .instance_size = sizeof(PCIDevice),
>> +        .class_init    = articia_pci_bridge_class_init,
>> +        .interfaces = (InterfaceInfo[]) {
>> +              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>> +              { },
>> +        },
>> +    },
>> +};
>> +
>> +DEFINE_TYPES(articia_types)
>>
>
>

On 05/10/2023 23:13, BALATON Zoltan wrote:

> The Articia S is a generic chipset supporting several different CPUs
> that were used on some PPC boards. This is a minimal emulation of the
> parts needed for emulating the AmigaOne board.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/pci-host/Kconfig           |   5 +
>   hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>   hw/pci-host/meson.build       |   2 +
>   include/hw/pci-host/articia.h |  17 +++
>   4 files changed, 290 insertions(+)
>   create mode 100644 hw/pci-host/articia.c
>   create mode 100644 include/hw/pci-host/articia.h
> 
> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
> index a07070eddf..33014c80a4 100644
> --- a/hw/pci-host/Kconfig
> +++ b/hw/pci-host/Kconfig
> @@ -73,6 +73,11 @@ config SH_PCI
>       bool
>       select PCI
>   
> +config ARTICIA
> +    bool
> +    select PCI
> +    select I8259
> +
>   config MV64361
>       bool
>       select PCI
> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
> new file mode 100644
> index 0000000000..80558e1c47
> --- /dev/null
> +++ b/hw/pci-host/articia.c
> @@ -0,0 +1,266 @@
> +/*
> + * Mai Logic Articia S emulation
> + *
> + * Copyright (c) 2023 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/pci/pci_device.h"
> +#include "hw/pci/pci_host.h"
> +#include "hw/irq.h"
> +#include "hw/i2c/bitbang_i2c.h"
> +#include "hw/intc/i8259.h"
> +#include "hw/pci-host/articia.h"
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
> +struct ArticiaHostState {
> +    PCIDevice parent_obj;
> +
> +    ArticiaState *as;
> +};
> +
> +/* TYPE_ARTICIA */
> +
> +struct ArticiaState {
> +    PCIHostState parent_obj;
> +
> +    qemu_irq irq[PCI_NUM_PINS];
> +    MemoryRegion io;
> +    MemoryRegion mem;
> +    MemoryRegion reg;
> +
> +    bitbang_i2c_interface smbus;
> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
> +    hwaddr gpio_base;
> +    MemoryRegion gpio_reg;
> +};

These types above should be in the header file and not in the C file, as per our 
current QOM guidelines.

> +static uint64_t articia_gpio_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +
> +    return (s->gpio >> (addr * 8)) & 0xff;
> +}
> +
> +static void articia_gpio_write(void *opaque, hwaddr addr, uint64_t val,
> +                               unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +    uint32_t sh = addr * 8;
> +
> +    if (addr == 0) {
> +        /* in bits read only? */
> +        return;
> +    }
> +
> +    if ((s->gpio & (0xff << sh)) != (val & 0xff) << sh) {
> +        s->gpio &= ~(0xff << sh | 0xff);
> +        s->gpio |= (val & 0xff) << sh;
> +        s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SDA,
> +                                   s->gpio & BIT(16) ?
> +                                   !!(s->gpio & BIT(8)) : 1);
> +        if ((s->gpio & BIT(17))) {
> +            s->gpio &= ~BIT(0);
> +            s->gpio |= bitbang_i2c_set(&s->smbus, BITBANG_I2C_SCL,
> +                                       !!(s->gpio & BIT(9)));
> +        }
> +    }
> +}
> +
> +static const MemoryRegionOps articia_gpio_ops = {
> +    .read = articia_gpio_read,
> +    .write = articia_gpio_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 1,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t articia_reg_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +    uint64_t ret = UINT_MAX;
> +
> +    switch (addr) {
> +    case 0xc00cf8:
> +        ret = pci_host_conf_le_ops.read(PCI_HOST_BRIDGE(s), 0, size);
> +        break;
> +    case 0xe00cfc ... 0xe00cff:
> +        ret = pci_host_data_le_ops.read(PCI_HOST_BRIDGE(s), addr - 0xe00cfc, size);
> +        break;
> +    case 0xf00000:
> +        ret = pic_read_irq(isa_pic);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register read 0x%"
> +                      HWADDR_PRIx " %d\n", __func__, addr, size);
> +        break;
> +    }
> +    return ret;
> +}
> +
> +static void articia_reg_write(void *opaque, hwaddr addr, uint64_t val,
> +                              unsigned int size)
> +{
> +    ArticiaState *s = opaque;
> +
> +    switch (addr) {
> +    case 0xc00cf8:
> +        pci_host_conf_le_ops.write(PCI_HOST_BRIDGE(s), 0, val, size);
> +        break;
> +    case 0xe00cfc ... 0xe00cff:
> +        pci_host_data_le_ops.write(PCI_HOST_BRIDGE(s), addr, val, size);
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register write 0x%"
> +                      HWADDR_PRIx " %d <- %"PRIx64"\n", __func__, addr, size, val);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps articia_reg_ops = {
> +    .read = articia_reg_read,
> +    .write = articia_reg_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void articia_pcihost_set_irq(void *opaque, int n, int level)
> +{
> +    ArticiaState *s = opaque;
> +    qemu_set_irq(s->irq[n], level);
> +}
> +
> +static void articia_realize(DeviceState *dev, Error **errp)
> +{
> +    ArticiaState *s = ARTICIA(dev);
> +    PCIHostState *h = PCI_HOST_BRIDGE(dev);
> +    PCIDevice *pdev;
> +
> +    bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
> +    memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
> +                          TYPE_ARTICIA, 4);
> +
> +    memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
> +    memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
> +    memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
> +                          TYPE_ARTICIA, 0x1000000);
> +    memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
> +
> +    /* devfn_min is 8 that matches first PCI slot in AmigaOne */
> +    h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
> +                                   pci_swizzle_map_irq_fn, dev, &s->mem,
> +                                   &s->io, PCI_DEVFN(8, 0), 4, TYPE_PCI_BUS);
> +    pdev = pci_create_simple_multifunction(h->bus, PCI_DEVFN(0, 0),
> +                                           TYPE_ARTICIA_PCI_HOST);
> +    ARTICIA_PCI_HOST(pdev)->as = s;
> +    pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
> +
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
> +    qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
> +}
> +
> +static void articia_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = articia_realize;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +}
> +
> +/* TYPE_ARTICIA_PCI_HOST */
> +
> +static void articia_pci_host_cfg_write(PCIDevice *d, uint32_t addr,
> +                                       uint32_t val, int len)
> +{
> +    ArticiaState *s = ARTICIA_PCI_HOST(d)->as;
> +
> +    pci_default_write_config(d, addr, val, len);
> +    switch (addr) {
> +    case 0x40:
> +        s->gpio_base = val;
> +        break;
> +    case 0x44:
> +        if (val != 0x11) {
> +            /* FIXME what do the bits actually mean? */
> +            break;
> +        }
> +        if (memory_region_is_mapped(&s->gpio_reg)) {
> +            memory_region_del_subregion(&s->io, &s->gpio_reg);
> +        }
> +        memory_region_add_subregion(&s->io, s->gpio_base + 0x38, &s->gpio_reg);
> +        break;
> +    }
> +}
> +
> +static void articia_pci_host_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->config_write = articia_pci_host_cfg_write;
> +    k->vendor_id = 0x10cc;
> +    k->device_id = 0x0660;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge,
> +     * not usable without the host-facing part
> +     */
> +    dc->user_creatable = false;
> +}
> +
> +/* TYPE_ARTICIA_PCI_BRIDGE */
> +
> +static void articia_pci_bridge_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->vendor_id = 0x10cc;
> +    k->device_id = 0x0661;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    /*
> +     * PCI-facing part of the host bridge,
> +     * not usable without the host-facing part
> +     */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo articia_types[] = {
> +    {
> +        .name          = TYPE_ARTICIA,
> +        .parent        = TYPE_PCI_HOST_BRIDGE,
> +        .instance_size = sizeof(ArticiaState),
> +        .class_init    = articia_class_init,
> +    },
> +    {
> +        .name          = TYPE_ARTICIA_PCI_HOST,
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(ArticiaHostState),
> +        .class_init    = articia_pci_host_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +              { },
> +        },
> +    },
> +    {
> +        .name          = TYPE_ARTICIA_PCI_BRIDGE,
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(PCIDevice),
> +        .class_init    = articia_pci_bridge_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +              { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +              { },
> +        },
> +    },
> +};
> +
> +DEFINE_TYPES(articia_types)
> diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build
> index 64eada76fe..40de48eb7f 100644
> --- a/hw/pci-host/meson.build
> +++ b/hw/pci-host/meson.build
> @@ -20,6 +20,8 @@ pci_ss.add(when: 'CONFIG_GRACKLE_PCI', if_true: files('grackle.c'))
>   pci_ss.add(when: 'CONFIG_UNIN_PCI', if_true: files('uninorth.c'))
>   # PowerPC E500 boards
>   pci_ss.add(when: 'CONFIG_PPCE500_PCI', if_true: files('ppce500.c'))
> +# AmigaOne
> +pci_ss.add(when: 'CONFIG_ARTICIA', if_true: files('articia.c'))
>   # Pegasos2
>   pci_ss.add(when: 'CONFIG_MV64361', if_true: files('mv64361.c'))
>   
> diff --git a/include/hw/pci-host/articia.h b/include/hw/pci-host/articia.h
> new file mode 100644
> index 0000000000..529c240274
> --- /dev/null
> +++ b/include/hw/pci-host/articia.h
> @@ -0,0 +1,17 @@
> +/*
> + * Mai Logic Articia S emulation
> + *
> + * Copyright (c) 2023 BALATON Zoltan
> + *
> + * This work is licensed under the GNU GPL license version 2 or later.
> + *
> + */
> +
> +#ifndef ARTICIA_H
> +#define ARTICIA_H
> +
> +#define TYPE_ARTICIA "articia"
> +#define TYPE_ARTICIA_PCI_HOST "articia-pci-host"
> +#define TYPE_ARTICIA_PCI_BRIDGE "articia-pci-bridge"
> +
> +#endif


ATB,

Mark.

On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
> On 05/10/2023 23:13, BALATON Zoltan wrote:
>
>> The Articia S is a generic chipset supporting several different CPUs
>> that were used on some PPC boards. This is a minimal emulation of the
>> parts needed for emulating the AmigaOne board.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/pci-host/Kconfig           |   5 +
>>   hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>>   hw/pci-host/meson.build       |   2 +
>>   include/hw/pci-host/articia.h |  17 +++
>>   4 files changed, 290 insertions(+)
>>   create mode 100644 hw/pci-host/articia.c
>>   create mode 100644 include/hw/pci-host/articia.h
>> 
>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>> index a07070eddf..33014c80a4 100644
>> --- a/hw/pci-host/Kconfig
>> +++ b/hw/pci-host/Kconfig
>> @@ -73,6 +73,11 @@ config SH_PCI
>>       bool
>>       select PCI
>>   +config ARTICIA
>> +    bool
>> +    select PCI
>> +    select I8259
>> +
>>   config MV64361
>>       bool
>>       select PCI
>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>> new file mode 100644
>> index 0000000000..80558e1c47
>> --- /dev/null
>> +++ b/hw/pci-host/articia.c
>> @@ -0,0 +1,266 @@
>> +/*
>> + * Mai Logic Articia S emulation
>> + *
>> + * Copyright (c) 2023 BALATON Zoltan
>> + *
>> + * This work is licensed under the GNU GPL license version 2 or later.
>> + *
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qapi/error.h"
>> +#include "hw/pci/pci_device.h"
>> +#include "hw/pci/pci_host.h"
>> +#include "hw/irq.h"
>> +#include "hw/i2c/bitbang_i2c.h"
>> +#include "hw/intc/i8259.h"
>> +#include "hw/pci-host/articia.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>> +struct ArticiaHostState {
>> +    PCIDevice parent_obj;
>> +
>> +    ArticiaState *as;
>> +};
>> +
>> +/* TYPE_ARTICIA */
>> +
>> +struct ArticiaState {
>> +    PCIHostState parent_obj;
>> +
>> +    qemu_irq irq[PCI_NUM_PINS];
>> +    MemoryRegion io;
>> +    MemoryRegion mem;
>> +    MemoryRegion reg;
>> +
>> +    bitbang_i2c_interface smbus;
>> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) 
>> */
>> +    hwaddr gpio_base;
>> +    MemoryRegion gpio_reg;
>> +};
>
> These types above should be in the header file and not in the C file, as per 
> our current QOM guidelines.

I don't think there's such a guideline, at least I did not find any 
mention of it in style and qom docs. It was necessary to move some type 
declarations to headers for types that are embedded in other objects 
because C needs the struct size for that, but I don't think that should be 
a general thing when it's not needed.

The reason for that is that moving these to the header exposes internal 
object structure to users that should not need to know that so it breaks 
object encapsulation and also needs moving a bunch of includes to the 
header which then makes the users of this type also include those headers 
when they don't really need them but only need the type defines to 
instantiate the object and that's all they should have access to. So I 
think declaring types in the header should only be done for types that 
aren't full devices and are meant to be embedded as part of another device 
or a SoC but otherwise it's better to keep implementation closed and local 
to the object and not expose it unless really needed, that's why these 
are here.

If you insist I can move these but I don't think there's really such 
recommendation and I don't think that's a good idea because of the above.

Regards,
BALATON Zoltan

On 08/10/2023 19:08, BALATON Zoltan wrote:

> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>
>>> The Articia S is a generic chipset supporting several different CPUs
>>> that were used on some PPC boards. This is a minimal emulation of the
>>> parts needed for emulating the AmigaOne board.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   hw/pci-host/Kconfig           |   5 +
>>>   hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>>>   hw/pci-host/meson.build       |   2 +
>>>   include/hw/pci-host/articia.h |  17 +++
>>>   4 files changed, 290 insertions(+)
>>>   create mode 100644 hw/pci-host/articia.c
>>>   create mode 100644 include/hw/pci-host/articia.h
>>>
>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>> index a07070eddf..33014c80a4 100644
>>> --- a/hw/pci-host/Kconfig
>>> +++ b/hw/pci-host/Kconfig
>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>       bool
>>>       select PCI
>>>   +config ARTICIA
>>> +    bool
>>> +    select PCI
>>> +    select I8259
>>> +
>>>   config MV64361
>>>       bool
>>>       select PCI
>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>> new file mode 100644
>>> index 0000000000..80558e1c47
>>> --- /dev/null
>>> +++ b/hw/pci-host/articia.c
>>> @@ -0,0 +1,266 @@
>>> +/*
>>> + * Mai Logic Articia S emulation
>>> + *
>>> + * Copyright (c) 2023 BALATON Zoltan
>>> + *
>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "qapi/error.h"
>>> +#include "hw/pci/pci_device.h"
>>> +#include "hw/pci/pci_host.h"
>>> +#include "hw/irq.h"
>>> +#include "hw/i2c/bitbang_i2c.h"
>>> +#include "hw/intc/i8259.h"
>>> +#include "hw/pci-host/articia.h"
>>> +
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>> +
>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>> +struct ArticiaHostState {
>>> +    PCIDevice parent_obj;
>>> +
>>> +    ArticiaState *as;
>>> +};
>>> +
>>> +/* TYPE_ARTICIA */
>>> +
>>> +struct ArticiaState {
>>> +    PCIHostState parent_obj;
>>> +
>>> +    qemu_irq irq[PCI_NUM_PINS];
>>> +    MemoryRegion io;
>>> +    MemoryRegion mem;
>>> +    MemoryRegion reg;
>>> +
>>> +    bitbang_i2c_interface smbus;
>>> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
>>> +    hwaddr gpio_base;
>>> +    MemoryRegion gpio_reg;
>>> +};
>>
>> These types above should be in the header file and not in the C file, as per our 
>> current QOM guidelines.
> 
> I don't think there's such a guideline, at least I did not find any mention of it in 
> style and qom docs. It was necessary to move some type declarations to headers for 
> types that are embedded in other objects because C needs the struct size for that, 
> but I don't think that should be a general thing when it's not needed.
> 
> The reason for that is that moving these to the header exposes internal object 
> structure to users that should not need to know that so it breaks object 
> encapsulation and also needs moving a bunch of includes to the header which then 
> makes the users of this type also include those headers when they don't really need 
> them but only need the type defines to instantiate the object and that's all they 
> should have access to. So I think declaring types in the header should only be done 
> for types that aren't full devices and are meant to be embedded as part of another 
> device or a SoC but otherwise it's better to keep implementation closed and local to 
> the object and not expose it unless really needed, that's why these are here.
> 
> If you insist I can move these but I don't think there's really such recommendation 
> and I don't think that's a good idea because of the above.

Maybe it was something that was missed out of the recent documentation updates, but 
you can clearly see this has been the standard pattern for some time, including for 
recent devices such as the xlnx-versal. If there are any devices that don't follow 
this pattern then it is likely because they are based on older code.

If you disagree with this, then start a new thread on qemu-devel with a new proposal 
and if everyone is agreement then that will be become the new standard.


ATB,

Mark.

On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
> On 08/10/2023 19:08, BALATON Zoltan wrote:
>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>> 
>>>> The Articia S is a generic chipset supporting several different CPUs
>>>> that were used on some PPC boards. This is a minimal emulation of the
>>>> parts needed for emulating the AmigaOne board.
>>>> 
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>   hw/pci-host/Kconfig           |   5 +
>>>>   hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>>>>   hw/pci-host/meson.build       |   2 +
>>>>   include/hw/pci-host/articia.h |  17 +++
>>>>   4 files changed, 290 insertions(+)
>>>>   create mode 100644 hw/pci-host/articia.c
>>>>   create mode 100644 include/hw/pci-host/articia.h
>>>> 
>>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>>> index a07070eddf..33014c80a4 100644
>>>> --- a/hw/pci-host/Kconfig
>>>> +++ b/hw/pci-host/Kconfig
>>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>>       bool
>>>>       select PCI
>>>>   +config ARTICIA
>>>> +    bool
>>>> +    select PCI
>>>> +    select I8259
>>>> +
>>>>   config MV64361
>>>>       bool
>>>>       select PCI
>>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>>> new file mode 100644
>>>> index 0000000000..80558e1c47
>>>> --- /dev/null
>>>> +++ b/hw/pci-host/articia.c
>>>> @@ -0,0 +1,266 @@
>>>> +/*
>>>> + * Mai Logic Articia S emulation
>>>> + *
>>>> + * Copyright (c) 2023 BALATON Zoltan
>>>> + *
>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>> + *
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>> +#include "qapi/error.h"
>>>> +#include "hw/pci/pci_device.h"
>>>> +#include "hw/pci/pci_host.h"
>>>> +#include "hw/irq.h"
>>>> +#include "hw/i2c/bitbang_i2c.h"
>>>> +#include "hw/intc/i8259.h"
>>>> +#include "hw/pci-host/articia.h"
>>>> +
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>>> +
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>>> +struct ArticiaHostState {
>>>> +    PCIDevice parent_obj;
>>>> +
>>>> +    ArticiaState *as;
>>>> +};
>>>> +
>>>> +/* TYPE_ARTICIA */
>>>> +
>>>> +struct ArticiaState {
>>>> +    PCIHostState parent_obj;
>>>> +
>>>> +    qemu_irq irq[PCI_NUM_PINS];
>>>> +    MemoryRegion io;
>>>> +    MemoryRegion mem;
>>>> +    MemoryRegion reg;
>>>> +
>>>> +    bitbang_i2c_interface smbus;
>>>> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 
>>>> out) */
>>>> +    hwaddr gpio_base;
>>>> +    MemoryRegion gpio_reg;
>>>> +};
>>> 
>>> These types above should be in the header file and not in the C file, as 
>>> per our current QOM guidelines.
>> 
>> I don't think there's such a guideline, at least I did not find any mention 
>> of it in style and qom docs. It was necessary to move some type 
>> declarations to headers for types that are embedded in other objects 
>> because C needs the struct size for that, but I don't think that should be 
>> a general thing when it's not needed.
>> 
>> The reason for that is that moving these to the header exposes internal 
>> object structure to users that should not need to know that so it breaks 
>> object encapsulation and also needs moving a bunch of includes to the 
>> header which then makes the users of this type also include those headers 
>> when they don't really need them but only need the type defines to 
>> instantiate the object and that's all they should have access to. So I 
>> think declaring types in the header should only be done for types that 
>> aren't full devices and are meant to be embedded as part of another device 
>> or a SoC but otherwise it's better to keep implementation closed and local 
>> to the object and not expose it unless really needed, that's why these are 
>> here.
>> 
>> If you insist I can move these but I don't think there's really such 
>> recommendation and I don't think that's a good idea because of the above.
>
> Maybe it was something that was missed out of the recent documentation 
> updates, but you can clearly see this has been the standard pattern for some 
> time, including for recent devices such as the xlnx-versal. If there are any 
> devices that don't follow this pattern then it is likely because they are 
> based on older code.
>
> If you disagree with this, then start a new thread on qemu-devel with a new 
> proposal and if everyone is agreement then that will be become the new 
> standard.

I think you should start a thread with a patch to style or qom docs about 
this to document this standard and if that's accepted then I also accept 
it as a real recommendation as my understanding of it was as above that it 
was needed for some deviecs to allow embedding them but not a general 
recommendation for all devices and I don't think it should be beacuse of 
braeaking encapsulation and introduces a lot of unneded includes so I'd 
keep it to those devices where it'e really needed which is what the docs 
currently say.

But I also said I can change this if you insist as for just this devices 
only used once it does not matter much so I take that as you still want 
this chnage so I can send another version but wait for the opinion of the 
maintainers if they want anything else changed so I cah do all remaining 
changes in next version.

Regards,
BALATON Zoltan

On 09/10/2023 22:57, BALATON Zoltan wrote:

> On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
>> On 08/10/2023 19:08, BALATON Zoltan wrote:
>>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>>
>>>>> The Articia S is a generic chipset supporting several different CPUs
>>>>> that were used on some PPC boards. This is a minimal emulation of the
>>>>> parts needed for emulating the AmigaOne board.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>>   hw/pci-host/Kconfig           |   5 +
>>>>>   hw/pci-host/articia.c         | 266 ++++++++++++++++++++++++++++++++++
>>>>>   hw/pci-host/meson.build       |   2 +
>>>>>   include/hw/pci-host/articia.h |  17 +++
>>>>>   4 files changed, 290 insertions(+)
>>>>>   create mode 100644 hw/pci-host/articia.c
>>>>>   create mode 100644 include/hw/pci-host/articia.h
>>>>>
>>>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>>>> index a07070eddf..33014c80a4 100644
>>>>> --- a/hw/pci-host/Kconfig
>>>>> +++ b/hw/pci-host/Kconfig
>>>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>>>       bool
>>>>>       select PCI
>>>>>   +config ARTICIA
>>>>> +    bool
>>>>> +    select PCI
>>>>> +    select I8259
>>>>> +
>>>>>   config MV64361
>>>>>       bool
>>>>>       select PCI
>>>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>>>> new file mode 100644
>>>>> index 0000000000..80558e1c47
>>>>> --- /dev/null
>>>>> +++ b/hw/pci-host/articia.c
>>>>> @@ -0,0 +1,266 @@
>>>>> +/*
>>>>> + * Mai Logic Articia S emulation
>>>>> + *
>>>>> + * Copyright (c) 2023 BALATON Zoltan
>>>>> + *
>>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include "qemu/osdep.h"
>>>>> +#include "qemu/log.h"
>>>>> +#include "qapi/error.h"
>>>>> +#include "hw/pci/pci_device.h"
>>>>> +#include "hw/pci/pci_host.h"
>>>>> +#include "hw/irq.h"
>>>>> +#include "hw/i2c/bitbang_i2c.h"
>>>>> +#include "hw/intc/i8259.h"
>>>>> +#include "hw/pci-host/articia.h"
>>>>> +
>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>>>> +
>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>>>> +struct ArticiaHostState {
>>>>> +    PCIDevice parent_obj;
>>>>> +
>>>>> +    ArticiaState *as;
>>>>> +};
>>>>> +
>>>>> +/* TYPE_ARTICIA */
>>>>> +
>>>>> +struct ArticiaState {
>>>>> +    PCIHostState parent_obj;
>>>>> +
>>>>> +    qemu_irq irq[PCI_NUM_PINS];
>>>>> +    MemoryRegion io;
>>>>> +    MemoryRegion mem;
>>>>> +    MemoryRegion reg;
>>>>> +
>>>>> +    bitbang_i2c_interface smbus;
>>>>> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out) */
>>>>> +    hwaddr gpio_base;
>>>>> +    MemoryRegion gpio_reg;
>>>>> +};
>>>>
>>>> These types above should be in the header file and not in the C file, as per our 
>>>> current QOM guidelines.
>>>
>>> I don't think there's such a guideline, at least I did not find any mention of it 
>>> in style and qom docs. It was necessary to move some type declarations to headers 
>>> for types that are embedded in other objects because C needs the struct size for 
>>> that, but I don't think that should be a general thing when it's not needed.
>>>
>>> The reason for that is that moving these to the header exposes internal object 
>>> structure to users that should not need to know that so it breaks object 
>>> encapsulation and also needs moving a bunch of includes to the header which then 
>>> makes the users of this type also include those headers when they don't really 
>>> need them but only need the type defines to instantiate the object and that's all 
>>> they should have access to. So I think declaring types in the header should only 
>>> be done for types that aren't full devices and are meant to be embedded as part of 
>>> another device or a SoC but otherwise it's better to keep implementation closed 
>>> and local to the object and not expose it unless really needed, that's why these 
>>> are here.
>>>
>>> If you insist I can move these but I don't think there's really such 
>>> recommendation and I don't think that's a good idea because of the above.
>>
>> Maybe it was something that was missed out of the recent documentation updates, but 
>> you can clearly see this has been the standard pattern for some time, including for 
>> recent devices such as the xlnx-versal. If there are any devices that don't follow 
>> this pattern then it is likely because they are based on older code.
>>
>> If you disagree with this, then start a new thread on qemu-devel with a new 
>> proposal and if everyone is agreement then that will be become the new standard.
> 
> I think you should start a thread with a patch to style or qom docs about this to 
> document this standard and if that's accepted then I also accept it as a real 
> recommendation as my understanding of it was as above that it was needed for some 
> deviecs to allow embedding them but not a general recommendation for all devices and 
> I don't think it should be beacuse of braeaking encapsulation and introduces a lot of 
> unneded includes so I'd keep it to those devices where it'e really needed which is 
> what the docs currently say.

Oh is there already a mention of this somewhere in the docs? Can you provide a link 
so we can check the wording? Certainly that's the way my own patches (and other 
people's patches) have been reviewed historically over the years.

> But I also said I can change this if you insist as for just this devices only used 
> once it does not matter much so I take that as you still want this chnage so I can 
> send another version but wait for the opinion of the maintainers if they want 
> anything else changed so I cah do all remaining changes in next version.

Having a separate header would certainly be part of my review criteria as I've been 
asked to make such changes in the past. But yeah, maybe wait for a bit and see what 
other review comments arrive.


ATB,

Mark.

On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
> On 09/10/2023 22:57, BALATON Zoltan wrote:
>> On Mon, 9 Oct 2023, Mark Cave-Ayland wrote:
>>> On 08/10/2023 19:08, BALATON Zoltan wrote:
>>>> On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
>>>>> On 05/10/2023 23:13, BALATON Zoltan wrote:
>>>>> 
>>>>>> The Articia S is a generic chipset supporting several different CPUs
>>>>>> that were used on some PPC boards. This is a minimal emulation of the
>>>>>> parts needed for emulating the AmigaOne board.
>>>>>> 
>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>> ---
>>>>>>   hw/pci-host/Kconfig           |   5 +
>>>>>>   hw/pci-host/articia.c         | 266 
>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>   hw/pci-host/meson.build       |   2 +
>>>>>>   include/hw/pci-host/articia.h |  17 +++
>>>>>>   4 files changed, 290 insertions(+)
>>>>>>   create mode 100644 hw/pci-host/articia.c
>>>>>>   create mode 100644 include/hw/pci-host/articia.h
>>>>>> 
>>>>>> diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
>>>>>> index a07070eddf..33014c80a4 100644
>>>>>> --- a/hw/pci-host/Kconfig
>>>>>> +++ b/hw/pci-host/Kconfig
>>>>>> @@ -73,6 +73,11 @@ config SH_PCI
>>>>>>       bool
>>>>>>       select PCI
>>>>>>   +config ARTICIA
>>>>>> +    bool
>>>>>> +    select PCI
>>>>>> +    select I8259
>>>>>> +
>>>>>>   config MV64361
>>>>>>       bool
>>>>>>       select PCI
>>>>>> diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
>>>>>> new file mode 100644
>>>>>> index 0000000000..80558e1c47
>>>>>> --- /dev/null
>>>>>> +++ b/hw/pci-host/articia.c
>>>>>> @@ -0,0 +1,266 @@
>>>>>> +/*
>>>>>> + * Mai Logic Articia S emulation
>>>>>> + *
>>>>>> + * Copyright (c) 2023 BALATON Zoltan
>>>>>> + *
>>>>>> + * This work is licensed under the GNU GPL license version 2 or later.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include "qemu/osdep.h"
>>>>>> +#include "qemu/log.h"
>>>>>> +#include "qapi/error.h"
>>>>>> +#include "hw/pci/pci_device.h"
>>>>>> +#include "hw/pci/pci_host.h"
>>>>>> +#include "hw/irq.h"
>>>>>> +#include "hw/i2c/bitbang_i2c.h"
>>>>>> +#include "hw/intc/i8259.h"
>>>>>> +#include "hw/pci-host/articia.h"
>>>>>> +
>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
>>>>>> +
>>>>>> +OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
>>>>>> +struct ArticiaHostState {
>>>>>> +    PCIDevice parent_obj;
>>>>>> +
>>>>>> +    ArticiaState *as;
>>>>>> +};
>>>>>> +
>>>>>> +/* TYPE_ARTICIA */
>>>>>> +
>>>>>> +struct ArticiaState {
>>>>>> +    PCIHostState parent_obj;
>>>>>> +
>>>>>> +    qemu_irq irq[PCI_NUM_PINS];
>>>>>> +    MemoryRegion io;
>>>>>> +    MemoryRegion mem;
>>>>>> +    MemoryRegion reg;
>>>>>> +
>>>>>> +    bitbang_i2c_interface smbus;
>>>>>> +    uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 
>>>>>> out) */
>>>>>> +    hwaddr gpio_base;
>>>>>> +    MemoryRegion gpio_reg;
>>>>>> +};
>>>>> 
>>>>> These types above should be in the header file and not in the C file, as 
>>>>> per our current QOM guidelines.
>>>> 
>>>> I don't think there's such a guideline, at least I did not find any 
>>>> mention of it in style and qom docs. It was necessary to move some type 
>>>> declarations to headers for types that are embedded in other objects 
>>>> because C needs the struct size for that, but I don't think that should 
>>>> be a general thing when it's not needed.
>>>> 
>>>> The reason for that is that moving these to the header exposes internal 
>>>> object structure to users that should not need to know that so it breaks 
>>>> object encapsulation and also needs moving a bunch of includes to the 
>>>> header which then makes the users of this type also include those headers 
>>>> when they don't really need them but only need the type defines to 
>>>> instantiate the object and that's all they should have access to. So I 
>>>> think declaring types in the header should only be done for types that 
>>>> aren't full devices and are meant to be embedded as part of another 
>>>> device or a SoC but otherwise it's better to keep implementation closed 
>>>> and local to the object and not expose it unless really needed, that's 
>>>> why these are here.
>>>> 
>>>> If you insist I can move these but I don't think there's really such 
>>>> recommendation and I don't think that's a good idea because of the above.
>>> 
>>> Maybe it was something that was missed out of the recent documentation 
>>> updates, but you can clearly see this has been the standard pattern for 
>>> some time, including for recent devices such as the xlnx-versal. If there 
>>> are any devices that don't follow this pattern then it is likely because 
>>> they are based on older code.
>>> 
>>> If you disagree with this, then start a new thread on qemu-devel with a 
>>> new proposal and if everyone is agreement then that will be become the new 
>>> standard.
>> 
>> I think you should start a thread with a patch to style or qom docs about 
>> this to document this standard and if that's accepted then I also accept it 
>> as a real recommendation as my understanding of it was as above that it was 
>> needed for some deviecs to allow embedding them but not a general 
>> recommendation for all devices and I don't think it should be beacuse of 
>> braeaking encapsulation and introduces a lot of unneded includes so I'd 
>> keep it to those devices where it'e really needed which is what the docs 
>> currently say.
>
> Oh is there already a mention of this somewhere in the docs? Can you provide 
> a link so we can check the wording? Certainly that's the way my own patches 
> (and other people's patches) have been reviewed historically over the years.

The only mention I could find is in docs/devel/qom.rst, section "Standard 
type declaration and definition macros" which says: "In types which do not 
require any virtual functions in the class, the OBJECT_DECLARE_SIMPLE_TYPE 
macro is suitable, and is commonly placed in the header file" and then 
continues as "The 'struct MyDevice' needs to be declared separately." so 
it does not say the type should be in the header file, only the 
declaration macro and even that is not a requirement just a common 
pattern.

As I said above I think the convention of putting typedefs in the header 
came as a result of Peter's invention of embedding devices into their 
parents that is to avoid tools warning about not freing them which is 
reall working around a defficiency in the memory management of QEMU but 
this requires the parents to know the size of the objects to embed them as 
members so their declaration had to be moved to their public header for 
this. But this then breaks object encapsulation, locality and goes against 
another effort to reduce the number of unneded includes so I think this 
practice should be limited only to cases where it's needed. I've used that 
in PPC440 cleanup series before where the SoC object embeds the devices it 
contains so these were moved to a public header but here we model a 
complete device that isn't meant to be embedded anywhere so the board code 
should not need its internal structure. Therefore that's best declared in 
the implementation. Moving the OBJECT_DECLARE macros to the header could 
be done but also not necessary as per current docs so I wote for keeping 
scope of these to only where they are really needed and not expose them 
unnecessarily.

Regards,
BALATON Zoltan

>> But I also said I can change this if you insist as for just this devices 
>> only used once it does not matter much so I take that as you still want 
>> this chnage so I can send another version but wait for the opinion of the 
>> maintainers if they want anything else changed so I cah do all remaining 
>> changes in next version.
>
> Having a separate header would certainly be part of my review criteria as 
> I've been asked to make such changes in the past. But yeah, maybe wait for a 
> bit and see what other review comments arrive.
>
>
> ATB,
>
> Mark.
>
>
>