Hi Cédric
> Subject: Re: [SPAM] [PATCH v2 04/14] hw/pci-host/aspeed: Add AST2600 PCIe
> Root Device support
>
> On 9/11/25 09:24, Jamin Lin wrote:
> > Introduce a PCIe Root Device for AST2600 platform.
> >
> > The AST2600 root complex exposes a PCIe root device at bus 80, devfn 0.
> > This root device is implemented as a child of the PCIe RC and modeled
> > as a host bridge PCI function (class_id = PCI_CLASS_BRIDGE_HOST).
> >
> > Key changes:
> > - Add a new device type "aspeed.pcie-root-device".
> > - Instantiate the root device as part of AspeedPCIERcState.
> > - Initialize it during RC realize() and attach it to the root bus.
> > - Mark the root device as non-user-creatable.
> > - Add RC boolean property "has-rd" to control whether the Root Device is
> > created (platforms can enable/disable it as needed).
> >
> > Note: Only AST2600 implements this PCIe root device. AST2700 does not
> > provide one.
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > include/hw/pci-host/aspeed_pcie.h | 11 +++++++
> > hw/pci-host/aspeed_pcie.c | 54
> +++++++++++++++++++++++++++++++
> > 2 files changed, 65 insertions(+)
> >
> > diff --git a/include/hw/pci-host/aspeed_pcie.h
> > b/include/hw/pci-host/aspeed_pcie.h
> > index e2c5dc6f62..e7c231e847 100644
> > --- a/include/hw/pci-host/aspeed_pcie.h
> > +++ b/include/hw/pci-host/aspeed_pcie.h
> > @@ -42,6 +42,13 @@ typedef struct AspeedPCIERegMap {
> > AspeedPCIERcRegs rc;
> > } AspeedPCIERegMap;
> >
> > +#define TYPE_ASPEED_PCIE_ROOT_DEVICE "aspeed.pcie-root-device"
> > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedPCIERootDeviceState,
> > +ASPEED_PCIE_ROOT_DEVICE);
> > +
> > +struct AspeedPCIERootDeviceState {
> > + PCIBridge parent_obj;
> > +};
> > +
> > #define TYPE_ASPEED_PCIE_RC "aspeed.pcie-rc"
> > OBJECT_DECLARE_SIMPLE_TYPE(AspeedPCIERcState, ASPEED_PCIE_RC);
> >
> > @@ -55,7 +62,10 @@ struct AspeedPCIERcState {
> >
> > uint32_t bus_nr;
> > char name[16];
> > + bool has_rd;
> > qemu_irq irq;
> > +
> > + AspeedPCIERootDeviceState root_device;
> > };
> >
> > /* Bridge between AHB bus and PCIe RC. */ @@ -80,6 +90,7 @@ struct
> > AspeedPCIECfgClass {
> >
> > uint64_t rc_bus_nr;
> > uint64_t nr_regs;
> > + bool rc_has_rd;
> > };
> >
> > #define TYPE_ASPEED_PCIE_PHY "aspeed.pcie-phy"
> > diff --git a/hw/pci-host/aspeed_pcie.c b/hw/pci-host/aspeed_pcie.c
> > index 9fb7c1ef67..fa8854fe7a 100644
> > --- a/hw/pci-host/aspeed_pcie.c
> > +++ b/hw/pci-host/aspeed_pcie.c
> > @@ -27,6 +27,44 @@
> > #include "hw/pci/msi.h"
> > #include "trace.h"
> >
> > +/*
> > + * PCIe Root Device
> > + * This device exists only on AST2600.
> > + */
> > +
> > +static void aspeed_pcie_root_device_class_init(ObjectClass *klass,
> > + const void *data)
> {
> > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > + set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> > + dc->desc = "ASPEED PCIe Root Device";
> > + k->vendor_id = PCI_VENDOR_ID_ASPEED;
> > + k->device_id = 0x2600;
> > + k->class_id = PCI_CLASS_BRIDGE_HOST;
> > + k->subsystem_vendor_id = k->vendor_id;
> > + k->subsystem_id = k->device_id;
> > + k->revision = 0;
> > +
> > + /*
> > + * PCI-facing part of the host bridge,
> > + * not usable without the host-facing part
> > + */
> > + dc->user_creatable = false;
> > +}
> > +
> > +static const TypeInfo aspeed_pcie_root_device_info = {
> > + .name = TYPE_ASPEED_PCIE_ROOT_DEVICE,
> > + .parent = TYPE_PCI_DEVICE,
> > + .instance_size = sizeof(AspeedPCIERootDeviceState),
> > + .class_init = aspeed_pcie_root_device_class_init,
> > + .interfaces = (const InterfaceInfo[]) {
> > + { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> > + { },
> > + },
> > +};
> > +
> > /*
> > * PCIe Root Complex (RC)
> > */
> > @@ -96,6 +134,16 @@ static void aspeed_pcie_rc_realize(DeviceState *dev,
> Error **errp)
> > aspeed_pcie_rc_map_irq, rc,
> &rc->mmio,
> > &rc->io, 0, 4,
> TYPE_PCIE_BUS);
> > pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
> > +
> > + /* setup root device */
> > + if (rc->has_rd) {
> > + object_initialize_child(OBJECT(rc), "root_device",
> &rc->root_device,
> > +
> TYPE_ASPEED_PCIE_ROOT_DEVICE);
> > + qdev_prop_set_int32(DEVICE(&rc->root_device), "addr",
> > + PCI_DEVFN(0, 0));
> > + qdev_prop_set_bit(DEVICE(&rc->root_device), "multifunction",
> false);
> > + qdev_realize(DEVICE(&rc->root_device), BUS(pci->bus),
> > + &error_fatal);
>
>
> why not pass 'errp' instead ?
>
Thanks for your review and suggestion.
Will fix it.
Jamin
>
> > + }
> > }
> >
> > static const char *aspeed_pcie_rc_root_bus_path(PCIHostState
> > *host_bridge, @@ -112,6 +160,7 @@ static const char
> > *aspeed_pcie_rc_root_bus_path(PCIHostState *host_bridge,
> >
> > static const Property aspeed_pcie_rc_props[] = {
> > DEFINE_PROP_UINT32("bus-nr", AspeedPCIERcState, bus_nr, 0),
> > + DEFINE_PROP_BOOL("has-rd", AspeedPCIERcState, has_rd, 0),
> > };
> >
> > static void aspeed_pcie_rc_class_init(ObjectClass *klass, const void
> > *data) @@ -404,6 +453,9 @@ static void
> aspeed_pcie_cfg_realize(DeviceState *dev, Error **errp)
> > object_property_set_int(OBJECT(&s->rc), "bus-nr",
> > apc->rc_bus_nr,
> > &error_abort);
> > + object_property_set_bool(OBJECT(&s->rc), "has-rd",
> > + apc->rc_has_rd,
> > + &error_abort);
> > if (!sysbus_realize(SYS_BUS_DEVICE(&s->rc), errp)) {
> > return;
> > }
> > @@ -436,6 +488,7 @@ static void aspeed_pcie_cfg_class_init(ObjectClass
> *klass, const void *data)
> > apc->reg_map = &aspeed_regmap;
> > apc->nr_regs = 0x100 >> 2;
> > apc->rc_bus_nr = 0x80;
> > + apc->rc_has_rd = true;
> > }
> >
> > static const TypeInfo aspeed_pcie_cfg_info = { @@ -573,6 +626,7 @@
> > static const TypeInfo aspeed_pcie_phy_info = {
> > static void aspeed_pcie_register_types(void)
> > {
> > type_register_static(&aspeed_pcie_rc_info);
> > + type_register_static(&aspeed_pcie_root_device_info);
> > type_register_static(&aspeed_pcie_cfg_info);
> > type_register_static(&aspeed_pcie_phy_info);
> > }
