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

[David Gibson]

On Wed, Mar 06, 2019 at 09:50:23AM +0100, Cédric Le Goater wrote:
> The LPC Controller on POWER9 is very similar to the one found on
> POWER8 but accesses are now done via on MMIOs, without the XSCOM and
> ECCB logic. The device tree is populated differently so we add a
> specific POWER9 routine for the purpose.
> 
> SerIRQ routing is yet to be done.
> 
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  include/hw/ppc/pnv.h     |   4 +
>  include/hw/ppc/pnv_lpc.h |  10 ++
>  hw/ppc/pnv.c             |  29 +++++-
>  hw/ppc/pnv_lpc.c         | 200 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 240 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 57d0337219be..2d68aabc212f 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -85,6 +85,7 @@ typedef struct Pnv9Chip {
>      /*< public >*/
>      PnvXive      xive;
>      PnvPsi       psi;
> +    PnvLpcController lpc;
>  } Pnv9Chip;
>  
>  typedef struct PnvChipClass {
> @@ -232,6 +233,9 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>  #define PNV9_XIVE_PC_SIZE            0x0000001000000000ull
>  #define PNV9_XIVE_PC_BASE(chip)      PNV9_CHIP_BASE(chip, 
> 0x0006018000000000ull)
>  
> +#define PNV9_LPCM_SIZE               0x0000000100000000ull
> +#define PNV9_LPCM_BASE(chip)         PNV9_CHIP_BASE(chip, 
> 0x0006030000000000ull)
> +
>  #define PNV9_PSIHB_SIZE              0x0000000000100000ull
>  #define PNV9_PSIHB_BASE(chip)        PNV9_CHIP_BASE(chip, 
> 0x0006030203000000ull)
>  
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index 242baecdcb93..24fe23f0f63b 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -28,6 +28,10 @@
>  #define PNV8_LPC(obj) \
>      OBJECT_CHECK(PnvLpc, (obj), TYPE_PNV8_LPC)
>  
> +#define TYPE_PNV9_LPC TYPE_PNV_LPC "-POWER9"
> +#define PNV9_LPC(obj) \
> +    OBJECT_CHECK(PnvLpc, (obj), TYPE_PNV9_LPC)
> +
>  typedef struct PnvLpcController {
>      DeviceState parent;
>  
> @@ -86,6 +90,12 @@ typedef struct PnvLpcClass {
>      DeviceRealize parent_realize;
>  } PnvLpcClass;
>  
> +/*
> + * Old compilers error on typdef forward declarations. Keep them happy.
> + */
> +struct PnvChip;
> +
>  ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error 
> **errp);
> +int pnv_dt_lpc(struct PnvChip *chip, void *fdt, int root_offset);
>  
>  #endif /* _PPC_PNV_LPC_H */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7176e1b68d0e..895be470af67 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -306,6 +306,8 @@ static void pnv_chip_power9_dt_populate(PnvChip *chip, 
> void *fdt)
>      if (chip->ram_size) {
>          pnv_dt_memory(fdt, chip->chip_id, chip->ram_start, chip->ram_size);
>      }
> +
> +    pnv_dt_lpc(chip, fdt, 0);
>  }
>  
>  static void pnv_dt_rtc(ISADevice *d, void *fdt, int lpc_off)
> @@ -422,8 +424,14 @@ static int pnv_chip_isa_offset(PnvChip *chip, void *fdt)
>      char *name;
>      int offset;
>  
> -    name = g_strdup_printf("/address@hidden" PRIx64 "/address@hidden",
> -                           (uint64_t) PNV_XSCOM_BASE(chip), 
> PNV_XSCOM_LPC_BASE);
> +    if (pnv_chip_is_power9(chip)) {

Again explicit chip type checks aren't very pretty.  Better to
redirect through a method.  Or, better, call into the common ISA DT
code from chip specific code passing the offset, rather than calling
out again to get it.

> +        name = g_strdup_printf("/address@hidden" PRIx64 "/address@hidden",
> +                               (uint64_t) PNV9_LPCM_BASE(chip));
> +    } else {
> +        name = g_strdup_printf("/address@hidden" PRIx64 "/address@hidden",
> +                               (uint64_t) PNV_XSCOM_BASE(chip),
> +                               PNV_XSCOM_LPC_BASE);
> +    }
>      offset = fdt_path_offset(fdt, name);
>      g_free(name);
>      return offset;
> @@ -559,7 +567,8 @@ static ISABus *pnv_chip_power8nvl_isa_create(PnvChip 
> *chip, Error **errp)
>  
>  static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
>  {
> -    return NULL;
> +    Pnv9Chip *chip9 = PNV9_CHIP(chip);
> +    return pnv_lpc_isa_create(&chip9->lpc, false, errp);
>  }
>  
>  static ISABus *pnv_isa_create(PnvChip *chip, Error **errp)
> @@ -954,6 +963,11 @@ static void pnv_chip_power9_instance_init(Object *obj)
>                              TYPE_PNV9_PSI, &error_abort, NULL);
>      object_property_add_const_link(OBJECT(&chip9->psi), "chip", obj,
>                                     &error_abort);
> +
> +    object_initialize_child(obj, "lpc",  &chip9->lpc, sizeof(chip9->lpc),
> +                            TYPE_PNV9_LPC, &error_abort, NULL);
> +    object_property_add_const_link(OBJECT(&chip9->lpc), "psi",
> +                                   OBJECT(&chip9->psi), &error_abort);
>  }
>  
>  static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
> @@ -997,6 +1011,15 @@ static void pnv_chip_power9_realize(DeviceState *dev, 
> Error **errp)
>      }
>      pnv_xscom_add_subregion(chip, PNV9_XSCOM_PSIHB_BASE,
>                              &chip9->psi.xscom_regs);
> +
> +    /* LPC */
> +    object_property_set_bool(OBJECT(&chip9->lpc), true, "realized", 
> &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    memory_region_add_subregion(get_system_memory(), PNV9_LPCM_BASE(chip),
> +                                &chip9->lpc.xscom_regs);
>  }
>  
>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 3c509a30a0af..6df694e0abc1 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -118,6 +118,100 @@ static int pnv_lpc_dt_xscom(PnvXScomInterface *dev, 
> void *fdt, int xscom_offset)
>      return 0;
>  }
>  
> +/* POWER9 only */
> +int pnv_dt_lpc(PnvChip *chip, void *fdt, int root_offset)
> +{
> +    const char compat[] = "ibm,power9-lpcm-opb\0simple-bus";
> +    const char lpc_compat[] = "ibm,power9-lpc\0ibm,lpc";
> +    char *name;
> +    int offset, lpcm_offset;
> +    uint64_t lpcm_addr = PNV9_LPCM_BASE(chip);
> +    uint32_t opb_ranges[8] = { 0,
> +                               cpu_to_be32(lpcm_addr >> 32),
> +                               cpu_to_be32((uint32_t)lpcm_addr),
> +                               cpu_to_be32(PNV9_LPCM_SIZE / 2),
> +                               cpu_to_be32(PNV9_LPCM_SIZE / 2),
> +                               cpu_to_be32(lpcm_addr >> 32),
> +                               cpu_to_be32(PNV9_LPCM_SIZE / 2),
> +                               cpu_to_be32(PNV9_LPCM_SIZE / 2),
> +    };
> +    uint32_t opb_reg[4] = { cpu_to_be32(lpcm_addr >> 32),
> +                            cpu_to_be32((uint32_t)lpcm_addr),
> +                            cpu_to_be32(PNV9_LPCM_SIZE >> 32),
> +                            cpu_to_be32((uint32_t)PNV9_LPCM_SIZE),
> +    };
> +    uint32_t reg[2];
> +
> +    /*
> +     * OPB bus
> +     */
> +    name = g_strdup_printf("address@hidden"PRIx64, lpcm_addr);
> +    lpcm_offset = fdt_add_subnode(fdt, root_offset, name);
> +    _FDT(lpcm_offset);
> +    g_free(name);
> +
> +    _FDT((fdt_setprop(fdt, lpcm_offset, "reg", opb_reg, sizeof(opb_reg))));
> +    _FDT((fdt_setprop_cell(fdt, lpcm_offset, "#address-cells", 1)));
> +    _FDT((fdt_setprop_cell(fdt, lpcm_offset, "#size-cells", 1)));
> +    _FDT((fdt_setprop(fdt, lpcm_offset, "compatible", compat, 
> sizeof(compat))));
> +    _FDT((fdt_setprop_cell(fdt, lpcm_offset, "ibm,chip-id", chip->chip_id)));
> +    _FDT((fdt_setprop(fdt, lpcm_offset, "ranges", opb_ranges,
> +                      sizeof(opb_ranges))));
> +
> +    /*
> +     * OPB Master registers
> +     */
> +    name = g_strdup_printf("address@hidden", LPC_OPB_REGS_OPB_ADDR);
> +    offset = fdt_add_subnode(fdt, lpcm_offset, name);
> +    _FDT(offset);
> +    g_free(name);
> +
> +    reg[0] = cpu_to_be32(LPC_OPB_REGS_OPB_ADDR);
> +    reg[1] = cpu_to_be32(LPC_OPB_REGS_OPB_SIZE);
> +    _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
> +    _FDT((fdt_setprop_string(fdt, offset, "compatible",
> +                             "ibm,power9-lpcm-opb-master")));
> +
> +    /*
> +     * OPB arbitrer registers
> +     */
> +    name = g_strdup_printf("address@hidden", LPC_OPB_REGS_OPBA_ADDR);
> +    offset = fdt_add_subnode(fdt, lpcm_offset, name);
> +    _FDT(offset);
> +    g_free(name);
> +
> +    reg[0] = cpu_to_be32(LPC_OPB_REGS_OPBA_ADDR);
> +    reg[1] = cpu_to_be32(LPC_OPB_REGS_OPBA_SIZE);
> +    _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
> +    _FDT((fdt_setprop_string(fdt, offset, "compatible",
> +                             "ibm,power9-lpcm-opb-arbiter")));
> +
> +    /*
> +     * LPC Host Controller registers
> +     */
> +    name = g_strdup_printf("address@hidden", LPC_HC_REGS_OPB_ADDR);
> +    offset = fdt_add_subnode(fdt, lpcm_offset, name);
> +    _FDT(offset);
> +    g_free(name);
> +
> +    reg[0] = cpu_to_be32(LPC_HC_REGS_OPB_ADDR);
> +    reg[1] = cpu_to_be32(LPC_HC_REGS_OPB_SIZE);
> +    _FDT((fdt_setprop(fdt, offset, "reg", reg, sizeof(reg))));
> +    _FDT((fdt_setprop_string(fdt, offset, "compatible",
> +                             "ibm,power9-lpc-controller")));
> +
> +    name = g_strdup_printf("address@hidden");
> +    offset = fdt_add_subnode(fdt, lpcm_offset, name);
> +    _FDT(offset);
> +    g_free(name);
> +    _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
> +    _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
> +    _FDT((fdt_setprop(fdt, offset, "compatible", lpc_compat,
> +                      sizeof(lpc_compat))));
> +
> +    return 0;
> +}
> +
>  /*
>   * These read/write handlers of the OPB address space should be common
>   * with the P9 LPC Controller which uses direct MMIOs.
> @@ -242,6 +336,74 @@ static const MemoryRegionOps pnv_lpc_xscom_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> +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?

> +}
> +
> +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