Re: [Qemu-ppc] [PATCH v2 3/7] ppc/pnv: Add XSCOM infrastructure

[Cédric Le Goater]

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.