[PATCH v5 0/3] pnv nest1 chiplet model

[PATCH v5 0/3] pnv nest1 chiplet model

[Chalapathi V]

Hello,

Thank you for the review and suggestions on V4.

The suggestions and changes requested in V4 are considered and incorporated in
V5.

In this version, couple of powerbus scom registers are modelled.
Hence the new qom-tree is as below.
(qemu) info qom-tree 
/machine (powernv10-machine)
  /chip[0] (power10_v2.0-pnv-chip)
    /nest1 (pnv-nest1-chiplet)
      /perv (pnv-pervasive-chiplet)
        /xscom-nest1-control-regs[0] (memory-region)
      /xscom-nest1-pb-scom-eq-regs[0] (memory-region)
      /xscom-nest1-pb-scom-es-regs[0] (memory-region)

Patches overview in V5.
PATCH1: Create a common perv chiplet model with control chiplet scom registers
PATCH2: Create a nest1 chiplet model, connect perv model to nest1 and implement
        powerbus scom registers.
PATCH3: Connect nest1 model to p10 chip.

Test covered:
These changes are tested on a single socket and 2 socket P10 machine.

Thank You,
Chalapathi

Chalapathi V (3):
  hw/ppc: Add pnv pervasive common chiplet units
  hw/ppc: Add nest1 chiplet model
  hw/ppc: Nest1 chiplet wiring

 include/hw/ppc/pnv_chip.h         |   2 +
 include/hw/ppc/pnv_nest_chiplet.h |  36 +++++
 include/hw/ppc/pnv_pervasive.h    |  37 +++++
 include/hw/ppc/pnv_xscom.h        |   9 ++
 hw/ppc/pnv.c                      |  14 ++
 hw/ppc/pnv_nest1_chiplet.c        | 197 +++++++++++++++++++++++++++
 hw/ppc/pnv_pervasive.c            | 217 ++++++++++++++++++++++++++++++
 hw/ppc/meson.build                |   2 +
 8 files changed, 514 insertions(+)
 create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
 create mode 100644 include/hw/ppc/pnv_pervasive.h
 create mode 100644 hw/ppc/pnv_nest1_chiplet.c
 create mode 100644 hw/ppc/pnv_pervasive.c

-- 
2.31.1

[PATCH v5 1/3] hw/ppc: Add pnv pervasive common chiplet units

[Chalapathi V]

This part of the patchset creates a common pervasive chiplet model where it
houses the common units of a chiplets.

The chiplet control unit is common across chiplets and this commit implements
the pervasive chiplet model with chiplet control registers.

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 include/hw/ppc/pnv_pervasive.h |  37 ++++++
 include/hw/ppc/pnv_xscom.h     |   3 +
 hw/ppc/pnv_pervasive.c         | 217 +++++++++++++++++++++++++++++++++
 hw/ppc/meson.build             |   1 +
 4 files changed, 258 insertions(+)
 create mode 100644 include/hw/ppc/pnv_pervasive.h
 create mode 100644 hw/ppc/pnv_pervasive.c

diff --git a/include/hw/ppc/pnv_pervasive.h b/include/hw/ppc/pnv_pervasive.h
new file mode 100644
index 0000000000..d83f86df7b
--- /dev/null
+++ b/include/hw/ppc/pnv_pervasive.h
@@ -0,0 +1,37 @@
+/*
+ * QEMU PowerPC pervasive common chiplet model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#ifndef PPC_PNV_PERVASIVE_H
+#define PPC_PNV_PERVASIVE_H
+
+#define TYPE_PNV_PERV "pnv-pervasive-chiplet"
+#define PNV_PERV(obj) OBJECT_CHECK(PnvPerv, (obj), TYPE_PNV_PERV)
+
+typedef struct PnvPervCtrlRegs {
+#define CPLT_CTRL_SIZE 6
+    uint64_t cplt_ctrl[CPLT_CTRL_SIZE];
+    uint64_t cplt_cfg0;
+    uint64_t cplt_cfg1;
+    uint64_t cplt_stat0;
+    uint64_t cplt_mask0;
+    uint64_t ctrl_protect_mode;
+    uint64_t ctrl_atomic_lock;
+} PnvPervCtrlRegs;
+
+typedef struct PnvPerv {
+    DeviceState parent;
+    char *parent_obj_name;
+    MemoryRegion xscom_perv_ctrl_regs;
+    PnvPervCtrlRegs control_regs;
+} PnvPerv;
+
+void pnv_perv_dt(PnvPerv *perv, uint32_t base_addr, void *fdt, int offset);
+#endif /*PPC_PNV_PERVASIVE_H */
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index f5becbab41..d09d10f32b 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -170,6 +170,9 @@ struct PnvXScomInterfaceClass {
 #define PNV10_XSCOM_XIVE2_BASE     0x2010800
 #define PNV10_XSCOM_XIVE2_SIZE     0x400
 
+#define PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE      0x3000000
+#define PNV10_XSCOM_CTRL_CHIPLET_SIZE            0x400
+
 #define PNV10_XSCOM_PEC_NEST_BASE  0x3011800 /* index goes downwards ... */
 #define PNV10_XSCOM_PEC_NEST_SIZE  0x100
 
diff --git a/hw/ppc/pnv_pervasive.c b/hw/ppc/pnv_pervasive.c
new file mode 100644
index 0000000000..c925070798
--- /dev/null
+++ b/hw/ppc/pnv_pervasive.c
@@ -0,0 +1,217 @@
+/*
+ * QEMU PowerPC pervasive common chiplet model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/qdev-properties.h"
+#include "hw/ppc/pnv.h"
+#include "hw/ppc/pnv_xscom.h"
+#include "hw/ppc/pnv_pervasive.h"
+#include "hw/ppc/fdt.h"
+#include <libfdt.h>
+
+#define CPLT_CONF0               0x08
+#define CPLT_CONF0_OR            0x18
+#define CPLT_CONF0_CLEAR         0x28
+#define CPLT_CONF1               0x09
+#define CPLT_CONF1_OR            0x19
+#define CPLT_CONF1_CLEAR         0x29
+#define CPLT_STAT0               0x100
+#define CPLT_MASK0               0x101
+#define CPLT_PROTECT_MODE        0x3FE
+#define CPLT_ATOMIC_CLOCK        0x3FF
+
+static uint64_t pnv_chiplet_ctrl_read(void *opaque, hwaddr addr, unsigned size)
+{
+    PnvPerv *perv = PNV_PERV(opaque);
+    int reg = addr >> 3;
+    uint64_t val = ~0ull;
+
+    /* CPLT_CTRL0 to CPLT_CTRL5 */
+    for (int i = 0; i < CPLT_CTRL_SIZE; i++) {
+        if (reg == i) {
+            return perv->control_regs.cplt_ctrl[i];
+        } else if ((reg == (i + 0x10)) || (reg == (i + 0x20))) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
+                                           "xscom read at 0x%" PRIx64 "\n",
+                                           __func__, (unsigned long)reg);
+            return val;
+        }
+    }
+
+    switch (reg) {
+    case CPLT_CONF0:
+        val = perv->control_regs.cplt_cfg0;
+        break;
+    case CPLT_CONF0_OR:
+    case CPLT_CONF0_CLEAR:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
+                                   "xscom read at 0x%" PRIx64 "\n",
+                                   __func__, (unsigned long)reg);
+        break;
+    case CPLT_CONF1:
+        val = perv->control_regs.cplt_cfg1;
+        break;
+    case CPLT_CONF1_OR:
+    case CPLT_CONF1_CLEAR:
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
+                                   "xscom read at 0x%" PRIx64 "\n",
+                                   __func__, (unsigned long)reg);
+        break;
+    case CPLT_STAT0:
+        val = perv->control_regs.cplt_stat0;
+        break;
+    case CPLT_MASK0:
+        val = perv->control_regs.cplt_mask0;
+        break;
+    case CPLT_PROTECT_MODE:
+        val = perv->control_regs.ctrl_protect_mode;
+        break;
+    case CPLT_ATOMIC_CLOCK:
+        val = perv->control_regs.ctrl_atomic_lock;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Chiplet_control_regs: Invalid xscom "
+                 "read at 0x%" PRIx64 "\n", __func__, (unsigned long)reg);
+    }
+    return val;
+}
+
+static void pnv_chiplet_ctrl_write(void *opaque, hwaddr addr,
+                                 uint64_t val, unsigned size)
+{
+    PnvPerv *perv = PNV_PERV(opaque);
+    int reg = addr >> 3;
+
+    /* CPLT_CTRL0 to CPLT_CTRL5 */
+    for (int i = 0; i < CPLT_CTRL_SIZE; i++) {
+        if (reg == i) {
+            perv->control_regs.cplt_ctrl[i] = val;
+            return;
+        } else if (reg == (i + 0x10)) {
+            perv->control_regs.cplt_ctrl[i] |= val;
+            return;
+        } else if (reg == (i + 0x20)) {
+            perv->control_regs.cplt_ctrl[i] &= ~val;
+            return;
+        }
+    }
+
+    switch (reg) {
+    case CPLT_CONF0:
+        perv->control_regs.cplt_cfg0 = val;
+        break;
+    case CPLT_CONF0_OR:
+        perv->control_regs.cplt_cfg0 |= val;
+        break;
+    case CPLT_CONF0_CLEAR:
+        perv->control_regs.cplt_cfg0 &= ~val;
+        break;
+    case CPLT_CONF1:
+        perv->control_regs.cplt_cfg1 = val;
+        break;
+    case CPLT_CONF1_OR:
+        perv->control_regs.cplt_cfg1 |= val;
+        break;
+    case CPLT_CONF1_CLEAR:
+        perv->control_regs.cplt_cfg1 &= ~val;
+        break;
+    case CPLT_STAT0:
+        perv->control_regs.cplt_stat0 = val;
+        break;
+    case CPLT_MASK0:
+        perv->control_regs.cplt_mask0 = val;
+        break;
+    case CPLT_PROTECT_MODE:
+        perv->control_regs.ctrl_protect_mode = val;
+        break;
+    case CPLT_ATOMIC_CLOCK:
+        perv->control_regs.ctrl_atomic_lock = val;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Chiplet_control_regs: Invalid xscom "
+                                 "write at 0x%" PRIx64 "\n",
+                                 __func__, (unsigned long)reg);
+    }
+}
+
+static const MemoryRegionOps pnv_perv_control_xscom_ops = {
+    .read = pnv_chiplet_ctrl_read,
+    .write = pnv_chiplet_ctrl_write,
+    .valid.min_access_size = 8,
+    .valid.max_access_size = 8,
+    .impl.min_access_size = 8,
+    .impl.max_access_size = 8,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void pnv_perv_realize(DeviceState *dev, Error **errp)
+{
+    PnvPerv *perv = PNV_PERV(dev);
+    g_autofree char *region_name = NULL;
+    region_name = g_strdup_printf("xscom-%s-control-regs",
+                                   perv->parent_obj_name);
+
+    /* Chiplet control scoms */
+    pnv_xscom_region_init(&perv->xscom_perv_ctrl_regs, OBJECT(perv),
+                          &pnv_perv_control_xscom_ops, perv, region_name,
+                          PNV10_XSCOM_CTRL_CHIPLET_SIZE);
+}
+
+void pnv_perv_dt(PnvPerv *perv, uint32_t base_addr, void *fdt, int offset)
+{
+    g_autofree char *name = NULL;
+    int perv_offset;
+    const char compat[] = "ibm,power10-perv-chiplet";
+    uint32_t reg[] = {
+        cpu_to_be32(base_addr),
+        cpu_to_be32(PNV10_XSCOM_CTRL_CHIPLET_SIZE)
+    };
+
+    name = g_strdup_printf("%s-perv@%x", perv->parent_obj_name, base_addr);
+    perv_offset = fdt_add_subnode(fdt, offset, name);
+    _FDT(perv_offset);
+
+    _FDT(fdt_setprop(fdt, perv_offset, "reg", reg, sizeof(reg)));
+    _FDT(fdt_setprop(fdt, perv_offset, "compatible", compat, sizeof(compat)));
+}
+
+static Property pnv_perv_properties[] = {
+    DEFINE_PROP_STRING("parent-obj-name", PnvPerv, parent_obj_name),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pnv_perv_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "PowerNV perv chiplet";
+    dc->realize = pnv_perv_realize;
+    device_class_set_props(dc, pnv_perv_properties);
+}
+
+static const TypeInfo pnv_perv_info = {
+    .name          = TYPE_PNV_PERV,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(PnvPerv),
+    .class_init    = pnv_perv_class_init,
+    .interfaces    = (InterfaceInfo[]) {
+        { TYPE_PNV_XSCOM_INTERFACE },
+        { }
+    }
+};
+
+static void pnv_perv_register_types(void)
+{
+    type_register_static(&pnv_perv_info);
+}
+
+type_init(pnv_perv_register_types);
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index ea44856d43..37a7a8935d 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -51,6 +51,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
   'pnv_bmc.c',
   'pnv_homer.c',
   'pnv_pnor.c',
+  'pnv_pervasive.c',
 ))
 # PowerPC 4xx boards
 ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(
-- 
2.31.1

Re: [PATCH v5 1/3] hw/ppc: Add pnv pervasive common chiplet units

[Nicholas Piggin]

On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
> This part of the patchset creates a common pervasive chiplet model where it
> houses the common units of a chiplets.
>
> The chiplet control unit is common across chiplets and this commit implements
> the pervasive chiplet model with chiplet control registers.

This might be reworded to be a bit clearer, could provide a bit of
background information too (in changelog or header comment):

 Status, configuration, and control of units in POWER chips is provided
 by the pervasive subsystem, which connects registers to the SCOM bus,
 which can be programmed by processor cores, other units on the chip,
 BMCs, or other POWER chips.

 A POWER10 chip is divided into logical pieces called chiplets. Chiplets
 are broadly divided into "core chiplets" (with the processor cores) and
 "nest chiplets" (with everything else). Each chiplet has an attachment
 to the pervasive bus (PIB) and with chiplet-specific registers. All nest
 chiplets have a common basic set of registers, which this model
 provides. They may also implement additional registers.

That's my undertsanding, does it look right? If yes and this file is
specifically for nest chiplets (not core), maybe the name should
reflect that?

>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  include/hw/ppc/pnv_pervasive.h |  37 ++++++
>  include/hw/ppc/pnv_xscom.h     |   3 +
>  hw/ppc/pnv_pervasive.c         | 217 +++++++++++++++++++++++++++++++++
>  hw/ppc/meson.build             |   1 +
>  4 files changed, 258 insertions(+)
>  create mode 100644 include/hw/ppc/pnv_pervasive.h
>  create mode 100644 hw/ppc/pnv_pervasive.c
>
> diff --git a/include/hw/ppc/pnv_pervasive.h b/include/hw/ppc/pnv_pervasive.h
> new file mode 100644
> index 0000000000..d83f86df7b
> --- /dev/null
> +++ b/include/hw/ppc/pnv_pervasive.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU PowerPC pervasive common chiplet model
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#ifndef PPC_PNV_PERVASIVE_H
> +#define PPC_PNV_PERVASIVE_H
> +
> +#define TYPE_PNV_PERV "pnv-pervasive-chiplet"
> +#define PNV_PERV(obj) OBJECT_CHECK(PnvPerv, (obj), TYPE_PNV_PERV)
> +
> +typedef struct PnvPervCtrlRegs {
> +#define CPLT_CTRL_SIZE 6
> +    uint64_t cplt_ctrl[CPLT_CTRL_SIZE];
> +    uint64_t cplt_cfg0;
> +    uint64_t cplt_cfg1;
> +    uint64_t cplt_stat0;
> +    uint64_t cplt_mask0;
> +    uint64_t ctrl_protect_mode;
> +    uint64_t ctrl_atomic_lock;
> +} PnvPervCtrlRegs;
> +
> +typedef struct PnvPerv {

I don't want to bikeshed the name too much, but we have perv and
pervasive, I'd prefer to use pervasive consistently.

I would like the name to reflect that it is for common nest chiplet
registers too, but maybe that's getting too ambitious...

PnvNestChipletCommonRegs
PnvNestChipletPervasive

?


> +    DeviceState parent;
> +    char *parent_obj_name;
> +    MemoryRegion xscom_perv_ctrl_regs;
> +    PnvPervCtrlRegs control_regs;
> +} PnvPerv;
> +
> +void pnv_perv_dt(PnvPerv *perv, uint32_t base_addr, void *fdt, int offset);
> +#endif /*PPC_PNV_PERVASIVE_H */
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> index f5becbab41..d09d10f32b 100644
> --- a/include/hw/ppc/pnv_xscom.h
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -170,6 +170,9 @@ struct PnvXScomInterfaceClass {
>  #define PNV10_XSCOM_XIVE2_BASE     0x2010800
>  #define PNV10_XSCOM_XIVE2_SIZE     0x400
>  
> +#define PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE      0x3000000
> +#define PNV10_XSCOM_CTRL_CHIPLET_SIZE            0x400
> +
>  #define PNV10_XSCOM_PEC_NEST_BASE  0x3011800 /* index goes downwards ... */
>  #define PNV10_XSCOM_PEC_NEST_SIZE  0x100
>  
> diff --git a/hw/ppc/pnv_pervasive.c b/hw/ppc/pnv_pervasive.c
> new file mode 100644
> index 0000000000..c925070798
> --- /dev/null
> +++ b/hw/ppc/pnv_pervasive.c
> @@ -0,0 +1,217 @@
> +/*
> + * QEMU PowerPC pervasive common chiplet model
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_xscom.h"
> +#include "hw/ppc/pnv_pervasive.h"
> +#include "hw/ppc/fdt.h"
> +#include <libfdt.h>
> +
> +#define CPLT_CONF0               0x08
> +#define CPLT_CONF0_OR            0x18
> +#define CPLT_CONF0_CLEAR         0x28
> +#define CPLT_CONF1               0x09
> +#define CPLT_CONF1_OR            0x19
> +#define CPLT_CONF1_CLEAR         0x29
> +#define CPLT_STAT0               0x100
> +#define CPLT_MASK0               0x101
> +#define CPLT_PROTECT_MODE        0x3FE
> +#define CPLT_ATOMIC_CLOCK        0x3FF
> +
> +static uint64_t pnv_chiplet_ctrl_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PnvPerv *perv = PNV_PERV(opaque);
> +    int reg = addr >> 3;
> +    uint64_t val = ~0ull;
> +
> +    /* CPLT_CTRL0 to CPLT_CTRL5 */
> +    for (int i = 0; i < CPLT_CTRL_SIZE; i++) {
> +        if (reg == i) {
> +            return perv->control_regs.cplt_ctrl[i];
> +        } else if ((reg == (i + 0x10)) || (reg == (i + 0x20))) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
> +                                           "xscom read at 0x%" PRIx64 "\n",
> +                                           __func__, (unsigned long)reg);
> +            return val;
> +        }
> +    }
> +
> +    switch (reg) {
> +    case CPLT_CONF0:
> +        val = perv->control_regs.cplt_cfg0;
> +        break;
> +    case CPLT_CONF0_OR:
> +    case CPLT_CONF0_CLEAR:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
> +                                   "xscom read at 0x%" PRIx64 "\n",
> +                                   __func__, (unsigned long)reg);
> +        break;
> +    case CPLT_CONF1:
> +        val = perv->control_regs.cplt_cfg1;
> +        break;
> +    case CPLT_CONF1_OR:
> +    case CPLT_CONF1_CLEAR:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
> +                                   "xscom read at 0x%" PRIx64 "\n",
> +                                   __func__, (unsigned long)reg);
> +        break;
> +    case CPLT_STAT0:
> +        val = perv->control_regs.cplt_stat0;
> +        break;
> +    case CPLT_MASK0:
> +        val = perv->control_regs.cplt_mask0;
> +        break;
> +    case CPLT_PROTECT_MODE:
> +        val = perv->control_regs.ctrl_protect_mode;
> +        break;
> +    case CPLT_ATOMIC_CLOCK:
> +        val = perv->control_regs.ctrl_atomic_lock;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Chiplet_control_regs: Invalid xscom "
> +                 "read at 0x%" PRIx64 "\n", __func__, (unsigned long)reg);
> +    }
> +    return val;
> +}
> +
> +static void pnv_chiplet_ctrl_write(void *opaque, hwaddr addr,
> +                                 uint64_t val, unsigned size)
> +{
> +    PnvPerv *perv = PNV_PERV(opaque);
> +    int reg = addr >> 3;
> +
> +    /* CPLT_CTRL0 to CPLT_CTRL5 */
> +    for (int i = 0; i < CPLT_CTRL_SIZE; i++) {
> +        if (reg == i) {
> +            perv->control_regs.cplt_ctrl[i] = val;
> +            return;
> +        } else if (reg == (i + 0x10)) {
> +            perv->control_regs.cplt_ctrl[i] |= val;
> +            return;
> +        } else if (reg == (i + 0x20)) {
> +            perv->control_regs.cplt_ctrl[i] &= ~val;
> +            return;
> +        }
> +    }
> +
> +    switch (reg) {
> +    case CPLT_CONF0:
> +        perv->control_regs.cplt_cfg0 = val;
> +        break;
> +    case CPLT_CONF0_OR:
> +        perv->control_regs.cplt_cfg0 |= val;
> +        break;
> +    case CPLT_CONF0_CLEAR:
> +        perv->control_regs.cplt_cfg0 &= ~val;
> +        break;
> +    case CPLT_CONF1:
> +        perv->control_regs.cplt_cfg1 = val;
> +        break;
> +    case CPLT_CONF1_OR:
> +        perv->control_regs.cplt_cfg1 |= val;
> +        break;
> +    case CPLT_CONF1_CLEAR:
> +        perv->control_regs.cplt_cfg1 &= ~val;
> +        break;
> +    case CPLT_STAT0:
> +        perv->control_regs.cplt_stat0 = val;
> +        break;
> +    case CPLT_MASK0:
> +        perv->control_regs.cplt_mask0 = val;
> +        break;
> +    case CPLT_PROTECT_MODE:
> +        perv->control_regs.ctrl_protect_mode = val;
> +        break;
> +    case CPLT_ATOMIC_CLOCK:
> +        perv->control_regs.ctrl_atomic_lock = val;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Chiplet_control_regs: Invalid xscom "
> +                                 "write at 0x%" PRIx64 "\n",
> +                                 __func__, (unsigned long)reg);
> +    }
> +}
> +
> +static const MemoryRegionOps pnv_perv_control_xscom_ops = {
> +    .read = pnv_chiplet_ctrl_read,
> +    .write = pnv_chiplet_ctrl_write,
> +    .valid.min_access_size = 8,
> +    .valid.max_access_size = 8,
> +    .impl.min_access_size = 8,
> +    .impl.max_access_size = 8,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void pnv_perv_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvPerv *perv = PNV_PERV(dev);
> +    g_autofree char *region_name = NULL;
> +    region_name = g_strdup_printf("xscom-%s-control-regs",
> +                                   perv->parent_obj_name);
> +
> +    /* Chiplet control scoms */
> +    pnv_xscom_region_init(&perv->xscom_perv_ctrl_regs, OBJECT(perv),
> +                          &pnv_perv_control_xscom_ops, perv, region_name,
> +                          PNV10_XSCOM_CTRL_CHIPLET_SIZE);
> +}
> +
> +void pnv_perv_dt(PnvPerv *perv, uint32_t base_addr, void *fdt, int offset)
> +{
> +    g_autofree char *name = NULL;
> +    int perv_offset;
> +    const char compat[] = "ibm,power10-perv-chiplet";

Should we provide this dt node?

powernv boot environment is basically the end of hostboot boot but
with device-tree instead of HDAT.

The way OPAL (skiboot) works when booting hostboot is to first
convert HDAT into device-tree. When booting QEMU it just uses QEMU
device-tree directly.

So adding new dt nodes is a bit strange. Normally if skiboot needs
something new, it will extend the HDAT converter to find that
thing and create a new device-tree entry from it. Then the QEMU
model would add that same dt entry.

So it's really skiboot that defines the device-tree IMO, and QEMU
should just follow it. Unless there is good reason to do something
else.

Thanks,
Nick

> +    uint32_t reg[] = {
> +        cpu_to_be32(base_addr),
> +        cpu_to_be32(PNV10_XSCOM_CTRL_CHIPLET_SIZE)
> +    };
> +
> +    name = g_strdup_printf("%s-perv@%x", perv->parent_obj_name, base_addr);
> +    perv_offset = fdt_add_subnode(fdt, offset, name);
> +    _FDT(perv_offset);
> +
> +    _FDT(fdt_setprop(fdt, perv_offset, "reg", reg, sizeof(reg)));
> +    _FDT(fdt_setprop(fdt, perv_offset, "compatible", compat, sizeof(compat)));
> +}
> +
> +static Property pnv_perv_properties[] = {
> +    DEFINE_PROP_STRING("parent-obj-name", PnvPerv, parent_obj_name),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pnv_perv_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->desc = "PowerNV perv chiplet";
> +    dc->realize = pnv_perv_realize;
> +    device_class_set_props(dc, pnv_perv_properties);
> +}
> +
> +static const TypeInfo pnv_perv_info = {
> +    .name          = TYPE_PNV_PERV,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(PnvPerv),
> +    .class_init    = pnv_perv_class_init,
> +    .interfaces    = (InterfaceInfo[]) {
> +        { TYPE_PNV_XSCOM_INTERFACE },
> +        { }
> +    }
> +};
> +
> +static void pnv_perv_register_types(void)
> +{
> +    type_register_static(&pnv_perv_info);
> +}
> +
> +type_init(pnv_perv_register_types);
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index ea44856d43..37a7a8935d 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -51,6 +51,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
>    'pnv_bmc.c',
>    'pnv_homer.c',
>    'pnv_pnor.c',
> +  'pnv_pervasive.c',
>  ))
>  # PowerPC 4xx boards
>  ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(

Re: [PATCH v5 1/3] hw/ppc: Add pnv pervasive common chiplet units

[Chalapathi V]

On 24-11-2023 16:42, Nicholas Piggin wrote:
> On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
>> This part of the patchset creates a common pervasive chiplet model where it
>> houses the common units of a chiplets.
>>
>> The chiplet control unit is common across chiplets and this commit implements
>> the pervasive chiplet model with chiplet control registers.
> This might be reworded to be a bit clearer, could provide a bit of
> background information too (in changelog or header comment):
>
>   Status, configuration, and control of units in POWER chips is provided
>   by the pervasive subsystem, which connects registers to the SCOM bus,
>   which can be programmed by processor cores, other units on the chip,
>   BMCs, or other POWER chips.
>
>   A POWER10 chip is divided into logical pieces called chiplets. Chiplets
>   are broadly divided into "core chiplets" (with the processor cores) and
>   "nest chiplets" (with everything else). Each chiplet has an attachment
>   to the pervasive bus (PIB) and with chiplet-specific registers. All nest
>   chiplets have a common basic set of registers, which this model
>   provides. They may also implement additional registers.
>
> That's my undertsanding, does it look right? If yes and this file is
> specifically for nest chiplets (not core), maybe the name should
> reflect that?

Sure Will change the name to  PnvNestChipletPervasive and add some more 
details to header comment.

Thank You,

Chalapathi

>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   include/hw/ppc/pnv_pervasive.h |  37 ++++++
>>   include/hw/ppc/pnv_xscom.h     |   3 +
>>   hw/ppc/pnv_pervasive.c         | 217 +++++++++++++++++++++++++++++++++
>>   hw/ppc/meson.build             |   1 +
>>   4 files changed, 258 insertions(+)
>>   create mode 100644 include/hw/ppc/pnv_pervasive.h
>>   create mode 100644 hw/ppc/pnv_pervasive.c
>>
>> diff --git a/include/hw/ppc/pnv_pervasive.h b/include/hw/ppc/pnv_pervasive.h
>> new file mode 100644
>> index 0000000000..d83f86df7b
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv_pervasive.h
>> @@ -0,0 +1,37 @@
>> +/*
>> + * QEMU PowerPC pervasive common chiplet model
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef PPC_PNV_PERVASIVE_H
>> +#define PPC_PNV_PERVASIVE_H
>> +
>> +#define TYPE_PNV_PERV "pnv-pervasive-chiplet"
>> +#define PNV_PERV(obj) OBJECT_CHECK(PnvPerv, (obj), TYPE_PNV_PERV)
>> +
>> +typedef struct PnvPervCtrlRegs {
>> +#define CPLT_CTRL_SIZE 6
>> +    uint64_t cplt_ctrl[CPLT_CTRL_SIZE];
>> +    uint64_t cplt_cfg0;
>> +    uint64_t cplt_cfg1;
>> +    uint64_t cplt_stat0;
>> +    uint64_t cplt_mask0;
>> +    uint64_t ctrl_protect_mode;
>> +    uint64_t ctrl_atomic_lock;
>> +} PnvPervCtrlRegs;
>> +
>> +typedef struct PnvPerv {
> I don't want to bikeshed the name too much, but we have perv and
> pervasive, I'd prefer to use pervasive consistently.
>
> I would like the name to reflect that it is for common nest chiplet
> registers too, but maybe that's getting too ambitious...
>
> PnvNestChipletCommonRegs
> PnvNestChipletPervasive
>
> ?
Sure Will modify to PnvNestChipletPervasive.
>
>> +    DeviceState parent;
>> +    char *parent_obj_name;
>> +    MemoryRegion xscom_perv_ctrl_regs;
>> +    PnvPervCtrlRegs control_regs;
>> +} PnvPerv;
>> +
>> +void pnv_perv_dt(PnvPerv *perv, uint32_t base_addr, void *fdt, int offset);
>> +#endif /*PPC_PNV_PERVASIVE_H */
>> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
>> index f5becbab41..d09d10f32b 100644
>> --- a/include/hw/ppc/pnv_xscom.h
>> +++ b/include/hw/ppc/pnv_xscom.h
>> @@ -170,6 +170,9 @@ struct PnvXScomInterfaceClass {
>>   #define PNV10_XSCOM_XIVE2_BASE     0x2010800
>>   #define PNV10_XSCOM_XIVE2_SIZE     0x400
>>   
>> +#define PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE      0x3000000
>> +#define PNV10_XSCOM_CTRL_CHIPLET_SIZE            0x400
>> +
>>   #define PNV10_XSCOM_PEC_NEST_BASE  0x3011800 /* index goes downwards ... */
>>   #define PNV10_XSCOM_PEC_NEST_SIZE  0x100
>>   
>> diff --git a/hw/ppc/pnv_pervasive.c b/hw/ppc/pnv_pervasive.c
>> new file mode 100644
>> index 0000000000..c925070798
>> --- /dev/null
>> +++ b/hw/ppc/pnv_pervasive.c
>> @@ -0,0 +1,217 @@
>> +/*
>> + * QEMU PowerPC pervasive common chiplet model
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/ppc/pnv.h"
>> +#include "hw/ppc/pnv_xscom.h"
>> +#include "hw/ppc/pnv_pervasive.h"
>> +#include "hw/ppc/fdt.h"
>> +#include <libfdt.h>
>> +
>> +#define CPLT_CONF0               0x08
>> +#define CPLT_CONF0_OR            0x18
>> +#define CPLT_CONF0_CLEAR         0x28
>> +#define CPLT_CONF1               0x09
>> +#define CPLT_CONF1_OR            0x19
>> +#define CPLT_CONF1_CLEAR         0x29
>> +#define CPLT_STAT0               0x100
>> +#define CPLT_MASK0               0x101
>> +#define CPLT_PROTECT_MODE        0x3FE
>> +#define CPLT_ATOMIC_CLOCK        0x3FF
>> +
>> +static uint64_t pnv_chiplet_ctrl_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    PnvPerv *perv = PNV_PERV(opaque);
>> +    int reg = addr >> 3;
>> +    uint64_t val = ~0ull;
>> +
>> +    /* CPLT_CTRL0 to CPLT_CTRL5 */
>> +    for (int i = 0; i < CPLT_CTRL_SIZE; i++) {
>> +        if (reg == i) {
>> +            return perv->control_regs.cplt_ctrl[i];
>> +        } else if ((reg == (i + 0x10)) || (reg == (i + 0x20))) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
>> +                                           "xscom read at 0x%" PRIx64 "\n",
>> +                                           __func__, (unsigned long)reg);
>> +            return val;
>> +        }
>> +    }
>> +
>> +    switch (reg) {
>> +    case CPLT_CONF0:
>> +        val = perv->control_regs.cplt_cfg0;
>> +        break;
>> +    case CPLT_CONF0_OR:
>> +    case CPLT_CONF0_CLEAR:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
>> +                                   "xscom read at 0x%" PRIx64 "\n",
>> +                                   __func__, (unsigned long)reg);
>> +        break;
>> +    case CPLT_CONF1:
>> +        val = perv->control_regs.cplt_cfg1;
>> +        break;
>> +    case CPLT_CONF1_OR:
>> +    case CPLT_CONF1_CLEAR:
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Write only register, ignoring "
>> +                                   "xscom read at 0x%" PRIx64 "\n",
>> +                                   __func__, (unsigned long)reg);
>> +        break;
>> +    case CPLT_STAT0:
>> +        val = perv->control_regs.cplt_stat0;
>> +        break;
>> +    case CPLT_MASK0:
>> +        val = perv->control_regs.cplt_mask0;
>> +        break;
>> +    case CPLT_PROTECT_MODE:
>> +        val = perv->control_regs.ctrl_protect_mode;
>> +        break;
>> +    case CPLT_ATOMIC_CLOCK:
>> +        val = perv->control_regs.ctrl_atomic_lock;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Chiplet_control_regs: Invalid xscom "
>> +                 "read at 0x%" PRIx64 "\n", __func__, (unsigned long)reg);
>> +    }
>> +    return val;
>> +}
>> +
>> +static void pnv_chiplet_ctrl_write(void *opaque, hwaddr addr,
>> +                                 uint64_t val, unsigned size)
>> +{
>> +    PnvPerv *perv = PNV_PERV(opaque);
>> +    int reg = addr >> 3;
>> +
>> +    /* CPLT_CTRL0 to CPLT_CTRL5 */
>> +    for (int i = 0; i < CPLT_CTRL_SIZE; i++) {
>> +        if (reg == i) {
>> +            perv->control_regs.cplt_ctrl[i] = val;
>> +            return;
>> +        } else if (reg == (i + 0x10)) {
>> +            perv->control_regs.cplt_ctrl[i] |= val;
>> +            return;
>> +        } else if (reg == (i + 0x20)) {
>> +            perv->control_regs.cplt_ctrl[i] &= ~val;
>> +            return;
>> +        }
>> +    }
>> +
>> +    switch (reg) {
>> +    case CPLT_CONF0:
>> +        perv->control_regs.cplt_cfg0 = val;
>> +        break;
>> +    case CPLT_CONF0_OR:
>> +        perv->control_regs.cplt_cfg0 |= val;
>> +        break;
>> +    case CPLT_CONF0_CLEAR:
>> +        perv->control_regs.cplt_cfg0 &= ~val;
>> +        break;
>> +    case CPLT_CONF1:
>> +        perv->control_regs.cplt_cfg1 = val;
>> +        break;
>> +    case CPLT_CONF1_OR:
>> +        perv->control_regs.cplt_cfg1 |= val;
>> +        break;
>> +    case CPLT_CONF1_CLEAR:
>> +        perv->control_regs.cplt_cfg1 &= ~val;
>> +        break;
>> +    case CPLT_STAT0:
>> +        perv->control_regs.cplt_stat0 = val;
>> +        break;
>> +    case CPLT_MASK0:
>> +        perv->control_regs.cplt_mask0 = val;
>> +        break;
>> +    case CPLT_PROTECT_MODE:
>> +        perv->control_regs.ctrl_protect_mode = val;
>> +        break;
>> +    case CPLT_ATOMIC_CLOCK:
>> +        perv->control_regs.ctrl_atomic_lock = val;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Chiplet_control_regs: Invalid xscom "
>> +                                 "write at 0x%" PRIx64 "\n",
>> +                                 __func__, (unsigned long)reg);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps pnv_perv_control_xscom_ops = {
>> +    .read = pnv_chiplet_ctrl_read,
>> +    .write = pnv_chiplet_ctrl_write,
>> +    .valid.min_access_size = 8,
>> +    .valid.max_access_size = 8,
>> +    .impl.min_access_size = 8,
>> +    .impl.max_access_size = 8,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>> +static void pnv_perv_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PnvPerv *perv = PNV_PERV(dev);
>> +    g_autofree char *region_name = NULL;
>> +    region_name = g_strdup_printf("xscom-%s-control-regs",
>> +                                   perv->parent_obj_name);
>> +
>> +    /* Chiplet control scoms */
>> +    pnv_xscom_region_init(&perv->xscom_perv_ctrl_regs, OBJECT(perv),
>> +                          &pnv_perv_control_xscom_ops, perv, region_name,
>> +                          PNV10_XSCOM_CTRL_CHIPLET_SIZE);
>> +}
>> +
>> +void pnv_perv_dt(PnvPerv *perv, uint32_t base_addr, void *fdt, int offset)
>> +{
>> +    g_autofree char *name = NULL;
>> +    int perv_offset;
>> +    const char compat[] = "ibm,power10-perv-chiplet";
> Should we provide this dt node?
>
> powernv boot environment is basically the end of hostboot boot but
> with device-tree instead of HDAT.
>
> The way OPAL (skiboot) works when booting hostboot is to first
> convert HDAT into device-tree. When booting QEMU it just uses QEMU
> device-tree directly.
>
> So adding new dt nodes is a bit strange. Normally if skiboot needs
> something new, it will extend the HDAT converter to find that
> thing and create a new device-tree entry from it. Then the QEMU
> model would add that same dt entry.
>
> So it's really skiboot that defines the device-tree IMO, and QEMU
> should just follow it. Unless there is good reason to do something
> else.
>
> Thanks,
> Nick
Thank You for the clarification. Will remove this node.
>> +    uint32_t reg[] = {
>> +        cpu_to_be32(base_addr),
>> +        cpu_to_be32(PNV10_XSCOM_CTRL_CHIPLET_SIZE)
>> +    };
>> +
>> +    name = g_strdup_printf("%s-perv@%x", perv->parent_obj_name, base_addr);
>> +    perv_offset = fdt_add_subnode(fdt, offset, name);
>> +    _FDT(perv_offset);
>> +
>> +    _FDT(fdt_setprop(fdt, perv_offset, "reg", reg, sizeof(reg)));
>> +    _FDT(fdt_setprop(fdt, perv_offset, "compatible", compat, sizeof(compat)));
>> +}
>> +
>> +static Property pnv_perv_properties[] = {
>> +    DEFINE_PROP_STRING("parent-obj-name", PnvPerv, parent_obj_name),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pnv_perv_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->desc = "PowerNV perv chiplet";
>> +    dc->realize = pnv_perv_realize;
>> +    device_class_set_props(dc, pnv_perv_properties);
>> +}
>> +
>> +static const TypeInfo pnv_perv_info = {
>> +    .name          = TYPE_PNV_PERV,
>> +    .parent        = TYPE_DEVICE,
>> +    .instance_size = sizeof(PnvPerv),
>> +    .class_init    = pnv_perv_class_init,
>> +    .interfaces    = (InterfaceInfo[]) {
>> +        { TYPE_PNV_XSCOM_INTERFACE },
>> +        { }
>> +    }
>> +};
>> +
>> +static void pnv_perv_register_types(void)
>> +{
>> +    type_register_static(&pnv_perv_info);
>> +}
>> +
>> +type_init(pnv_perv_register_types);
>> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
>> index ea44856d43..37a7a8935d 100644
>> --- a/hw/ppc/meson.build
>> +++ b/hw/ppc/meson.build
>> @@ -51,6 +51,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
>>     'pnv_bmc.c',
>>     'pnv_homer.c',
>>     'pnv_pnor.c',
>> +  'pnv_pervasive.c',
>>   ))
>>   # PowerPC 4xx boards
>>   ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(

[PATCH v5 2/3] hw/ppc: Add nest1 chiplet model

[Chalapathi V]

The nest1 chiplet handle the high speed i/o traffic over PCIe and others.
The nest1 chiplet consists of PowerBus Fabric controller,
nest Memory Management Unit, chiplet control unit and more.

This commit creates a nest1 chiplet model and initialize and realize the
pervasive chiplet model where chiplet control registers are implemented.

This commit also implement the read/write method for the powerbus scom
registers

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 include/hw/ppc/pnv_nest_chiplet.h |  36 ++++++
 include/hw/ppc/pnv_xscom.h        |   6 +
 hw/ppc/pnv_nest1_chiplet.c        | 197 ++++++++++++++++++++++++++++++
 hw/ppc/meson.build                |   1 +
 4 files changed, 240 insertions(+)
 create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
 create mode 100644 hw/ppc/pnv_nest1_chiplet.c

diff --git a/include/hw/ppc/pnv_nest_chiplet.h b/include/hw/ppc/pnv_nest_chiplet.h
new file mode 100644
index 0000000000..845030fb1a
--- /dev/null
+++ b/include/hw/ppc/pnv_nest_chiplet.h
@@ -0,0 +1,36 @@
+/*
+ * QEMU PowerPC nest chiplet model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef PPC_PNV_NEST1_CHIPLET_H
+#define PPC_PNV_NEST1_CHIPLET_H
+
+#include "hw/ppc/pnv_pervasive.h"
+
+#define TYPE_PNV_NEST1 "pnv-nest1-chiplet"
+#define PNV_NEST1(obj) OBJECT_CHECK(PnvNest1, (obj), TYPE_PNV_NEST1)
+
+typedef struct pb_scom {
+    uint64_t mode;
+    uint64_t hp_mode2_curr;
+} pb_scom;
+
+typedef struct PnvNest1 {
+    DeviceState parent;
+    MemoryRegion xscom_pb_eq_regs;
+    MemoryRegion xscom_pb_es_regs;
+    /* common pervasive chiplet unit */
+    PnvPerv perv;
+    /* powerbus racetrack registers */
+    pb_scom eq[8];
+    pb_scom es[4];
+} PnvNest1;
+#endif /*PPC_PNV_NEST1 */
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index d09d10f32b..df68a1c20e 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -173,6 +173,12 @@ struct PnvXScomInterfaceClass {
 #define PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE      0x3000000
 #define PNV10_XSCOM_CTRL_CHIPLET_SIZE            0x400
 
+#define PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE      0x3011000
+#define PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE      0x200
+
+#define PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE      0x3011300
+#define PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE      0x100
+
 #define PNV10_XSCOM_PEC_NEST_BASE  0x3011800 /* index goes downwards ... */
 #define PNV10_XSCOM_PEC_NEST_SIZE  0x100
 
diff --git a/hw/ppc/pnv_nest1_chiplet.c b/hw/ppc/pnv_nest1_chiplet.c
new file mode 100644
index 0000000000..609d5f1be4
--- /dev/null
+++ b/hw/ppc/pnv_nest1_chiplet.c
@@ -0,0 +1,197 @@
+/*
+ * QEMU PowerPC nest1 chiplet model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This code is licensed under the GPL version 2 or later. See the
+ * COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/qdev-properties.h"
+#include "hw/ppc/pnv.h"
+#include "hw/ppc/pnv_xscom.h"
+#include "hw/ppc/pnv_nest_chiplet.h"
+#include "hw/ppc/pnv_pervasive.h"
+#include "hw/ppc/fdt.h"
+#include <libfdt.h>
+
+/*
+ * The nest1 chiplet contains chiplet control unit,
+ * PowerBus/RaceTrack/Bridge logic, nest Memory Management Unit(nMMU)
+ * and more.
+ */
+
+#define PB_SCOM_EQ0_HP_MODE2_CURR      0xe
+#define PB_SCOM_ES3_MODE               0x8a
+
+static uint64_t pnv_nest1_pb_scom_eq_read(void *opaque, hwaddr addr,
+                                                  unsigned size)
+{
+    PnvNest1 *nest1 = PNV_NEST1(opaque);
+    int reg = addr >> 3;
+    uint64_t val = ~0ull;
+
+    switch (reg) {
+    case PB_SCOM_EQ0_HP_MODE2_CURR:
+        val = nest1->eq[0].hp_mode2_curr;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom read at 0x%" PRIx32 "\n",
+                      __func__, reg);
+    }
+    return val;
+}
+
+static void pnv_nest1_pb_scom_eq_write(void *opaque, hwaddr addr,
+                                               uint64_t val, unsigned size)
+{
+    PnvNest1 *nest1 = PNV_NEST1(opaque);
+    int reg = addr >> 3;
+
+    switch (reg) {
+    case PB_SCOM_EQ0_HP_MODE2_CURR:
+        nest1->eq[0].hp_mode2_curr = val;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom write at 0x%" PRIx32 "\n",
+                      __func__, reg);
+    }
+}
+
+static const MemoryRegionOps pnv_nest1_pb_scom_eq_ops = {
+    .read = pnv_nest1_pb_scom_eq_read,
+    .write = pnv_nest1_pb_scom_eq_write,
+    .valid.min_access_size = 8,
+    .valid.max_access_size = 8,
+    .impl.min_access_size = 8,
+    .impl.max_access_size = 8,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static uint64_t pnv_nest1_pb_scom_es_read(void *opaque, hwaddr addr,
+                                          unsigned size)
+{
+    PnvNest1 *nest1 = PNV_NEST1(opaque);
+    int reg = addr >> 3;
+    uint64_t val = ~0ull;
+
+    switch (reg) {
+    case PB_SCOM_ES3_MODE:
+        val = nest1->es[3].mode;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom read at 0x%" PRIx32 "\n",
+                      __func__, reg);
+    }
+    return val;
+}
+
+static void pnv_nest1_pb_scom_es_write(void *opaque, hwaddr addr,
+                                               uint64_t val, unsigned size)
+{
+    PnvNest1 *nest1 = PNV_NEST1(opaque);
+    int reg = addr >> 3;
+
+    switch (reg) {
+    case PB_SCOM_ES3_MODE:
+        nest1->es[3].mode = val;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom write at 0x%" PRIx32 "\n",
+                      __func__, reg);
+    }
+}
+
+static const MemoryRegionOps pnv_nest1_pb_scom_es_ops = {
+    .read = pnv_nest1_pb_scom_es_read,
+    .write = pnv_nest1_pb_scom_es_write,
+    .valid.min_access_size = 8,
+    .valid.max_access_size = 8,
+    .impl.min_access_size = 8,
+    .impl.max_access_size = 8,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void pnv_nest1_realize(DeviceState *dev, Error **errp)
+{
+    PnvNest1 *nest1 = PNV_NEST1(dev);
+
+    /* perv chiplet initialize and realize */
+    object_initialize_child(OBJECT(nest1), "perv", &nest1->perv, TYPE_PNV_PERV);
+    object_property_set_str(OBJECT(&nest1->perv), "parent-obj-name", "nest1",
+                                   errp);
+    if (!qdev_realize(DEVICE(&nest1->perv), NULL, errp)) {
+        return;
+    }
+
+    /* Nest1 chiplet power bus EQ xscom region */
+    pnv_xscom_region_init(&nest1->xscom_pb_eq_regs, OBJECT(nest1),
+                          &pnv_nest1_pb_scom_eq_ops, nest1,
+                          "xscom-nest1-pb-scom-eq-regs",
+                          PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE);
+
+    /* Nest1 chiplet power bus ES xscom region */
+    pnv_xscom_region_init(&nest1->xscom_pb_es_regs, OBJECT(nest1),
+                          &pnv_nest1_pb_scom_es_ops, nest1,
+                          "xscom-nest1-pb-scom-es-regs",
+                          PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE);
+}
+
+static int pnv_nest1_dt_xscom(PnvXScomInterface *dev, void *fdt,
+                             int offset)
+{
+    PnvNest1 *nest1 = PNV_NEST1(dev);
+    g_autofree char *name = NULL;
+    int nest1_offset = 0;
+    const char compat[] = "ibm,power10-nest1-chiplet";
+    uint32_t reg[] = {
+        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE),
+        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE),
+        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE),
+        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE)
+    };
+
+    /* populate perv_chiplet control_regs */
+    pnv_perv_dt(&nest1->perv, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE, fdt, offset);
+
+    name = g_strdup_printf("nest1@%x", PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE);
+    nest1_offset = fdt_add_subnode(fdt, offset, name);
+    _FDT(nest1_offset);
+
+    _FDT(fdt_setprop(fdt, nest1_offset, "reg", reg, sizeof(reg)));
+    _FDT(fdt_setprop(fdt, nest1_offset, "compatible", compat, sizeof(compat)));
+    return 0;
+}
+
+static void pnv_nest1_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PnvXScomInterfaceClass *xscomc = PNV_XSCOM_INTERFACE_CLASS(klass);
+
+    xscomc->dt_xscom = pnv_nest1_dt_xscom;
+
+    dc->desc = "PowerNV nest1 chiplet";
+    dc->realize = pnv_nest1_realize;
+}
+
+static const TypeInfo pnv_nest1_info = {
+    .name          = TYPE_PNV_NEST1,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(PnvNest1),
+    .class_init    = pnv_nest1_class_init,
+    .interfaces    = (InterfaceInfo[]) {
+        { TYPE_PNV_XSCOM_INTERFACE },
+        { }
+    }
+};
+
+static void pnv_nest1_register_types(void)
+{
+    type_register_static(&pnv_nest1_info);
+}
+
+type_init(pnv_nest1_register_types);
diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
index 37a7a8935d..7b8b87596a 100644
--- a/hw/ppc/meson.build
+++ b/hw/ppc/meson.build
@@ -52,6 +52,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
   'pnv_homer.c',
   'pnv_pnor.c',
   'pnv_pervasive.c',
+  'pnv_nest1_chiplet.c',
 ))
 # PowerPC 4xx boards
 ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(
-- 
2.31.1

Re: [PATCH v5 2/3] hw/ppc: Add nest1 chiplet model

[Nicholas Piggin]

For this and actually the last patch too, it would be good to mention
(possibly in a header comment in the file too) what actual functionality
is being provided/modeled. It looks like it's just modeling behaviour of
reads and writes for some registers.

Oh, and sorry I didn't follow development and comments on this too
closely, so forgive me if I've missed things already said. I'll go
back and read through the series.

On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
> The nest1 chiplet handle the high speed i/o traffic over PCIe and others.
> The nest1 chiplet consists of PowerBus Fabric controller,
> nest Memory Management Unit, chiplet control unit and more.
>
> This commit creates a nest1 chiplet model and initialize and realize the
> pervasive chiplet model where chiplet control registers are implemented.
>
> This commit also implement the read/write method for the powerbus scom
> registers

The powerbus scom registers, are those specifically for the PowerBus
Fabric controller mentioned in the first paragraph, or is it a more
general set of registers for the chiplet?

>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  include/hw/ppc/pnv_nest_chiplet.h |  36 ++++++
>  include/hw/ppc/pnv_xscom.h        |   6 +
>  hw/ppc/pnv_nest1_chiplet.c        | 197 ++++++++++++++++++++++++++++++
>  hw/ppc/meson.build                |   1 +
>  4 files changed, 240 insertions(+)
>  create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
>  create mode 100644 hw/ppc/pnv_nest1_chiplet.c
>
> diff --git a/include/hw/ppc/pnv_nest_chiplet.h b/include/hw/ppc/pnv_nest_chiplet.h
> new file mode 100644
> index 0000000000..845030fb1a
> --- /dev/null
> +++ b/include/hw/ppc/pnv_nest_chiplet.h
> @@ -0,0 +1,36 @@
> +/*
> + * QEMU PowerPC nest chiplet model
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef PPC_PNV_NEST1_CHIPLET_H
> +#define PPC_PNV_NEST1_CHIPLET_H
> +
> +#include "hw/ppc/pnv_pervasive.h"
> +
> +#define TYPE_PNV_NEST1 "pnv-nest1-chiplet"
> +#define PNV_NEST1(obj) OBJECT_CHECK(PnvNest1, (obj), TYPE_PNV_NEST1)
> +
> +typedef struct pb_scom {
> +    uint64_t mode;
> +    uint64_t hp_mode2_curr;
> +} pb_scom;
> +
> +typedef struct PnvNest1 {

Naming nitpicking again... 

The main ifndef guard for header files should match the file name, so
the file should be called pnv_nest1_chiplet.h (and that matches the .c
file too).

I think this struct should be called Nest1Chiplet too. Nest1 is
unambiguously the name of a specific chiplet, but it's a little easy
to miss the 1, and if we get a bunch more chiplets I think it will
be nicer to have Chiplet in the name.

Thanks,
Nick

> +    DeviceState parent;
> +    MemoryRegion xscom_pb_eq_regs;
> +    MemoryRegion xscom_pb_es_regs;
> +    /* common pervasive chiplet unit */
> +    PnvPerv perv;
> +    /* powerbus racetrack registers */
> +    pb_scom eq[8];
> +    pb_scom es[4];
> +} PnvNest1;
> +#endif /*PPC_PNV_NEST1 */
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> index d09d10f32b..df68a1c20e 100644
> --- a/include/hw/ppc/pnv_xscom.h
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -173,6 +173,12 @@ struct PnvXScomInterfaceClass {
>  #define PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE      0x3000000
>  #define PNV10_XSCOM_CTRL_CHIPLET_SIZE            0x400
>  
> +#define PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE      0x3011000
> +#define PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE      0x200
> +
> +#define PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE      0x3011300
> +#define PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE      0x100
> +
>  #define PNV10_XSCOM_PEC_NEST_BASE  0x3011800 /* index goes downwards ... */
>  #define PNV10_XSCOM_PEC_NEST_SIZE  0x100
>  
> diff --git a/hw/ppc/pnv_nest1_chiplet.c b/hw/ppc/pnv_nest1_chiplet.c
> new file mode 100644
> index 0000000000..609d5f1be4
> --- /dev/null
> +++ b/hw/ppc/pnv_nest1_chiplet.c
> @@ -0,0 +1,197 @@
> +/*
> + * QEMU PowerPC nest1 chiplet model
> + *
> + * Copyright (c) 2023, IBM Corporation.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This code is licensed under the GPL version 2 or later. See the
> + * COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/ppc/pnv.h"
> +#include "hw/ppc/pnv_xscom.h"
> +#include "hw/ppc/pnv_nest_chiplet.h"
> +#include "hw/ppc/pnv_pervasive.h"
> +#include "hw/ppc/fdt.h"
> +#include <libfdt.h>
> +
> +/*
> + * The nest1 chiplet contains chiplet control unit,
> + * PowerBus/RaceTrack/Bridge logic, nest Memory Management Unit(nMMU)
> + * and more.
> + */
> +
> +#define PB_SCOM_EQ0_HP_MODE2_CURR      0xe
> +#define PB_SCOM_ES3_MODE               0x8a
> +
> +static uint64_t pnv_nest1_pb_scom_eq_read(void *opaque, hwaddr addr,
> +                                                  unsigned size)
> +{
> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
> +    int reg = addr >> 3;
> +    uint64_t val = ~0ull;
> +
> +    switch (reg) {
> +    case PB_SCOM_EQ0_HP_MODE2_CURR:
> +        val = nest1->eq[0].hp_mode2_curr;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom read at 0x%" PRIx32 "\n",
> +                      __func__, reg);
> +    }
> +    return val;
> +}
> +
> +static void pnv_nest1_pb_scom_eq_write(void *opaque, hwaddr addr,
> +                                               uint64_t val, unsigned size)
> +{
> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
> +    int reg = addr >> 3;
> +
> +    switch (reg) {
> +    case PB_SCOM_EQ0_HP_MODE2_CURR:
> +        nest1->eq[0].hp_mode2_curr = val;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom write at 0x%" PRIx32 "\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static const MemoryRegionOps pnv_nest1_pb_scom_eq_ops = {
> +    .read = pnv_nest1_pb_scom_eq_read,
> +    .write = pnv_nest1_pb_scom_eq_write,
> +    .valid.min_access_size = 8,
> +    .valid.max_access_size = 8,
> +    .impl.min_access_size = 8,
> +    .impl.max_access_size = 8,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static uint64_t pnv_nest1_pb_scom_es_read(void *opaque, hwaddr addr,
> +                                          unsigned size)
> +{
> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
> +    int reg = addr >> 3;
> +    uint64_t val = ~0ull;
> +
> +    switch (reg) {
> +    case PB_SCOM_ES3_MODE:
> +        val = nest1->es[3].mode;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom read at 0x%" PRIx32 "\n",
> +                      __func__, reg);
> +    }
> +    return val;
> +}
> +
> +static void pnv_nest1_pb_scom_es_write(void *opaque, hwaddr addr,
> +                                               uint64_t val, unsigned size)
> +{
> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
> +    int reg = addr >> 3;
> +
> +    switch (reg) {
> +    case PB_SCOM_ES3_MODE:
> +        nest1->es[3].mode = val;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom write at 0x%" PRIx32 "\n",
> +                      __func__, reg);
> +    }
> +}
> +
> +static const MemoryRegionOps pnv_nest1_pb_scom_es_ops = {
> +    .read = pnv_nest1_pb_scom_es_read,
> +    .write = pnv_nest1_pb_scom_es_write,
> +    .valid.min_access_size = 8,
> +    .valid.max_access_size = 8,
> +    .impl.min_access_size = 8,
> +    .impl.max_access_size = 8,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +};
> +
> +static void pnv_nest1_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvNest1 *nest1 = PNV_NEST1(dev);
> +
> +    /* perv chiplet initialize and realize */
> +    object_initialize_child(OBJECT(nest1), "perv", &nest1->perv, TYPE_PNV_PERV);
> +    object_property_set_str(OBJECT(&nest1->perv), "parent-obj-name", "nest1",
> +                                   errp);
> +    if (!qdev_realize(DEVICE(&nest1->perv), NULL, errp)) {
> +        return;
> +    }
> +
> +    /* Nest1 chiplet power bus EQ xscom region */
> +    pnv_xscom_region_init(&nest1->xscom_pb_eq_regs, OBJECT(nest1),
> +                          &pnv_nest1_pb_scom_eq_ops, nest1,
> +                          "xscom-nest1-pb-scom-eq-regs",
> +                          PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE);
> +
> +    /* Nest1 chiplet power bus ES xscom region */
> +    pnv_xscom_region_init(&nest1->xscom_pb_es_regs, OBJECT(nest1),
> +                          &pnv_nest1_pb_scom_es_ops, nest1,
> +                          "xscom-nest1-pb-scom-es-regs",
> +                          PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE);
> +}
> +
> +static int pnv_nest1_dt_xscom(PnvXScomInterface *dev, void *fdt,
> +                             int offset)
> +{
> +    PnvNest1 *nest1 = PNV_NEST1(dev);
> +    g_autofree char *name = NULL;
> +    int nest1_offset = 0;
> +    const char compat[] = "ibm,power10-nest1-chiplet";
> +    uint32_t reg[] = {
> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE),
> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE),
> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE),
> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE)
> +    };
> +
> +    /* populate perv_chiplet control_regs */
> +    pnv_perv_dt(&nest1->perv, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE, fdt, offset);
> +
> +    name = g_strdup_printf("nest1@%x", PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE);
> +    nest1_offset = fdt_add_subnode(fdt, offset, name);
> +    _FDT(nest1_offset);
> +
> +    _FDT(fdt_setprop(fdt, nest1_offset, "reg", reg, sizeof(reg)));
> +    _FDT(fdt_setprop(fdt, nest1_offset, "compatible", compat, sizeof(compat)));
> +    return 0;
> +}
> +
> +static void pnv_nest1_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PnvXScomInterfaceClass *xscomc = PNV_XSCOM_INTERFACE_CLASS(klass);
> +
> +    xscomc->dt_xscom = pnv_nest1_dt_xscom;
> +
> +    dc->desc = "PowerNV nest1 chiplet";
> +    dc->realize = pnv_nest1_realize;
> +}
> +
> +static const TypeInfo pnv_nest1_info = {
> +    .name          = TYPE_PNV_NEST1,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(PnvNest1),
> +    .class_init    = pnv_nest1_class_init,
> +    .interfaces    = (InterfaceInfo[]) {
> +        { TYPE_PNV_XSCOM_INTERFACE },
> +        { }
> +    }
> +};
> +
> +static void pnv_nest1_register_types(void)
> +{
> +    type_register_static(&pnv_nest1_info);
> +}
> +
> +type_init(pnv_nest1_register_types);
> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
> index 37a7a8935d..7b8b87596a 100644
> --- a/hw/ppc/meson.build
> +++ b/hw/ppc/meson.build
> @@ -52,6 +52,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
>    'pnv_homer.c',
>    'pnv_pnor.c',
>    'pnv_pervasive.c',
> +  'pnv_nest1_chiplet.c',
>  ))
>  # PowerPC 4xx boards
>  ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(

Re: [PATCH v5 2/3] hw/ppc: Add nest1 chiplet model

[Cédric Le Goater]

On 11/24/23 12:26, Nicholas Piggin wrote:
> For this and actually the last patch too, it would be good to mention
> (possibly in a header comment in the file too) what actual functionality
> is being provided/modeled. It looks like it's just modeling behaviour of
> reads and writes for some registers.
> 
> Oh, and sorry I didn't follow development and comments on this too
> closely, so forgive me if I've missed things already said. I'll go
> back and read through the series.
> 
> On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
>> The nest1 chiplet handle the high speed i/o traffic over PCIe and others.
>> The nest1 chiplet consists of PowerBus Fabric controller,
>> nest Memory Management Unit, chiplet control unit and more.
>>
>> This commit creates a nest1 chiplet model and initialize and realize the
>> pervasive chiplet model where chiplet control registers are implemented.
>>
>> This commit also implement the read/write method for the powerbus scom
>> registers
> 
> The powerbus scom registers, are those specifically for the PowerBus
> Fabric controller mentioned in the first paragraph, or is it a more
> general set of registers for the chiplet?
> 
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   include/hw/ppc/pnv_nest_chiplet.h |  36 ++++++
>>   include/hw/ppc/pnv_xscom.h        |   6 +
>>   hw/ppc/pnv_nest1_chiplet.c        | 197 ++++++++++++++++++++++++++++++
>>   hw/ppc/meson.build                |   1 +
>>   4 files changed, 240 insertions(+)
>>   create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
>>   create mode 100644 hw/ppc/pnv_nest1_chiplet.c
>>
>> diff --git a/include/hw/ppc/pnv_nest_chiplet.h b/include/hw/ppc/pnv_nest_chiplet.h
>> new file mode 100644
>> index 0000000000..845030fb1a
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv_nest_chiplet.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * QEMU PowerPC nest chiplet model
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef PPC_PNV_NEST1_CHIPLET_H
>> +#define PPC_PNV_NEST1_CHIPLET_H
>> +
>> +#include "hw/ppc/pnv_pervasive.h"
>> +
>> +#define TYPE_PNV_NEST1 "pnv-nest1-chiplet"
>> +#define PNV_NEST1(obj) OBJECT_CHECK(PnvNest1, (obj), TYPE_PNV_NEST1)
>> +
>> +typedef struct pb_scom {
>> +    uint64_t mode;
>> +    uint64_t hp_mode2_curr;
>> +} pb_scom;
>> +
>> +typedef struct PnvNest1 {
> 
> Naming nitpicking again...
> 
> The main ifndef guard for header files should match the file name, so
> the file should be called pnv_nest1_chiplet.h (and that matches the .c
> file too).
> 
> I think this struct should be called Nest1Chiplet too.

I asked Chalapathi to do the exact opposit :)

I don't mind really, my argument was that most models represent HW logic
units or subunits of a bigger unit. I don't see the point in adding a
chip/chiplet suffix apart from PnvChip since it represents a socket or
processor.

You choose. I will keep quiet :)

Thanks,

C.



> Nest1 is
> unambiguously the name of a specific chiplet, but it's a little easy
> to miss the 1, and if we get a bunch more chiplets I think it will
> be nicer to have Chiplet in the name.
> 
> Thanks,
> Nick
> 
>> +    DeviceState parent;
>> +    MemoryRegion xscom_pb_eq_regs;
>> +    MemoryRegion xscom_pb_es_regs;
>> +    /* common pervasive chiplet unit */
>> +    PnvPerv perv;
>> +    /* powerbus racetrack registers */
>> +    pb_scom eq[8];
>> +    pb_scom es[4];
>> +} PnvNest1;
>> +#endif /*PPC_PNV_NEST1 */
>> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
>> index d09d10f32b..df68a1c20e 100644
>> --- a/include/hw/ppc/pnv_xscom.h
>> +++ b/include/hw/ppc/pnv_xscom.h
>> @@ -173,6 +173,12 @@ struct PnvXScomInterfaceClass {
>>   #define PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE      0x3000000
>>   #define PNV10_XSCOM_CTRL_CHIPLET_SIZE            0x400
>>   
>> +#define PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE      0x3011000
>> +#define PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE      0x200
>> +
>> +#define PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE      0x3011300
>> +#define PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE      0x100
>> +
>>   #define PNV10_XSCOM_PEC_NEST_BASE  0x3011800 /* index goes downwards ... */
>>   #define PNV10_XSCOM_PEC_NEST_SIZE  0x100
>>   
>> diff --git a/hw/ppc/pnv_nest1_chiplet.c b/hw/ppc/pnv_nest1_chiplet.c
>> new file mode 100644
>> index 0000000000..609d5f1be4
>> --- /dev/null
>> +++ b/hw/ppc/pnv_nest1_chiplet.c
>> @@ -0,0 +1,197 @@
>> +/*
>> + * QEMU PowerPC nest1 chiplet model
>> + *
>> + * Copyright (c) 2023, IBM Corporation.
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/ppc/pnv.h"
>> +#include "hw/ppc/pnv_xscom.h"
>> +#include "hw/ppc/pnv_nest_chiplet.h"
>> +#include "hw/ppc/pnv_pervasive.h"
>> +#include "hw/ppc/fdt.h"
>> +#include <libfdt.h>
>> +
>> +/*
>> + * The nest1 chiplet contains chiplet control unit,
>> + * PowerBus/RaceTrack/Bridge logic, nest Memory Management Unit(nMMU)
>> + * and more.
>> + */
>> +
>> +#define PB_SCOM_EQ0_HP_MODE2_CURR      0xe
>> +#define PB_SCOM_ES3_MODE               0x8a
>> +
>> +static uint64_t pnv_nest1_pb_scom_eq_read(void *opaque, hwaddr addr,
>> +                                                  unsigned size)
>> +{
>> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
>> +    int reg = addr >> 3;
>> +    uint64_t val = ~0ull;
>> +
>> +    switch (reg) {
>> +    case PB_SCOM_EQ0_HP_MODE2_CURR:
>> +        val = nest1->eq[0].hp_mode2_curr;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom read at 0x%" PRIx32 "\n",
>> +                      __func__, reg);
>> +    }
>> +    return val;
>> +}
>> +
>> +static void pnv_nest1_pb_scom_eq_write(void *opaque, hwaddr addr,
>> +                                               uint64_t val, unsigned size)
>> +{
>> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
>> +    int reg = addr >> 3;
>> +
>> +    switch (reg) {
>> +    case PB_SCOM_EQ0_HP_MODE2_CURR:
>> +        nest1->eq[0].hp_mode2_curr = val;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom write at 0x%" PRIx32 "\n",
>> +                      __func__, reg);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps pnv_nest1_pb_scom_eq_ops = {
>> +    .read = pnv_nest1_pb_scom_eq_read,
>> +    .write = pnv_nest1_pb_scom_eq_write,
>> +    .valid.min_access_size = 8,
>> +    .valid.max_access_size = 8,
>> +    .impl.min_access_size = 8,
>> +    .impl.max_access_size = 8,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>> +static uint64_t pnv_nest1_pb_scom_es_read(void *opaque, hwaddr addr,
>> +                                          unsigned size)
>> +{
>> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
>> +    int reg = addr >> 3;
>> +    uint64_t val = ~0ull;
>> +
>> +    switch (reg) {
>> +    case PB_SCOM_ES3_MODE:
>> +        val = nest1->es[3].mode;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom read at 0x%" PRIx32 "\n",
>> +                      __func__, reg);
>> +    }
>> +    return val;
>> +}
>> +
>> +static void pnv_nest1_pb_scom_es_write(void *opaque, hwaddr addr,
>> +                                               uint64_t val, unsigned size)
>> +{
>> +    PnvNest1 *nest1 = PNV_NEST1(opaque);
>> +    int reg = addr >> 3;
>> +
>> +    switch (reg) {
>> +    case PB_SCOM_ES3_MODE:
>> +        nest1->es[3].mode = val;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Invalid xscom write at 0x%" PRIx32 "\n",
>> +                      __func__, reg);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps pnv_nest1_pb_scom_es_ops = {
>> +    .read = pnv_nest1_pb_scom_es_read,
>> +    .write = pnv_nest1_pb_scom_es_write,
>> +    .valid.min_access_size = 8,
>> +    .valid.max_access_size = 8,
>> +    .impl.min_access_size = 8,
>> +    .impl.max_access_size = 8,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>> +static void pnv_nest1_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PnvNest1 *nest1 = PNV_NEST1(dev);
>> +
>> +    /* perv chiplet initialize and realize */
>> +    object_initialize_child(OBJECT(nest1), "perv", &nest1->perv, TYPE_PNV_PERV);
>> +    object_property_set_str(OBJECT(&nest1->perv), "parent-obj-name", "nest1",
>> +                                   errp);
>> +    if (!qdev_realize(DEVICE(&nest1->perv), NULL, errp)) {
>> +        return;
>> +    }
>> +
>> +    /* Nest1 chiplet power bus EQ xscom region */
>> +    pnv_xscom_region_init(&nest1->xscom_pb_eq_regs, OBJECT(nest1),
>> +                          &pnv_nest1_pb_scom_eq_ops, nest1,
>> +                          "xscom-nest1-pb-scom-eq-regs",
>> +                          PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE);
>> +
>> +    /* Nest1 chiplet power bus ES xscom region */
>> +    pnv_xscom_region_init(&nest1->xscom_pb_es_regs, OBJECT(nest1),
>> +                          &pnv_nest1_pb_scom_es_ops, nest1,
>> +                          "xscom-nest1-pb-scom-es-regs",
>> +                          PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE);
>> +}
>> +
>> +static int pnv_nest1_dt_xscom(PnvXScomInterface *dev, void *fdt,
>> +                             int offset)
>> +{
>> +    PnvNest1 *nest1 = PNV_NEST1(dev);
>> +    g_autofree char *name = NULL;
>> +    int nest1_offset = 0;
>> +    const char compat[] = "ibm,power10-nest1-chiplet";
>> +    uint32_t reg[] = {
>> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE),
>> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_EQ_SIZE),
>> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE),
>> +        cpu_to_be32(PNV10_XSCOM_NEST1_PB_SCOM_ES_SIZE)
>> +    };
>> +
>> +    /* populate perv_chiplet control_regs */
>> +    pnv_perv_dt(&nest1->perv, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE, fdt, offset);
>> +
>> +    name = g_strdup_printf("nest1@%x", PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE);
>> +    nest1_offset = fdt_add_subnode(fdt, offset, name);
>> +    _FDT(nest1_offset);
>> +
>> +    _FDT(fdt_setprop(fdt, nest1_offset, "reg", reg, sizeof(reg)));
>> +    _FDT(fdt_setprop(fdt, nest1_offset, "compatible", compat, sizeof(compat)));
>> +    return 0;
>> +}
>> +
>> +static void pnv_nest1_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    PnvXScomInterfaceClass *xscomc = PNV_XSCOM_INTERFACE_CLASS(klass);
>> +
>> +    xscomc->dt_xscom = pnv_nest1_dt_xscom;
>> +
>> +    dc->desc = "PowerNV nest1 chiplet";
>> +    dc->realize = pnv_nest1_realize;
>> +}
>> +
>> +static const TypeInfo pnv_nest1_info = {
>> +    .name          = TYPE_PNV_NEST1,
>> +    .parent        = TYPE_DEVICE,
>> +    .instance_size = sizeof(PnvNest1),
>> +    .class_init    = pnv_nest1_class_init,
>> +    .interfaces    = (InterfaceInfo[]) {
>> +        { TYPE_PNV_XSCOM_INTERFACE },
>> +        { }
>> +    }
>> +};
>> +
>> +static void pnv_nest1_register_types(void)
>> +{
>> +    type_register_static(&pnv_nest1_info);
>> +}
>> +
>> +type_init(pnv_nest1_register_types);
>> diff --git a/hw/ppc/meson.build b/hw/ppc/meson.build
>> index 37a7a8935d..7b8b87596a 100644
>> --- a/hw/ppc/meson.build
>> +++ b/hw/ppc/meson.build
>> @@ -52,6 +52,7 @@ ppc_ss.add(when: 'CONFIG_POWERNV', if_true: files(
>>     'pnv_homer.c',
>>     'pnv_pnor.c',
>>     'pnv_pervasive.c',
>> +  'pnv_nest1_chiplet.c',
>>   ))
>>   # PowerPC 4xx boards
>>   ppc_ss.add(when: 'CONFIG_PPC405', if_true: files(
> 

Re: [PATCH v5 2/3] hw/ppc: Add nest1 chiplet model

[Nicholas Piggin]

On Fri Nov 24, 2023 at 10:19 PM AEST, Cédric Le Goater wrote:
> On 11/24/23 12:26, Nicholas Piggin wrote:
> > For this and actually the last patch too, it would be good to mention
> > (possibly in a header comment in the file too) what actual functionality
> > is being provided/modeled. It looks like it's just modeling behaviour of
> > reads and writes for some registers.
> > 
> > Oh, and sorry I didn't follow development and comments on this too
> > closely, so forgive me if I've missed things already said. I'll go
> > back and read through the series.
> > 
> > On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
> >> The nest1 chiplet handle the high speed i/o traffic over PCIe and others.
> >> The nest1 chiplet consists of PowerBus Fabric controller,
> >> nest Memory Management Unit, chiplet control unit and more.
> >>
> >> This commit creates a nest1 chiplet model and initialize and realize the
> >> pervasive chiplet model where chiplet control registers are implemented.
> >>
> >> This commit also implement the read/write method for the powerbus scom
> >> registers
> > 
> > The powerbus scom registers, are those specifically for the PowerBus
> > Fabric controller mentioned in the first paragraph, or is it a more
> > general set of registers for the chiplet?
> > 
> >>
> >> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> >> ---
> >>   include/hw/ppc/pnv_nest_chiplet.h |  36 ++++++
> >>   include/hw/ppc/pnv_xscom.h        |   6 +
> >>   hw/ppc/pnv_nest1_chiplet.c        | 197 ++++++++++++++++++++++++++++++
> >>   hw/ppc/meson.build                |   1 +
> >>   4 files changed, 240 insertions(+)
> >>   create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
> >>   create mode 100644 hw/ppc/pnv_nest1_chiplet.c
> >>
> >> diff --git a/include/hw/ppc/pnv_nest_chiplet.h b/include/hw/ppc/pnv_nest_chiplet.h
> >> new file mode 100644
> >> index 0000000000..845030fb1a
> >> --- /dev/null
> >> +++ b/include/hw/ppc/pnv_nest_chiplet.h
> >> @@ -0,0 +1,36 @@
> >> +/*
> >> + * QEMU PowerPC nest chiplet model
> >> + *
> >> + * Copyright (c) 2023, IBM Corporation.
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + *
> >> + * This code is licensed under the GPL version 2 or later. See the
> >> + * COPYING file in the top-level directory.
> >> + *
> >> + */
> >> +
> >> +#ifndef PPC_PNV_NEST1_CHIPLET_H
> >> +#define PPC_PNV_NEST1_CHIPLET_H
> >> +
> >> +#include "hw/ppc/pnv_pervasive.h"
> >> +
> >> +#define TYPE_PNV_NEST1 "pnv-nest1-chiplet"
> >> +#define PNV_NEST1(obj) OBJECT_CHECK(PnvNest1, (obj), TYPE_PNV_NEST1)
> >> +
> >> +typedef struct pb_scom {
> >> +    uint64_t mode;
> >> +    uint64_t hp_mode2_curr;
> >> +} pb_scom;
> >> +
> >> +typedef struct PnvNest1 {
> > 
> > Naming nitpicking again...
> > 
> > The main ifndef guard for header files should match the file name, so
> > the file should be called pnv_nest1_chiplet.h (and that matches the .c
> > file too).
> > 
> > I think this struct should be called Nest1Chiplet too.
>
> I asked Chalapathi to do the exact opposit :)

Oops :)

> I don't mind really, my argument was that most models represent HW logic
> units or subunits of a bigger unit. I don't see the point in adding a
> chip/chiplet suffix apart from PnvChip since it represents a socket or
> processor.
>
> You choose. I will keep quiet :)

Ah. I can see that side of it. And for many of the nest chiplets (MC,
PAU, PCI) that makes sense. For Nest0 and Nest1... it's a bit
overloaded. First of all, all the nest chiplets are "nest". Then
there is also some nest units inside the processor chiplets (L2, L3,
NCU are considered to be nest). And then the nest also has a Pervasive
Chiplet itself, and we also have these pervasive registers in each
chiplet, etc., etc.

So my worry is we'll run into confusion if we shorten names too much.

We can always rename things, so it won't be the end of the world, but
thinking about the pervasive chiplet, I think we can already see that
"PnvPervasive" would not be a good name for it.

The chiplets have short names actually if that would help. Nest 1 is
called N1, so we could call it PnvN1Chiplet. That seems the usual
way to refer to them in docs, so I think a better name.

Thanks,
Nick

Re: [PATCH v5 2/3] hw/ppc: Add nest1 chiplet model

[Chalapathi V]

On 24-11-2023 18:31, Nicholas Piggin wrote:
> On Fri Nov 24, 2023 at 10:19 PM AEST, Cédric Le Goater wrote:
>> On 11/24/23 12:26, Nicholas Piggin wrote:
>>> For this and actually the last patch too, it would be good to mention
>>> (possibly in a header comment in the file too) what actual functionality
>>> is being provided/modeled. It looks like it's just modeling behaviour of
>>> reads and writes for some registers.
>>>
>>> Oh, and sorry I didn't follow development and comments on this too
>>> closely, so forgive me if I've missed things already said. I'll go
>>> back and read through the series.
>>>
>>> On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
>>>> The nest1 chiplet handle the high speed i/o traffic over PCIe and others.
>>>> The nest1 chiplet consists of PowerBus Fabric controller,
>>>> nest Memory Management Unit, chiplet control unit and more.
>>>>
>>>> This commit creates a nest1 chiplet model and initialize and realize the
>>>> pervasive chiplet model where chiplet control registers are implemented.
>>>>
>>>> This commit also implement the read/write method for the powerbus scom
>>>> registers
>>> The powerbus scom registers, are those specifically for the PowerBus
>>> Fabric controller mentioned in the first paragraph, or is it a more
>>> general set of registers for the chiplet?
Yes, They are for the PowerBus racetrack unit.
>>>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>>>> ---
>>>>    include/hw/ppc/pnv_nest_chiplet.h |  36 ++++++
>>>>    include/hw/ppc/pnv_xscom.h        |   6 +
>>>>    hw/ppc/pnv_nest1_chiplet.c        | 197 ++++++++++++++++++++++++++++++
>>>>    hw/ppc/meson.build                |   1 +
>>>>    4 files changed, 240 insertions(+)
>>>>    create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
>>>>    create mode 100644 hw/ppc/pnv_nest1_chiplet.c
>>>>
>>>> diff --git a/include/hw/ppc/pnv_nest_chiplet.h b/include/hw/ppc/pnv_nest_chiplet.h
>>>> new file mode 100644
>>>> index 0000000000..845030fb1a
>>>> --- /dev/null
>>>> +++ b/include/hw/ppc/pnv_nest_chiplet.h
>>>> @@ -0,0 +1,36 @@
>>>> +/*
>>>> + * QEMU PowerPC nest chiplet model
>>>> + *
>>>> + * Copyright (c) 2023, IBM Corporation.
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + *
>>>> + * This code is licensed under the GPL version 2 or later. See the
>>>> + * COPYING file in the top-level directory.
>>>> + *
>>>> + */
>>>> +
>>>> +#ifndef PPC_PNV_NEST1_CHIPLET_H
>>>> +#define PPC_PNV_NEST1_CHIPLET_H
>>>> +
>>>> +#include "hw/ppc/pnv_pervasive.h"
>>>> +
>>>> +#define TYPE_PNV_NEST1 "pnv-nest1-chiplet"
>>>> +#define PNV_NEST1(obj) OBJECT_CHECK(PnvNest1, (obj), TYPE_PNV_NEST1)
>>>> +
>>>> +typedef struct pb_scom {
>>>> +    uint64_t mode;
>>>> +    uint64_t hp_mode2_curr;
>>>> +} pb_scom;
>>>> +
>>>> +typedef struct PnvNest1 {
>>> Naming nitpicking again...
>>>
>>> The main ifndef guard for header files should match the file name, so
>>> the file should be called pnv_nest1_chiplet.h (and that matches the .c
>>> file too).
>>>
>>> I think this struct should be called Nest1Chiplet too.
>> I asked Chalapathi to do the exact opposit :)
> Oops :)
>
>> I don't mind really, my argument was that most models represent HW logic
>> units or subunits of a bigger unit. I don't see the point in adding a
>> chip/chiplet suffix apart from PnvChip since it represents a socket or
>> processor.
>>
>> You choose. I will keep quiet :)
> Ah. I can see that side of it. And for many of the nest chiplets (MC,
> PAU, PCI) that makes sense. For Nest0 and Nest1... it's a bit
> overloaded. First of all, all the nest chiplets are "nest". Then
> there is also some nest units inside the processor chiplets (L2, L3,
> NCU are considered to be nest). And then the nest also has a Pervasive
> Chiplet itself, and we also have these pervasive registers in each
> chiplet, etc., etc.
>
> So my worry is we'll run into confusion if we shorten names too much.
>
> We can always rename things, so it won't be the end of the world, but
> thinking about the pervasive chiplet, I think we can already see that
> "PnvPervasive" would not be a good name for it.
>
> The chiplets have short names actually if that would help. Nest 1 is
> called N1, so we could call it PnvN1Chiplet. That seems the usual
> way to refer to them in docs, so I think a better name.
>
> Thanks,
> Nick
Sure. Will rename this to PnvN1Chiplet.

[PATCH v5 3/3] hw/ppc: Nest1 chiplet wiring

[Chalapathi V]

This part of the patchset connects the nest1 chiplet model to p10 chip.

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 include/hw/ppc/pnv_chip.h |  2 ++
 hw/ppc/pnv.c              | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 0ab5c42308..59a3158a6b 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -4,6 +4,7 @@
 #include "hw/pci-host/pnv_phb4.h"
 #include "hw/ppc/pnv_core.h"
 #include "hw/ppc/pnv_homer.h"
+#include "hw/ppc/pnv_nest_chiplet.h"
 #include "hw/ppc/pnv_lpc.h"
 #include "hw/ppc/pnv_occ.h"
 #include "hw/ppc/pnv_psi.h"
@@ -113,6 +114,7 @@ struct Pnv10Chip {
     PnvOCC       occ;
     PnvSBE       sbe;
     PnvHomer     homer;
+    PnvNest1     nest1;
 
     uint32_t     nr_quads;
     PnvQuad      *quads;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0297871bdd..ba3dfab557 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1680,6 +1680,7 @@ static void pnv_chip_power10_instance_init(Object *obj)
     object_initialize_child(obj, "occ",  &chip10->occ, TYPE_PNV10_OCC);
     object_initialize_child(obj, "sbe",  &chip10->sbe, TYPE_PNV10_SBE);
     object_initialize_child(obj, "homer", &chip10->homer, TYPE_PNV10_HOMER);
+    object_initialize_child(obj, "nest1", &chip10->nest1, TYPE_PNV_NEST1);
 
     chip->num_pecs = pcc->num_pecs;
 
@@ -1849,6 +1850,19 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), PNV10_HOMER_BASE(chip),
                                 &chip10->homer.regs);
 
+    /* nest1 chiplet */
+    if (!qdev_realize(DEVICE(&chip10->nest1), NULL, errp)) {
+        return;
+    }
+    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE,
+             &chip10->nest1.perv.xscom_perv_ctrl_regs);
+
+    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE,
+                           &chip10->nest1.xscom_pb_eq_regs);
+
+    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE,
+                           &chip10->nest1.xscom_pb_es_regs);
+
     /* PHBs */
     pnv_chip_power10_phb_realize(chip, &local_err);
     if (local_err) {
-- 
2.31.1

Re: [PATCH v5 3/3] hw/ppc: Nest1 chiplet wiring

[Nicholas Piggin]

On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
> This part of the patchset connects the nest1 chiplet model to p10 chip.

Seems fine to me. Should it just be squashed into patch 2?

Thanks,
Nick

>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  include/hw/ppc/pnv_chip.h |  2 ++
>  hw/ppc/pnv.c              | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
> index 0ab5c42308..59a3158a6b 100644
> --- a/include/hw/ppc/pnv_chip.h
> +++ b/include/hw/ppc/pnv_chip.h
> @@ -4,6 +4,7 @@
>  #include "hw/pci-host/pnv_phb4.h"
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/ppc/pnv_homer.h"
> +#include "hw/ppc/pnv_nest_chiplet.h"
>  #include "hw/ppc/pnv_lpc.h"
>  #include "hw/ppc/pnv_occ.h"
>  #include "hw/ppc/pnv_psi.h"
> @@ -113,6 +114,7 @@ struct Pnv10Chip {
>      PnvOCC       occ;
>      PnvSBE       sbe;
>      PnvHomer     homer;
> +    PnvNest1     nest1;
>  
>      uint32_t     nr_quads;
>      PnvQuad      *quads;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0297871bdd..ba3dfab557 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1680,6 +1680,7 @@ static void pnv_chip_power10_instance_init(Object *obj)
>      object_initialize_child(obj, "occ",  &chip10->occ, TYPE_PNV10_OCC);
>      object_initialize_child(obj, "sbe",  &chip10->sbe, TYPE_PNV10_SBE);
>      object_initialize_child(obj, "homer", &chip10->homer, TYPE_PNV10_HOMER);
> +    object_initialize_child(obj, "nest1", &chip10->nest1, TYPE_PNV_NEST1);
>  
>      chip->num_pecs = pcc->num_pecs;
>  
> @@ -1849,6 +1850,19 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(get_system_memory(), PNV10_HOMER_BASE(chip),
>                                  &chip10->homer.regs);
>  
> +    /* nest1 chiplet */
> +    if (!qdev_realize(DEVICE(&chip10->nest1), NULL, errp)) {
> +        return;
> +    }
> +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE,
> +             &chip10->nest1.perv.xscom_perv_ctrl_regs);
> +
> +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE,
> +                           &chip10->nest1.xscom_pb_eq_regs);
> +
> +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE,
> +                           &chip10->nest1.xscom_pb_es_regs);
> +
>      /* PHBs */
>      pnv_chip_power10_phb_realize(chip, &local_err);
>      if (local_err) {

Re: [PATCH v5 3/3] hw/ppc: Nest1 chiplet wiring

[Cédric Le Goater]

On 11/24/23 12:28, Nicholas Piggin wrote:
> On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
>> This part of the patchset connects the nest1 chiplet model to p10 chip.
> 
> Seems fine to me. Should it just be squashed into patch 2?

It is better to keep the model a part from the wiring because the
same model could be plugged in different board/machine. It clarifies
the interfaces, which should be limited to irq connects and memory
mappings and it makes modeling shortcuts more visible: backpointers,
looping on the machine mappings to find a core, etc.

I didn't comment on the PnvChiptod proposal but it could/should
be done the same.

This patch proposal is nice and clean.

Thanks,

C.




> 
> Thanks,
> Nick
> 
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   include/hw/ppc/pnv_chip.h |  2 ++
>>   hw/ppc/pnv.c              | 14 ++++++++++++++
>>   2 files changed, 16 insertions(+)
>>
>> diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
>> index 0ab5c42308..59a3158a6b 100644
>> --- a/include/hw/ppc/pnv_chip.h
>> +++ b/include/hw/ppc/pnv_chip.h
>> @@ -4,6 +4,7 @@
>>   #include "hw/pci-host/pnv_phb4.h"
>>   #include "hw/ppc/pnv_core.h"
>>   #include "hw/ppc/pnv_homer.h"
>> +#include "hw/ppc/pnv_nest_chiplet.h"
>>   #include "hw/ppc/pnv_lpc.h"
>>   #include "hw/ppc/pnv_occ.h"
>>   #include "hw/ppc/pnv_psi.h"
>> @@ -113,6 +114,7 @@ struct Pnv10Chip {
>>       PnvOCC       occ;
>>       PnvSBE       sbe;
>>       PnvHomer     homer;
>> +    PnvNest1     nest1;
>>   
>>       uint32_t     nr_quads;
>>       PnvQuad      *quads;
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 0297871bdd..ba3dfab557 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -1680,6 +1680,7 @@ static void pnv_chip_power10_instance_init(Object *obj)
>>       object_initialize_child(obj, "occ",  &chip10->occ, TYPE_PNV10_OCC);
>>       object_initialize_child(obj, "sbe",  &chip10->sbe, TYPE_PNV10_SBE);
>>       object_initialize_child(obj, "homer", &chip10->homer, TYPE_PNV10_HOMER);
>> +    object_initialize_child(obj, "nest1", &chip10->nest1, TYPE_PNV_NEST1);
>>   
>>       chip->num_pecs = pcc->num_pecs;
>>   
>> @@ -1849,6 +1850,19 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>>       memory_region_add_subregion(get_system_memory(), PNV10_HOMER_BASE(chip),
>>                                   &chip10->homer.regs);
>>   
>> +    /* nest1 chiplet */
>> +    if (!qdev_realize(DEVICE(&chip10->nest1), NULL, errp)) {
>> +        return;
>> +    }
>> +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_CTRL_CHIPLET_BASE,
>> +             &chip10->nest1.perv.xscom_perv_ctrl_regs);
>> +
>> +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_EQ_BASE,
>> +                           &chip10->nest1.xscom_pb_eq_regs);
>> +
>> +    pnv_xscom_add_subregion(chip, PNV10_XSCOM_NEST1_PB_SCOM_ES_BASE,
>> +                           &chip10->nest1.xscom_pb_es_regs);
>> +
>>       /* PHBs */
>>       pnv_chip_power10_phb_realize(chip, &local_err);
>>       if (local_err) {
> 

Re: [PATCH v5 3/3] hw/ppc: Nest1 chiplet wiring

[Nicholas Piggin]

On Fri Nov 24, 2023 at 10:26 PM AEST, Cédric Le Goater wrote:
> On 11/24/23 12:28, Nicholas Piggin wrote:
> > On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
> >> This part of the patchset connects the nest1 chiplet model to p10 chip.
> > 
> > Seems fine to me. Should it just be squashed into patch 2?
>
> It is better to keep the model a part from the wiring because the
> same model could be plugged in different board/machine. It clarifies
> the interfaces, which should be limited to irq connects and memory
> mappings and it makes modeling shortcuts more visible: backpointers,
> looping on the machine mappings to find a core, etc.

Okay that makes sense.

> I didn't comment on the PnvChiptod proposal but it could/should
> be done the same.

I'll look at splitting it too then.

Thanks,
Nick