On Wed, Apr 05, 2017 at 02:41:29PM +0200, Cédric Le Goater wrote:
> Multi chip systems only have one LPC bus, on chip 0. The PnvLPC object
> will still be created under the PnvChip objects but only the one under
> chip 0 will be advertise in the device tree.
> 
> Also remove the comment which is slightly wrong. Only chip 0 has a LPC
> device node : xscom@3fc0000000000/isa@b0020
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This seems a very round about way of accomplishing the goal.  Wouldn't
it make more sense for the chip to only construct (or only realize)
the LPC if it's chip zero, rather than passing the chip id through to
the lpc object.

> ---
>  hw/ppc/pnv.c             |  2 ++
>  hw/ppc/pnv_lpc.c         | 20 ++++++++++++--------
>  include/hw/ppc/pnv_lpc.h |  2 ++
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 78133e5d20e1..493c7eed7980 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -811,6 +811,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>      g_free(typename);
>  
>      /* Create LPC controller */
> +    object_property_set_int(OBJECT(&chip->lpc), chip->chip_id, "chip-id",
> +                            &error_fatal);
>      object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
>                               &error_fatal);
>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 20cbb6a0dbbd..1a212a2a399f 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -92,14 +92,6 @@ enum {
>  #define LPC_HC_REGS_OPB_SIZE    0x00001000
>  
>  
> -/*
> - * TODO: the "primary" cell should only be added on chip 0. This is
> - * how skiboot chooses the default LPC controller on multichip
> - * systems.
> - *
> - * It would be easly done if we can change the populate() interface to
> - * replace the PnvXScomInterface parameter by a PnvChip one
> - */
>  static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>  {
>      const char compat[] = "ibm,power8-lpc\0ibm,lpc";
> @@ -110,6 +102,12 @@ static int pnv_lpc_populate(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>          cpu_to_be32(lpc_pcba),
>          cpu_to_be32(PNV_XSCOM_LPC_SIZE)
>      };
> +    PnvLpcController *lpc = PNV_LPC(dev);
> +
> +    /* Only populate one LPC bus per system, the one on chip 0.*/
> +    if (lpc->chip_id) {
> +        return 0;
> +    }
>  
>      name = g_strdup_printf("isa@%x", lpc_pcba);
>      offset = fdt_add_subnode(fdt, xscom_offset, name);
> @@ -486,6 +484,11 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      lpc->psi = PNV_PSI(obj);
>  }
>  
> +static Property pnv_lpc_properties[] = {
> +    DEFINE_PROP_UINT32("chip-id", PnvLpcController, chip_id, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -494,6 +497,7 @@ static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>      xdc->populate = pnv_lpc_populate;
>  
>      dc->realize = pnv_lpc_realize;
> +    dc->props = pnv_lpc_properties;
>  }
>  
>  static const TypeInfo pnv_lpc_info = {
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index 53040026c37b..dcfadda90090 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -67,6 +67,8 @@ typedef struct PnvLpcController {
>  
>      /* PSI to generate interrupts */
>      PnvPsi *psi;
> +
> +    uint32_t chip_id;
>  } PnvLpcController;
>  
>  #define   LPC_HC_IRQ_SERIRQ0            0x80000000 /* all bits down to ... */

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