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

[BALATON Zoltan]

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