Re: [RESEND PATCH v3 2/4] ppc4xx_pci: Rename QOM type name define

[BALATON Zoltan]

On Thu, 6 Jul 2023, Daniel Henrique Barboza wrote:
> On 7/6/23 08:16, BALATON Zoltan wrote:
> > Rename the TYPE_PPC4xx_PCI_HOST_BRIDGE define and its string value to
> > match each other and other similar types and to avoid confusion with
> > "ppc4xx-host-bridge" type defined in same file.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
>
> I struggled a bit to understand what's to gain with this change, but it makes
> more sense when we consider the changes made in the next patch (where a
> TYPE_PPC4xx_HOST_BRIDGE macro is introduced to match the "ppc4xx-host-bridge"
> name).
>
> I also understand the comments Phil made in version 1. We have several 
> QOM names that are too similar ("ppc4xx-pci-host", "ppc4xx-host-bridge" 
> in the next patch, "ppc440-pcix-host" in patch 4), all of them being PCI 
> host bridges. I am uncertain whether renaming the QOM name of these 
> devices to make them less similar is worth it.

These SoCs have slighlty different PCI hosts in them. I think it may 
started with a simple PCI controller in 405 then a PCIX controller in 440 
and finally PCIe controllers in addition to PCIX in 460EX (although there 
also exists a 405EX so later these were just mix and matched in different 
SoCs). These all work slightly differently but are also similar in some 
ways as likely these were designed based on the previous one. Also 
modeling them was probebly done independently and only partially so we 
ended up with these devices. Maybe at one point we can clean it up and 
refactor these but at least for the PCIe controller I could not find any 
docs so it's not easy to find out how could these be rationalised.

> Matching the macro names with the actual QOM name is a step in the right 
> direction though.

Originally I wanted to improve PCIe emulation for sam460ex but I could not 
yet make that work (missing some details on how it should work without 
docs) so for now only these clean ups came out of that which should be 
steps forward but not all the way there yet. I try to continue when I will 
have time for it again sometimes but I think these at least should clean 
it up this a bit and we can then continue from here.

> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Thanks, Regards,
BALATON Zoltan


> >   hw/ppc/ppc440_bamboo.c  | 3 +--
> >   hw/ppc/ppc4xx_pci.c     | 6 +++---
> >   include/hw/ppc/ppc4xx.h | 2 +-
> >   3 files changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> > index f061b8cf3b..45f409c838 100644
> > --- a/hw/ppc/ppc440_bamboo.c
> > +++ b/hw/ppc/ppc440_bamboo.c
> > @@ -205,8 +205,7 @@ static void bamboo_init(MachineState *machine)
> >       ppc4xx_sdram_ddr_enable(PPC4xx_SDRAM_DDR(dev));
> >         /* PCI */
> > -    dev = sysbus_create_varargs(TYPE_PPC4xx_PCI_HOST_BRIDGE,
> > -                                PPC440EP_PCI_CONFIG,
> > +    dev = sysbus_create_varargs(TYPE_PPC4xx_PCI_HOST, PPC440EP_PCI_CONFIG,
> >                                   qdev_get_gpio_in(uicdev, pci_irq_nrs[0]),
> >                                   qdev_get_gpio_in(uicdev, pci_irq_nrs[1]),
> >                                   qdev_get_gpio_in(uicdev, pci_irq_nrs[2]),
> > diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
> > index 1d4a50fa7c..fbdf8266d8 100644
> > --- a/hw/ppc/ppc4xx_pci.c
> > +++ b/hw/ppc/ppc4xx_pci.c
> > @@ -46,7 +46,7 @@ struct PCITargetMap {
> >       uint32_t la;
> >   };
> >   -OBJECT_DECLARE_SIMPLE_TYPE(PPC4xxPCIState, PPC4xx_PCI_HOST_BRIDGE)
> > +OBJECT_DECLARE_SIMPLE_TYPE(PPC4xxPCIState, PPC4xx_PCI_HOST)
> >     #define PPC4xx_PCI_NR_PMMS 3
> >   #define PPC4xx_PCI_NR_PTMS 2
> > @@ -321,7 +321,7 @@ static void ppc4xx_pcihost_realize(DeviceState *dev, 
> > Error **errp)
> >       int i;
> >         h = PCI_HOST_BRIDGE(dev);
> > -    s = PPC4xx_PCI_HOST_BRIDGE(dev);
> > +    s = PPC4xx_PCI_HOST(dev);
> >         for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> >           sysbus_init_irq(sbd, &s->irq[i]);
> > @@ -386,7 +386,7 @@ static void ppc4xx_pcihost_class_init(ObjectClass 
> > *klass, void *data)
> >   }
> >     static const TypeInfo ppc4xx_pcihost_info = {
> > -    .name          = TYPE_PPC4xx_PCI_HOST_BRIDGE,
> > +    .name          = TYPE_PPC4xx_PCI_HOST,
> >       .parent        = TYPE_PCI_HOST_BRIDGE,
> >       .instance_size = sizeof(PPC4xxPCIState),
> >       .class_init    = ppc4xx_pcihost_class_init,
> > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> > index 39ca602442..e053b9751b 100644
> > --- a/include/hw/ppc/ppc4xx.h
> > +++ b/include/hw/ppc/ppc4xx.h
> > @@ -29,7 +29,7 @@
> >   #include "exec/memory.h"
> >   #include "hw/sysbus.h"
> >   -#define TYPE_PPC4xx_PCI_HOST_BRIDGE "ppc4xx-pcihost"
> > +#define TYPE_PPC4xx_PCI_HOST "ppc4xx-pci-host"
> >   #define TYPE_PPC460EX_PCIE_HOST "ppc460ex-pcie-host"
> >     /*