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

[Chalapathi V <chalapathi.v@linux.ibm.com>]

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(