Re: [Qemu-ppc] [PATCH 04/21] ppc/pnv: enable only one LPC bus

[Cédric Le Goater]

On 04/06/2017 06:23 AM, David Gibson wrote:
> 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 : address@hidden/address@hidden
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> Cc: Benjamin Herrenschmidt <address@hidden>
> 
> 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.

hmm, yes. I can do better on this. 

The object will be initialized which raises some concern because we don't 
have the chip id at the moment but the object is still valid in some way. 
I think I need to remove it from the list of children of the chip or use 
a pointer instead.

Thanks,

C. 

> 
>> ---
>>  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("address@hidden", 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 ... 
>> */
>