On 3/8/19 1:19 AM, David Gibson wrote:
> On Thu, Mar 07, 2019 at 08:07:41AM +0100, Cédric Le Goater wrote:
>> On 3/7/19 5:18 AM, David Gibson wrote:
>>> On Wed, Mar 06, 2019 at 09:50:23AM +0100, Cédric Le Goater wrote:
> [snip]
>>>> +static uint64_t pnv_lpc_mmio_read(void *opaque, hwaddr addr, unsigned
>>>> size)
>>>> +{
>>>> + PnvLpcController *lpc = PNV_LPC(opaque);
>>>> + uint64_t val = 0;
>>>> + uint32_t opb_addr = addr & ECCB_CTL_ADDR_MASK;
>>>> + MemTxResult result;
>>>> +
>>>> + switch (size) {
>>>> + case 4:
>>>> + val = address_space_ldl(&lpc->opb_as, opb_addr,
>>>> MEMTXATTRS_UNSPECIFIED,
>>>> + &result);
>>>> + break;
>>>> + case 1:
>>>> + val = address_space_ldub(&lpc->opb_as, opb_addr,
>>>> MEMTXATTRS_UNSPECIFIED,
>>>> + &result);
>>>> + break;
>>>> + default:
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "OPB read failed at @0x%"
>>>> + HWADDR_PRIx " invalid size %d\n", addr, size);
>>>> + return 0;
>>>> + }
>>>> +
>>>> + if (result != MEMTX_OK) {
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "OPB read failed at @0x%"
>>>> + HWADDR_PRIx "\n", addr);
>>>> + }
>>>> +
>>>> + return val;
>>>
>>> Couldn't you just map the relevant portion of the OPB AS into the MMIO
>>> AS, rather than having to forward the IOs with explicit read/write
>>> functions?
>>
>> The underlying memory regions (ISA space, LPC HC, OPB regs) are the
>> same on POWER8. So this is one way to share the overall initialization.
>
> I don't understand how that relates to my question. If anything that
> sounds like it makes even more sense to map the common MR with the
> actual registers into the MMIO AS as a subregion, rather than having a
> redirect via the OPB AS.
ok. I will try that.
Thanks,
C.
>
>> What I would have liked to do is to simplified the ECCB interface
>> (see pnv_lpc_do_eccb()).
>>
>> Thanks,
>>
>> C.
>>
>>
>>>> +}
>>>> +
>>>> +static void pnv_lpc_mmio_write(void *opaque, hwaddr addr,
>>>> + uint64_t val, unsigned size)
>>>> +{
>>>> + PnvLpcController *lpc = PNV_LPC(opaque);
>>>> + uint32_t opb_addr = addr & ECCB_CTL_ADDR_MASK;
>>>> + MemTxResult result;
>>>> +
>>>> + switch (size) {
>>>> + case 4:
>>>> + address_space_stl(&lpc->opb_as, opb_addr, val,
>>>> MEMTXATTRS_UNSPECIFIED,
>>>> + &result);
>>>> + break;
>>>> + case 1:
>>>> + address_space_stb(&lpc->opb_as, opb_addr, val,
>>>> MEMTXATTRS_UNSPECIFIED,
>>>> + &result);
>>>> + break;
>>>> + default:
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "OPB write failed at @0x%"
>>>> + HWADDR_PRIx " invalid size %d\n", addr, size);
>>>> + return;
>>>> + }
>>>> +
>>>> + if (result != MEMTX_OK) {
>>>> + qemu_log_mask(LOG_GUEST_ERROR, "OPB write failed at @0x%"
>>>> + HWADDR_PRIx "\n", addr);
>>>> + }
>>>> +}
>>>> +
>>>> +static const MemoryRegionOps pnv_lpc_mmio_ops = {
>>>> + .read = pnv_lpc_mmio_read,
>>>> + .write = pnv_lpc_mmio_write,
>>>> + .impl = {
>>>> + .min_access_size = 1,
>>>> + .max_access_size = 4,
>>>> + },
>>>> + .endianness = DEVICE_BIG_ENDIAN,
>>>> +};
>>>> +
>>>> static void pnv_lpc_eval_irqs(PnvLpcController *lpc)
>>>> {
>>>> bool lpc_to_opb_irq = false;
>>>> @@ -465,6 +627,43 @@ static const TypeInfo pnv_lpc_power8_info = {
>>>> }
>>>> };
>>>>
>>>> +static void pnv_lpc_power9_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> + PnvLpcController *lpc = PNV_LPC(dev);
>>>> + PnvLpcClass *plc = PNV_LPC_GET_CLASS(dev);
>>>> + Error *local_err = NULL;
>>>> +
>>>> + plc->parent_realize(dev, &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* P9 uses a MMIO region */
>>>> + memory_region_init_io(&lpc->xscom_regs, OBJECT(lpc),
>>>> &pnv_lpc_mmio_ops,
>>>> + lpc, "lpcm", PNV9_LPCM_SIZE);
>>>> +}
>>>> +
>>>> +static void pnv_lpc_power9_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>>> + PnvLpcClass *plc = PNV_LPC_CLASS(klass);
>>>> +
>>>> + dc->desc = "PowerNV LPC Controller POWER9";
>>>> +
>>>> + plc->psi_irq = PSIHB9_IRQ_LPCHC;
>>>> +
>>>> + device_class_set_parent_realize(dc, pnv_lpc_power9_realize,
>>>> + &plc->parent_realize);
>>>> +}
>>>> +
>>>> +static const TypeInfo pnv_lpc_power9_info = {
>>>> + .name = TYPE_PNV9_LPC,
>>>> + .parent = TYPE_PNV_LPC,
>>>> + .instance_size = sizeof(PnvLpcController),
>>>> + .class_init = pnv_lpc_power9_class_init,
>>>> +};
>>>> +
>>>> static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>>>> {
>>>> PnvLpcController *lpc = PNV_LPC(dev);
>>>> @@ -540,6 +739,7 @@ static void pnv_lpc_register_types(void)
>>>> {
>>>> type_register_static(&pnv_lpc_info);
>>>> type_register_static(&pnv_lpc_power8_info);
>>>> + type_register_static(&pnv_lpc_power9_info);
>>>> }
>>>>
>>>> type_init(pnv_lpc_register_types)
>>>
>>
>
