Re: [Qemu-devel] [PATCH v2 05/15] ppc/pnv: add a 'dt_isa_nodename' to the chip

["Cédric Le Goater" <clg@kaod.org>]

On 3/8/19 1:01 AM, David Gibson wrote:
> On Thu, Mar 07, 2019 at 11:35:38PM +0100, Cédric Le Goater wrote:
>> The ISA bus has a different DT nodename on POWER9. Compute the name
>> when the PnvChip is realized, that is before it is used by the machine
>> to populate the device tree with the ISA devices.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> I still tend to think that passing an offset into pnv_dt_isa would
> have been a better solution, but this will serve.  Applied.

Do you mean something like below possibly ? 

   int isa_offset = fdt_path_offset(fdt, pnv->chips[0]->dt_isa_nodename);

   /* Populate ISA devices on chip 0 */
   pnv_dt_isa(pnv->isa_bus, fdt, isa_offset);


pnv_dt_isa() is called at the machine level but we could change it
to be called at the chip level for chip0 only.

C.


>> ---
>>  include/hw/ppc/pnv.h |  2 ++
>>  hw/ppc/pnv.c         | 18 +++++-------------
>>  2 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 8d80cb34eebb..c81f157f41a9 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -58,6 +58,8 @@ typedef struct PnvChip {
>>      MemoryRegion xscom_mmio;
>>      MemoryRegion xscom;
>>      AddressSpace xscom_as;
>> +
>> +    gchar        *dt_isa_nodename;
>>  } PnvChip;
>>  
>>  #define TYPE_PNV8_CHIP "pnv8-chip"
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 922e3ec48bb5..6625562d276d 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -417,24 +417,12 @@ static int pnv_dt_isa_device(DeviceState *dev, void *opaque)
>>      return 0;
>>  }
>>  
>> -static int pnv_chip_isa_offset(PnvChip *chip, void *fdt)
>> -{
>> -    char *name;
>> -    int offset;
>> -
>> -    name = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
>> -                           (uint64_t) PNV_XSCOM_BASE(chip), PNV_XSCOM_LPC_BASE);
>> -    offset = fdt_path_offset(fdt, name);
>> -    g_free(name);
>> -    return offset;
>> -}
>> -
>>  /* The default LPC bus of a multichip system is on chip 0. It's
>>   * recognized by the firmware (skiboot) using a "primary" property.
>>   */
>>  static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
>>  {
>> -    int isa_offset = pnv_chip_isa_offset(pnv->chips[0], fdt);
>> +    int isa_offset = fdt_path_offset(fdt, pnv->chips[0]->dt_isa_nodename);
>>      ForeachPopulateArgs args = {
>>          .fdt = fdt,
>>          .offset = isa_offset,
>> @@ -866,6 +854,10 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>>                               &error_fatal);
>>      pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
>>  
>> +    chip->dt_isa_nodename = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
>> +                                            (uint64_t) PNV_XSCOM_BASE(chip),
>> +                                            PNV_XSCOM_LPC_BASE);
>> +
>>      /* Interrupt Management Area. This is the memory region holding
>>       * all the Interrupt Control Presenter (ICP) registers */
>>      pnv_chip_icp_realize(chip8, &local_err);
>