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

[Cédric Le Goater]

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.
> 
>