[v2,3/7] ppc/pnv: Add XSCOM infrastructure

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

XSCOM is an interface to a sideband bus provided by the POWER8 chip
pervasive unit, which gives access to a number of facilities in the
chip that are needed by the OPAL firmware and to a lesser extent,
Linux. This is among others how the PCI Host bridges get configured
at boot or how the LPC bus is accessed.

This provides a simple bus and device type for devices sitting on
XSCOM along with some facilities to optionally generate corresponding
device-tree nodes

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[clg: updated for qemu-2.7
      ported on new sPowerNVMachineState which was merged with PnvSystem
      removed TRACE_XSCOM
      fixed checkpatch errors
      replaced assert with error_setg in xscom_realize()
      reworked xscom_create
      introduced the use of the chip_class for chip model contants
      ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 They were some discussions on whether we should use a qemu
 address_space instead of the xscom ranges defined in this patch. 
 I gave it try, it is possible but it brings extra unnecessary calls
 and complexity. I think the current solution is better.

 hw/ppc/Makefile.objs       |   2 +-
 hw/ppc/pnv.c               |  11 ++
 hw/ppc/pnv_xscom.c         | 408 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h       |   2 +
 include/hw/ppc/pnv_xscom.h |  75 +++++++++
 5 files changed, 497 insertions(+), 1 deletion(-)
 create mode 100644 hw/ppc/pnv_xscom.c
 create mode 100644 include/hw/ppc/pnv_xscom.h

On Wed, Aug 31, 2016 at 06:34:11PM +0200, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> XSCOM is an interface to a sideband bus provided by the POWER8 chip
> pervasive unit, which gives access to a number of facilities in the
> chip that are needed by the OPAL firmware and to a lesser extent,
> Linux. This is among others how the PCI Host bridges get configured
> at boot or how the LPC bus is accessed.
> 
> This provides a simple bus and device type for devices sitting on
> XSCOM along with some facilities to optionally generate corresponding
> device-tree nodes
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> [clg: updated for qemu-2.7
>       ported on new sPowerNVMachineState which was merged with PnvSystem
>       removed TRACE_XSCOM
>       fixed checkpatch errors
>       replaced assert with error_setg in xscom_realize()
>       reworked xscom_create
>       introduced the use of the chip_class for chip model contants
>       ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  They were some discussions on whether we should use a qemu
>  address_space instead of the xscom ranges defined in this patch. 
>  I gave it try, it is possible but it brings extra unnecessary calls
>  and complexity. I think the current solution is better.
> 
>  hw/ppc/Makefile.objs       |   2 +-
>  hw/ppc/pnv.c               |  11 ++
>  hw/ppc/pnv_xscom.c         | 408 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h       |   2 +
>  include/hw/ppc/pnv_xscom.h |  75 +++++++++
>  5 files changed, 497 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/pnv_xscom.c
>  create mode 100644 include/hw/ppc/pnv_xscom.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 8105db7d5600..f580e5c41413 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  # IBM PowerNV
> -obj-$(CONFIG_POWERNV) += pnv.o
> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 06051268e200..a6e7f66b2c0a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -39,6 +39,8 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/cutils.h"
>  
> +#include "hw/ppc/pnv_xscom.h"
> +
>  #include <libfdt.h>
>  
>  #define FDT_ADDR                0x01000000
> @@ -103,6 +105,7 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
>      char *buf;
>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
>      int off;
> +    int i;
>  
>      fdt = g_malloc0(FDT_MAX_SIZE);
>      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> @@ -142,6 +145,11 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
>      /* Memory */
>      powernv_populate_memory(fdt);
>  
> +    /* Populate XSCOM for each chip */
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        xscom_populate_fdt(pnv->chips[i]->xscom, fdt, 0);
> +    }

There will be a bunch of per-chip things in the fdt - I suggest a
common function to do all the fdt creation for a single chip.  Since
you've moved to using the fdt_rw functions exclusively, you don't have
the sequence constraints that would have made that awkward previously.

>      return fdt;
>  }
>  
> @@ -305,6 +313,9 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>      PnvChip *chip = PNV_CHIP(dev);
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
> +    /* Set up XSCOM bus */
> +    chip->xscom = xscom_create(chip);

So, this creates the xscom as a new device object, unfortunately doing
that from another object's realize() violations QOM lifetime rules as
I understand them.  I think - I have to admit I'm pretty confused on
this topic myself.

You could construct the scom from chip init, then realize it from chip
realize, but I think using object_new() (via qdev_create()) from
another object's init also breaks the rules.  You are however allowed
to allocate the scom state statically within the chip and use
object_initialize() instead, AIUI.

Alternatively.. it might be simpler to just drop the SCOM as a
separate device.  I think you could just hang the scom bus directly
off the chip object.  IIUC the scom is basically the only chip-level
control mechanism, so this does make a certain amount of sense.

>      pcc->realize(chip, errp);
>  }
>  
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> new file mode 100644
> index 000000000000..7ed3804f4b3a
> --- /dev/null
> +++ b/hw/ppc/pnv_xscom.c
> @@ -0,0 +1,408 @@
> +
> +/*
> + * QEMU PowerNV XSCOM bus definitions
> + *
> + * Copyright (c) 2010 David Gibson, IBM Corporation <dwg@au1.ibm.com>
> + * Based on the s390 virtio bus code:
> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + *
> + * 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/>.
> + */
> +
> +/* TODO: Add some infrastructure for "random stuff" and FIRs that
> + * various units might want to deal with without creating actual
> + * XSCOM devices.
> + *
> + * For example, HB LPC XSCOM in the PIBAM
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/hw.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/boards.h"
> +#include "monitor/monitor.h"
> +#include "hw/loader.h"
> +#include "elf.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/kvm.h"
> +#include "sysemu/device_tree.h"
> +#include "hw/ppc/fdt.h"
> +
> +#include "hw/ppc/pnv_xscom.h"
> +
> +#include <libfdt.h>
> +
> +#define TYPE_XSCOM "xscom"
> +#define XSCOM(obj) OBJECT_CHECK(XScomState, (obj), TYPE_XSCOM)
> +
> +#define XSCOM_SIZE        0x800000000ull
> +#define XSCOM_BASE(chip)  (0x3fc0000000000ull + ((uint64_t)(chip)) * XSCOM_SIZE)
> +
> +
> +typedef struct XScomState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion mem;
> +    int32_t chip_id;

Merging scom and chip would avoid the duplication of this field too.

> +    PnvChipClass *chip_class;
> +    XScomBus *bus;
> +} XScomState;
> +
> +static uint32_t xscom_to_pcb_addr(uint64_t addr)
> +{
> +        addr &= (XSCOM_SIZE - 1);
> +        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> +}
> +
> +static void xscom_complete(uint64_t hmer_bits)
> +{
> +    CPUState *cs = current_cpu;
> +    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 XScomDevice *xscom_find_target(XScomState *s, uint32_t pcb_addr,
> +                                      uint32_t *range)
> +{
> +    BusChild *bc;
> +
> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
> +        DeviceState *qd = bc->child;
> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> +        unsigned int i;
> +
> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> +            if (xd->ranges[i].addr <= pcb_addr &&
> +                (xd->ranges[i].addr + xd->ranges[i].size) > pcb_addr) {
> +                *range = i;
> +                return xd;
> +            }
> +        }
> +    }

Hmm.. you could set up a SCOM local address space using the
infrastructure in memory.c, rather than doing your own dispatch.

> +    return NULL;
> +}
> +
> +static bool xscom_dispatch_read(XScomState *s, uint32_t pcb_addr,
> +                                uint64_t *out_val)
> +{
> +    uint32_t range, offset;
> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
> +    XScomDeviceClass *xc;
> +
> +    if (!xd) {
> +        return false;
> +    }
> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
> +    if (!xc->read) {
> +        return false;
> +    }
> +    offset = pcb_addr - xd->ranges[range].addr;
> +    return xc->read(xd, range, offset, out_val);
> +}
> +
> +static bool xscom_dispatch_write(XScomState *s, uint32_t pcb_addr, uint64_t val)
> +{
> +    uint32_t range, offset;
> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
> +    XScomDeviceClass *xc;
> +
> +    if (!xd) {
> +        return false;
> +    }
> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
> +    if (!xc->write) {
> +        return false;
> +    }
> +    offset = pcb_addr - xd->ranges[range].addr;
> +    return xc->write(xd, range, offset, val);
> +}
> +
> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
> +{
> +    XScomState *s = opaque;
> +    uint32_t pcba = xscom_to_pcb_addr(addr);
> +    uint64_t val;
> +
> +    assert(width == 8);
> +
> +    /* Handle some SCOMs here before dispatch */
> +    switch (pcba) {
> +    case 0xf000f:
> +        val = s->chip_class->chip_f000f;
> +        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;
> +    default:
> +        if (!xscom_dispatch_read(s, pcba, &val)) {
> +            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)
> +{
> +    XScomState *s = opaque;
> +    uint32_t pcba = xscom_to_pcb_addr(addr);
> +
> +    assert(width == 8);
> +
> +    /* 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, pcba, val)) {
> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
> +            return;
> +        }
> +    }
> +
> +    xscom_complete(HMER_XSCOM_DONE);
> +}
> +
> +static const MemoryRegionOps 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 int xscom_init(SysBusDevice *dev)
> +{
> +    XScomState *s = XSCOM(dev);
> +
> +    s->chip_id = -1;
> +    return 0;
> +}
> +
> +static void xscom_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    XScomState *s = XSCOM(dev);
> +    char *name;
> +
> +    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), &xscom_ops, s, name, XSCOM_SIZE);
> +    sysbus_init_mmio(sbd, &s->mem);
> +    sysbus_mmio_map(sbd, 0, XSCOM_BASE(s->chip_id));
> +}
> +
> +static Property xscom_properties[] = {
> +        DEFINE_PROP_INT32("chip_id", XScomState, chip_id, 0),
> +        DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void xscom_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = xscom_properties;
> +    dc->realize = xscom_realize;
> +    k->init = xscom_init;
> +}
> +
> +static const TypeInfo xscom_info = {
> +    .name          = TYPE_XSCOM,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(XScomState),
> +    .class_init    = xscom_class_init,
> +};
> +
> +static void xscom_bus_class_init(ObjectClass *klass, void *data)
> +{
> +}
> +
> +static const TypeInfo xscom_bus_info = {
> +    .name = TYPE_XSCOM_BUS,
> +    .parent = TYPE_BUS,
> +    .class_init = xscom_bus_class_init,
> +    .instance_size = sizeof(XScomBus),
> +};
> +
> +XScomBus *xscom_create(PnvChip *chip)
> +{
> +    DeviceState *dev;
> +    XScomState *xdev;
> +    BusState *qbus;
> +    XScomBus *xb;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +
> +    dev = qdev_create(NULL, TYPE_XSCOM);
> +    qdev_prop_set_uint32(dev, "chip_id", chip->chip_id);
> +    qdev_init_nofail(dev);
> +
> +    /* Create bus on bridge device */
> +    qbus = qbus_create(TYPE_XSCOM_BUS, dev, "xscom");
> +    xb = DO_UPCAST(XScomBus, bus, qbus);
> +    xb->chip_id = chip->chip_id;
> +    xdev = XSCOM(dev);
> +    xdev->bus = xb;
> +    xdev->chip_class = pcc;
> +
> +    return xb;
> +}
> +
> +int xscom_populate_fdt(XScomBus *xb, void *fdt, int root_offset)

What is the root_offset parameter for, since it is always 0?

> +{
> +    BusChild *bc;
> +    char *name;
> +    const char compat[] = "ibm,power8-xscom\0ibm,xscom";
> +    uint64_t reg[] = { cpu_to_be64(XSCOM_BASE(xb->chip_id)),
> +                       cpu_to_be64(XSCOM_SIZE) };
> +    int xscom_offset;
> +
> +    name = g_strdup_printf("xscom@%llx", (unsigned long long)
> +                           be64_to_cpu(reg[0]));

Nit: in qemu the PRIx64 etc. macros are usually preferred to (unsigned
long long) casts of this type.

> +    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", xb->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)));
> +
> +    QTAILQ_FOREACH(bc, &xb->bus.children, sibling) {
> +        DeviceState *qd = bc->child;
> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> +        XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xd);
> +        uint32_t reg[MAX_XSCOM_RANGES * 2];
> +        unsigned int i, sz = 0;
> +        void *cp, *p;
> +        int child_offset;
> +
> +        /* Some XSCOM slaves may not be represented in the DT */
> +        if (!xc->dt_name) {
> +            continue;
> +        }
> +        name = g_strdup_printf("%s@%x", xc->dt_name, xd->ranges[0].addr);
> +        child_offset = fdt_add_subnode(fdt, xscom_offset, name);
> +        _FDT(child_offset);
> +        g_free(name);
> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> +            if (xd->ranges[i].size == 0) {
> +                break;
> +            }
> +            reg[sz++] = cpu_to_be32(xd->ranges[i].addr);
> +            reg[sz++] = cpu_to_be32(xd->ranges[i].size);
> +        }
> +        _FDT((fdt_setprop(fdt, child_offset, "reg", reg, sz * 4)));

Use of fdt_appendprop() might make this a little neater.

> +        if (xc->devnode) {
> +            _FDT((xc->devnode(xd, fdt, child_offset)));
> +        }
> +#define MAX_COMPATIBLE_PROP     1024
> +        cp = p = g_malloc0(MAX_COMPATIBLE_PROP);
> +        i = 0;
> +        while ((p - cp) < MAX_COMPATIBLE_PROP) {
> +            int l;
> +            if (xc->dt_compatible[i] == NULL) {
> +                break;
> +            }
> +            l = strlen(xc->dt_compatible[i]);
> +            if (l >= (MAX_COMPATIBLE_PROP - i)) {
> +                break;
> +            }
> +            strcpy(p, xc->dt_compatible[i++]);
> +            p += l + 1;
> +        }
> +        _FDT((fdt_setprop(fdt, child_offset, "compatible", cp, p - cp)));

Might it be easier to just require the individual scom device to set
the compatible property from its ->devnode callback?  That way it can
just
	const char compat = "foo\0bar\0baz";
	fdt_setprop(..., compat, sizeof(compat));

and avoid this fiddling around with arrays.

> +    }
> +
> +    return 0;
> +}
> +
> +static int xscom_qdev_init(DeviceState *qdev)
> +{
> +    XScomDevice *xdev = (XScomDevice *)qdev;
> +    XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xdev);
> +
> +    if (xc->init) {
> +        return xc->init(xdev);
> +    }
> +    return 0;
> +}
> +
> +static void xscom_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *k = DEVICE_CLASS(klass);
> +    k->init = xscom_qdev_init;
> +    k->bus_type = TYPE_XSCOM_BUS;
> +}
> +
> +static const TypeInfo xscom_dev_info = {
> +    .name = TYPE_XSCOM_DEVICE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(XScomDevice),
> +    .abstract = true,
> +    .class_size = sizeof(XScomDeviceClass),
> +    .class_init = xscom_device_class_init,
> +};
> +
> +static void xscom_register_types(void)
> +{
> +    type_register_static(&xscom_info);
> +    type_register_static(&xscom_bus_info);
> +    type_register_static(&xscom_dev_info);
> +}
> +
> +type_init(xscom_register_types)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1f32573dedff..bc6e1f80096b 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -35,12 +35,14 @@ typedef enum PnvChipType {
>      PNV_CHIP_P8NVL, /* AKA Naples */
>  } PnvChipType;
>  
> +typedef struct XScomBus XScomBus;
>  typedef struct PnvChip {
>      /*< private >*/
>      SysBusDevice parent_obj;
>  
>      /*< public >*/
>      uint32_t     chip_id;
> +    XScomBus     *xscom;
>  } PnvChip;
>  
>  typedef struct PnvChipClass {
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> new file mode 100644
> index 000000000000..386ad21c5aa5
> --- /dev/null
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -0,0 +1,75 @@
> +#ifndef _HW_XSCOM_H
> +#define _HW_XSCOM_H
> +/*
> + * QEMU PowerNV XSCOM bus definitions
> + *
> + * Copyright (c) 2010 David Gibson <david@gibson.dropbear.id.au>, IBM Corp.
> + * Based on the s390 virtio bus definitions:
> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + *
> + * 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 <hw/ppc/pnv.h>
> +
> +#define TYPE_XSCOM_DEVICE "xscom-device"
> +#define XSCOM_DEVICE(obj) \
> +     OBJECT_CHECK(XScomDevice, (obj), TYPE_XSCOM_DEVICE)
> +#define XSCOM_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(XScomDeviceClass, (klass), TYPE_XSCOM_DEVICE)
> +#define XSCOM_DEVICE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(XScomDeviceClass, (obj), TYPE_XSCOM_DEVICE)
> +
> +#define TYPE_XSCOM_BUS "xscom-bus"
> +#define XSCOM_BUS(obj) OBJECT_CHECK(XScomBus, (obj), TYPE_XSCOM_BUS)
> +
> +typedef struct XScomDevice XScomDevice;
> +typedef struct XScomBus XScomBus;
> +
> +typedef struct XScomDeviceClass {
> +    DeviceClass parent_class;
> +
> +    const char *dt_name;
> +    const char **dt_compatible;
> +    int (*init)(XScomDevice *dev);
> +    int (*devnode)(XScomDevice *dev, void *fdt, int offset);
> +
> +    /* Actual XScom accesses */
> +    bool (*read)(XScomDevice *dev, uint32_t range, uint32_t offset,
> +                 uint64_t *out_val);
> +    bool (*write)(XScomDevice *dev, uint32_t range, uint32_t offset,
> +                  uint64_t val);
> +} XScomDeviceClass;
> +
> +typedef struct XScomRange {
> +    uint32_t addr;
> +    uint32_t size;
> +} XScomRange;
> +
> +struct XScomDevice {
> +    DeviceState qdev;
> +#define MAX_XSCOM_RANGES 4
> +    struct XScomRange ranges[MAX_XSCOM_RANGES];
> +};
> +
> +struct XScomBus {
> +    BusState bus;
> +    uint32_t chip_id;
> +};
> +
> +extern XScomBus *xscom_create(PnvChip *chip);
> +extern int xscom_populate_fdt(XScomBus *xscom, void *fdt, int offset);
> +
> +
> +#endif /* _HW_XSCOM_H */

On Wed, Aug 31, 2016 at 06:34:11PM +0200, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> XSCOM is an interface to a sideband bus provided by the POWER8 chip
> pervasive unit, which gives access to a number of facilities in the
> chip that are needed by the OPAL firmware and to a lesser extent,
> Linux. This is among others how the PCI Host bridges get configured
> at boot or how the LPC bus is accessed.
> 
> This provides a simple bus and device type for devices sitting on
> XSCOM along with some facilities to optionally generate corresponding
> device-tree nodes
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> [clg: updated for qemu-2.7
>       ported on new sPowerNVMachineState which was merged with PnvSystem
>       removed TRACE_XSCOM
>       fixed checkpatch errors
>       replaced assert with error_setg in xscom_realize()
>       reworked xscom_create
>       introduced the use of the chip_class for chip model contants
>       ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  They were some discussions on whether we should use a qemu
>  address_space instead of the xscom ranges defined in this patch. 
>  I gave it try, it is possible but it brings extra unnecessary calls
>  and complexity. I think the current solution is better.
> 
>  hw/ppc/Makefile.objs       |   2 +-
>  hw/ppc/pnv.c               |  11 ++
>  hw/ppc/pnv_xscom.c         | 408 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h       |   2 +
>  include/hw/ppc/pnv_xscom.h |  75 +++++++++
>  5 files changed, 497 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/pnv_xscom.c
>  create mode 100644 include/hw/ppc/pnv_xscom.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index 8105db7d5600..f580e5c41413 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  # IBM PowerNV
> -obj-$(CONFIG_POWERNV) += pnv.o
> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 06051268e200..a6e7f66b2c0a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -39,6 +39,8 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/cutils.h"
>  
> +#include "hw/ppc/pnv_xscom.h"
> +
>  #include <libfdt.h>
>  
>  #define FDT_ADDR                0x01000000
> @@ -103,6 +105,7 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
>      char *buf;
>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
>      int off;
> +    int i;
>  
>      fdt = g_malloc0(FDT_MAX_SIZE);
>      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> @@ -142,6 +145,11 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
>      /* Memory */
>      powernv_populate_memory(fdt);
>  
> +    /* Populate XSCOM for each chip */
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        xscom_populate_fdt(pnv->chips[i]->xscom, fdt, 0);
> +    }
> +
>      return fdt;
>  }
>  
> @@ -305,6 +313,9 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>      PnvChip *chip = PNV_CHIP(dev);
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
> +    /* Set up XSCOM bus */
> +    chip->xscom = xscom_create(chip);
> +
>      pcc->realize(chip, errp);
>  }
>  
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> new file mode 100644
> index 000000000000..7ed3804f4b3a
> --- /dev/null
> +++ b/hw/ppc/pnv_xscom.c
> @@ -0,0 +1,408 @@
> +
> +/*
> + * QEMU PowerNV XSCOM bus definitions
> + *
> + * Copyright (c) 2010 David Gibson, IBM Corporation <dwg@au1.ibm.com>
> + * Based on the s390 virtio bus code:
> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + *
> + * 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/>.
> + */
> +
> +/* TODO: Add some infrastructure for "random stuff" and FIRs that
> + * various units might want to deal with without creating actual
> + * XSCOM devices.
> + *
> + * For example, HB LPC XSCOM in the PIBAM
> + */
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/hw.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/boards.h"
> +#include "monitor/monitor.h"
> +#include "hw/loader.h"
> +#include "elf.h"
> +#include "hw/sysbus.h"
> +#include "sysemu/kvm.h"
> +#include "sysemu/device_tree.h"
> +#include "hw/ppc/fdt.h"
> +
> +#include "hw/ppc/pnv_xscom.h"
> +
> +#include <libfdt.h>
> +
> +#define TYPE_XSCOM "xscom"
> +#define XSCOM(obj) OBJECT_CHECK(XScomState, (obj), TYPE_XSCOM)
> +
> +#define XSCOM_SIZE        0x800000000ull
> +#define XSCOM_BASE(chip)  (0x3fc0000000000ull + ((uint64_t)(chip)) * XSCOM_SIZE)
> +
> +
> +typedef struct XScomState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion mem;
> +    int32_t chip_id;
> +    PnvChipClass *chip_class;
> +    XScomBus *bus;
> +} XScomState;
> +
> +static uint32_t xscom_to_pcb_addr(uint64_t addr)
> +{
> +        addr &= (XSCOM_SIZE - 1);
> +        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> +}
> +
> +static void xscom_complete(uint64_t hmer_bits)
> +{
> +    CPUState *cs = current_cpu;
> +    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 XScomDevice *xscom_find_target(XScomState *s, uint32_t pcb_addr,
> +                                      uint32_t *range)
> +{
> +    BusChild *bc;
> +
> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
> +        DeviceState *qd = bc->child;
> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> +        unsigned int i;
> +
> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> +            if (xd->ranges[i].addr <= pcb_addr &&
> +                (xd->ranges[i].addr + xd->ranges[i].size) > pcb_addr) {
> +                *range = i;
> +                return xd;
> +            }
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static bool xscom_dispatch_read(XScomState *s, uint32_t pcb_addr,
> +                                uint64_t *out_val)
> +{
> +    uint32_t range, offset;
> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
> +    XScomDeviceClass *xc;
> +
> +    if (!xd) {
> +        return false;
> +    }
> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
> +    if (!xc->read) {
> +        return false;
> +    }
> +    offset = pcb_addr - xd->ranges[range].addr;
> +    return xc->read(xd, range, offset, out_val);
> +}
> +
> +static bool xscom_dispatch_write(XScomState *s, uint32_t pcb_addr, uint64_t val)
> +{
> +    uint32_t range, offset;
> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
> +    XScomDeviceClass *xc;
> +
> +    if (!xd) {
> +        return false;
> +    }
> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
> +    if (!xc->write) {
> +        return false;
> +    }
> +    offset = pcb_addr - xd->ranges[range].addr;
> +    return xc->write(xd, range, offset, val);
> +}
> +
> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
> +{
> +    XScomState *s = opaque;
> +    uint32_t pcba = xscom_to_pcb_addr(addr);
> +    uint64_t val;
> +
> +    assert(width == 8);
> +
> +    /* Handle some SCOMs here before dispatch */
> +    switch (pcba) {
> +    case 0xf000f:
> +        val = s->chip_class->chip_f000f;
> +        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;
> +    default:
> +        if (!xscom_dispatch_read(s, pcba, &val)) {
> +            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)
> +{
> +    XScomState *s = opaque;
> +    uint32_t pcba = xscom_to_pcb_addr(addr);
> +
> +    assert(width == 8);
> +
> +    /* 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, pcba, val)) {
> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
> +            return;
> +        }
> +    }
> +
> +    xscom_complete(HMER_XSCOM_DONE);
> +}
> +
> +static const MemoryRegionOps 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 int xscom_init(SysBusDevice *dev)
> +{
> +    XScomState *s = XSCOM(dev);
> +
> +    s->chip_id = -1;
> +    return 0;
> +}
> +
> +static void xscom_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    XScomState *s = XSCOM(dev);
> +    char *name;
> +
> +    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), &xscom_ops, s, name, XSCOM_SIZE);
> +    sysbus_init_mmio(sbd, &s->mem);
> +    sysbus_mmio_map(sbd, 0, XSCOM_BASE(s->chip_id));
> +}
> +
> +static Property xscom_properties[] = {
> +        DEFINE_PROP_INT32("chip_id", XScomState, chip_id, 0),
> +        DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void xscom_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = xscom_properties;
> +    dc->realize = xscom_realize;
> +    k->init = xscom_init;
> +}
> +
> +static const TypeInfo xscom_info = {
> +    .name          = TYPE_XSCOM,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(XScomState),
> +    .class_init    = xscom_class_init,
> +};
> +
> +static void xscom_bus_class_init(ObjectClass *klass, void *data)
> +{
> +}
> +
> +static const TypeInfo xscom_bus_info = {
> +    .name = TYPE_XSCOM_BUS,
> +    .parent = TYPE_BUS,
> +    .class_init = xscom_bus_class_init,
> +    .instance_size = sizeof(XScomBus),
> +};
> +
> +XScomBus *xscom_create(PnvChip *chip)
> +{
> +    DeviceState *dev;
> +    XScomState *xdev;
> +    BusState *qbus;
> +    XScomBus *xb;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +
> +    dev = qdev_create(NULL, TYPE_XSCOM);
> +    qdev_prop_set_uint32(dev, "chip_id", chip->chip_id);
> +    qdev_init_nofail(dev);
> +
> +    /* Create bus on bridge device */
> +    qbus = qbus_create(TYPE_XSCOM_BUS, dev, "xscom");
> +    xb = DO_UPCAST(XScomBus, bus, qbus);
> +    xb->chip_id = chip->chip_id;
> +    xdev = XSCOM(dev);
> +    xdev->bus = xb;
> +    xdev->chip_class = pcc;
> +
> +    return xb;
> +}
> +
> +int xscom_populate_fdt(XScomBus *xb, void *fdt, int root_offset)
> +{
> +    BusChild *bc;
> +    char *name;
> +    const char compat[] = "ibm,power8-xscom\0ibm,xscom";
> +    uint64_t reg[] = { cpu_to_be64(XSCOM_BASE(xb->chip_id)),
> +                       cpu_to_be64(XSCOM_SIZE) };
> +    int xscom_offset;
> +
> +    name = g_strdup_printf("xscom@%llx", (unsigned long long)
> +                           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", xb->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)));
> +
> +    QTAILQ_FOREACH(bc, &xb->bus.children, sibling) {
> +        DeviceState *qd = bc->child;
> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> +        XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xd);
> +        uint32_t reg[MAX_XSCOM_RANGES * 2];
> +        unsigned int i, sz = 0;
> +        void *cp, *p;
> +        int child_offset;
> +
> +        /* Some XSCOM slaves may not be represented in the DT */
> +        if (!xc->dt_name) {
> +            continue;
> +        }
> +        name = g_strdup_printf("%s@%x", xc->dt_name, xd->ranges[0].addr);
> +        child_offset = fdt_add_subnode(fdt, xscom_offset, name);
> +        _FDT(child_offset);
> +        g_free(name);
> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> +            if (xd->ranges[i].size == 0) {
> +                break;
> +            }
> +            reg[sz++] = cpu_to_be32(xd->ranges[i].addr);
> +            reg[sz++] = cpu_to_be32(xd->ranges[i].size);
> +        }
> +        _FDT((fdt_setprop(fdt, child_offset, "reg", reg, sz * 4)));
> +        if (xc->devnode) {
> +            _FDT((xc->devnode(xd, fdt, child_offset)));
> +        }
> +#define MAX_COMPATIBLE_PROP     1024
> +        cp = p = g_malloc0(MAX_COMPATIBLE_PROP);
> +        i = 0;
> +        while ((p - cp) < MAX_COMPATIBLE_PROP) {
> +            int l;
> +            if (xc->dt_compatible[i] == NULL) {
> +                break;
> +            }
> +            l = strlen(xc->dt_compatible[i]);
> +            if (l >= (MAX_COMPATIBLE_PROP - i)) {

The use of 'i' above doesn't look right. Should the check be more like this?
               if ((l + 1) >= (MAX_COMPATIBLE_PROP - (p - cp))) {

> +                break;
> +            }
> +            strcpy(p, xc->dt_compatible[i++]);
> +            p += l + 1;
> +        }
> +        _FDT((fdt_setprop(fdt, child_offset, "compatible", cp, p - cp)));
> +    }
> +
> +    return 0;
> +}
> +
> +static int xscom_qdev_init(DeviceState *qdev)
> +{
> +    XScomDevice *xdev = (XScomDevice *)qdev;
> +    XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xdev);
> +
> +    if (xc->init) {
> +        return xc->init(xdev);
> +    }
> +    return 0;
> +}
> +
> +static void xscom_device_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *k = DEVICE_CLASS(klass);
> +    k->init = xscom_qdev_init;
> +    k->bus_type = TYPE_XSCOM_BUS;
> +}
> +
> +static const TypeInfo xscom_dev_info = {
> +    .name = TYPE_XSCOM_DEVICE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(XScomDevice),
> +    .abstract = true,
> +    .class_size = sizeof(XScomDeviceClass),
> +    .class_init = xscom_device_class_init,
> +};
> +
> +static void xscom_register_types(void)
> +{
> +    type_register_static(&xscom_info);
> +    type_register_static(&xscom_bus_info);
> +    type_register_static(&xscom_dev_info);
> +}
> +
> +type_init(xscom_register_types)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1f32573dedff..bc6e1f80096b 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -35,12 +35,14 @@ typedef enum PnvChipType {
>      PNV_CHIP_P8NVL, /* AKA Naples */
>  } PnvChipType;
>  
> +typedef struct XScomBus XScomBus;
>  typedef struct PnvChip {
>      /*< private >*/
>      SysBusDevice parent_obj;
>  
>      /*< public >*/
>      uint32_t     chip_id;
> +    XScomBus     *xscom;
>  } PnvChip;
>  
>  typedef struct PnvChipClass {
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> new file mode 100644
> index 000000000000..386ad21c5aa5
> --- /dev/null
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -0,0 +1,75 @@
> +#ifndef _HW_XSCOM_H
> +#define _HW_XSCOM_H
> +/*
> + * QEMU PowerNV XSCOM bus definitions
> + *
> + * Copyright (c) 2010 David Gibson <david@gibson.dropbear.id.au>, IBM Corp.
> + * Based on the s390 virtio bus definitions:
> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + *
> + * 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 <hw/ppc/pnv.h>
> +
> +#define TYPE_XSCOM_DEVICE "xscom-device"
> +#define XSCOM_DEVICE(obj) \
> +     OBJECT_CHECK(XScomDevice, (obj), TYPE_XSCOM_DEVICE)
> +#define XSCOM_DEVICE_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(XScomDeviceClass, (klass), TYPE_XSCOM_DEVICE)
> +#define XSCOM_DEVICE_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(XScomDeviceClass, (obj), TYPE_XSCOM_DEVICE)
> +
> +#define TYPE_XSCOM_BUS "xscom-bus"
> +#define XSCOM_BUS(obj) OBJECT_CHECK(XScomBus, (obj), TYPE_XSCOM_BUS)
> +
> +typedef struct XScomDevice XScomDevice;
> +typedef struct XScomBus XScomBus;
> +
> +typedef struct XScomDeviceClass {
> +    DeviceClass parent_class;
> +
> +    const char *dt_name;
> +    const char **dt_compatible;
> +    int (*init)(XScomDevice *dev);
> +    int (*devnode)(XScomDevice *dev, void *fdt, int offset);
> +
> +    /* Actual XScom accesses */
> +    bool (*read)(XScomDevice *dev, uint32_t range, uint32_t offset,
> +                 uint64_t *out_val);
> +    bool (*write)(XScomDevice *dev, uint32_t range, uint32_t offset,
> +                  uint64_t val);
> +} XScomDeviceClass;
> +
> +typedef struct XScomRange {
> +    uint32_t addr;
> +    uint32_t size;
> +} XScomRange;
> +
> +struct XScomDevice {
> +    DeviceState qdev;
> +#define MAX_XSCOM_RANGES 4
> +    struct XScomRange ranges[MAX_XSCOM_RANGES];
> +};
> +
> +struct XScomBus {
> +    BusState bus;
> +    uint32_t chip_id;
> +};
> +
> +extern XScomBus *xscom_create(PnvChip *chip);
> +extern int xscom_populate_fdt(XScomBus *xscom, void *fdt, int offset);
> +
> +
> +#endif /* _HW_XSCOM_H */
> -- 
> 2.7.4
>

On Mon, 2016-09-05 at 13:39 +1000, David Gibson wrote:
> > +static XScomDevice *xscom_find_target(XScomState *s, uint32_t

> pcb_addr,

> > +                                      uint32_t *range)

> > +{

> > +    BusChild *bc;

> > +

> > +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {

> > +        DeviceState *qd = bc->child;

> > +        XScomDevice *xd = XSCOM_DEVICE(qd);

> > +        unsigned int i;

> > +

> > +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {

> > +            if (xd->ranges[i].addr <= pcb_addr &&

> > +                (xd->ranges[i].addr + xd->ranges[i].size) >

> pcb_addr) {

> > +                *range = i;

> > +                return xd;

> > +            }

> > +        }

> > +    }

> 

> Hmm.. you could set up a SCOM local address space using the

> infrastructure in memory.c, rather than doing your own dispatch.


There are pros and cons to this approach. The memory.c stuff comes with
quite a lot of baggage, not all of it very shinny to be honest ;-) I
still *hate* how it forces upon us a whole 128-bit integer arithmetic
library just so that it can represent 1_0000_0000_0000_0000 ... 

It would be make more sense to use inclusive start/end instead and
stick to 64-bits.

That being said, we could do that. We'd have to shift the XSCOM
addresses left by 3 since each address is an 8 bytes reigster and
forbid non-8-bytes accesses.

Ben.

On Mon, Sep 05, 2016 at 05:11:53PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2016-09-05 at 13:39 +1000, David Gibson wrote:
> > > +static XScomDevice *xscom_find_target(XScomState *s, uint32_t
> > pcb_addr,
> > > +                                      uint32_t *range)
> > > +{
> > > +    BusChild *bc;
> > > +
> > > +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
> > > +        DeviceState *qd = bc->child;
> > > +        XScomDevice *xd = XSCOM_DEVICE(qd);
> > > +        unsigned int i;
> > > +
> > > +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> > > +            if (xd->ranges[i].addr <= pcb_addr &&
> > > +                (xd->ranges[i].addr + xd->ranges[i].size) >
> > pcb_addr) {
> > > +                *range = i;
> > > +                return xd;
> > > +            }
> > > +        }
> > > +    }
> > 
> > Hmm.. you could set up a SCOM local address space using the
> > infrastructure in memory.c, rather than doing your own dispatch.
> 
> There are pros and cons to this approach. The memory.c stuff comes with
> quite a lot of baggage, not all of it very shinny to be honest ;-) I
> still *hate* how it forces upon us a whole 128-bit integer arithmetic
> library just so that it can represent 1_0000_0000_0000_0000 ... 

Ugh, yeah.  I tried to argue against this when it first came in, but
was overruled.

> It would be make more sense to use inclusive start/end instead and
> stick to 64-bits.
> 
> That being said, we could do that. We'd have to shift the XSCOM
> addresses left by 3 since each address is an 8 bytes reigster and
> forbid non-8-bytes accesses.

Ok.  I'm not particularly fussed either way.

On 09/05/2016 05:39 AM, David Gibson wrote:
> On Wed, Aug 31, 2016 at 06:34:11PM +0200, Cédric Le Goater wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> XSCOM is an interface to a sideband bus provided by the POWER8 chip
>> pervasive unit, which gives access to a number of facilities in the
>> chip that are needed by the OPAL firmware and to a lesser extent,
>> Linux. This is among others how the PCI Host bridges get configured
>> at boot or how the LPC bus is accessed.
>>
>> This provides a simple bus and device type for devices sitting on
>> XSCOM along with some facilities to optionally generate corresponding
>> device-tree nodes
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> [clg: updated for qemu-2.7
>>       ported on new sPowerNVMachineState which was merged with PnvSystem
>>       removed TRACE_XSCOM
>>       fixed checkpatch errors
>>       replaced assert with error_setg in xscom_realize()
>>       reworked xscom_create
>>       introduced the use of the chip_class for chip model contants
>>       ]
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  They were some discussions on whether we should use a qemu
>>  address_space instead of the xscom ranges defined in this patch. 
>>  I gave it try, it is possible but it brings extra unnecessary calls
>>  and complexity. I think the current solution is better.
>>
>>  hw/ppc/Makefile.objs       |   2 +-
>>  hw/ppc/pnv.c               |  11 ++
>>  hw/ppc/pnv_xscom.c         | 408 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h       |   2 +
>>  include/hw/ppc/pnv_xscom.h |  75 +++++++++
>>  5 files changed, 497 insertions(+), 1 deletion(-)
>>  create mode 100644 hw/ppc/pnv_xscom.c
>>  create mode 100644 include/hw/ppc/pnv_xscom.h
>>
>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
>> index 8105db7d5600..f580e5c41413 100644
>> --- a/hw/ppc/Makefile.objs
>> +++ b/hw/ppc/Makefile.objs
>> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>>  # IBM PowerNV
>> -obj-$(CONFIG_POWERNV) += pnv.o
>> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o
>>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>>  obj-y += spapr_pci_vfio.o
>>  endif
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 06051268e200..a6e7f66b2c0a 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -39,6 +39,8 @@
>>  #include "exec/address-spaces.h"
>>  #include "qemu/cutils.h"
>>  
>> +#include "hw/ppc/pnv_xscom.h"
>> +
>>  #include <libfdt.h>
>>  
>>  #define FDT_ADDR                0x01000000
>> @@ -103,6 +105,7 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
>>      char *buf;
>>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
>>      int off;
>> +    int i;
>>  
>>      fdt = g_malloc0(FDT_MAX_SIZE);
>>      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
>> @@ -142,6 +145,11 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
>>      /* Memory */
>>      powernv_populate_memory(fdt);
>>  
>> +    /* Populate XSCOM for each chip */
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        xscom_populate_fdt(pnv->chips[i]->xscom, fdt, 0);
>> +    }
> 
> There will be a bunch of per-chip things in the fdt - I suggest a
> common function to do all the fdt creation for a single chip.  Since
> you've moved to using the fdt_rw functions exclusively, you don't have
> the sequence constraints that would have made that awkward previously.

ok.

>>      return fdt;
>>  }
>>  
>> @@ -305,6 +313,9 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>>      PnvChip *chip = PNV_CHIP(dev);
>>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>  
>> +    /* Set up XSCOM bus */
>> +    chip->xscom = xscom_create(chip);
> 
> So, this creates the xscom as a new device object, unfortunately doing
> that from another object's realize() violations QOM lifetime rules as
> I understand them.  I think - I have to admit I'm pretty confused on
> this topic myself.
> 
> You could construct the scom from chip init, then realize it from chip
> realize, but I think using object_new() (via qdev_create()) from
> another object's init also breaks the rules.  You are however allowed
> to allocate the scom state statically within the chip and use
> object_initialize() instead, AIUI.
> 
> Alternatively.. it might be simpler to just drop the SCOM as a
> separate device.  I think you could just hang the scom bus directly
> off the chip object.  IIUC the scom is basically the only chip-level
> control mechanism, so this does make a certain amount of sense.

yes. I am exposing more and more stuff of the chip object under the 
xscom object so we should merge them. I agree. We will still need 
some XScomDevice of some sort.

>>      pcc->realize(chip, errp);
>>  }
>>  
>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>> new file mode 100644
>> index 000000000000..7ed3804f4b3a
>> --- /dev/null
>> +++ b/hw/ppc/pnv_xscom.c
>> @@ -0,0 +1,408 @@
>> +
>> +/*
>> + * QEMU PowerNV XSCOM bus definitions
>> + *
>> + * Copyright (c) 2010 David Gibson, IBM Corporation <dwg@au1.ibm.com>
>> + * Based on the s390 virtio bus code:
>> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>> + *
>> + * 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/>.
>> + */
>> +
>> +/* TODO: Add some infrastructure for "random stuff" and FIRs that
>> + * various units might want to deal with without creating actual
>> + * XSCOM devices.
>> + *
>> + * For example, HB LPC XSCOM in the PIBAM
>> + */
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "hw/hw.h"
>> +#include "sysemu/sysemu.h"
>> +#include "hw/boards.h"
>> +#include "monitor/monitor.h"
>> +#include "hw/loader.h"
>> +#include "elf.h"
>> +#include "hw/sysbus.h"
>> +#include "sysemu/kvm.h"
>> +#include "sysemu/device_tree.h"
>> +#include "hw/ppc/fdt.h"
>> +
>> +#include "hw/ppc/pnv_xscom.h"
>> +
>> +#include <libfdt.h>
>> +
>> +#define TYPE_XSCOM "xscom"
>> +#define XSCOM(obj) OBJECT_CHECK(XScomState, (obj), TYPE_XSCOM)
>> +
>> +#define XSCOM_SIZE        0x800000000ull
>> +#define XSCOM_BASE(chip)  (0x3fc0000000000ull + ((uint64_t)(chip)) * XSCOM_SIZE)
>> +
>> +
>> +typedef struct XScomState {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +    /*< public >*/
>> +
>> +    MemoryRegion mem;
>> +    int32_t chip_id;
> 
> Merging scom and chip would avoid the duplication of this field too.

yes.

>> +    PnvChipClass *chip_class;
>> +    XScomBus *bus;
>> +} XScomState;
>> +
>> +static uint32_t xscom_to_pcb_addr(uint64_t addr)
>> +{
>> +        addr &= (XSCOM_SIZE - 1);
>> +        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
>> +}
>> +
>> +static void xscom_complete(uint64_t hmer_bits)
>> +{
>> +    CPUState *cs = current_cpu;
>> +    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 XScomDevice *xscom_find_target(XScomState *s, uint32_t pcb_addr,
>> +                                      uint32_t *range)
>> +{
>> +    BusChild *bc;
>> +
>> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
>> +        DeviceState *qd = bc->child;
>> +        XScomDevice *xd = XSCOM_DEVICE(qd);
>> +        unsigned int i;
>> +
>> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
>> +            if (xd->ranges[i].addr <= pcb_addr &&
>> +                (xd->ranges[i].addr + xd->ranges[i].size) > pcb_addr) {
>> +                *range = i;
>> +                return xd;
>> +            }
>> +        }
>> +    }
> 
> Hmm.. you could set up a SCOM local address space using the
> infrastructure in memory.c, rather than doing your own dispatch.

I need to study this option a little deeper. 

> 
>> +    return NULL;
>> +}
>> +
>> +static bool xscom_dispatch_read(XScomState *s, uint32_t pcb_addr,
>> +                                uint64_t *out_val)
>> +{
>> +    uint32_t range, offset;
>> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
>> +    XScomDeviceClass *xc;
>> +
>> +    if (!xd) {
>> +        return false;
>> +    }
>> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
>> +    if (!xc->read) {
>> +        return false;
>> +    }
>> +    offset = pcb_addr - xd->ranges[range].addr;
>> +    return xc->read(xd, range, offset, out_val);
>> +}
>> +
>> +static bool xscom_dispatch_write(XScomState *s, uint32_t pcb_addr, uint64_t val)
>> +{
>> +    uint32_t range, offset;
>> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
>> +    XScomDeviceClass *xc;
>> +
>> +    if (!xd) {
>> +        return false;
>> +    }
>> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
>> +    if (!xc->write) {
>> +        return false;
>> +    }
>> +    offset = pcb_addr - xd->ranges[range].addr;
>> +    return xc->write(xd, range, offset, val);
>> +}
>> +
>> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
>> +{
>> +    XScomState *s = opaque;
>> +    uint32_t pcba = xscom_to_pcb_addr(addr);
>> +    uint64_t val;
>> +
>> +    assert(width == 8);
>> +
>> +    /* Handle some SCOMs here before dispatch */
>> +    switch (pcba) {
>> +    case 0xf000f:
>> +        val = s->chip_class->chip_f000f;
>> +        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;
>> +    default:
>> +        if (!xscom_dispatch_read(s, pcba, &val)) {
>> +            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)
>> +{
>> +    XScomState *s = opaque;
>> +    uint32_t pcba = xscom_to_pcb_addr(addr);
>> +
>> +    assert(width == 8);
>> +
>> +    /* 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, pcba, val)) {
>> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
>> +            return;
>> +        }
>> +    }
>> +
>> +    xscom_complete(HMER_XSCOM_DONE);
>> +}
>> +
>> +static const MemoryRegionOps 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 int xscom_init(SysBusDevice *dev)
>> +{
>> +    XScomState *s = XSCOM(dev);
>> +
>> +    s->chip_id = -1;
>> +    return 0;
>> +}
>> +
>> +static void xscom_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    XScomState *s = XSCOM(dev);
>> +    char *name;
>> +
>> +    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), &xscom_ops, s, name, XSCOM_SIZE);
>> +    sysbus_init_mmio(sbd, &s->mem);
>> +    sysbus_mmio_map(sbd, 0, XSCOM_BASE(s->chip_id));
>> +}
>> +
>> +static Property xscom_properties[] = {
>> +        DEFINE_PROP_INT32("chip_id", XScomState, chip_id, 0),
>> +        DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void xscom_class_init(ObjectClass *klass, void *data)
>> +{
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->props = xscom_properties;
>> +    dc->realize = xscom_realize;
>> +    k->init = xscom_init;
>> +}
>> +
>> +static const TypeInfo xscom_info = {
>> +    .name          = TYPE_XSCOM,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(XScomState),
>> +    .class_init    = xscom_class_init,
>> +};
>> +
>> +static void xscom_bus_class_init(ObjectClass *klass, void *data)
>> +{
>> +}
>> +
>> +static const TypeInfo xscom_bus_info = {
>> +    .name = TYPE_XSCOM_BUS,
>> +    .parent = TYPE_BUS,
>> +    .class_init = xscom_bus_class_init,
>> +    .instance_size = sizeof(XScomBus),
>> +};
>> +
>> +XScomBus *xscom_create(PnvChip *chip)
>> +{
>> +    DeviceState *dev;
>> +    XScomState *xdev;
>> +    BusState *qbus;
>> +    XScomBus *xb;
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +
>> +    dev = qdev_create(NULL, TYPE_XSCOM);
>> +    qdev_prop_set_uint32(dev, "chip_id", chip->chip_id);
>> +    qdev_init_nofail(dev);
>> +
>> +    /* Create bus on bridge device */
>> +    qbus = qbus_create(TYPE_XSCOM_BUS, dev, "xscom");
>> +    xb = DO_UPCAST(XScomBus, bus, qbus);
>> +    xb->chip_id = chip->chip_id;
>> +    xdev = XSCOM(dev);
>> +    xdev->bus = xb;
>> +    xdev->chip_class = pcc;
>> +
>> +    return xb;
>> +}
>> +
>> +int xscom_populate_fdt(XScomBus *xb, void *fdt, int root_offset)
> 
> What is the root_offset parameter for, since it is always 0?

yes ...

>> +{
>> +    BusChild *bc;
>> +    char *name;
>> +    const char compat[] = "ibm,power8-xscom\0ibm,xscom";
>> +    uint64_t reg[] = { cpu_to_be64(XSCOM_BASE(xb->chip_id)),
>> +                       cpu_to_be64(XSCOM_SIZE) };
>> +    int xscom_offset;
>> +
>> +    name = g_strdup_printf("xscom@%llx", (unsigned long long)
>> +                           be64_to_cpu(reg[0]));
> 
> Nit: in qemu the PRIx64 etc. macros are usually preferred to (unsigned
> long long) casts of this type.

sure.

>> +    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", xb->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)));
>> +
>> +    QTAILQ_FOREACH(bc, &xb->bus.children, sibling) {
>> +        DeviceState *qd = bc->child;
>> +        XScomDevice *xd = XSCOM_DEVICE(qd);
>> +        XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xd);
>> +        uint32_t reg[MAX_XSCOM_RANGES * 2];
>> +        unsigned int i, sz = 0;
>> +        void *cp, *p;
>> +        int child_offset;
>> +
>> +        /* Some XSCOM slaves may not be represented in the DT */
>> +        if (!xc->dt_name) {
>> +            continue;
>> +        }
>> +        name = g_strdup_printf("%s@%x", xc->dt_name, xd->ranges[0].addr);
>> +        child_offset = fdt_add_subnode(fdt, xscom_offset, name);
>> +        _FDT(child_offset);
>> +        g_free(name);
>> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
>> +            if (xd->ranges[i].size == 0) {
>> +                break;
>> +            }
>> +            reg[sz++] = cpu_to_be32(xd->ranges[i].addr);
>> +            reg[sz++] = cpu_to_be32(xd->ranges[i].size);
>> +        }
>> +        _FDT((fdt_setprop(fdt, child_offset, "reg", reg, sz * 4)));
> 
> Use of fdt_appendprop() might make this a little neater.

ok.

>> +        if (xc->devnode) {
>> +            _FDT((xc->devnode(xd, fdt, child_offset)));
>> +        }
>> +#define MAX_COMPATIBLE_PROP     1024
>> +        cp = p = g_malloc0(MAX_COMPATIBLE_PROP);
>> +        i = 0;
>> +        while ((p - cp) < MAX_COMPATIBLE_PROP) {
>> +            int l;
>> +            if (xc->dt_compatible[i] == NULL) {
>> +                break;
>> +            }
>> +            l = strlen(xc->dt_compatible[i]);
>> +            if (l >= (MAX_COMPATIBLE_PROP - i)) {
>> +                break;
>> +            }
>> +            strcpy(p, xc->dt_compatible[i++]);
>> +            p += l + 1;
>> +        }
>> +        _FDT((fdt_setprop(fdt, child_offset, "compatible", cp, p - cp)));
> 
> Might it be easier to just require the individual scom device to set
> the compatible property from its ->devnode callback?  That way it can
> just
> 	const char compat = "foo\0bar\0baz";
> 	fdt_setprop(..., compat, sizeof(compat));
> 
> and avoid this fiddling around with arrays.

Yes. xscom_populate_fdt() will shrink quite a bit. This is a good idea.

Thanks,

C.

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int xscom_qdev_init(DeviceState *qdev)
>> +{
>> +    XScomDevice *xdev = (XScomDevice *)qdev;
>> +    XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xdev);
>> +
>> +    if (xc->init) {
>> +        return xc->init(xdev);
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void xscom_device_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *k = DEVICE_CLASS(klass);
>> +    k->init = xscom_qdev_init;
>> +    k->bus_type = TYPE_XSCOM_BUS;
>> +}
>> +
>> +static const TypeInfo xscom_dev_info = {
>> +    .name = TYPE_XSCOM_DEVICE,
>> +    .parent = TYPE_DEVICE,
>> +    .instance_size = sizeof(XScomDevice),
>> +    .abstract = true,
>> +    .class_size = sizeof(XScomDeviceClass),
>> +    .class_init = xscom_device_class_init,
>> +};
>> +
>> +static void xscom_register_types(void)
>> +{
>> +    type_register_static(&xscom_info);
>> +    type_register_static(&xscom_bus_info);
>> +    type_register_static(&xscom_dev_info);
>> +}
>> +
>> +type_init(xscom_register_types)
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 1f32573dedff..bc6e1f80096b 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -35,12 +35,14 @@ typedef enum PnvChipType {
>>      PNV_CHIP_P8NVL, /* AKA Naples */
>>  } PnvChipType;
>>  
>> +typedef struct XScomBus XScomBus;
>>  typedef struct PnvChip {
>>      /*< private >*/
>>      SysBusDevice parent_obj;
>>  
>>      /*< public >*/
>>      uint32_t     chip_id;
>> +    XScomBus     *xscom;
>>  } PnvChip;
>>  
>>  typedef struct PnvChipClass {
>> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
>> new file mode 100644
>> index 000000000000..386ad21c5aa5
>> --- /dev/null
>> +++ b/include/hw/ppc/pnv_xscom.h
>> @@ -0,0 +1,75 @@
>> +#ifndef _HW_XSCOM_H
>> +#define _HW_XSCOM_H
>> +/*
>> + * QEMU PowerNV XSCOM bus definitions
>> + *
>> + * Copyright (c) 2010 David Gibson <david@gibson.dropbear.id.au>, IBM Corp.
>> + * Based on the s390 virtio bus definitions:
>> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>> + *
>> + * 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 <hw/ppc/pnv.h>
>> +
>> +#define TYPE_XSCOM_DEVICE "xscom-device"
>> +#define XSCOM_DEVICE(obj) \
>> +     OBJECT_CHECK(XScomDevice, (obj), TYPE_XSCOM_DEVICE)
>> +#define XSCOM_DEVICE_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(XScomDeviceClass, (klass), TYPE_XSCOM_DEVICE)
>> +#define XSCOM_DEVICE_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(XScomDeviceClass, (obj), TYPE_XSCOM_DEVICE)
>> +
>> +#define TYPE_XSCOM_BUS "xscom-bus"
>> +#define XSCOM_BUS(obj) OBJECT_CHECK(XScomBus, (obj), TYPE_XSCOM_BUS)
>> +
>> +typedef struct XScomDevice XScomDevice;
>> +typedef struct XScomBus XScomBus;
>> +
>> +typedef struct XScomDeviceClass {
>> +    DeviceClass parent_class;
>> +
>> +    const char *dt_name;
>> +    const char **dt_compatible;
>> +    int (*init)(XScomDevice *dev);
>> +    int (*devnode)(XScomDevice *dev, void *fdt, int offset);
>> +
>> +    /* Actual XScom accesses */
>> +    bool (*read)(XScomDevice *dev, uint32_t range, uint32_t offset,
>> +                 uint64_t *out_val);
>> +    bool (*write)(XScomDevice *dev, uint32_t range, uint32_t offset,
>> +                  uint64_t val);
>> +} XScomDeviceClass;
>> +
>> +typedef struct XScomRange {
>> +    uint32_t addr;
>> +    uint32_t size;
>> +} XScomRange;
>> +
>> +struct XScomDevice {
>> +    DeviceState qdev;
>> +#define MAX_XSCOM_RANGES 4
>> +    struct XScomRange ranges[MAX_XSCOM_RANGES];
>> +};
>> +
>> +struct XScomBus {
>> +    BusState bus;
>> +    uint32_t chip_id;
>> +};
>> +
>> +extern XScomBus *xscom_create(PnvChip *chip);
>> +extern int xscom_populate_fdt(XScomBus *xscom, void *fdt, int offset);
>> +
>> +
>> +#endif /* _HW_XSCOM_H */
>

On 09/06/2016 02:48 AM, David Gibson wrote:
> On Mon, Sep 05, 2016 at 05:11:53PM +1000, Benjamin Herrenschmidt wrote:
>> On Mon, 2016-09-05 at 13:39 +1000, David Gibson wrote:
>>>> +static XScomDevice *xscom_find_target(XScomState *s, uint32_t
>>> pcb_addr,
>>>> +                                      uint32_t *range)
>>>> +{
>>>> +    BusChild *bc;
>>>> +
>>>> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
>>>> +        DeviceState *qd = bc->child;
>>>> +        XScomDevice *xd = XSCOM_DEVICE(qd);
>>>> +        unsigned int i;
>>>> +
>>>> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
>>>> +            if (xd->ranges[i].addr <= pcb_addr &&
>>>> +                (xd->ranges[i].addr + xd->ranges[i].size) >
>>> pcb_addr) {
>>>> +                *range = i;
>>>> +                return xd;
>>>> +            }
>>>> +        }
>>>> +    }
>>>
>>> Hmm.. you could set up a SCOM local address space using the
>>> infrastructure in memory.c, rather than doing your own dispatch.
>>
>> There are pros and cons to this approach. The memory.c stuff comes with
>> quite a lot of baggage, not all of it very shinny to be honest ;-) I
>> still *hate* how it forces upon us a whole 128-bit integer arithmetic
>> library just so that it can represent 1_0000_0000_0000_0000 ... 
> 
> Ugh, yeah.  I tried to argue against this when it first came in, but
> was overruled.
> 
>> It would be make more sense to use inclusive start/end instead and
>> stick to 64-bits.
>>
>> That being said, we could do that. We'd have to shift the XSCOM
>> addresses left by 3 since each address is an 8 bytes reigster and
>> forbid non-8-bytes accesses.
> 
> Ok.  I'm not particularly fussed either way.


The change does seem too invasive. I can give it a try in next
version.

When a memory region is triggered, the impacted device will have
to convert the address with xscom_to_pcb_addr(). There is some 
dependency on the chip model because the translation is different. 
That would be an extra op in the xscom device model I guess. 

Also, the main purpose of the XscomBus is to loop on the devices 
to populate the device tree. I am wondering if we could just use 
a simple list under the chip for that purpose. 

Thanks,

C.

Hello Sam,

>> +        }
>> +#define MAX_COMPATIBLE_PROP     1024
>> +        cp = p = g_malloc0(MAX_COMPATIBLE_PROP);
>> +        i = 0;
>> +        while ((p - cp) < MAX_COMPATIBLE_PROP) {
>> +            int l;
>> +            if (xc->dt_compatible[i] == NULL) {
>> +                break;
>> +            }
>> +            l = strlen(xc->dt_compatible[i]);
>> +            if (l >= (MAX_COMPATIBLE_PROP - i)) {
> 
> The use of 'i' above doesn't look right. Should the check be more like this?
>                if ((l + 1) >= (MAX_COMPATIBLE_PROP - (p - cp))) {

David just proposed to move the compatible property setting in the
devnode op of the device, and so all this code should disappear at 
the same time.

Thanks,

C. 

>> +                break;
>> +            }
>> +            strcpy(p, xc->dt_compatible[i++]);
>> +            p += l + 1;
>> +        }
>> +        _FDT((fdt_setprop(fdt, child_offset, "compatible", cp, p - cp)));
>> +    }
>> +
>> +    return 0;
>> +}
>> +

On Tue, 2016-09-06 at 16:42 +0200, Cédric Le Goater wrote:
> > Alternatively.. it might be simpler to just drop the SCOM as a
> > separate device.  I think you could just hang the scom bus directly
> > off the chip object.  IIUC the scom is basically the only chip-
> level
> > control mechanism, so this does make a certain amount of sense.
> 
> yes. I am exposing more and more stuff of the chip object under the 
> xscom object so we should merge them. I agree. We will still need 
> some XScomDevice of some sort.

What you can do is split it this way which matches the HW:

 - The chip object is the XSCOM parent, it owns the XSCOM bus,
and expose functions (methods) to read/write XSCOMs. WE could rename
XSCOM to "PIB" or "PCB" which is the real name of the bus ;-) But that
might confuse things more than help .

 - A separate ADU object on each chip that is a SysDevice and does the
MMIO bridge to XSCOM. It decodes the MMIO range for that chip and calls
the above accessors.

That makes it easy to generate XSCOMs using different mechanisms if we
wish to do so, which could come in handy, such as monitor commands, or
if we ever do cosimulation with a separate BMC, a simulated FSI, all by
just calling the first object's methods.

Cheers,
Ben.

On Tue, 2016-09-06 at 16:42 +0200, Cédric Le Goater wrote:
> 
> The change does seem too invasive. I can give it a try in next
> version.
> 
> When a memory region is triggered, the impacted device will have
> to convert the address with xscom_to_pcb_addr(). There is some 
> dependency on the chip model because the translation is different. 
> That would be an extra op in the xscom device model I guess. 

No. If you split the XSCOM bus from the MMIO -> XSCOM bridge (the ADU)
then the conversion only happens in the former. You don't directly
route the MMIOs over ! You intercept the MMIOs and use use the
address_space_rw to "generate" the XSCOM accesses on the other side,
like I do for the LPC bus.

We need that anyway because of the way XSCOMs can manipulate the HMER
etc...

> Also, the main purpose of the XscomBus is to loop on the devices 
> to populate the device tree. I am wondering if we could just use 
> a simple list under the chip for that purpose.

On Wed, 2016-09-07 at 07:47 +1000, Benjamin Herrenschmidt wrote:
> d be an extra op in the xscom device model I guess. 
> 
> No. If you split the XSCOM bus from the MMIO -> XSCOM bridge (the
> ADU)
> then the conversion only happens in the former. You don't directly
> route the MMIOs over ! You intercept the MMIOs and use use the
> address_space_rw to "generate" the XSCOM accesses on the other side,
> like I do for the LPC bus.
> 
> We need that anyway because of the way XSCOMs can manipulate the HMER
> etc...
> 
> > 
> > Also, the main purpose of the XscomBus is to loop on the devices 
> > to populate the device tree. I am wondering if we could just use 
> > a simple list under the chip for that purpose.

In fact, if you do the above, you no longer need a XSCOM device...

A number of "devices" can exist below a chip, all they need to have
XSCOMs is to register memory regions that are child of that chip's
xscom_region.

For device-tree, well, we could have a generic interface that anything
that can populate DT has and iterate through them. Or make a "chiplet"
class or something.

Cheers,
Ben.

On Tue, Sep 06, 2016 at 04:42:01PM +0200, Cédric Le Goater wrote:
> On 09/05/2016 05:39 AM, David Gibson wrote:
> > On Wed, Aug 31, 2016 at 06:34:11PM +0200, Cédric Le Goater wrote:
> >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>
> >> XSCOM is an interface to a sideband bus provided by the POWER8 chip
> >> pervasive unit, which gives access to a number of facilities in the
> >> chip that are needed by the OPAL firmware and to a lesser extent,
> >> Linux. This is among others how the PCI Host bridges get configured
> >> at boot or how the LPC bus is accessed.
> >>
> >> This provides a simple bus and device type for devices sitting on
> >> XSCOM along with some facilities to optionally generate corresponding
> >> device-tree nodes
> >>
> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >> [clg: updated for qemu-2.7
> >>       ported on new sPowerNVMachineState which was merged with PnvSystem
> >>       removed TRACE_XSCOM
> >>       fixed checkpatch errors
> >>       replaced assert with error_setg in xscom_realize()
> >>       reworked xscom_create
> >>       introduced the use of the chip_class for chip model contants
> >>       ]
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >>  They were some discussions on whether we should use a qemu
> >>  address_space instead of the xscom ranges defined in this patch. 
> >>  I gave it try, it is possible but it brings extra unnecessary calls
> >>  and complexity. I think the current solution is better.
> >>
> >>  hw/ppc/Makefile.objs       |   2 +-
> >>  hw/ppc/pnv.c               |  11 ++
> >>  hw/ppc/pnv_xscom.c         | 408 +++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h       |   2 +
> >>  include/hw/ppc/pnv_xscom.h |  75 +++++++++
> >>  5 files changed, 497 insertions(+), 1 deletion(-)
> >>  create mode 100644 hw/ppc/pnv_xscom.c
> >>  create mode 100644 include/hw/ppc/pnv_xscom.h
> >>
> >> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >> index 8105db7d5600..f580e5c41413 100644
> >> --- a/hw/ppc/Makefile.objs
> >> +++ b/hw/ppc/Makefile.objs
> >> @@ -6,7 +6,7 @@ obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> >>  obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >>  # IBM PowerNV
> >> -obj-$(CONFIG_POWERNV) += pnv.o
> >> +obj-$(CONFIG_POWERNV) += pnv.o pnv_xscom.o
> >>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >>  obj-y += spapr_pci_vfio.o
> >>  endif
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 06051268e200..a6e7f66b2c0a 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -39,6 +39,8 @@
> >>  #include "exec/address-spaces.h"
> >>  #include "qemu/cutils.h"
> >>  
> >> +#include "hw/ppc/pnv_xscom.h"
> >> +
> >>  #include <libfdt.h>
> >>  
> >>  #define FDT_ADDR                0x01000000
> >> @@ -103,6 +105,7 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
> >>      char *buf;
> >>      const char plat_compat[] = "qemu,powernv\0ibm,powernv";
> >>      int off;
> >> +    int i;
> >>  
> >>      fdt = g_malloc0(FDT_MAX_SIZE);
> >>      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> >> @@ -142,6 +145,11 @@ static void *powernv_create_fdt(PnvMachineState *pnv,
> >>      /* Memory */
> >>      powernv_populate_memory(fdt);
> >>  
> >> +    /* Populate XSCOM for each chip */
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        xscom_populate_fdt(pnv->chips[i]->xscom, fdt, 0);
> >> +    }
> > 
> > There will be a bunch of per-chip things in the fdt - I suggest a
> > common function to do all the fdt creation for a single chip.  Since
> > you've moved to using the fdt_rw functions exclusively, you don't have
> > the sequence constraints that would have made that awkward previously.
> 
> ok.
> 
> >>      return fdt;
> >>  }
> >>  
> >> @@ -305,6 +313,9 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
> >>      PnvChip *chip = PNV_CHIP(dev);
> >>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >>  
> >> +    /* Set up XSCOM bus */
> >> +    chip->xscom = xscom_create(chip);
> > 
> > So, this creates the xscom as a new device object, unfortunately doing
> > that from another object's realize() violations QOM lifetime rules as
> > I understand them.  I think - I have to admit I'm pretty confused on
> > this topic myself.
> > 
> > You could construct the scom from chip init, then realize it from chip
> > realize, but I think using object_new() (via qdev_create()) from
> > another object's init also breaks the rules.  You are however allowed
> > to allocate the scom state statically within the chip and use
> > object_initialize() instead, AIUI.
> > 
> > Alternatively.. it might be simpler to just drop the SCOM as a
> > separate device.  I think you could just hang the scom bus directly
> > off the chip object.  IIUC the scom is basically the only chip-level
> > control mechanism, so this does make a certain amount of sense.
> 
> yes. I am exposing more and more stuff of the chip object under the 
> xscom object so we should merge them. I agree. We will still need 
> some XScomDevice of some sort.
> 
> >>      pcc->realize(chip, errp);
> >>  }
> >>  
> >> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> >> new file mode 100644
> >> index 000000000000..7ed3804f4b3a
> >> --- /dev/null
> >> +++ b/hw/ppc/pnv_xscom.c
> >> @@ -0,0 +1,408 @@
> >> +
> >> +/*
> >> + * QEMU PowerNV XSCOM bus definitions
> >> + *
> >> + * Copyright (c) 2010 David Gibson, IBM Corporation <dwg@au1.ibm.com>
> >> + * Based on the s390 virtio bus code:
> >> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> >> + *
> >> + * 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/>.
> >> + */
> >> +
> >> +/* TODO: Add some infrastructure for "random stuff" and FIRs that
> >> + * various units might want to deal with without creating actual
> >> + * XSCOM devices.
> >> + *
> >> + * For example, HB LPC XSCOM in the PIBAM
> >> + */
> >> +#include "qemu/osdep.h"
> >> +#include "qapi/error.h"
> >> +#include "hw/hw.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "hw/boards.h"
> >> +#include "monitor/monitor.h"
> >> +#include "hw/loader.h"
> >> +#include "elf.h"
> >> +#include "hw/sysbus.h"
> >> +#include "sysemu/kvm.h"
> >> +#include "sysemu/device_tree.h"
> >> +#include "hw/ppc/fdt.h"
> >> +
> >> +#include "hw/ppc/pnv_xscom.h"
> >> +
> >> +#include <libfdt.h>
> >> +
> >> +#define TYPE_XSCOM "xscom"
> >> +#define XSCOM(obj) OBJECT_CHECK(XScomState, (obj), TYPE_XSCOM)
> >> +
> >> +#define XSCOM_SIZE        0x800000000ull
> >> +#define XSCOM_BASE(chip)  (0x3fc0000000000ull + ((uint64_t)(chip)) * XSCOM_SIZE)
> >> +
> >> +
> >> +typedef struct XScomState {
> >> +    /*< private >*/
> >> +    SysBusDevice parent_obj;
> >> +    /*< public >*/
> >> +
> >> +    MemoryRegion mem;
> >> +    int32_t chip_id;
> > 
> > Merging scom and chip would avoid the duplication of this field too.
> 
> yes.
> 
> >> +    PnvChipClass *chip_class;
> >> +    XScomBus *bus;
> >> +} XScomState;
> >> +
> >> +static uint32_t xscom_to_pcb_addr(uint64_t addr)
> >> +{
> >> +        addr &= (XSCOM_SIZE - 1);
> >> +        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> >> +}
> >> +
> >> +static void xscom_complete(uint64_t hmer_bits)
> >> +{
> >> +    CPUState *cs = current_cpu;
> >> +    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 XScomDevice *xscom_find_target(XScomState *s, uint32_t pcb_addr,
> >> +                                      uint32_t *range)
> >> +{
> >> +    BusChild *bc;
> >> +
> >> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
> >> +        DeviceState *qd = bc->child;
> >> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> >> +        unsigned int i;
> >> +
> >> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> >> +            if (xd->ranges[i].addr <= pcb_addr &&
> >> +                (xd->ranges[i].addr + xd->ranges[i].size) > pcb_addr) {
> >> +                *range = i;
> >> +                return xd;
> >> +            }
> >> +        }
> >> +    }
> > 
> > Hmm.. you could set up a SCOM local address space using the
> > infrastructure in memory.c, rather than doing your own dispatch.
> 
> I need to study this option a little deeper. 

So.. something I realized a bit later.

The obvious way of changing XScomDevice to a QOM interface isn't
really compatible with using the address space infrastructure.  You'd
have read/write methods in the interface, and since that's not the
same interface as an MR you'd need to do your own address decode /
dispatch as you do above.

That does suggest an alternative approach though.  You could remove
XScomDevice entirely from QOM existence, and just expose the xscom
address space globally, much like address_space_memory.  The
individual devices could just register their own subregions within it.

I'm not sure if the latter is a good idea, though.

> 
> > 
> >> +    return NULL;
> >> +}
> >> +
> >> +static bool xscom_dispatch_read(XScomState *s, uint32_t pcb_addr,
> >> +                                uint64_t *out_val)
> >> +{
> >> +    uint32_t range, offset;
> >> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
> >> +    XScomDeviceClass *xc;
> >> +
> >> +    if (!xd) {
> >> +        return false;
> >> +    }
> >> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
> >> +    if (!xc->read) {
> >> +        return false;
> >> +    }
> >> +    offset = pcb_addr - xd->ranges[range].addr;
> >> +    return xc->read(xd, range, offset, out_val);
> >> +}
> >> +
> >> +static bool xscom_dispatch_write(XScomState *s, uint32_t pcb_addr, uint64_t val)
> >> +{
> >> +    uint32_t range, offset;
> >> +    struct XScomDevice *xd = xscom_find_target(s, pcb_addr, &range);
> >> +    XScomDeviceClass *xc;
> >> +
> >> +    if (!xd) {
> >> +        return false;
> >> +    }
> >> +    xc = XSCOM_DEVICE_GET_CLASS(xd);
> >> +    if (!xc->write) {
> >> +        return false;
> >> +    }
> >> +    offset = pcb_addr - xd->ranges[range].addr;
> >> +    return xc->write(xd, range, offset, val);
> >> +}
> >> +
> >> +static uint64_t xscom_read(void *opaque, hwaddr addr, unsigned width)
> >> +{
> >> +    XScomState *s = opaque;
> >> +    uint32_t pcba = xscom_to_pcb_addr(addr);
> >> +    uint64_t val;
> >> +
> >> +    assert(width == 8);
> >> +
> >> +    /* Handle some SCOMs here before dispatch */
> >> +    switch (pcba) {
> >> +    case 0xf000f:
> >> +        val = s->chip_class->chip_f000f;
> >> +        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;
> >> +    default:
> >> +        if (!xscom_dispatch_read(s, pcba, &val)) {
> >> +            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)
> >> +{
> >> +    XScomState *s = opaque;
> >> +    uint32_t pcba = xscom_to_pcb_addr(addr);
> >> +
> >> +    assert(width == 8);
> >> +
> >> +    /* 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, pcba, val)) {
> >> +            xscom_complete(HMER_XSCOM_FAIL | HMER_XSCOM_DONE);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    xscom_complete(HMER_XSCOM_DONE);
> >> +}
> >> +
> >> +static const MemoryRegionOps 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 int xscom_init(SysBusDevice *dev)
> >> +{
> >> +    XScomState *s = XSCOM(dev);
> >> +
> >> +    s->chip_id = -1;
> >> +    return 0;
> >> +}
> >> +
> >> +static void xscom_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >> +    XScomState *s = XSCOM(dev);
> >> +    char *name;
> >> +
> >> +    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), &xscom_ops, s, name, XSCOM_SIZE);
> >> +    sysbus_init_mmio(sbd, &s->mem);
> >> +    sysbus_mmio_map(sbd, 0, XSCOM_BASE(s->chip_id));
> >> +}
> >> +
> >> +static Property xscom_properties[] = {
> >> +        DEFINE_PROP_INT32("chip_id", XScomState, chip_id, 0),
> >> +        DEFINE_PROP_END_OF_LIST(),
> >> +};
> >> +
> >> +static void xscom_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> >> +    DeviceClass *dc = DEVICE_CLASS(klass);
> >> +
> >> +    dc->props = xscom_properties;
> >> +    dc->realize = xscom_realize;
> >> +    k->init = xscom_init;
> >> +}
> >> +
> >> +static const TypeInfo xscom_info = {
> >> +    .name          = TYPE_XSCOM,
> >> +    .parent        = TYPE_SYS_BUS_DEVICE,
> >> +    .instance_size = sizeof(XScomState),
> >> +    .class_init    = xscom_class_init,
> >> +};
> >> +
> >> +static void xscom_bus_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +}
> >> +
> >> +static const TypeInfo xscom_bus_info = {
> >> +    .name = TYPE_XSCOM_BUS,
> >> +    .parent = TYPE_BUS,
> >> +    .class_init = xscom_bus_class_init,
> >> +    .instance_size = sizeof(XScomBus),
> >> +};
> >> +
> >> +XScomBus *xscom_create(PnvChip *chip)
> >> +{
> >> +    DeviceState *dev;
> >> +    XScomState *xdev;
> >> +    BusState *qbus;
> >> +    XScomBus *xb;
> >> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >> +
> >> +    dev = qdev_create(NULL, TYPE_XSCOM);
> >> +    qdev_prop_set_uint32(dev, "chip_id", chip->chip_id);
> >> +    qdev_init_nofail(dev);
> >> +
> >> +    /* Create bus on bridge device */
> >> +    qbus = qbus_create(TYPE_XSCOM_BUS, dev, "xscom");
> >> +    xb = DO_UPCAST(XScomBus, bus, qbus);
> >> +    xb->chip_id = chip->chip_id;
> >> +    xdev = XSCOM(dev);
> >> +    xdev->bus = xb;
> >> +    xdev->chip_class = pcc;
> >> +
> >> +    return xb;
> >> +}
> >> +
> >> +int xscom_populate_fdt(XScomBus *xb, void *fdt, int root_offset)
> > 
> > What is the root_offset parameter for, since it is always 0?
> 
> yes ...
> 
> >> +{
> >> +    BusChild *bc;
> >> +    char *name;
> >> +    const char compat[] = "ibm,power8-xscom\0ibm,xscom";
> >> +    uint64_t reg[] = { cpu_to_be64(XSCOM_BASE(xb->chip_id)),
> >> +                       cpu_to_be64(XSCOM_SIZE) };
> >> +    int xscom_offset;
> >> +
> >> +    name = g_strdup_printf("xscom@%llx", (unsigned long long)
> >> +                           be64_to_cpu(reg[0]));
> > 
> > Nit: in qemu the PRIx64 etc. macros are usually preferred to (unsigned
> > long long) casts of this type.
> 
> sure.
> 
> >> +    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", xb->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)));
> >> +
> >> +    QTAILQ_FOREACH(bc, &xb->bus.children, sibling) {
> >> +        DeviceState *qd = bc->child;
> >> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> >> +        XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xd);
> >> +        uint32_t reg[MAX_XSCOM_RANGES * 2];
> >> +        unsigned int i, sz = 0;
> >> +        void *cp, *p;
> >> +        int child_offset;
> >> +
> >> +        /* Some XSCOM slaves may not be represented in the DT */
> >> +        if (!xc->dt_name) {
> >> +            continue;
> >> +        }
> >> +        name = g_strdup_printf("%s@%x", xc->dt_name, xd->ranges[0].addr);
> >> +        child_offset = fdt_add_subnode(fdt, xscom_offset, name);
> >> +        _FDT(child_offset);
> >> +        g_free(name);
> >> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> >> +            if (xd->ranges[i].size == 0) {
> >> +                break;
> >> +            }
> >> +            reg[sz++] = cpu_to_be32(xd->ranges[i].addr);
> >> +            reg[sz++] = cpu_to_be32(xd->ranges[i].size);
> >> +        }
> >> +        _FDT((fdt_setprop(fdt, child_offset, "reg", reg, sz * 4)));
> > 
> > Use of fdt_appendprop() might make this a little neater.
> 
> ok.
> 
> >> +        if (xc->devnode) {
> >> +            _FDT((xc->devnode(xd, fdt, child_offset)));
> >> +        }
> >> +#define MAX_COMPATIBLE_PROP     1024
> >> +        cp = p = g_malloc0(MAX_COMPATIBLE_PROP);
> >> +        i = 0;
> >> +        while ((p - cp) < MAX_COMPATIBLE_PROP) {
> >> +            int l;
> >> +            if (xc->dt_compatible[i] == NULL) {
> >> +                break;
> >> +            }
> >> +            l = strlen(xc->dt_compatible[i]);
> >> +            if (l >= (MAX_COMPATIBLE_PROP - i)) {
> >> +                break;
> >> +            }
> >> +            strcpy(p, xc->dt_compatible[i++]);
> >> +            p += l + 1;
> >> +        }
> >> +        _FDT((fdt_setprop(fdt, child_offset, "compatible", cp, p - cp)));
> > 
> > Might it be easier to just require the individual scom device to set
> > the compatible property from its ->devnode callback?  That way it can
> > just
> > 	const char compat = "foo\0bar\0baz";
> > 	fdt_setprop(..., compat, sizeof(compat));
> > 
> > and avoid this fiddling around with arrays.
> 
> Yes. xscom_populate_fdt() will shrink quite a bit. This is a good idea.
> 
> Thanks,
> 
> C.
> 
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int xscom_qdev_init(DeviceState *qdev)
> >> +{
> >> +    XScomDevice *xdev = (XScomDevice *)qdev;
> >> +    XScomDeviceClass *xc = XSCOM_DEVICE_GET_CLASS(xdev);
> >> +
> >> +    if (xc->init) {
> >> +        return xc->init(xdev);
> >> +    }
> >> +    return 0;
> >> +}
> >> +
> >> +static void xscom_device_class_init(ObjectClass *klass, void *data)
> >> +{
> >> +    DeviceClass *k = DEVICE_CLASS(klass);
> >> +    k->init = xscom_qdev_init;
> >> +    k->bus_type = TYPE_XSCOM_BUS;
> >> +}
> >> +
> >> +static const TypeInfo xscom_dev_info = {
> >> +    .name = TYPE_XSCOM_DEVICE,
> >> +    .parent = TYPE_DEVICE,
> >> +    .instance_size = sizeof(XScomDevice),
> >> +    .abstract = true,
> >> +    .class_size = sizeof(XScomDeviceClass),
> >> +    .class_init = xscom_device_class_init,
> >> +};
> >> +
> >> +static void xscom_register_types(void)
> >> +{
> >> +    type_register_static(&xscom_info);
> >> +    type_register_static(&xscom_bus_info);
> >> +    type_register_static(&xscom_dev_info);
> >> +}
> >> +
> >> +type_init(xscom_register_types)
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 1f32573dedff..bc6e1f80096b 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -35,12 +35,14 @@ typedef enum PnvChipType {
> >>      PNV_CHIP_P8NVL, /* AKA Naples */
> >>  } PnvChipType;
> >>  
> >> +typedef struct XScomBus XScomBus;
> >>  typedef struct PnvChip {
> >>      /*< private >*/
> >>      SysBusDevice parent_obj;
> >>  
> >>      /*< public >*/
> >>      uint32_t     chip_id;
> >> +    XScomBus     *xscom;
> >>  } PnvChip;
> >>  
> >>  typedef struct PnvChipClass {
> >> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> >> new file mode 100644
> >> index 000000000000..386ad21c5aa5
> >> --- /dev/null
> >> +++ b/include/hw/ppc/pnv_xscom.h
> >> @@ -0,0 +1,75 @@
> >> +#ifndef _HW_XSCOM_H
> >> +#define _HW_XSCOM_H
> >> +/*
> >> + * QEMU PowerNV XSCOM bus definitions
> >> + *
> >> + * Copyright (c) 2010 David Gibson <david@gibson.dropbear.id.au>, IBM Corp.
> >> + * Based on the s390 virtio bus definitions:
> >> + * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> >> + *
> >> + * 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 <hw/ppc/pnv.h>
> >> +
> >> +#define TYPE_XSCOM_DEVICE "xscom-device"
> >> +#define XSCOM_DEVICE(obj) \
> >> +     OBJECT_CHECK(XScomDevice, (obj), TYPE_XSCOM_DEVICE)
> >> +#define XSCOM_DEVICE_CLASS(klass) \
> >> +     OBJECT_CLASS_CHECK(XScomDeviceClass, (klass), TYPE_XSCOM_DEVICE)
> >> +#define XSCOM_DEVICE_GET_CLASS(obj) \
> >> +     OBJECT_GET_CLASS(XScomDeviceClass, (obj), TYPE_XSCOM_DEVICE)
> >> +
> >> +#define TYPE_XSCOM_BUS "xscom-bus"
> >> +#define XSCOM_BUS(obj) OBJECT_CHECK(XScomBus, (obj), TYPE_XSCOM_BUS)
> >> +
> >> +typedef struct XScomDevice XScomDevice;
> >> +typedef struct XScomBus XScomBus;
> >> +
> >> +typedef struct XScomDeviceClass {
> >> +    DeviceClass parent_class;
> >> +
> >> +    const char *dt_name;
> >> +    const char **dt_compatible;
> >> +    int (*init)(XScomDevice *dev);
> >> +    int (*devnode)(XScomDevice *dev, void *fdt, int offset);
> >> +
> >> +    /* Actual XScom accesses */
> >> +    bool (*read)(XScomDevice *dev, uint32_t range, uint32_t offset,
> >> +                 uint64_t *out_val);
> >> +    bool (*write)(XScomDevice *dev, uint32_t range, uint32_t offset,
> >> +                  uint64_t val);
> >> +} XScomDeviceClass;
> >> +
> >> +typedef struct XScomRange {
> >> +    uint32_t addr;
> >> +    uint32_t size;
> >> +} XScomRange;
> >> +
> >> +struct XScomDevice {
> >> +    DeviceState qdev;
> >> +#define MAX_XSCOM_RANGES 4
> >> +    struct XScomRange ranges[MAX_XSCOM_RANGES];
> >> +};
> >> +
> >> +struct XScomBus {
> >> +    BusState bus;
> >> +    uint32_t chip_id;
> >> +};
> >> +
> >> +extern XScomBus *xscom_create(PnvChip *chip);
> >> +extern int xscom_populate_fdt(XScomBus *xscom, void *fdt, int offset);
> >> +
> >> +
> >> +#endif /* _HW_XSCOM_H */
> > 
>

On Tue, Sep 06, 2016 at 04:42:58PM +0200, Cédric Le Goater wrote:
> On 09/06/2016 02:48 AM, David Gibson wrote:
> > On Mon, Sep 05, 2016 at 05:11:53PM +1000, Benjamin Herrenschmidt wrote:
> >> On Mon, 2016-09-05 at 13:39 +1000, David Gibson wrote:
> >>>> +static XScomDevice *xscom_find_target(XScomState *s, uint32_t
> >>> pcb_addr,
> >>>> +                                      uint32_t *range)
> >>>> +{
> >>>> +    BusChild *bc;
> >>>> +
> >>>> +    QTAILQ_FOREACH(bc, &s->bus->bus.children, sibling) {
> >>>> +        DeviceState *qd = bc->child;
> >>>> +        XScomDevice *xd = XSCOM_DEVICE(qd);
> >>>> +        unsigned int i;
> >>>> +
> >>>> +        for (i = 0; i < MAX_XSCOM_RANGES; i++) {
> >>>> +            if (xd->ranges[i].addr <= pcb_addr &&
> >>>> +                (xd->ranges[i].addr + xd->ranges[i].size) >
> >>> pcb_addr) {
> >>>> +                *range = i;
> >>>> +                return xd;
> >>>> +            }
> >>>> +        }
> >>>> +    }
> >>>
> >>> Hmm.. you could set up a SCOM local address space using the
> >>> infrastructure in memory.c, rather than doing your own dispatch.
> >>
> >> There are pros and cons to this approach. The memory.c stuff comes with
> >> quite a lot of baggage, not all of it very shinny to be honest ;-) I
> >> still *hate* how it forces upon us a whole 128-bit integer arithmetic
> >> library just so that it can represent 1_0000_0000_0000_0000 ... 
> > 
> > Ugh, yeah.  I tried to argue against this when it first came in, but
> > was overruled.
> > 
> >> It would be make more sense to use inclusive start/end instead and
> >> stick to 64-bits.
> >>
> >> That being said, we could do that. We'd have to shift the XSCOM
> >> addresses left by 3 since each address is an 8 bytes reigster and
> >> forbid non-8-bytes accesses.
> > 
> > Ok.  I'm not particularly fussed either way.
> 
> 
> The change does seem too invasive. I can give it a try in next
> version.
> 
> When a memory region is triggered, the impacted device will have
> to convert the address with xscom_to_pcb_addr(). There is some 
> dependency on the chip model because the translation is different. 
> That would be an extra op in the xscom device model I guess. 

Actually, I was still thinking of having an MR for the scom interface
unit, which does the xscom_to_pcb_addr() then re-issues the access in
the PCB address space.

But your suggestion might work too.

> Also, the main purpose of the XscomBus is to loop on the devices 
> to populate the device tree. I am wondering if we could just use 
> a simple list under the chip for that purpose. 
> 
> Thanks,
> 
> C. 
>

On Wed, Sep 07, 2016 at 07:45:49AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2016-09-06 at 16:42 +0200, Cédric Le Goater wrote:
> > > Alternatively.. it might be simpler to just drop the SCOM as a
> > > separate device.  I think you could just hang the scom bus directly
> > > off the chip object.  IIUC the scom is basically the only chip-
> > level
> > > control mechanism, so this does make a certain amount of sense.
> > 
> > yes. I am exposing more and more stuff of the chip object under the 
> > xscom object so we should merge them. I agree. We will still need 
> > some XScomDevice of some sort.
> 
> What you can do is split it this way which matches the HW:
> 
>  - The chip object is the XSCOM parent, it owns the XSCOM bus,
> and expose functions (methods) to read/write XSCOMs. WE could rename
> XSCOM to "PIB" or "PCB" which is the real name of the bus ;-) But that
> might confuse things more than help .
> 
>  - A separate ADU object on each chip that is a SysDevice and does the
> MMIO bridge to XSCOM. It decodes the MMIO range for that chip and calls
> the above accessors.
> 
> That makes it easy to generate XSCOMs using different mechanisms if we
> wish to do so, which could come in handy, such as monitor commands, or
> if we ever do cosimulation with a separate BMC, a simulated FSI, all by
> just calling the first object's methods.

Not what I had in mind - I still was thinking of having the xscom
access unit do the translation and re-issue into the pcb bus address
space.

But sounds like this other approach could have some advantages.

On Wed, Sep 07, 2016 at 07:47:16AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2016-09-06 at 16:42 +0200, Cédric Le Goater wrote:
> > 
> > The change does seem too invasive. I can give it a try in next
> > version.
> > 
> > When a memory region is triggered, the impacted device will have
> > to convert the address with xscom_to_pcb_addr(). There is some 
> > dependency on the chip model because the translation is different. 
> > That would be an extra op in the xscom device model I guess. 
> 
> No. If you split the XSCOM bus from the MMIO -> XSCOM bridge (the ADU)
> then the conversion only happens in the former. You don't directly
> route the MMIOs over ! You intercept the MMIOs and use use the
> address_space_rw to "generate" the XSCOM accesses on the other side,
> like I do for the LPC bus.

Oh.. wait.. that is what I had in mind, I think I misinterpreted
something one of you said.

> 
> We need that anyway because of the way XSCOMs can manipulate the HMER
> etc...
> 
> > Also, the main purpose of the XscomBus is to loop on the devices 
> > to populate the device tree. I am wondering if we could just use 
> > a simple list under the chip for that purpose.
> 
>

On Wed, 2016-09-07 at 11:59 +1000, David Gibson wrote:
> 
> That does suggest an alternative approach though.  You could remove
> XScomDevice entirely from QOM existence, and just expose the xscom
> address space globally, much like address_space_memory.  The
> individual devices could just register their own subregions within
> it.
> 
> I'm not sure if the latter is a good idea, though.

Not globally, per chip.

Cheers,
Ben.

On Wed, Sep 07, 2016 at 03:27:46PM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2016-09-07 at 11:59 +1000, David Gibson wrote:
> > 
> > That does suggest an alternative approach though.  You could remove
> > XScomDevice entirely from QOM existence, and just expose the xscom
> > address space globally, much like address_space_memory.  The
> > individual devices could just register their own subregions within
> > it.
> > 
> > I'm not sure if the latter is a good idea, though.
> 
> Not globally, per chip.

Right, that's probably better.  Not immediately sure how the
scomdevice would get hold of its chip's scom AS, but we can probably
figure out something.

On Wed, 2016-09-07 at 15:46 +1000, David Gibson wrote:
> Right, that's probably better.  Not immediately sure how the
> scomdevice would get hold of its chip's scom AS, but we can probably
> figure out something.

Passed at instanciation ?

Cheers,
Ben.

On 09/06/2016 11:47 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2016-09-06 at 16:42 +0200, Cédric Le Goater wrote:
>>
>> The change does seem too invasive. I can give it a try in next
>> version.
>>
>> When a memory region is triggered, the impacted device will have
>> to convert the address with xscom_to_pcb_addr(). There is some 
>> dependency on the chip model because the translation is different. 
>> That would be an extra op in the xscom device model I guess. 
> 
> No. If you split the XSCOM bus from the MMIO -> XSCOM bridge (the ADU)
> then the conversion only happens in the former. You don't directly
> route the MMIOs over ! You intercept the MMIOs and use use the
> address_space_rw to "generate" the XSCOM accesses on the other side,
> like I do for the LPC bus.

Yes. That is what I have been experimenting with. The mmio read/write 
ops and the current xscom read/write ops are quite compatible so It
did cost too much to do so : 

+static uint64_t pnv_lpc_xscom_mr_read(void *opaque, hwaddr addr, unsigned size)
+{
+    XScomDevice *xd = XSCOM_DEVICE(opaque);
+    uint64_t val = 0;
+
+    pnv_lpc_xscom_read(xd, 0, xscom_to_pcb_addr(xd->chip_type, addr), &val);
+    return val;
+}
+
+static void pnv_lpc_xscom_mr_write(void *opaque, hwaddr addr,
+                                   uint64_t val, unsigned size)
+{
+    XScomDevice *xd = XSCOM_DEVICE(opaque);
+    pnv_lpc_xscom_write(xd, 0, xscom_to_pcb_addr(xd->chip_type, addr), val);
+}
+
+static const MemoryRegionOps lpc_xscom_mr_ops = {
+    .read = pnv_lpc_xscom_mr_read,
+    .write = pnv_lpc_xscom_mr_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_lpc_realize(DeviceState *dev, Error **errp)
 {
     PnvLpcController *lpc = PNV_LPC_CONTROLLER(dev);
+    XScomBus *bus = XSCOM_BUS(qdev_get_parent_bus(dev));
 
     /* LPC XSCOM address is fixed */
+    memory_region_init_io(&lpc->xd.xscom_mr, OBJECT(dev), &lpc_xscom_mr_ops,
+                          lpc, "lpc-xscom", 4 * 8);
+    memory_region_add_subregion(bus->xscom_mr, 0xb00200, &lpc->xd.xscom_mr);
+
     lpc->xd.ranges[0].addr = 0xb0020;
     lpc->xd.ranges[0].size = 4;


To hack my way through, I have put the address space and the backend 
region under the XscomBus, bc it's easy to capture from the device. 
So that might be a reason to keep this bus/device model.

The xscom_to_pcb_addr() translation should probably done at the upper 
level, at the bridge/ADU level. I think that is what you are asking 
for above. 

As for the mapping, I don't think it should be here. It should be done 
at the chip/bus level which controls the address space, but not in the
devices. 

> We need that anyway because of the way XSCOMs can manipulate the HMER
> etc...

ah. Another thing I need to look at !

Thanks,

C.
 
>> Also, the main purpose of the XscomBus is to loop on the devices 
>> to populate the device tree. I am wondering if we could just use 
>> a simple list under the chip for that purpose.
> 
>

On 09/06/2016 11:49 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2016-09-07 at 07:47 +1000, Benjamin Herrenschmidt wrote:
>> d be an extra op in the xscom device model I guess. 
>>
>> No. If you split the XSCOM bus from the MMIO -> XSCOM bridge (the
>> ADU)
>> then the conversion only happens in the former. You don't directly
>> route the MMIOs over ! You intercept the MMIOs and use use the
>> address_space_rw to "generate" the XSCOM accesses on the other side,
>> like I do for the LPC bus.
>>
>> We need that anyway because of the way XSCOMs can manipulate the HMER
>> etc...
>>
>>>
>>> Also, the main purpose of the XscomBus is to loop on the devices 
>>> to populate the device tree. I am wondering if we could just use 
>>> a simple list under the chip for that purpose.
> 
> In fact, if you do the above, you no longer need a XSCOM device...
> 
> A number of "devices" can exist below a chip, all they need to have
> XSCOMs is to register memory regions that are child of that chip's
> xscom_region.

yes. To hack my way through again, I have added a memory region under
the XScomDevice, but we should have a list like the ranges[] because of 
the PHB3 PCBQs.

> For device-tree, well, we could have a generic interface that anything
> that can populate DT has and iterate through them. Or make a "chiplet"
> class or something.

yes, something like the XScomDeviceClass, which serves well the purpose
anyhow.

Thanks,
C.
 
> Cheers,
> Ben.
>

On 09/06/2016 11:45 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2016-09-06 at 16:42 +0200, Cédric Le Goater wrote:
>>> Alternatively.. it might be simpler to just drop the SCOM as a
>>> separate device.  I think you could just hang the scom bus directly
>>> off the chip object.  IIUC the scom is basically the only chip-
>> level
>>> control mechanism, so this does make a certain amount of sense.
>>
>> yes. I am exposing more and more stuff of the chip object under the 
>> xscom object so we should merge them. I agree. We will still need 
>> some XScomDevice of some sort.
> 
> What you can do is split it this way which matches the HW:
> 
>  - The chip object is the XSCOM parent, it owns the XSCOM bus,
> and expose functions (methods) to read/write XSCOMs. WE could rename
> XSCOM to "PIB" or "PCB" which is the real name of the bus ;-) But that
> might confuse things more than help .

Well, "Pervasive" should be mentioned somewhere, it is central to
PowerPC architecture. I will add a comment for the "PIB" (Pervasive 
Interconnect Bus) aka XSCOM bus

>  - A separate ADU object on each chip that is a SysDevice and does the
> MMIO bridge to XSCOM. It decodes the MMIO range for that chip and calls
> the above accessors.

ok. So the ADU is the address space holder. 

> That makes it easy to generate XSCOMs using different mechanisms if we
> wish to do so, which could come in handy, such as monitor commands, 

yes, that will be helpful to trigger some behavior from the command
line.

> or if we ever do cosimulation with a separate BMC, a simulated FSI, 
> all by just calling the first object's methods.

The cosimulation is not a long term goal, for the ipmi-bt part. But 
yes, having a more complete model would be excellent.

Cheers,

C.

On Wed, 2016-09-07 at 17:55 +0200, Cédric Le Goater wrote:
> 
> yes. To hack my way through again, I have added a memory region under
> the XScomDevice, but we should have a list like the ranges[] because of 
> the PHB3 PCBQs.

You have the parent region in the chip. Then each device can create and attach
memory regions below if it feels like it. No need to have a generic XScomDevice
with a fixed list of regions I think. I mean, you can ... but you don't have to.

> > For device-tree, well, we could have a generic interface that anything
> > that can populate DT has and iterate through them. Or make a "chiplet"
> > class or something.
> 
> yes, something like the XScomDeviceClass, which serves well the purpose
> anyhow.

On Wed, 2016-09-07 at 17:47 +0200, Cédric Le Goater wrote:
> 
> +static uint64_t pnv_lpc_xscom_mr_read(void *opaque, hwaddr addr,
> unsigned size)
> +{
> +    XScomDevice *xd = XSCOM_DEVICE(opaque);
> +    uint64_t val = 0;
> +
> +    pnv_lpc_xscom_read(xd, 0, xscom_to_pcb_addr(xd->chip_type,
> addr), &val);
> +    return val;
> +}
> +
> +static void pnv_lpc_xscom_mr_write(void *opaque, hwaddr addr,
> +                                   uint64_t val, unsigned size)
> +{
> +    XScomDevice *xd = XSCOM_DEVICE(opaque);
> +    pnv_lpc_xscom_write(xd, 0, xscom_to_pcb_addr(xd->chip_type,
> addr), val);
> +}
> 

I don't understand. That's not at all why I suggested or I'm missing
something.

What I suggest is that you have a memory region per-chip (which is NOT
hooked onto the main address space) which represents the PCB space.
Calling xscom_region. Hook it up to its own address_space.

Thus, the various devices (LPC, OCC, etc...) all just register a sub-
region of that address space using the PCB addresses directly (well,
shifted left by 3 because it's 8 bytes registers but that's it).

The XSCOM "controller" AKA ADU is the one doing the bridge. It
registers an MMIO region in the main address space (SysBusDevice ?)
and from the MMIO accessors, it does the address mangling, and use
address_space_rw() to trigger accesses onto that chip's xscom_region.

Ben.

On 09/07/2016 11:53 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2016-09-07 at 17:47 +0200, Cédric Le Goater wrote:
>>  
>> +static uint64_t pnv_lpc_xscom_mr_read(void *opaque, hwaddr addr,
>> unsigned size)
>> +{
>> +    XScomDevice *xd = XSCOM_DEVICE(opaque);
>> +    uint64_t val = 0;
>> +
>> +    pnv_lpc_xscom_read(xd, 0, xscom_to_pcb_addr(xd->chip_type,
>> addr), &val);
>> +    return val;
>> +}
>> +
>> +static void pnv_lpc_xscom_mr_write(void *opaque, hwaddr addr,
>> +                                   uint64_t val, unsigned size)
>> +{
>> +    XScomDevice *xd = XSCOM_DEVICE(opaque);
>> +    pnv_lpc_xscom_write(xd, 0, xscom_to_pcb_addr(xd->chip_type,
>> addr), val);
>> +}
>>
> 
> I don't understand. That's not at all why I suggested or I'm missing
> something.

This was a preliminary hack on the full powernv tree to study the 
question. I made the two options cohabitate in the same qemu to see 
what were the issues and possible solutions.

The result is very much what you describe below. I need to start 
from the beginning now, and not the end, to make something cleaner.

> What I suggest is that you have a memory region per-chip (which is NOT
> hooked onto the main address space) which represents the PCB space.
> Calling xscom_region. Hook it up to its own address_space.
>
> Thus, the various devices (LPC, OCC, etc...) all just register a sub-
> region of that address space using the PCB addresses directly (well,
> shifted left by 3 because it's 8 bytes registers but that's it).
> 
> The XSCOM "controller" AKA ADU is the one doing the bridge. It
> registers an MMIO region in the main address space (SysBusDevice ?)
> and from the MMIO accessors, it does the address mangling, and use
> address_space_rw() to trigger accesses onto that chip's xscom_region.

yes. this is what your XSCOM bridge does already.

    name = g_strdup_printf("xscom-%x", s->chip_id);
    memory_region_init_io(&s->mem, OBJECT(s), &xscom_ops, s, name, XSCOM_SIZE);
    sysbus_init_mmio(sbd, &s->mem);
    sysbus_mmio_map(sbd, 0, XSCOM_BASE(s->chip_id));


Thanks,

C.