Re: [Qemu-ppc] [PATCH v3 07/10] ppc/pnv: add XSCOM infrastructure

[David Gibson]

On Thu, Sep 22, 2016 at 10:25:59AM +0200, Cédric Le Goater wrote:
> >> @@ -493,6 +525,8 @@ static void pnv_chip_power9_class_init(ObjectClass 
> >> *klass, void *data)
> >>      k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */
> >>      k->cores_mask = POWER9_CORE_MASK;
> >>      k->core_pir = pnv_chip_core_pir_p9;
> >> +    k->xscom_addr = pnv_chip_xscom_addr_p9;
> >> +    k->xscom_pcba = pnv_chip_xscom_pcba_p9;
> > 
> > So if you do as BenH (and I) suggested and have the "scom address
> > space" actually be addressed by (pcba << 3), I think you can probably
> > avoid these.  
> 
> I will look at that option again. 
> 
> I was trying to untangle a few things at the same time. I have better
> view of the problem to solve now. The bus is gone, that's was one 
> thing. How we map these xscom regions is the next. 
> 
> Ben suggested to add some P7/P8 mangling before the dispatch in 
> the &address_space_xscom. This should make things cleaner. I had 
> not thought of doing that and this is why I introduced these helpers :
> 
> +uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr)
> +uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba)
> 
> which I don't really like ...
> 
> but we must make sure that we can do the mapping of the xscom 
> subregions in the &address_space_xscom using (pcba << 3)
> 
> 
> > Instead you can handle it in the chip or ADU realize function by either:
> > 
> >     P8: * map one big subregion for the ADU into &address_space_memory
> >         * have the handler for that subregion do the address mangling,
> >           then redispatch into the xscom address space
> >
> >     P9: * Map the appropriate chunk of the xscom address space
> >           directly into address_space_memory
> 
> Yes that was my feeling for a better solution but Ben chimed in with the 
> HMER topic. I need to look at that.

Right.  Doesn't change the basic concept though - it just means you
need (slightly different) redispatchers for both P8 and P9.

> 
> 
> >>      dc->desc = "PowerNV Chip POWER9";
> >>  }
> >>  
> >> @@ -527,6 +561,16 @@ static void pnv_chip_core_sanitize(PnvChip *chip)
> >>      chip->cores_mask &= pcc->cores_mask;
> >>  }
> >>  
> >> +static void pnv_chip_init(Object *obj)
> >> +{
> >> +    PnvChip *chip = PNV_CHIP(obj);
> >> +
> >> +    object_initialize(&chip->xscom, sizeof(chip->xscom), TYPE_PNV_XSCOM);
> >> +    object_property_add_child(obj, "xscom", OBJECT(&chip->xscom), NULL);
> >> +    object_property_add_const_link(OBJECT(&chip->xscom), "chip",
> >> +                                   OBJECT(chip), &error_abort);
> >> +}
> >> +
> >>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      PnvChip *chip = PNV_CHIP(dev);
> >> @@ -540,6 +584,12 @@ static void pnv_chip_realize(DeviceState *dev, Error 
> >> **errp)
> >>          return;
> >>      }
> >>  
> >> +    /* XSCOM bridge */
> >> +    object_property_set_bool(OBJECT(&chip->xscom), true, "realized",
> >> +                             &error_fatal);
> >> +    sysbus_mmio_map(SYS_BUS_DEVICE(&chip->xscom), 0,
> >> +                    PNV_XSCOM_BASE(chip->chip_id));
> >> +
> >>      /* Early checks on the core settings */
> >>      pnv_chip_core_sanitize(chip);
> >>  
> >> @@ -597,6 +647,7 @@ static const TypeInfo pnv_chip_info = {
> >>      .name          = TYPE_PNV_CHIP,
> >>      .parent        = TYPE_SYS_BUS_DEVICE,
> >>      .class_init    = pnv_chip_class_init,
> >> +    .instance_init = pnv_chip_init,
> >>      .class_size    = sizeof(PnvChipClass),
> >>      .abstract      = true,
> >>  };
> >> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> >> new file mode 100644
> >> index 000000000000..019cd85428de
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv_xscom.c
> >> @@ -0,0 +1,308 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV XSCOM bus
> >> + *
> >> + * Copyright (c) 2016, IBM Corporation.
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see 
> >> <http://www.gnu.org/licenses/>.
> >> + */
> >> +#include "qemu/osdep.h"
> >> +#include "qapi/error.h"
> >> +#include "hw/hw.h"
> >> +#include "qemu/log.h"
> >> +#include "sysemu/kvm.h"
> >> +#include "target-ppc/cpu.h"
> >> +#include "hw/sysbus.h"
> >> +
> >> +#include "hw/ppc/fdt.h"
> >> +#include "hw/ppc/pnv_xscom.h"
> >> +#include "hw/ppc/pnv.h"
> >> +
> >> +#include <libfdt.h>
> >> +
> >> +static void xscom_complete(uint64_t hmer_bits)
> >> +{
> >> +    CPUState *cs = current_cpu;
> > 
> > Hmm.. is current_cpu a safe thing to use in the case of KVM or MTTCG?
> 
> hmm, indeed. I need to look at that.
> 
> >> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +    CPUPPCState *env = &cpu->env;
> >> +
> >> +    cpu_synchronize_state(cs);
> >> +    env->spr[SPR_HMER] |= hmer_bits;
> >> +
> >> +    /* XXX Need a CPU helper to set HMER, also handle gneeration
> >> +     * of HMIs
> >> +     */
> >> +}
> >> +
> >> +static bool xscom_dispatch_read(PnvXScom *xscom, hwaddr addr, uint64_t 
> >> *val)
> >> +{
> >> +    uint32_t success;
> >> +    uint8_t data[8];
> >> +
> >> +    success = !address_space_rw(&xscom->xscom_as, addr, 
> >> MEMTXATTRS_UNSPECIFIED,
> >> +                                data, 8, false);
> >> +    *val = (((uint64_t) data[0]) << 56 |
> >> +            ((uint64_t) data[1]) << 48 |
> >> +            ((uint64_t) data[2]) << 40 |
> >> +            ((uint64_t) data[3]) << 32 |
> >> +            ((uint64_t) data[4]) << 24 |
> >> +            ((uint64_t) data[5]) << 16 |
> >> +            ((uint64_t) data[6]) << 8  |
> >> +            ((uint64_t) data[7]));
> > 
> > AFAICT this is basically assuming data is always encoded BE.  With the
> > right choice of endian flags on the individual SCOM device
> > registrations with the scom address space, I think you should be able
> > to avoid this mangling.
> 
> yes. I should but curiously I had to do this, and this works the same on
> an intel host or a ppc64 host.

Hmm.. I suspect what you actually need is NATIVE_ENDIAN on the
individual SCOM devices, with BIG_ENDIAN on the redispatcher region.

> >> +    return success;
> >> +}
> >> +
> >> +static bool xscom_dispatch_write(PnvXScom *xscom, hwaddr addr, uint64_t 
> >> val)
> >> +{
> >> +    uint32_t success;
> >> +    uint8_t data[8];
> >> +
> >> +    data[0] = (val >> 56) & 0xff;
> >> +    data[1] = (val >> 48) & 0xff;
> >> +    data[2] = (val >> 40) & 0xff;
> >> +    data[3] = (val >> 32) & 0xff;
> >> +    data[4] = (val >> 24) & 0xff;
> >> +    data[5] = (val >> 16) & 0xff;
> >> +    data[6] = (val >> 8) & 0xff;
> >> +    data[7] = val & 0xff;
> >> +
> >> +    success = !address_space_rw(&xscom->xscom_as, addr, 
> >> MEMTXATTRS_UNSPECIFIED,
> >> +                           data, 8, true);
> >> +    return success;
> >> +}
> >> +
> >> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
> >> +{
> >> +    PnvXScom *s = opaque;
> >> +    uint32_t pcba = s->chip_class->xscom_pcba(addr);
> >> +    uint64_t val = 0;
> >> +
> >> +    /* Handle some SCOMs here before dispatch */
> >> +    switch (pcba) {
> >> +    case 0xf000f:
> >> +        val = s->chip_class->chip_cfam_id;
> >> +        break;
> >> +    case 0x1010c00:     /* PIBAM FIR */
> >> +    case 0x1010c03:     /* PIBAM FIR MASK */
> >> +    case 0x2020007:     /* ADU stuff */
> >> +    case 0x2020009:     /* ADU stuff */
> >> +    case 0x202000f:     /* ADU stuff */
> >> +        val = 0;
> >> +        break;
> >> +    case 0x2013f00:     /* PBA stuff */
> >> +    case 0x2013f01:     /* PBA stuff */
> >> +    case 0x2013f02:     /* PBA stuff */
> >> +    case 0x2013f03:     /* PBA stuff */
> >> +    case 0x2013f04:     /* PBA stuff */
> >> +    case 0x2013f05:     /* PBA stuff */
> >> +    case 0x2013f06:     /* PBA stuff */
> >> +    case 0x2013f07:     /* PBA stuff */
> >> +        val = 0;
> >> +        break;
> > 
> > It'd be theoretically nicer to actually register regions for these
> > special case addresses, but handling it here is a reasonable hack to
> > get things working quickly for the time being.
> 
> I will make a default region on the whole xscomm address space to catch 
> these.

Ok.

> 
> >> +    default:
> >> +        if (!xscom_dispatch_read(s, addr, &val)) {
> >> +            qemu_log_mask(LOG_GUEST_ERROR, "XSCOM read failed at @0x%"
> >> +                          HWADDR_PRIx " pcba=0x%08x\n", addr, pcba);
> >> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
> >> +            return 0;
> >> +        }
> >> +    }
> >> +
> >> +    xscom_complete(HMER_XSCOM_DONE);
> >> +    return val;
> >> +}
> >> +
> >> +static void xscom_write(void *opaque, hwaddr addr, uint64_t val,
> >> +                        unsigned width)
> >> +{
> >> +    PnvXScom *s = opaque;
> >> +    uint32_t pcba = s->chip_class->xscom_pcba(addr);
> >> +
> >> +    /* Handle some SCOMs here before dispatch */
> >> +    switch (pcba) {
> >> +        /* We ignore writes to these */
> >> +    case 0xf000f:       /* chip id is RO */
> >> +    case 0x1010c00:     /* PIBAM FIR */
> >> +    case 0x1010c01:     /* PIBAM FIR */
> >> +    case 0x1010c02:     /* PIBAM FIR */
> >> +    case 0x1010c03:     /* PIBAM FIR MASK */
> >> +    case 0x1010c04:     /* PIBAM FIR MASK */
> >> +    case 0x1010c05:     /* PIBAM FIR MASK */
> >> +    case 0x2020007:     /* ADU stuff */
> >> +    case 0x2020009:     /* ADU stuff */
> >> +    case 0x202000f:     /* ADU stuff */
> >> +        break;
> >> +    default:
> >> +        if (!xscom_dispatch_write(s, addr, val)) {
> >> +            qemu_log_mask(LOG_GUEST_ERROR, "XSCOM write failed at @0x%"
> >> +                          HWADDR_PRIx " pcba=0x%08x data=0x%" PRIx64 "\n",
> >> +                          addr, pcba, val);
> >> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    xscom_complete(HMER_XSCOM_DONE);
> >> +}
> >> +
> >> +const MemoryRegionOps pnv_xscom_ops = {
> >> +    .read = xscom_read,
> >> +    .write = xscom_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_xscom_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >> +    PnvXScom *s = PNV_XSCOM(dev);
> >> +    char *name;
> >> +    Object *obj;
> >> +    Error *err = NULL;
> >> +
> >> +    obj = object_property_get_link(OBJECT(s), "chip", &err);
> >> +    if (!obj) {
> >> +        error_setg(errp, "%s: required link 'chip' not found: %s",
> >> +                     __func__, error_get_pretty(err));
> >> +        return;
> >> +    }
> >> +
> >> +    s->chip_class = PNV_CHIP_GET_CLASS(obj);
> >> +    s->chip_id = PNV_CHIP(obj)->chip_id;
> >> +
> >> +    if (s->chip_id < 0) {
> >> +        error_setg(errp, "invalid chip id '%d'", s->chip_id);
> >> +        return;
> >> +    }
> >> +
> >> +    name = g_strdup_printf("xscom-%x", s->chip_id);
> >> +    memory_region_init_io(&s->mem, OBJECT(s), &pnv_xscom_ops, s, name,
> >> +                          PNV_XSCOM_SIZE);
> >> +    sysbus_init_mmio(sbd, &s->mem);
> >> +
> >> +    memory_region_init(&s->xscom_mr, OBJECT(s), name, PNV_XSCOM_SIZE);
> >> +    address_space_init(&s->xscom_as, &s->xscom_mr, name);
> >> +    g_free(name);
> >> +
> >> +}
> >> +
> >> +static void pnv_xscom_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >> +
> >> +    dc->realize = pnv_xscom_realize;
> >> +}
> >> +
> >> +static const TypeInfo pnv_xscom_info = {
> >> +    .name          = TYPE_PNV_XSCOM,
> >> +    .parent        = TYPE_SYS_BUS_DEVICE,
> >> +    .instance_size = sizeof(PnvXScom),
> >> +    .class_init    = pnv_xscom_class_init,
> >> +};
> >> +
> >> +static const TypeInfo pnv_xscom_interface_info = {
> >> +    .name = TYPE_PNV_XSCOM_INTERFACE,
> >> +    .parent = TYPE_INTERFACE,
> >> +    .class_size = sizeof(PnvXScomInterfaceClass),
> >> +};
> >> +
> >> +static void pnv_xscom_register_types(void)
> >> +{
> >> +    type_register_static(&pnv_xscom_info);
> >> +    type_register_static(&pnv_xscom_interface_info);
> >> +}
> >> +
> >> +type_init(pnv_xscom_register_types)
> >> +
> >> +typedef struct ForeachPopulateArgs {
> >> +    void *fdt;
> >> +    int xscom_offset;
> >> +} ForeachPopulateArgs;
> >> +
> >> +static int xscom_populate_child(Object *child, void *opaque)
> >> +{
> >> +    if (object_dynamic_cast(child, TYPE_PNV_XSCOM_INTERFACE)) {
> >> +        ForeachPopulateArgs *args = opaque;
> >> +        PnvXScomInterface *xd = PNV_XSCOM_INTERFACE(child);;
> >> +        PnvXScomInterfaceClass *xc = PNV_XSCOM_INTERFACE_GET_CLASS(xd);
> >> +
> >> +        if (xc->devnode) {
> >> +            _FDT((xc->devnode(xd, args->fdt, args->xscom_offset)));
> >> +        }
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +int pnv_xscom_populate_fdt(PnvXScom *adu, void *fdt, int root_offset)
> >> +{
> >> +    const char compat[] = "ibm,power8-xscom\0ibm,xscom";
> >> +    uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(adu->chip_id)),
> >> +                       cpu_to_be64(PNV_XSCOM_SIZE) };
> >> +    int xscom_offset;
> >> +    ForeachPopulateArgs args;
> >> +    char *name;
> >> +
> >> +    name = g_strdup_printf("address@hidden" PRIx64, be64_to_cpu(reg[0]));
> >> +    xscom_offset = fdt_add_subnode(fdt, root_offset, name);
> >> +    _FDT(xscom_offset);
> >> +    g_free(name);
> >> +    _FDT((fdt_setprop_cell(fdt, xscom_offset, "ibm,chip-id", 
> >> adu->chip_id)));
> >> +    _FDT((fdt_setprop_cell(fdt, xscom_offset, "#address-cells", 1)));
> >> +    _FDT((fdt_setprop_cell(fdt, xscom_offset, "#size-cells", 1)));
> >> +    _FDT((fdt_setprop(fdt, xscom_offset, "reg", reg, sizeof(reg))));
> >> +    _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat,
> >> +                      sizeof(compat))));
> >> +    _FDT((fdt_setprop(fdt, xscom_offset, "scom-controller", NULL, 0)));
> >> +
> >> +    args.fdt = fdt;
> >> +    args.xscom_offset = xscom_offset;
> >> +
> >> +    object_child_foreach(OBJECT(adu), xscom_populate_child, &args);
> >> +    return 0;
> >> +}
> >> +
> >> +/*
> >> + * XScom address translation depends on the chip type and not all
> >> + * objects have backlink to it. Here's a helper to handle this case.
> >> + * To be improved.
> >> + */
> > 
> > Yeah.. this seems a bit hacky as you say.  Passing the individual
> > device to these helpers just seems wrong, since it's a chip / machine
> > dependent mapping.  Does this go away if you use the (pcba << 3)
> > encoding?
> 
> Hopefully. the goal is to solve that in v4.
> 
> >> +uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr)
> >> +{
> >> +    PnvXScomInterfaceClass *xc = PNV_XSCOM_INTERFACE_GET_CLASS(dev);
> >> +
> >> +    if (!xc->xscom_pcba) {
> >> +        PnvMachineState *pnv = POWERNV_MACHINE(qdev_get_machine());
> >> +        PnvChipClass *pcc = PNV_CHIP_GET_CLASS(pnv->chips[0]);
> >> +
> >> +        xc->xscom_pcba = pcc->xscom_pcba;
> >> +    }
> >> +
> >> +    return xc->xscom_pcba(addr);
> >> +}
> >> +
> >> +uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba)
> >> +{
> >> +     PnvXScomInterfaceClass *xc = PNV_XSCOM_INTERFACE_GET_CLASS(dev);
> >> +
> >> +    if (!xc->xscom_addr) {
> >> +        PnvMachineState *pnv = POWERNV_MACHINE(qdev_get_machine());
> >> +        PnvChipClass *pcc = PNV_CHIP_GET_CLASS(pnv->chips[0]);
> >> +
> >> +        xc->xscom_addr = pcc->xscom_addr;
> >> +    }
> >> +
> >> +    return xc->xscom_addr(pcba);
> >> +}
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 262faa59a75f..0371710b1882 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -21,6 +21,7 @@
> >>  
> >>  #include "hw/boards.h"
> >>  #include "hw/sysbus.h"
> >> +#include "hw/ppc/pnv_xscom.h"
> >>  
> >>  #define TYPE_PNV_CHIP "powernv-chip"
> >>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> >> @@ -42,6 +43,7 @@ typedef struct PnvChip {
> >>  
> >>      /*< public >*/
> >>      uint32_t     chip_id;
> >> +    PnvXScom     xscom;
> >>  
> >>      uint32_t  nr_cores;
> >>      uint64_t  cores_mask;
> >> @@ -60,6 +62,8 @@ typedef struct PnvChipClass {
> >>  
> >>      void (*realize)(PnvChip *dev, Error **errp);
> >>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> >> +    uint64_t (*xscom_addr)(uint32_t pcba);
> >> +    uint32_t (*xscom_pcba)(uint64_t addr);
> >>  } PnvChipClass;
> >>  
> >>  #define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E"
> >> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> >> new file mode 100644
> >> index 000000000000..0a03d533db59
> >> --- /dev/null
> >> +++ b/include/hw/ppc/pnv_xscom.h
> >> @@ -0,0 +1,74 @@
> >> +/*
> >> + * QEMU PowerPC PowerNV XSCOM bus definitions
> >> + *
> >> + * Copyright (c) 2016, IBM Corporation.
> >> + *
> >> + * This library is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU Lesser General Public
> >> + * License as published by the Free Software Foundation; either
> >> + * version 2 of the License, or (at your option) any later version.
> >> + *
> >> + * This library is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * Lesser General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU Lesser General Public
> >> + * License along with this library; if not, see 
> >> <http://www.gnu.org/licenses/>.
> >> + */
> >> +#ifndef _PPC_PNV_XSCOM_H
> >> +#define _PPC_PNV_XSCOM_H
> >> +
> >> +#include "hw/sysbus.h"
> >> +
> >> +typedef struct PnvXScomInterface {
> >> +    Object parent;
> >> +} PnvXScomInterface;
> >> +
> >> +#define TYPE_PNV_XSCOM_INTERFACE "pnv-xscom-interface"
> >> +#define PNV_XSCOM_INTERFACE(obj) \
> >> +     OBJECT_CHECK(PnvXScomInterface, (obj), TYPE_PNV_XSCOM_INTERFACE)
> >> +#define PNV_XSCOM_INTERFACE_CLASS(klass)                \
> >> +    OBJECT_CLASS_CHECK(PnvXScomInterfaceClass, (klass), \
> >> +                       TYPE_PNV_XSCOM_INTERFACE)
> >> +#define PNV_XSCOM_INTERFACE_GET_CLASS(obj) \
> >> +     OBJECT_GET_CLASS(PnvXScomInterfaceClass, (obj), 
> >> TYPE_PNV_XSCOM_INTERFACE)
> >> +
> >> +typedef struct PnvXScomInterfaceClass {
> >> +    InterfaceClass parent;
> >> +    int (*devnode)(PnvXScomInterface *dev, void *fdt, int offset);
> >> +
> >> +    uint64_t (*xscom_addr)(uint32_t pcba);
> >> +    uint32_t (*xscom_pcba)(uint64_t addr);
> > 
> > Allowing the SCOM device to override the mapping just seems bogus.
> > Surely this should always go via the chip.
> 
> yes.
> 
> >> +} PnvXScomInterfaceClass;
> >> +
> >> +#define TYPE_PNV_XSCOM "pnv-xscom"
> >> +#define PNV_XSCOM(obj) OBJECT_CHECK(PnvXScom, (obj), TYPE_PNV_XSCOM)
> >> +
> >> +typedef struct PnvChipClass PnvChipClass;
> >> +
> >> +typedef struct PnvXScom {
> >> +    /*< private >*/
> >> +    SysBusDevice parent_obj;
> >> +    /*< public >*/
> >> +
> >> +    MemoryRegion mem;
> >> +    int32_t chip_id;
> >> +    PnvChipClass *chip_class;
> > 
> > Just having a pointer to the chip seems simpler than storing both id
> > and class here.  But I guess it'll go away anyway if you merge the ADU
> > into the chip object itself.
> 
> I will merge in v4.
> 
> Thanks,
> 
> C. 
> 
> >> +    MemoryRegion xscom_mr;
> >> +    AddressSpace xscom_as;
> >> +} PnvXScom;
> >> +
> >> +#define PNV_XSCOM_SIZE        0x800000000ull
> >> +#define PNV_XSCOM_BASE(chip)                                    \
> >> +    (0x3fc0000000000ull + ((uint64_t)(chip)) * PNV_XSCOM_SIZE)
> >> +
> >> +extern int pnv_xscom_populate_fdt(PnvXScom *xscom, void *fdt, int offset);
> >> +
> >> +/*
> >> + * helpers to translate to XScomm PCB addresses
> >> + */
> >> +extern uint32_t pnv_xscom_pcba(PnvXScomInterface *dev, uint64_t addr);
> >> +extern uint64_t pnv_xscom_addr(PnvXScomInterface *dev, uint32_t pcba);
> >> +
> >> +#endif /* _PPC_PNV_XSCOM_H */
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature