Re: [Qemu-devel] [Qemu-ppc] [PATCH 27/77] ppc/pnv: Add XSCOM infrastructure

[Benjamin Herrenschmidt]

On Tue, 2015-11-24 at 14:20 +1100, David Gibson wrote:
> 
> > +static uint32_t xscom_to_pcb_addr(uint64_t addr)
> > +{
> > +        addr &= (XSCOM_SIZE - 1);
> > +        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> 
> Wow, that's a pretty weird address transform.

Indeed :-) That's how it is in HW. It's also a bit different between
chip generations.

> > +}
> > +
> > +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
> 
> Not sure what you're referring to here.  Nothing more should be
> needed to set the HMER - because you've called
> cpu_synchronize_state() it will be marked dirty and flushed back to
> KVM before re-entry.

No it's not that. Setting HMER can potentially generate an HMI
interrupt (if enabled in HMEER). We never use the interrupt
corresponding to XSCOMs in FW though, so that's why I haven't bothered
yet.

> > +     */
> > +}
> > +
> > +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;
> > +            }
> > +        }
> > +    }
> 
> I'm wondering if it makes sense to construct a custom AddressSpace
> and
> use the existing address space lookup logic from exec.c and memory.c
> rather than implementing your own.

Maybe but we'd then have to shift everything by 3 bits, which means the
"XSCOM addresses" would no longer match the doc unless we use some kind
of macro to do the shifting.

> > +    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);
> > +
> > +#ifdef TRACE_SCOMS
> > +    printf("XSCOM_READ(0x%x:0x%x)\n", s->chip_id, pcba);
> > +#endif
> 
> You should be using the built in trace infrastructure here - it's
> really not that much of a pain.  Put
>         trace_xscom_read(s->chip_id, pcba)
> here, put a suitable format in trace-events, and ./configure
> --enable-trace-backends=stderr

I'll investigate this.

> > +
> > +    /* Handle some SCOMs here before dispatch */
> > +    switch(pcba) {
> > +    case 0xf000f:
> > +        val = 0x221EF04980000000;
> > +        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);
> > +
> > +#ifdef TRACE_SCOMS
> > +    printf("XSCOM_WRITE(0x%x:0x%x, 0x%016llx)\n",
> > +           s->chip_id, pcba, (unsigned long long)val);
> > +#endif
> > +    /* 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;
> > +
> > +    assert(s->chip_id >= 0);
> 
> So, this assert could be tripped if the user explicitly instantiated
> an xscom device which they probably shouldn't do, but could.  So, it
> probably makes sense to use error_setg() here instead of assert().

No idea what error_setg() is, I'll look into it :)

> > +    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),
> > +};
> > +
> > +void xscom_create(PnvChip *chip)
> > +{
> > +    DeviceState *dev;
> > +    XScomState *xdev;
> > +    BusState *qbus;
> > +    XScomBus *xb;
> > +
> > +    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;
> > +    chip->xscom = xb;
> 
> I believe the qbus_create() is usually invoked by the bridge's init
> function, rather than externally.

Init or realize ?

Cheers,
Ben.