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.
