Re: [Qemu-devel] [PATCH v3 08/10] ppc/pnv: add a XScomDevice to PnvCore

[Cédric Le Goater]

On 09/21/2016 08:12 AM, David Gibson wrote:
> On Thu, Sep 15, 2016 at 02:45:58PM +0200, Cédric Le Goater wrote:
>> Now that we are using real HW ids for the cores in PowerNV chips, we
>> can route the XSCOM accesses to them. We just need to attach a
>> specific XSCOM memory region to each core in the appropriate window
>> for the core number.
>>
>> To start with, let's install the DTS (Digital Thermal Sensor) handlers
>> which should return 38°C for each core.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>
>>  Changes since v2:
>>
>>  - added a XSCOM memory region to handle access to the EX core
>>    registers   
>>  - extended the PnvCore object with a XSCOM_INTERFACE so that we can
>>    use pnv_xscom_pcba() and pnv_xscom_addr() to handle XSCOM address
>>    translation.
>>
>>  hw/ppc/pnv.c               |  4 ++++
>>  hw/ppc/pnv_core.c          | 55 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv_core.h  |  2 ++
>>  include/hw/ppc/pnv_xscom.h | 19 ++++++++++++++++
>>  4 files changed, 80 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 7dcdf18a9e6b..6a3d1fbf8403 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -619,6 +619,10 @@ static void pnv_chip_realize(DeviceState *dev, Error 
>> **errp)
>>                                   &error_fatal);
>>          object_unref(OBJECT(pnv_core));
>>          i++;
>> +
>> +        memory_region_add_subregion(&chip->xscom.xscom_mr,
>> +                         pcc->xscom_addr(PNV_XSCOM_EX_CORE_BASE(core_hwid)),
>> +                         &PNV_CORE(pnv_core)->xscom_regs);
> 
> I think the core realize function should be doing this itself.

When working on the support of the AST2{4,5}00 SoC for qemu, these 
are the BMC chips for the OpenPOWER systems, we were asked to do all 
the mmio mappings for the devices at the board level. 

I think we can consider that the powernv chip is the board level for 
the xscom address space and to all the mapping there.

This has some benefits on the view of the address space as it is 
located in one file and not spread in multiple areas of the code.

> 
>>      }
>>      g_free(typename);
>>  
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> index 6fed5a208536..81b83d0f41b3 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -19,6 +19,7 @@
>>  #include "qemu/osdep.h"
>>  #include "sysemu/sysemu.h"
>>  #include "qapi/error.h"
>> +#include "qemu/log.h"
>>  #include "target-ppc/cpu.h"
>>  #include "hw/ppc/ppc.h"
>>  #include "hw/ppc/pnv.h"
>> @@ -57,6 +58,51 @@ static void powernv_cpu_init(PowerPCCPU *cpu, Error 
>> **errp)
>>      powernv_cpu_reset(cpu);
>>  }
>>  
>> +/*
>> + * These values are read by the powernv hw monitors under Linux
>> + */
>> +#define DTS_RESULT0     0x50000
>> +#define DTS_RESULT1     0x50001
>> +
>> +static uint64_t pnv_core_xscom_read(void *opaque, hwaddr addr,
>> +                                    unsigned int width)
>> +{
>> +    uint32_t offset = pnv_xscom_pcba(opaque, addr);
>> +    uint64_t val = 0;
>> +
>> +    /* The result should be 38 C */
>> +    switch (offset) {
>> +    case DTS_RESULT0:
>> +        val = 0x26f024f023f0000ull;
>> +        break;
>> +    case DTS_RESULT1:
>> +        val = 0x24f000000000000ull;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "Warning: reading reg=0x%" HWADDR_PRIx,
>> +                  addr);
>> +    }
>> +
>> +    return val;
>> +}
>> +
>> +static void pnv_core_xscom_write(void *opaque, hwaddr addr, uint64_t val,
>> +                                 unsigned int width)
>> +{
>> +    qemu_log_mask(LOG_UNIMP, "Warning: writing to reg=0x%" HWADDR_PRIx,
>> +                  addr);
>> +}
> 
> You should double check, but I think you can implement an RO region in
> an address space by just leaving the write function as NULL.

OK.

Thanks,

C.

>> +
>> +static const MemoryRegionOps pnv_core_xscom_ops = {
>> +    .read = pnv_core_xscom_read,
>> +    .write = pnv_core_xscom_write,
>> +    .valid.min_access_size = 8,
>> +    .valid.max_access_size = 8,
>> +    .impl.min_access_size = 8,
>> +    .impl.max_access_size = 8,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +};
>> +
>>  static void pnv_core_realize_child(Object *child, Error **errp)
>>  {
>>      Error *local_err = NULL;
>> @@ -117,6 +163,11 @@ static void pnv_core_realize(DeviceState *dev, Error 
>> **errp)
>>              goto err;
>>          }
>>      }
>> +
>> +    snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
>> +    memory_region_init_io(&pc->xscom_regs, OBJECT(dev), &pnv_core_xscom_ops,
>> +                          pc, name, pnv_xscom_addr(PNV_XSCOM_INTERFACE(dev),
>> +                                                   PNV_XSCOM_EX_CORE_SIZE));
>>      return;
>>  
>>  err:
>> @@ -169,6 +220,10 @@ static void pnv_core_register_types(void)
>>              .instance_size = sizeof(PnvCore),
>>              .class_init = pnv_core_class_init,
>>              .class_data = (void *) pnv_core_models[i],
>> +            .interfaces    = (InterfaceInfo[]) {
>> +                { TYPE_PNV_XSCOM_INTERFACE },
>> +                { }
>> +            }
>>          };
>>          ti.name = pnv_core_typename(pnv_core_models[i]);
>>          type_register(&ti);
>> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
>> index a151e281c017..2955a41c901f 100644
>> --- a/include/hw/ppc/pnv_core.h
>> +++ b/include/hw/ppc/pnv_core.h
>> @@ -36,6 +36,8 @@ typedef struct PnvCore {
>>      /*< public >*/
>>      void *threads;
>>      uint32_t pir;
>> +
>> +    MemoryRegion xscom_regs;
>>  } PnvCore;
>>  
>>  typedef struct PnvCoreClass {
>> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
>> index 0a03d533db59..31e5e8847b90 100644
>> --- a/include/hw/ppc/pnv_xscom.h
>> +++ b/include/hw/ppc/pnv_xscom.h
>> @@ -63,6 +63,25 @@ typedef struct PnvXScom {
>>  #define PNV_XSCOM_BASE(chip)                                    \
>>      (0x3fc0000000000ull + ((uint64_t)(chip)) * PNV_XSCOM_SIZE)
>>  
>> +/*
>> + * Layout of Xscom PCB addresses for EX core 1
>> + *
>> + *   GPIO        0x1100xxxx
>> + *   SCOM        0x1101xxxx
>> + *   OHA         0x1102xxxx
>> + *   CLOCK CTL   0x1103xxxx
>> + *   FIR         0x1104xxxx
>> + *   THERM       0x1105xxxx
>> + *   <reserved>  0x1106xxxx
>> + *               ..
>> + *               0x110Exxxx
>> + *   PCB SLAVE   0x110Fxxxx
>> + */
>> +
>> +#define PNV_XSCOM_EX_BASE         0x10000000
>> +#define PNV_XSCOM_EX_CORE_BASE(i) (PNV_XSCOM_EX_BASE | (((uint64_t)i) << 
>> 24))
>> +#define PNV_XSCOM_EX_CORE_SIZE    0x100000
>> +
>>  extern int pnv_xscom_populate_fdt(PnvXScom *xscom, void *fdt, int offset);
>>  
>>  /*
>