Re: [Qemu-devel] [PATCH 18/27] ppc/pnv: add a LPC Controller model for POWER9

[Cédric Le Goater]

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