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

[Cédric Le Goater]

On 06/15/2018 04:38 AM, David Gibson wrote:
> 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 <address@hidden>
> 
> 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.

knowing that the LPC controller is on chip0 is important, that's how
we build the ISA bus of the machine and I am trying to untangle this
relation : 

        pnv->isa_bus <-> pnv->chip->lpc->regions 

The chip is in the middle and it is problematic to introduce an 
abstract PnvChip class. 

Still working on it.

Thanks,

C. 


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