Re: [PATCH v2 2/9] pnv/phb4: Add reset logic to PHB4

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Dec 30, 2025 at 04:21:20AM -0600, Saif Abrar wrote:
> Add a method to be invoked on QEMU reset.
> Also add CFG and PBL core-blocks reset logic using
> appropriate bits of PHB_PCIE_CRESET register.
> 
> Tested by reading the reset value of a register.
> 
> Signed-off-by: Saif Abrar <saif.abrar@linux.vnet.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> v1 -> v2:
> - Using the ResettableClass.
> - Reset of the root complex registers done in pnv_phb_root_port_reset_hold().
> 
>  hw/pci-host/pnv_phb.c               |   1 +
>  hw/pci-host/pnv_phb4.c              | 101 +++++++++++++++++++++++++++-
>  include/hw/pci-host/pnv_phb4.h      |   1 +
>  include/hw/pci-host/pnv_phb4_regs.h |  16 ++++-
>  tests/qtest/pnv-phb4-test.c         |  28 +++++++-
>  5 files changed, 143 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
> index 85fcc3b686..bc08d7488d 100644
> --- a/hw/pci-host/pnv_phb.c
> +++ b/hw/pci-host/pnv_phb.c
> @@ -233,6 +233,7 @@ static void pnv_phb_root_port_reset_hold(Object *obj, ResetType type)
>      pci_set_long(conf + PCI_PREF_BASE_UPPER32, 0x1); /* Hack */
>      pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0xffffffff);
>      pci_config_set_interrupt_pin(conf, 0);
> +    pnv_phb4_cfg_core_reset(d);
>  }
>  
>  static void pnv_phb_root_port_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> index 396bc47817..bf21f955c8 100644
> --- a/hw/pci-host/pnv_phb4.c
> +++ b/hw/pci-host/pnv_phb4.c
> @@ -1,7 +1,8 @@
>  /*
>   * QEMU PowerPC PowerNV (POWER9) PHB4 model
> + * QEMU PowerPC PowerNV (POWER10) PHB5 model
>   *
> - * Copyright (c) 2018-2020, IBM Corporation.
> + * Copyright (c) 2018-2025, IBM Corporation.
>   *
>   * This code is licensed under the GPL version 2 or later. See the
>   * COPYING file in the top-level directory.
> @@ -22,6 +23,7 @@
>  #include "hw/core/qdev-properties.h"
>  #include "qom/object.h"
>  #include "trace.h"
> +#include "system/reset.h"
>  
>  #define phb_error(phb, fmt, ...)                                        \
>      qemu_log_mask(LOG_GUEST_ERROR, "phb4[%d:%d]: " fmt "\n",            \
> @@ -499,6 +501,81 @@ static void pnv_phb4_update_xsrc(PnvPHB4 *phb)
>      }
>  }
>  
> +/*
> + * Get the PCI-E capability offset from the root-port
> + */
> +static uint32_t get_exp_offset(PCIDevice *pdev)
> +{
> +    PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(pdev);
> +    return rpc->exp_offset;
> +}
> +
> +void pnv_phb4_cfg_core_reset(PCIDevice *d)
> +{
> +    uint8_t *conf = d->config;

add an empty line here after declaration.

> +    pci_set_word(conf + PCI_COMMAND, PCI_COMMAND_SERR);
> +    pci_set_word(conf + PCI_STATUS, PCI_STATUS_CAP_LIST);
> +    pci_set_long(conf + PCI_CLASS_REVISION, 0x06040000);
> +    pci_set_long(conf + PCI_CACHE_LINE_SIZE, BIT(16));
> +    pci_set_word(conf + PCI_MEMORY_BASE, BIT(4));
> +    pci_set_word(conf + PCI_PREF_MEMORY_BASE, BIT(0) | BIT(4));
> +    pci_set_word(conf + PCI_PREF_MEMORY_LIMIT, PCI_PREF_RANGE_TYPE_64);
> +    pci_set_long(conf + PCI_CAPABILITY_LIST, BIT(6));
> +    pci_set_long(conf + PCI_CAPABILITY_LIST, BIT(6));
> +    pci_set_word(conf + PCI_BRIDGE_CONTROL, PCI_BRIDGE_CTL_SERR);
> +    pci_set_long(conf + PCI_BRIDGE_CONTROL + PCI_PM_PMC, 0xC8034801);
> +
> +    uint32_t exp_offset = get_exp_offset(d);
> +    pci_set_long(conf + exp_offset, 0x420010);
> +    pci_set_long(conf + exp_offset + PCI_EXP_DEVCAP,  0x8022);
> +    pci_set_long(conf + exp_offset + PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_EXT_TAG
> +                                              | PCI_EXP_DEVCTL_PAYLOAD_512B);
> +    pci_set_long(conf + exp_offset + PCI_EXP_LNKCAP, PCI_EXP_LNKCAP_LBNC
> +                 | PCI_EXP_LNKCAP_DLLLARC | BIT(8) | PCI_EXP_LNKCAP_SLS_32_0GB);
> +    pci_set_word(conf + exp_offset + PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RCB);
> +    pci_set_word(conf + exp_offset + PCI_EXP_LNKSTA,
> +                       (PCI_EXP_LNKSTA_NLW_X8 << 2) | PCI_EXP_LNKSTA_CLS_2_5GB);
> +    pci_set_long(conf + exp_offset + PCI_EXP_SLTCTL,
> +                                                   PCI_EXP_SLTCTL_ASPL_DISABLE);
> +    pci_set_long(conf + exp_offset + PCI_EXP_DEVCAP2, BIT(16)
> +                  | PCI_EXP_DEVCAP2_ARI | PCI_EXP_DEVCAP2_COMP_TMOUT_DIS | 0xF);
> +    pci_set_long(conf + exp_offset + PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ARI);
> +    pci_set_long(conf + exp_offset + PCI_EXP_LNKCAP2, BIT(23)
> +                       | PCI_EXP_LNKCAP2_SLS_32_0GB
> +                       | PCI_EXP_LNKCAP2_SLS_16_0GB | PCI_EXP_LNKCAP2_SLS_8_0GB
> +                       | PCI_EXP_LNKCAP2_SLS_5_0GB | PCI_EXP_LNKCAP2_SLS_2_5GB);
> +    pci_set_long(conf + PHB_AER_ECAP, PCI_EXT_CAP(0x1, 0x1, 0x148));
> +    pci_set_long(conf + PHB_SEC_ECAP, (0x1A0 << 20) | BIT(16)
> +                                                       | PCI_EXT_CAP_ID_SECPCI);
> +    pci_set_long(conf + PHB_LMR_ECAP, 0x1E810027);
> +    /* LMR - Margining Lane Control / Status Register # 2 to 16 */
> +    int i;

declare at the beginning of the block please.
and add an empty line here after declaration.
Or, declare inside the for loop.

> +    for (i = PHB_LMR_CTLSTA_2 ; i <= PHB_LMR_CTLSTA_16 ; i += 4) {
> +        pci_set_long(conf + i, 0x9C38);
> +    }
> +
> +    pci_set_long(conf + PHB_DLF_ECAP, 0x1F410025);
> +    pci_set_long(conf + PHB_DLF_CAP,  0x80000001);
> +    pci_set_long(conf + P16_ECAP, 0x22410026);
> +    pci_set_long(conf + P32_ECAP, 0x1002A);
> +    pci_set_long(conf + P32_CAP,  0x103);
> +}
> +
> +static void pnv_phb4_pbl_core_reset(PnvPHB4 *phb)
> +{
> +    /* Zero all registers initially */
> +    int i;

an empty line here after declaration.
Or, declare inside the for loop.

> +    for (i = PHB_PBL_CONTROL ; i <= PHB_PBL_ERR1_STATUS_MASK ; i += 8) {
> +        phb->regs[i >> 3] = 0x0;
> +    }
> +
> +    /* Set specific register values */
> +    phb->regs[PHB_PBL_CONTROL       >> 3] = 0xC009000000000000;
> +    phb->regs[PHB_PBL_TIMEOUT_CTRL  >> 3] = 0x2020000000000000;
> +    phb->regs[PHB_PBL_NPTAG_ENABLE  >> 3] = 0xFFFFFFFF00000000;
> +    phb->regs[PHB_PBL_SYS_LINK_INIT >> 3] = 0x80088B4642473000;
> +}
> +
>  static void pnv_phb4_reg_write(void *opaque, hwaddr off, uint64_t val,
>                                 unsigned size)
>  {
> @@ -612,6 +689,17 @@ static void pnv_phb4_reg_write(void *opaque, hwaddr off, uint64_t val,
>          pnv_phb4_update_xsrc(phb);
>          break;
>  
> +    /* Reset core blocks */
> +    case PHB_PCIE_CRESET:
> +        if (val & PHB_PCIE_CRESET_CFG_CORE) {
> +            PCIHostState *pci = PCI_HOST_BRIDGE(phb->phb_base);

an empty line here after declaration.

> +            pnv_phb4_cfg_core_reset(pci_find_device(pci->bus, 0, 0));
> +        }
> +        if (val & PHB_PCIE_CRESET_PBL) {
> +            pnv_phb4_pbl_core_reset(phb);
> +        }
> +        break;
> +
>      /* Silent simple writes */
>      case PHB_ASN_CMPM:
>      case PHB_CONFIG_ADDRESS:
> @@ -1532,6 +1620,12 @@ static PCIIOMMUOps pnv_phb4_iommu_ops = {
>      .get_address_space = pnv_phb4_dma_iommu,
>  };
>  
> +static void pnv_phb4_reset(Object *obj, ResetType type)
> +{
> +    PnvPHB4 *phb = PNV_PHB4(obj);

en emoty line here after declaration

> +    pnv_phb4_pbl_core_reset(phb);
> +}
> +
>  static void pnv_phb4_instance_init(Object *obj)
>  {
>      PnvPHB4 *phb = PNV_PHB4(obj);
> @@ -1608,6 +1702,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error **errp)
>      phb->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
>  
>      pnv_phb4_xscom_realize(phb);
> +
> +    qemu_register_resettable(OBJECT(dev));
>  }
>  
>  /*
> @@ -1707,6 +1803,9 @@ static void pnv_phb4_class_init(ObjectClass *klass, const void *data)
>      dc->user_creatable  = false;
>  
>      xfc->notify         = pnv_phb4_xive_notify;
> +
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);

declarations at the beginning of the block please.

> +    rc->phases.enter = pnv_phb4_reset;
>  }
>  
>  static const TypeInfo pnv_phb4_type_info = {
> diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> index de996e718b..47a5c3edf5 100644
> --- a/include/hw/pci-host/pnv_phb4.h
> +++ b/include/hw/pci-host/pnv_phb4.h
> @@ -160,6 +160,7 @@ void pnv_phb4_pic_print_info(PnvPHB4 *phb, GString *buf);
>  int pnv_phb4_pec_get_phb_id(PnvPhb4PecState *pec, int stack_index);
>  PnvPhb4PecState *pnv_pec_add_phb(PnvChip *chip, PnvPHB *phb, Error **errp);
>  void pnv_phb4_bus_init(DeviceState *dev, PnvPHB4 *phb);
> +void pnv_phb4_cfg_core_reset(PCIDevice *d);
>  extern const MemoryRegionOps pnv_phb4_xscom_ops;
>  
>  /*
> diff --git a/include/hw/pci-host/pnv_phb4_regs.h b/include/hw/pci-host/pnv_phb4_regs.h
> index bea96f4d91..6892e21cc9 100644
> --- a/include/hw/pci-host/pnv_phb4_regs.h
> +++ b/include/hw/pci-host/pnv_phb4_regs.h
> @@ -343,6 +343,18 @@
>  #define PHB_RC_CONFIG_BASE                      0x1000
>  #define   PHB_RC_CONFIG_SIZE                    0x800
>  
> +#define PHB_AER_ECAP                            0x100
> +#define PHB_AER_CAPCTRL                         0x118
> +#define PHB_SEC_ECAP                            0x148
> +#define PHB_LMR_ECAP                            0x1A0
> +#define PHB_LMR_CTLSTA_2                        0x1AC
> +#define PHB_LMR_CTLSTA_16                       0x1E4
> +#define PHB_DLF_ECAP                            0x1E8
> +#define PHB_DLF_CAP                             0x1EC
> +#define P16_ECAP                                0x1F4
> +#define P32_ECAP                                0x224
> +#define P32_CAP                                 0x228
> +
>  /* PHB4 REGB registers */
>  
>  /* PBL core */
> @@ -368,7 +380,7 @@
>  #define PHB_PCIE_SCR                    0x1A00
>  #define   PHB_PCIE_SCR_SLOT_CAP         PPC_BIT(15)
>  #define   PHB_PCIE_SCR_MAXLINKSPEED     PPC_BITMASK(32, 35)
> -
> +#define PHB_PCIE_BNR                    0x1A08
>  
>  #define PHB_PCIE_CRESET                 0x1A10
>  #define   PHB_PCIE_CRESET_CFG_CORE      PPC_BIT(0)
> @@ -423,6 +435,8 @@
>  #define PHB_PCIE_LANE_EQ_CNTL23         0x1B08 /* DD1 only */
>  #define PHB_PCIE_TRACE_CTRL             0x1B20
>  #define PHB_PCIE_MISC_STRAP             0x1B30
> +#define PHB_PCIE_PHY_RXEQ_STAT_G3_00_03 0x1B40
> +#define PHB_PCIE_PHY_RXEQ_STAT_G5_12_15 0x1B98
>  
>  /* Error */
>  #define PHB_REGB_ERR_STATUS             0x1C00
> diff --git a/tests/qtest/pnv-phb4-test.c b/tests/qtest/pnv-phb4-test.c
> index 3890b4f970..3957c743a3 100644
> --- a/tests/qtest/pnv-phb4-test.c
> +++ b/tests/qtest/pnv-phb4-test.c
> @@ -35,6 +35,29 @@ static uint64_t pnv_phb_xscom_read(QTestState *qts, const PnvChip *chip,
>      return qtest_readq(qts, pnv_xscom_addr(chip, (scom >> 3) + indirect_data));
>  }
>  
> +#define phb4_xscom_read(a) pnv_phb_xscom_read(qts, \
> +                                   &pnv_chips[PNV_P10_CHIP_INDEX], PHB4_XSCOM, \
> +                                   PHB_SCOM_HV_IND_ADDR, PHB_SCOM_HV_IND_DATA, \
> +                                   PPC_BIT(0) | a)


macros in UPPER CASE please, and parameters should be in ():
	PPC_BIT(0) | (a)

> +
> +/* Assert that 'PHB PBL Control' register has correct reset value */
> +static void phb4_reset_test(QTestState *qts)
> +{
> +    g_assert_cmpuint(phb4_xscom_read(PHB_PBL_CONTROL), ==, 0xC009000000000000);
> +}
> +
> +static void phb4_tests(void)
> +{
> +    QTestState *qts = NULL;
> +
> +    qts = qtest_initf("-machine powernv10 -accel tcg");
> +
> +    /* Check reset value of a register */
> +    phb4_reset_test(qts);
> +
> +    qtest_quit(qts);
> +}
> +
>  /* Assert that 'PHB - Version Register' bits-[24:31] are as expected */
>  static void phb_version_test(const void *data)
>  {
> @@ -71,8 +94,6 @@ static void phb_version_test(const void *data)
>      /* PHB Version register bits [24:31] */
>      ver = ver >> (63 - 31);
>      g_assert_cmpuint(ver, ==, expected_ver);
> -
> -    qtest_quit(qts);
>  }
>  
>  /* Verify versions of all supported PHB's */
> @@ -95,5 +116,8 @@ int main(int argc, char **argv)
>      /* PHB[345] tests */
>      add_phbX_version_test();
>  
> +    /* PHB4 specific tests */
> +    qtest_add_func("phb4", phb4_tests);
> +
>      return g_test_run();
>  }
> -- 
> 2.47.3