Re: [Qemu-devel] [PATCH 1/6] ppc/pnv: introduce a 'primary' field under the LPC model

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5323 bytes --]

On Thu, Jun 14, 2018 at 04:00:38PM +0200, Cédric Le Goater wrote:
> When a PowerNV system is started, the firmware (skiboot) looks for a
> "primary" property to determine which LPC bus is the default on a
> multichip system. This property is currently populated in the main
> routine creating the device tree of a chip, which is the not the right
> place to do so.
> 
> Check the chip id to flag the LPC controller as "primary", or not, and
> use that to add the property in the LPC device tree routine.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

This doesn't seem particularly useful to me.  Is there ever likely to
be a case where we want the primary LPC to be something other than the
one on chip 0?

If not, we seem to be just creating properties and states and a bunch
of stuff just to cache a (chip# == 0) test.

> ---
>  include/hw/ppc/pnv_lpc.h |  2 ++
>  hw/ppc/pnv.c             | 19 ++++++-------------
>  hw/ppc/pnv_lpc.c         | 29 ++++++++++++++++++++---------
>  3 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index 53fdd5bb6450..fddcb1c054b3 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -68,6 +68,8 @@ typedef struct PnvLpcController {
>  
>      /* PSI to generate interrupts */
>      PnvPsi *psi;
> +
> +    bool   primary;
>  } PnvLpcController;
>  
>  qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 031488131629..b419d3323100 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -285,16 +285,6 @@ static void pnv_dt_chip(PnvChip *chip, void *fdt)
>  
>      pnv_dt_xscom(chip, fdt, 0);
>  
> -    /* The default LPC bus of a multichip system is on chip 0. It's
> -     * recognized by the firmware (skiboot) using a "primary"
> -     * property.
> -     */
> -    if (chip->chip_id == 0x0) {
> -        int lpc_offset = pnv_chip_lpc_offset(chip, fdt);
> -
> -        _FDT((fdt_setprop(fdt, lpc_offset, "primary", NULL, 0)));
> -    }
> -
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
>  
> @@ -814,9 +804,12 @@ static void pnv_chip_init(Object *obj)
>      object_property_add_const_link(OBJECT(&chip->occ), "psi",
>                                     OBJECT(&chip->psi), &error_abort);
>  
> -    /* The LPC controller needs PSI to generate interrupts */
> -    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
> -                                   OBJECT(&chip->psi), &error_abort);
> +    /*
> +     * The LPC controller needs a few things from the chip : to know
> +     * if it's primary and PSI to generate interrupts
> +     */
> +    object_property_add_const_link(OBJECT(&chip->lpc), "chip",
> +                                   OBJECT(chip), &error_abort);
>  }
>  
>  static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 402c4fefa886..1e70c8c19d52 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -113,6 +113,14 @@ static int pnv_lpc_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>      _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
>      _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
>      _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
> +
> +    /*
> +     * The default LPC bus of a multichip system is on chip 0. It's
> +     * recognized by the firmware (skiboot) using a "primary" property.
> +     */
> +    if (PNV_LPC(dev)->primary) {
> +        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
> +    }
>      return 0;
>  }
>  
> @@ -416,6 +424,18 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      PnvLpcController *lpc = PNV_LPC(dev);
>      Object *obj;
>      Error *error = NULL;
> +    PnvChip *chip;
> +
> +    /* get PSI object from chip */
> +    obj = object_property_get_link(OBJECT(dev), "chip", &error);
> +    if (!obj) {
> +        error_propagate(errp, error);
> +        error_prepend(errp, "required link 'chip' not found: ");
> +        return;
> +    }
> +    chip = PNV_CHIP(obj);
> +    lpc->psi = &chip->psi;
> +    lpc->primary = chip->chip_id == 0;
>  
>      /* Reg inits */
>      lpc->lpc_hc_fw_rd_acc_size = LPC_HC_FW_RD_4B;
> @@ -460,15 +480,6 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      pnv_xscom_region_init(&lpc->xscom_regs, OBJECT(dev),
>                            &pnv_lpc_xscom_ops, lpc, "xscom-lpc",
>                            PNV_XSCOM_LPC_SIZE);
> -
> -    /* get PSI object from chip */
> -    obj = object_property_get_link(OBJECT(dev), "psi", &error);
> -    if (!obj) {
> -        error_setg(errp, "%s: required link 'psi' not found: %s",
> -                   __func__, error_get_pretty(error));
> -        return;
> -    }
> -    lpc->psi = PNV_PSI(obj);
>  }
>  
>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]