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

[David Gibson]

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.

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature