Hi Cédric > Subject: Re: [SPAM] [PATCH v2 02/14] hw/pci-host/aspeed: Add AST2600 PCIe > PHY model > > Hello Jamin, > > On 9/11/25 09:24, Jamin Lin wrote: > > This patch introduces an initial ASPEED PCIe PHY/host controller model > > to support the AST2600 SoC. It provides a simple register block with > > MMIO read/write callbacks, integration into the build system, and > > trace events for debugging. > > > > Key changes: > > > > 1. PCIe PHY MMIO read/write callbacks > > Implemented aspeed_pcie_phy_read() and aspeed_pcie_phy_write() to > > handle 32-bit register accesses. > > > > 2. Build system and Kconfig integration > > Added CONFIG_PCI_EXPRESS_ASPEED in hw/pci-host/Kconfig and > meson > > rules. > > Updated ASPEED_SOC in hw/arm/Kconfig to imply PCI_DEVICES and > select > > PCI_EXPRESS_ASPEED. > > > > 3. Trace events for debug > > New tracepoints aspeed_pcie_phy_read and aspeed_pcie_phy_write > allow > > monitoring MMIO accesses. > > > > 4. Register space and defaults (AST2600 reference) > > Expose a 0x100 register space, as documented in the AST2600 > datasheet. > > On reset, set default values: > > PEHR_ID: Vendor ID = ASPEED, Device ID = 0x1150 > > PEHR_CLASS_CODE = 0x06040006 > > PEHR_DATALINK = 0xD7040022 > > PEHR_LINK: bit[5] set to 1 to indicate link up. > > > > This provides a skeleton device for the AST2600 platform. It enables > > firmware to detect the PCIe link as up by default and allows future > > extension. > > There, you could say : based on previous work from me, plus add a summary of > the modifications that have been made. >
Thanks for your review and suggestion. I will add the following messages in this commit log This commit is the starting point of the series to introduce ASPEED PCIe Root Complex (RC) support. Based on previous work from Cédric Le Goater, the following commits in this series extend and refine the implementation: - Add a PCIe Root Port so that devices can be attached without requiring an extra bridge. - Restrict the Root Port device instantiation to the AST2600 platform. - Integrate aspeed_cfg_translate_write() to support both AST2600 and AST2700. - Add MSI support and a preliminary RC IOMMU address space. - Fix issues with MSI interrupt clearing. - Extend support to the AST2700 SoC. - Drop the AST2600 RC_L support. - Introduce PCIe RC functional tests covering both AST2600 and AST2700. > > > > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com> > > --- > > include/hw/pci-host/aspeed_pcie.h | 44 +++++++++ > > hw/pci-host/aspeed_pcie.c | 159 > ++++++++++++++++++++++++++++++ > > hw/arm/Kconfig | 3 + > > hw/pci-host/Kconfig | 4 + > > hw/pci-host/meson.build | 1 + > > hw/pci-host/trace-events | 4 + > > 6 files changed, 215 insertions(+) > > create mode 100644 include/hw/pci-host/aspeed_pcie.h > > create mode 100644 hw/pci-host/aspeed_pcie.c > > > > diff --git a/include/hw/pci-host/aspeed_pcie.h > > b/include/hw/pci-host/aspeed_pcie.h > > new file mode 100644 > > index 0000000000..faf87073ab > > --- /dev/null > > +++ b/include/hw/pci-host/aspeed_pcie.h > > @@ -0,0 +1,44 @@ > > +/* > > + * ASPEED PCIe Host Controller > > + * > > + * Copyright (C) 2025 ASPEED Technology Inc. > > + * Copyright (c) 2022 Cédric Le Goater <c...@kaod.org> > > + * > > + * Jamin Lin <jamin_...@aspeedtech.com> > > I would add an "Authors" section and add our two names under it. > > > + * > > + * SPDX-License-Identifier: GPL-2.0-or-later > > + * > > + * This file is based on Cédric Le Goater's patch: > > + * "pci: Add Aspeed host bridge (WIP)" > > + * > > + > https://github.com/legoater/qemu/commit/d1b97b0c7844219d847122410dc1 > > + 89854f9d26df > > I don't think adding a reference on a github commit has much value since I > could remove the branch, which I do regularly. So, I would remove it and > simply keep the "Based on previous work from ... " > boiler plate. > Thanks for your review and suggestion. LICENSE HEADER will be updated as following. /* * ASPEED PCIe Host Controller * * Copyright (C) 2025 ASPEED Technology Inc. * Copyright (c) 2022 Cédric Le Goater <c...@kaod.org> * * Authors: * Cédric Le Goater <c...@kaod.org> * Jamin Lin <jamin_...@aspeedtech.com> * * SPDX-License-Identifier: GPL-2.0-or-later * * Based on previous work from Cédric Le Goater. * Modifications extend support for the ASPEED AST2600 and AST2700 platforms. */ > The rest looks good. > > > + * Modifications have been made to support the ASPEED AST2600 and > > + AST2700 > > + * platforms. > > + */ > > + > > +#ifndef ASPEED_PCIE_H > > +#define ASPEED_PCIE_H > > + > > +#include "hw/sysbus.h" > > +#include "hw/pci/pci_bridge.h" > > +#include "hw/pci/pcie_host.h" > > +#include "qom/object.h" > > + > > +#define TYPE_ASPEED_PCIE_PHY "aspeed.pcie-phy" > > +OBJECT_DECLARE_TYPE(AspeedPCIEPhyState, AspeedPCIEPhyClass, > > +ASPEED_PCIE_PHY); > > + > > +struct AspeedPCIEPhyState { > > + SysBusDevice parent_obj; > > + > > + MemoryRegion mmio; > > + uint32_t *regs; > > + uint32_t id; > > +}; > > + > > +struct AspeedPCIEPhyClass { > > + SysBusDeviceClass parent_class; > > + > > + uint64_t nr_regs; > > +}; > > + > > +#endif /* ASPEED_PCIE_H */ > > diff --git a/hw/pci-host/aspeed_pcie.c b/hw/pci-host/aspeed_pcie.c new > > file mode 100644 index 0000000000..5584449b17 > > --- /dev/null > > +++ b/hw/pci-host/aspeed_pcie.c > > @@ -0,0 +1,159 @@ > > +/* > > + * ASPEED PCIe Host Controller > > + * > > + * Copyright (C) 2025 ASPEED Technology Inc. > > + * Copyright (c) 2022 Cédric Le Goater <c...@kaod.org> > > + * > > + * Jamin Lin <jamin_...@aspeedtech.com> > > + * > > + * SPDX-License-Identifier: GPL-2.0-or-later > > + * > > + * This file is based on Cédric Le Goater's patch: > > + * "pci: Add Aspeed host bridge (WIP)" > > + * > > > +https://github.com/legoater/qemu/commit/d1b97b0c7844219d847122410dc > 18 > > +9854f9d26df > > + * > > + * Modifications have been made to support the ASPEED AST2600 and > > +AST2700 > > + * platforms. > > Same comment > Thanks for your review and suggestion. LICENSE HEADER will be updated as following /* * ASPEED PCIe Host Controller * * Copyright (C) 2025 ASPEED Technology Inc. * Copyright (c) 2022 Cédric Le Goater <c...@kaod.org> * * Authors: * Cédric Le Goater <c...@kaod.org> * Jamin Lin <jamin_...@aspeedtech.com> * * SPDX-License-Identifier: GPL-2.0-or-later * * Based on previous work from Cédric Le Goater. * Modifications extend support for the ASPEED AST2600 and AST2700 platforms. */ Thanks-Jamin > The rest looks good. > > Thanks, > > C. > > > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "qemu/log.h" > > +#include "qapi/error.h" > > +#include "hw/qdev-properties.h" > > +#include "hw/registerfields.h" > > +#include "hw/irq.h" > > +#include "hw/pci/pci_host.h" > > +#include "hw/pci-host/aspeed_pcie.h" > > +#include "hw/pci/msi.h" > > +#include "trace.h" > > + > > +/* > > + * PCIe PHY > > + * > > + * PCIe Host Controller (PCIEH) > > + */ > > + > > +/* AST2600 */ > > +REG32(PEHR_ID, 0x00) > > + FIELD(PEHR_ID, DEV, 16, 16) > > +REG32(PEHR_CLASS_CODE, 0x04) > > +REG32(PEHR_DATALINK, 0x10) > > +REG32(PEHR_PROTECT, 0x7C) > > + FIELD(PEHR_PROTECT, LOCK, 0, 8) > > +REG32(PEHR_LINK, 0xC0) > > + FIELD(PEHR_LINK, STS, 5, 1) > > + > > +#define ASPEED_PCIE_PHY_UNLOCK 0xA8 > > + > > +static uint64_t aspeed_pcie_phy_read(void *opaque, hwaddr addr, > > + unsigned int size) { > > + AspeedPCIEPhyState *s = ASPEED_PCIE_PHY(opaque); > > + uint32_t reg = addr >> 2; > > + uint32_t value = 0; > > + > > + value = s->regs[reg]; > > + > > + trace_aspeed_pcie_phy_read(s->id, addr, value); > > + > > + return value; > > +} > > + > > +static void aspeed_pcie_phy_write(void *opaque, hwaddr addr, uint64_t > data, > > + unsigned int size) { > > + AspeedPCIEPhyState *s = ASPEED_PCIE_PHY(opaque); > > + uint32_t reg = addr >> 2; > > + > > + trace_aspeed_pcie_phy_write(s->id, addr, data); > > + > > + switch (reg) { > > + case R_PEHR_PROTECT: > > + data &= R_PEHR_PROTECT_LOCK_MASK; > > + s->regs[reg] = !!(data == ASPEED_PCIE_PHY_UNLOCK); > > + break; > > + default: > > + s->regs[reg] = data; > > + break; > > + } > > +} > > + > > +static const MemoryRegionOps aspeed_pcie_phy_ops = { > > + .read = aspeed_pcie_phy_read, > > + .write = aspeed_pcie_phy_write, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > + .valid = { > > + .min_access_size = 1, > > + .max_access_size = 4, > > + }, > > +}; > > + > > +static void aspeed_pcie_phy_reset(DeviceState *dev) { > > + AspeedPCIEPhyState *s = ASPEED_PCIE_PHY(dev); > > + AspeedPCIEPhyClass *apc = ASPEED_PCIE_PHY_GET_CLASS(s); > > + > > + memset(s->regs, 0, apc->nr_regs << 2); > > + > > + s->regs[R_PEHR_ID] = > > + (0x1150 << R_PEHR_ID_DEV_SHIFT) | PCI_VENDOR_ID_ASPEED; > > + s->regs[R_PEHR_CLASS_CODE] = 0x06040006; > > + s->regs[R_PEHR_DATALINK] = 0xD7040022; > > + s->regs[R_PEHR_LINK] = R_PEHR_LINK_STS_MASK; } > > + > > +static void aspeed_pcie_phy_realize(DeviceState *dev, Error **errp) { > > + AspeedPCIEPhyState *s = ASPEED_PCIE_PHY(dev); > > + AspeedPCIEPhyClass *apc = ASPEED_PCIE_PHY_GET_CLASS(s); > > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > > + g_autofree char *name; > > + > > + s->regs = g_new(uint32_t, apc->nr_regs); > > + name = g_strdup_printf(TYPE_ASPEED_PCIE_PHY ".regs.%d", s->id); > > + memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_pcie_phy_ops, > s, name, > > + apc->nr_regs << 2); > > + sysbus_init_mmio(sbd, &s->mmio); > > +} > > + > > +static void aspeed_pcie_phy_unrealize(DeviceState *dev) { > > + AspeedPCIEPhyState *s = ASPEED_PCIE_PHY(dev); > > + > > + g_free(s->regs); > > + s->regs = NULL; > > +} > > + > > +static const Property aspeed_pcie_phy_props[] = { > > + DEFINE_PROP_UINT32("id", AspeedPCIEPhyState, id, 0), }; > > + > > +static void aspeed_pcie_phy_class_init(ObjectClass *klass, const void > > +*data) { > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + AspeedPCIEPhyClass *apc = ASPEED_PCIE_PHY_CLASS(klass); > > + > > + dc->desc = "ASPEED PCIe Phy"; > > + dc->realize = aspeed_pcie_phy_realize; > > + dc->unrealize = aspeed_pcie_phy_unrealize; > > + device_class_set_legacy_reset(dc, aspeed_pcie_phy_reset); > > + device_class_set_props(dc, aspeed_pcie_phy_props); > > + > > + apc->nr_regs = 0x100 >> 2; > > +} > > + > > +static const TypeInfo aspeed_pcie_phy_info = { > > + .name = TYPE_ASPEED_PCIE_PHY, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(AspeedPCIEPhyState), > > + .class_init = aspeed_pcie_phy_class_init, > > + .class_size = sizeof(AspeedPCIEPhyClass), }; > > + > > +static void aspeed_pcie_register_types(void) { > > + type_register_static(&aspeed_pcie_phy_info); > > +} > > + > > +type_init(aspeed_pcie_register_types); > > + > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index > > 2aa4b5d778..83d2676f6c 100644 > > --- a/hw/arm/Kconfig > > +++ b/hw/arm/Kconfig > > @@ -544,6 +544,7 @@ config ASPEED_SOC > > bool > > default y > > depends on TCG && ARM > > + imply PCI_DEVICES > > select DS1338 > > select FTGMAC100 > > select I2C > > @@ -564,6 +565,8 @@ config ASPEED_SOC > > select MAX31785 > > select FSI_APB2OPB_ASPEED > > select AT24C > > + select PCI_EXPRESS > > + select PCI_EXPRESS_ASPEED > > > > config MPS2 > > bool > > diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig index > > 9824fa188d..8cbb8304a3 100644 > > --- a/hw/pci-host/Kconfig > > +++ b/hw/pci-host/Kconfig > > @@ -46,6 +46,10 @@ config PCI_I440FX > > select PCI > > select PAM > > > > +config PCI_EXPRESS_ASPEED > > + bool > > + select PCI_EXPRESS > > + > > config PCI_EXPRESS_Q35 > > bool > > select PCI_EXPRESS > > diff --git a/hw/pci-host/meson.build b/hw/pci-host/meson.build index > > 937a0f72ac..86b754d0b0 100644 > > --- a/hw/pci-host/meson.build > > +++ b/hw/pci-host/meson.build > > @@ -2,6 +2,7 @@ pci_ss = ss.source_set() > > pci_ss.add(when: 'CONFIG_PAM', if_true: files('pam.c')) > > pci_ss.add(when: 'CONFIG_PCI_BONITO', if_true: files('bonito.c')) > > pci_ss.add(when: 'CONFIG_GT64120', if_true: files('gt64120.c')) > > +pci_ss.add(when: 'CONFIG_PCI_EXPRESS_ASPEED', if_true: > > +files('aspeed_pcie.c')) > > pci_ss.add(when: 'CONFIG_PCI_EXPRESS_DESIGNWARE', if_true: > files('designware.c')) > > pci_ss.add(when: 'CONFIG_PCI_EXPRESS_GENERIC_BRIDGE', if_true: > files('gpex.c')) > > pci_ss.add(when: ['CONFIG_PCI_EXPRESS_GENERIC_BRIDGE', > > 'CONFIG_ACPI'], if_true: files('gpex-acpi.c')) diff --git > > a/hw/pci-host/trace-events b/hw/pci-host/trace-events index > > 0a816b9aa1..3438516756 100644 > > --- a/hw/pci-host/trace-events > > +++ b/hw/pci-host/trace-events > > @@ -1,5 +1,9 @@ > > # See docs/devel/tracing.rst for syntax documentation. > > > > +# aspeed_pcie.c > > +aspeed_pcie_phy_read(uint32_t id, uint64_t addr, uint32_t value) "%d: > > +addr 0x%" PRIx64 " value 0x%" PRIx32 aspeed_pcie_phy_write(uint32_t > > +id, uint64_t addr, uint32_t value) "%d: addr 0x%" PRIx64 " value 0x%" > > +PRIx32 > > + > > # bonito.c > > bonito_spciconf_small_access(uint64_t addr, unsigned size) "PCI config > address is smaller then 32-bit, addr: 0x%"PRIx64", size: %u" > >
