[PATCH 00/13] ppc/pnv: Get rid of chip_type attributes

[PATCH 00/13] ppc/pnv: Get rid of chip_type attributes

[Greg Kurz]

The PnvChipClass type has a chip_type attribute which identifies various
POWER CPU chip types that can be used in a powernv machine.

typedef enum PnvChipType {
    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
    PNV_CHIP_POWER8,      /* AKA Venice */
    PNV_CHIP_POWER8NVL,   /* AKA Naples */
    PNV_CHIP_POWER9,      /* AKA Nimbus */
    PNV_CHIP_POWER10,     /* AKA TBD */
} PnvChipType;

This attribute is used in many places where we want a different behaviour
depending on the CPU type, either directly like:

    switch (PNV_CHIP_GET_CLASS(chip)->chip_type) {
    case PNV_CHIP_POWER8E:
    case PNV_CHIP_POWER8:
    case PNV_CHIP_POWER8NVL:
        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
    case PNV_CHIP_POWER9:
    case PNV_CHIP_POWER10:
        return addr >> 3;
    default:
        g_assert_not_reached();
    }

or through various helpers that rely on it:

        /* Each core has an XSCOM MMIO region */
        if (pnv_chip_is_power10(chip)) {
            xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid);
        } else if (pnv_chip_is_power9(chip)) {
            xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid);
        } else {
            xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid);
        }

The chip_type is also duplicated in the PnvPsiClass type.

It looks a bit unfortunate to implement manually something that falls into
the scope of QOM. Especially since we don't seem to need a finer grain than
the CPU familly, ie. POWER8, POWER9, POWER10, ..., and we already have
specialized versions of PnvChipClass and PnvPsiClass for these.

This series basically QOM-ifies all the places where we check on the chip
type, and gets rid of the chip_type attributes and the is_powerXX() helpers.

Patch 1 was recently posted to the list but it isn't available in David's
ppc-for-5.0 tree yet, so I include it in this series for convenience.

--
Greg

---

Greg Kurz (13):
      ppc: Drop useless extern annotation for functions
      ppc/pnv: Introduce PnvPsiClass::compat
      ppc/pnv: Drop PnvPsiClass::chip_type
      ppc/pnv: Introduce PnvMachineClass and PnvMachineClass::compat
      ppc/pnv: Introduce PnvMachineClass::dt_power_mgt()
      ppc/pnv: Drop pnv_is_power9() and pnv_is_power10() helpers
      ppc/pnv: Introduce PnvChipClass::intc_print_info() method
      ppc/pnv: Introduce PnvChipClass::xscom_core_base() method
      ppc/pnv: Pass XSCOM base address and address size to pnv_dt_xscom()
      ppc/pnv: Pass content of the "compatible" property to pnv_dt_xscom()
      ppc/pnv: Drop pnv_chip_is_power9() and pnv_chip_is_power10() helpers
      ppc/pnv: Introduce PnvChipClass::xscom_pcba() method
      ppc/pnv: Drop PnvChipClass::type


 hw/ppc/pnv.c               |  150 +++++++++++++++++++++++++++++++++-----------
 hw/ppc/pnv_psi.c           |   28 +++-----
 hw/ppc/pnv_xscom.c         |   48 ++------------
 include/hw/ppc/pnv.h       |   53 ++++++----------
 include/hw/ppc/pnv_psi.h   |    3 +
 include/hw/ppc/pnv_xscom.h |   24 ++++---
 include/hw/ppc/spapr_vio.h |    6 +-
 7 files changed, 169 insertions(+), 143 deletions(-)

[PATCH 01/13] ppc: Drop useless extern annotation for functions

[Greg Kurz]

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/ppc/pnv_xscom.h |   22 +++++++++++-----------
 include/hw/ppc/spapr_vio.h |    6 +++---
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index 09188d74b06b..2bdb7ae84fd3 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -113,16 +113,16 @@ typedef struct PnvXScomInterfaceClass {
 #define PNV10_XSCOM_PSIHB_BASE     0x3011D00
 #define PNV10_XSCOM_PSIHB_SIZE     0x100
 
-extern void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp);
-extern int pnv_dt_xscom(PnvChip *chip, void *fdt, int offset);
-
-extern void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset,
-                                    MemoryRegion *mr);
-extern void pnv_xscom_region_init(MemoryRegion *mr,
-                                  struct Object *owner,
-                                  const MemoryRegionOps *ops,
-                                  void *opaque,
-                                  const char *name,
-                                  uint64_t size);
+void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp);
+int pnv_dt_xscom(PnvChip *chip, void *fdt, int offset);
+
+void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset,
+                             MemoryRegion *mr);
+void pnv_xscom_region_init(MemoryRegion *mr,
+                           struct Object *owner,
+                           const MemoryRegionOps *ops,
+                           void *opaque,
+                           const char *name,
+                           uint64_t size);
 
 #endif /* PPC_PNV_XSCOM_H */
diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index 72762ed16b70..ce6d9b0c6605 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -80,10 +80,10 @@ struct SpaprVioBus {
     uint32_t next_reg;
 };
 
-extern SpaprVioBus *spapr_vio_bus_init(void);
-extern SpaprVioDevice *spapr_vio_find_by_reg(SpaprVioBus *bus, uint32_t reg);
+SpaprVioBus *spapr_vio_bus_init(void);
+SpaprVioDevice *spapr_vio_find_by_reg(SpaprVioBus *bus, uint32_t reg);
 void spapr_dt_vdevice(SpaprVioBus *bus, void *fdt);
-extern gchar *spapr_vio_stdout_path(SpaprVioBus *bus);
+gchar *spapr_vio_stdout_path(SpaprVioBus *bus);
 
 static inline void spapr_vio_irq_pulse(SpaprVioDevice *dev)
 {

[PATCH 02/13] ppc/pnv: Introduce PnvPsiClass::compat

[Greg Kurz]

The Processor Service Interface (PSI) model has a chip_type class level
attribute, which is used to generate the content of the "compatible" DT
property according to the CPU type.

Since the PSI model already has specialized classes for each supported
CPU type, it seems cleaner to achieve this with QOM. Provide the content
of the "compatible" property with a new class level attribute.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv_psi.c         |   25 +++++++++++--------------
 include/hw/ppc/pnv_psi.h |    2 ++
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 572924388b3c..98a82b25e01f 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -536,10 +536,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
     qemu_register_reset(pnv_psi_reset, dev);
 }
 
-static const char compat_p8[] = "ibm,power8-psihb-x\0ibm,psihb-x";
-static const char compat_p9[] = "ibm,power9-psihb-x\0ibm,psihb-x";
-static const char compat_p10[] = "ibm,power10-psihb-x\0ibm,psihb-x";
-
 static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
 {
     PnvPsiClass *ppc = PNV_PSI_GET_CLASS(dev);
@@ -558,16 +554,8 @@ static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
     _FDT(fdt_setprop(fdt, offset, "reg", reg, sizeof(reg)));
     _FDT(fdt_setprop_cell(fdt, offset, "#address-cells", 2));
     _FDT(fdt_setprop_cell(fdt, offset, "#size-cells", 1));
-    if (ppc->chip_type == PNV_CHIP_POWER10) {
-        _FDT(fdt_setprop(fdt, offset, "compatible", compat_p10,
-                         sizeof(compat_p10)));
-    } else if (ppc->chip_type == PNV_CHIP_POWER9) {
-        _FDT(fdt_setprop(fdt, offset, "compatible", compat_p9,
-                         sizeof(compat_p9)));
-    } else {
-        _FDT(fdt_setprop(fdt, offset, "compatible", compat_p8,
-                         sizeof(compat_p8)));
-    }
+    _FDT(fdt_setprop(fdt, offset, "compatible", ppc->compat,
+                     ppc->compat_size));
     return 0;
 }
 
@@ -581,6 +569,7 @@ static void pnv_psi_power8_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvPsiClass *ppc = PNV_PSI_CLASS(klass);
+    static const char compat[] = "ibm,power8-psihb-x\0ibm,psihb-x";
 
     dc->desc    = "PowerNV PSI Controller POWER8";
     dc->realize = pnv_psi_power8_realize;
@@ -590,6 +579,8 @@ static void pnv_psi_power8_class_init(ObjectClass *klass, void *data)
     ppc->xscom_size = PNV_XSCOM_PSIHB_SIZE;
     ppc->bar_mask   = PSIHB_BAR_MASK;
     ppc->irq_set    = pnv_psi_power8_irq_set;
+    ppc->compat     = compat;
+    ppc->compat_size = sizeof(compat);
 }
 
 static const TypeInfo pnv_psi_power8_info = {
@@ -888,6 +879,7 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvPsiClass *ppc = PNV_PSI_CLASS(klass);
     XiveNotifierClass *xfc = XIVE_NOTIFIER_CLASS(klass);
+    static const char compat[] = "ibm,power9-psihb-x\0ibm,psihb-x";
 
     dc->desc    = "PowerNV PSI Controller POWER9";
     dc->realize = pnv_psi_power9_realize;
@@ -897,6 +889,8 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
     ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
     ppc->bar_mask   = PSIHB9_BAR_MASK;
     ppc->irq_set    = pnv_psi_power9_irq_set;
+    ppc->compat     = compat;
+    ppc->compat_size = sizeof(compat);
 
     xfc->notify      = pnv_psi_notify;
 }
@@ -917,12 +911,15 @@ static void pnv_psi_power10_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvPsiClass *ppc = PNV_PSI_CLASS(klass);
+    static const char compat[] = "ibm,power10-psihb-x\0ibm,psihb-x";
 
     dc->desc    = "PowerNV PSI Controller POWER10";
 
     ppc->chip_type  = PNV_CHIP_POWER10;
     ppc->xscom_pcba = PNV10_XSCOM_PSIHB_BASE;
     ppc->xscom_size = PNV10_XSCOM_PSIHB_SIZE;
+    ppc->compat     = compat;
+    ppc->compat_size = sizeof(compat);
 }
 
 static const TypeInfo pnv_psi_power10_info = {
diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
index a044aab304ae..fc068c95e543 100644
--- a/include/hw/ppc/pnv_psi.h
+++ b/include/hw/ppc/pnv_psi.h
@@ -83,6 +83,8 @@ typedef struct PnvPsiClass {
     uint32_t xscom_pcba;
     uint32_t xscom_size;
     uint64_t bar_mask;
+    const char *compat;
+    int compat_size;
 
     void (*irq_set)(PnvPsi *psi, int, bool state);
 } PnvPsiClass;

Re: [PATCH 02/13] ppc/pnv: Introduce PnvPsiClass::compat

[Cédric Le Goater]

On 13/12/2019 12:59, Greg Kurz wrote:
> The Processor Service Interface (PSI) model has a chip_type class level
> attribute, which is used to generate the content of the "compatible" DT
> property according to the CPU type.
> 
> Since the PSI model already has specialized classes for each supported
> CPU type, it seems cleaner to achieve this with QOM. Provide the content
> of the "compatible" property with a new class level attribute.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv_psi.c         |   25 +++++++++++--------------
>  include/hw/ppc/pnv_psi.h |    2 ++
>  2 files changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 572924388b3c..98a82b25e01f 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -536,10 +536,6 @@ static void pnv_psi_power8_realize(DeviceState *dev, Error **errp)
>      qemu_register_reset(pnv_psi_reset, dev);
>  }
>  
> -static const char compat_p8[] = "ibm,power8-psihb-x\0ibm,psihb-x";
> -static const char compat_p9[] = "ibm,power9-psihb-x\0ibm,psihb-x";
> -static const char compat_p10[] = "ibm,power10-psihb-x\0ibm,psihb-x";
> -
>  static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>  {
>      PnvPsiClass *ppc = PNV_PSI_GET_CLASS(dev);
> @@ -558,16 +554,8 @@ static int pnv_psi_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>      _FDT(fdt_setprop(fdt, offset, "reg", reg, sizeof(reg)));
>      _FDT(fdt_setprop_cell(fdt, offset, "#address-cells", 2));
>      _FDT(fdt_setprop_cell(fdt, offset, "#size-cells", 1));
> -    if (ppc->chip_type == PNV_CHIP_POWER10) {
> -        _FDT(fdt_setprop(fdt, offset, "compatible", compat_p10,
> -                         sizeof(compat_p10)));
> -    } else if (ppc->chip_type == PNV_CHIP_POWER9) {
> -        _FDT(fdt_setprop(fdt, offset, "compatible", compat_p9,
> -                         sizeof(compat_p9)));
> -    } else {
> -        _FDT(fdt_setprop(fdt, offset, "compatible", compat_p8,
> -                         sizeof(compat_p8)));
> -    }
> +    _FDT(fdt_setprop(fdt, offset, "compatible", ppc->compat,
> +                     ppc->compat_size));
>      return 0;
>  }
>  
> @@ -581,6 +569,7 @@ static void pnv_psi_power8_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvPsiClass *ppc = PNV_PSI_CLASS(klass);
> +    static const char compat[] = "ibm,power8-psihb-x\0ibm,psihb-x";
>  
>      dc->desc    = "PowerNV PSI Controller POWER8";
>      dc->realize = pnv_psi_power8_realize;
> @@ -590,6 +579,8 @@ static void pnv_psi_power8_class_init(ObjectClass *klass, void *data)
>      ppc->xscom_size = PNV_XSCOM_PSIHB_SIZE;
>      ppc->bar_mask   = PSIHB_BAR_MASK;
>      ppc->irq_set    = pnv_psi_power8_irq_set;
> +    ppc->compat     = compat;
> +    ppc->compat_size = sizeof(compat);
>  }
>  
>  static const TypeInfo pnv_psi_power8_info = {
> @@ -888,6 +879,7 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvPsiClass *ppc = PNV_PSI_CLASS(klass);
>      XiveNotifierClass *xfc = XIVE_NOTIFIER_CLASS(klass);
> +    static const char compat[] = "ibm,power9-psihb-x\0ibm,psihb-x";
>  
>      dc->desc    = "PowerNV PSI Controller POWER9";
>      dc->realize = pnv_psi_power9_realize;
> @@ -897,6 +889,8 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>      ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
>      ppc->bar_mask   = PSIHB9_BAR_MASK;
>      ppc->irq_set    = pnv_psi_power9_irq_set;
> +    ppc->compat     = compat;
> +    ppc->compat_size = sizeof(compat);
>  
>      xfc->notify      = pnv_psi_notify;
>  }
> @@ -917,12 +911,15 @@ static void pnv_psi_power10_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvPsiClass *ppc = PNV_PSI_CLASS(klass);
> +    static const char compat[] = "ibm,power10-psihb-x\0ibm,psihb-x";
>  
>      dc->desc    = "PowerNV PSI Controller POWER10";
>  
>      ppc->chip_type  = PNV_CHIP_POWER10;
>      ppc->xscom_pcba = PNV10_XSCOM_PSIHB_BASE;
>      ppc->xscom_size = PNV10_XSCOM_PSIHB_SIZE;
> +    ppc->compat     = compat;
> +    ppc->compat_size = sizeof(compat);
>  }
>  
>  static const TypeInfo pnv_psi_power10_info = {
> diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
> index a044aab304ae..fc068c95e543 100644
> --- a/include/hw/ppc/pnv_psi.h
> +++ b/include/hw/ppc/pnv_psi.h
> @@ -83,6 +83,8 @@ typedef struct PnvPsiClass {
>      uint32_t xscom_pcba;
>      uint32_t xscom_size;
>      uint64_t bar_mask;
> +    const char *compat;
> +    int compat_size;
>  
>      void (*irq_set)(PnvPsi *psi, int, bool state);
>  } PnvPsiClass;
> 

[PATCH 03/13] ppc/pnv: Drop PnvPsiClass::chip_type

[Greg Kurz]

It isn't used anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv_psi.c         |    3 ---
 include/hw/ppc/pnv_psi.h |    1 -
 2 files changed, 4 deletions(-)

diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
index 98a82b25e01f..75e20d9da08b 100644
--- a/hw/ppc/pnv_psi.c
+++ b/hw/ppc/pnv_psi.c
@@ -574,7 +574,6 @@ static void pnv_psi_power8_class_init(ObjectClass *klass, void *data)
     dc->desc    = "PowerNV PSI Controller POWER8";
     dc->realize = pnv_psi_power8_realize;
 
-    ppc->chip_type =  PNV_CHIP_POWER8;
     ppc->xscom_pcba = PNV_XSCOM_PSIHB_BASE;
     ppc->xscom_size = PNV_XSCOM_PSIHB_SIZE;
     ppc->bar_mask   = PSIHB_BAR_MASK;
@@ -884,7 +883,6 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
     dc->desc    = "PowerNV PSI Controller POWER9";
     dc->realize = pnv_psi_power9_realize;
 
-    ppc->chip_type  = PNV_CHIP_POWER9;
     ppc->xscom_pcba = PNV9_XSCOM_PSIHB_BASE;
     ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
     ppc->bar_mask   = PSIHB9_BAR_MASK;
@@ -915,7 +913,6 @@ static void pnv_psi_power10_class_init(ObjectClass *klass, void *data)
 
     dc->desc    = "PowerNV PSI Controller POWER10";
 
-    ppc->chip_type  = PNV_CHIP_POWER10;
     ppc->xscom_pcba = PNV10_XSCOM_PSIHB_BASE;
     ppc->xscom_size = PNV10_XSCOM_PSIHB_SIZE;
     ppc->compat     = compat;
diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
index fc068c95e543..f0f5b5519767 100644
--- a/include/hw/ppc/pnv_psi.h
+++ b/include/hw/ppc/pnv_psi.h
@@ -79,7 +79,6 @@ typedef struct Pnv9Psi {
 typedef struct PnvPsiClass {
     SysBusDeviceClass parent_class;
 
-    int chip_type;
     uint32_t xscom_pcba;
     uint32_t xscom_size;
     uint64_t bar_mask;

Re: [PATCH 03/13] ppc/pnv: Drop PnvPsiClass::chip_type

[Cédric Le Goater]

On 13/12/2019 12:59, Greg Kurz wrote:
> It isn't used anymore.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv_psi.c         |    3 ---
>  include/hw/ppc/pnv_psi.h |    1 -
>  2 files changed, 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 98a82b25e01f..75e20d9da08b 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -574,7 +574,6 @@ static void pnv_psi_power8_class_init(ObjectClass *klass, void *data)
>      dc->desc    = "PowerNV PSI Controller POWER8";
>      dc->realize = pnv_psi_power8_realize;
>  
> -    ppc->chip_type =  PNV_CHIP_POWER8;
>      ppc->xscom_pcba = PNV_XSCOM_PSIHB_BASE;
>      ppc->xscom_size = PNV_XSCOM_PSIHB_SIZE;
>      ppc->bar_mask   = PSIHB_BAR_MASK;
> @@ -884,7 +883,6 @@ static void pnv_psi_power9_class_init(ObjectClass *klass, void *data)
>      dc->desc    = "PowerNV PSI Controller POWER9";
>      dc->realize = pnv_psi_power9_realize;
>  
> -    ppc->chip_type  = PNV_CHIP_POWER9;
>      ppc->xscom_pcba = PNV9_XSCOM_PSIHB_BASE;
>      ppc->xscom_size = PNV9_XSCOM_PSIHB_SIZE;
>      ppc->bar_mask   = PSIHB9_BAR_MASK;
> @@ -915,7 +913,6 @@ static void pnv_psi_power10_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc    = "PowerNV PSI Controller POWER10";
>  
> -    ppc->chip_type  = PNV_CHIP_POWER10;
>      ppc->xscom_pcba = PNV10_XSCOM_PSIHB_BASE;
>      ppc->xscom_size = PNV10_XSCOM_PSIHB_SIZE;
>      ppc->compat     = compat;
> diff --git a/include/hw/ppc/pnv_psi.h b/include/hw/ppc/pnv_psi.h
> index fc068c95e543..f0f5b5519767 100644
> --- a/include/hw/ppc/pnv_psi.h
> +++ b/include/hw/ppc/pnv_psi.h
> @@ -79,7 +79,6 @@ typedef struct Pnv9Psi {
>  typedef struct PnvPsiClass {
>      SysBusDeviceClass parent_class;
>  
> -    int chip_type;
>      uint32_t xscom_pcba;
>      uint32_t xscom_size;
>      uint64_t bar_mask;
> 

[PATCH 04/13] ppc/pnv: Introduce PnvMachineClass and PnvMachineClass::compat

[Greg Kurz]

The pnv_dt_create() function generates different contents for the
"compatible" property of the root node in the DT, depending on the
CPU type. This is open coded with multiple ifs using pnv_is_powerXX()
helpers.

It seems cleaner to achieve with QOM. Introduce a base class for the
powernv machine and a compat attribute that each child class can use
to provide the value for the "compatible" property.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c         |   33 +++++++++++++++++++--------------
 include/hw/ppc/pnv.h |   13 +++++++++++++
 2 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0be0b6b411c3..5ac149b149d8 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt)
 
 static void *pnv_dt_create(MachineState *machine)
 {
-    const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
-    const char plat_compat9[] = "qemu,powernv9\0ibm,powernv";
-    const char plat_compat10[] = "qemu,powernv10\0ibm,powernv";
+    PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine);
     PnvMachineState *pnv = PNV_MACHINE(machine);
     void *fdt;
     char *buf;
@@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine)
     _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2)));
     _FDT((fdt_setprop_string(fdt, 0, "model",
                              "IBM PowerNV (emulated by qemu)")));
-    if (pnv_is_power10(pnv)) {
-        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10,
-                          sizeof(plat_compat10))));
-    } else if (pnv_is_power9(pnv)) {
-        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9,
-                          sizeof(plat_compat9))));
-    } else {
-        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8,
-                          sizeof(plat_compat8))));
-    }
-
+    _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat,
+                      sizeof(pmc->compat))));
 
     buf =  qemu_uuid_unparse_strdup(&qemu_uuid);
     _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf)));
@@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
+    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
+    static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
 
     mc->desc = "IBM PowerNV (Non-Virtualized) POWER8";
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
@@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
     xic->icp_get = pnv_icp_get;
     xic->ics_get = pnv_ics_get;
     xic->ics_resend = pnv_ics_resend;
+
+    pmc->compat = compat;
+    pmc->compat_size = sizeof(compat);
 }
 
 static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
+    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
+    static const char compat[] = "qemu,powernv9\0ibm,powernv";
 
     mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
     xfc->match_nvt = pnv_match_nvt;
 
     mc->alias = "powernv";
+
+    pmc->compat = compat;
+    pmc->compat_size = sizeof(compat);
 }
 
 static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
+    static const char compat[] = "qemu,powernv10\0ibm,powernv";
 
     mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0");
+
+    pmc->compat = compat;
+    pmc->compat_size = sizeof(compat);
 }
 
 static void pnv_machine_class_init(ObjectClass *oc, void *data)
@@ -1796,6 +1800,7 @@ static const TypeInfo types[] = {
         .instance_size = sizeof(PnvMachineState),
         .instance_init = pnv_machine_instance_init,
         .class_init    = pnv_machine_class_init,
+        .class_size    = sizeof(PnvMachineClass),
         .interfaces = (InterfaceInfo[]) {
             { TYPE_INTERRUPT_STATS_PROVIDER },
             { },
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 92f80b1ccead..d534746bd493 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
 #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 #define PNV_MACHINE(obj) \
     OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE)
+#define PNV_MACHINE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE)
+#define PNV_MACHINE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE)
+
+typedef struct PnvMachineClass {
+    /*< private >*/
+    MachineClass parent_class;
+
+    /*< public >*/
+    const char *compat;
+    int compat_size;
+} PnvMachineClass;
 
 typedef struct PnvMachineState {
     /*< private >*/

Re: [PATCH 04/13] ppc/pnv: Introduce PnvMachineClass and PnvMachineClass::compat

[Cédric Le Goater]

On 13/12/2019 12:59, Greg Kurz wrote:
> The pnv_dt_create() function generates different contents for the
> "compatible" property of the root node in the DT, depending on the
> CPU type. This is open coded with multiple ifs using pnv_is_powerXX()
> helpers.
> 
> It seems cleaner to achieve with QOM. Introduce a base class for the
> powernv machine and a compat attribute that each child class can use
> to provide the value for the "compatible" property.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  hw/ppc/pnv.c         |   33 +++++++++++++++++++--------------
>  include/hw/ppc/pnv.h |   13 +++++++++++++
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0be0b6b411c3..5ac149b149d8 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt)
>  
>  static void *pnv_dt_create(MachineState *machine)
>  {
> -    const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
> -    const char plat_compat9[] = "qemu,powernv9\0ibm,powernv";
> -    const char plat_compat10[] = "qemu,powernv10\0ibm,powernv";
> +    PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine);
>      PnvMachineState *pnv = PNV_MACHINE(machine);
>      void *fdt;
>      char *buf;
> @@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine)
>      _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2)));
>      _FDT((fdt_setprop_string(fdt, 0, "model",
>                               "IBM PowerNV (emulated by qemu)")));
> -    if (pnv_is_power10(pnv)) {
> -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10,
> -                          sizeof(plat_compat10))));
> -    } else if (pnv_is_power9(pnv)) {
> -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9,
> -                          sizeof(plat_compat9))));
> -    } else {
> -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8,
> -                          sizeof(plat_compat8))));
> -    }
> -
> +    _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat,
> +                      sizeof(pmc->compat))));
>  
>      buf =  qemu_uuid_unparse_strdup(&qemu_uuid);
>      _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf)));
> @@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> +    static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized) POWER8";
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> @@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
>      xic->icp_get = pnv_icp_get;
>      xic->ics_get = pnv_ics_get;
>      xic->ics_resend = pnv_ics_resend;
> +
> +    pmc->compat = compat;
> +    pmc->compat_size = sizeof(compat);
>  }
>  
>  static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
>      XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
> +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> +    static const char compat[] = "qemu,powernv9\0ibm,powernv";
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
>      xfc->match_nvt = pnv_match_nvt;
>  
>      mc->alias = "powernv";
> +
> +    pmc->compat = compat;
> +    pmc->compat_size = sizeof(compat);
>  }
>  
>  static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> +    static const char compat[] = "qemu,powernv10\0ibm,powernv";
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
>      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0");
> +
> +    pmc->compat = compat;
> +    pmc->compat_size = sizeof(compat);
>  }
>  
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
> @@ -1796,6 +1800,7 @@ static const TypeInfo types[] = {
>          .instance_size = sizeof(PnvMachineState),
>          .instance_init = pnv_machine_instance_init,
>          .class_init    = pnv_machine_class_init,
> +        .class_size    = sizeof(PnvMachineClass),
>          .interfaces = (InterfaceInfo[]) {
>              { TYPE_INTERRUPT_STATS_PROVIDER },
>              { },
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 92f80b1ccead..d534746bd493 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
>  #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>  #define PNV_MACHINE(obj) \
>      OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE)
> +#define PNV_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE)
> +#define PNV_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE)
> +
> +typedef struct PnvMachineClass {
> +    /*< private >*/
> +    MachineClass parent_class;
> +
> +    /*< public >*/
> +    const char *compat;
> +    int compat_size;
> +} PnvMachineClass;
>  
>  typedef struct PnvMachineState {
>      /*< private >*/
> 

Re: [PATCH 04/13] ppc/pnv: Introduce PnvMachineClass and PnvMachineClass::compat

[Greg Kurz]

On Fri, 13 Dec 2019 13:44:48 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 13/12/2019 12:59, Greg Kurz wrote:
> > The pnv_dt_create() function generates different contents for the
> > "compatible" property of the root node in the DT, depending on the
> > CPU type. This is open coded with multiple ifs using pnv_is_powerXX()
> > helpers.
> > 
> > It seems cleaner to achieve with QOM. Introduce a base class for the
> > powernv machine and a compat attribute that each child class can use
> > to provide the value for the "compatible" property.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> 
> > ---
> >  hw/ppc/pnv.c         |   33 +++++++++++++++++++--------------
> >  include/hw/ppc/pnv.h |   13 +++++++++++++
> >  2 files changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0be0b6b411c3..5ac149b149d8 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt)
> >  
> >  static void *pnv_dt_create(MachineState *machine)
> >  {
> > -    const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
> > -    const char plat_compat9[] = "qemu,powernv9\0ibm,powernv";
> > -    const char plat_compat10[] = "qemu,powernv10\0ibm,powernv";
> > +    PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine);
> >      PnvMachineState *pnv = PNV_MACHINE(machine);
> >      void *fdt;
> >      char *buf;
> > @@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine)
> >      _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2)));
> >      _FDT((fdt_setprop_string(fdt, 0, "model",
> >                               "IBM PowerNV (emulated by qemu)")));
> > -    if (pnv_is_power10(pnv)) {
> > -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10,
> > -                          sizeof(plat_compat10))));
> > -    } else if (pnv_is_power9(pnv)) {
> > -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9,
> > -                          sizeof(plat_compat9))));
> > -    } else {
> > -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8,
> > -                          sizeof(plat_compat8))));
> > -    }
> > -
> > +    _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat,
> > +                      sizeof(pmc->compat))));

Of course the size should be pmc->compat_size ... David, can you fix this
in your tree or should I post a v2 ?

> >  
> >      buf =  qemu_uuid_unparse_strdup(&qemu_uuid);
> >      _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf)));
> > @@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> >      XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> > +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > +    static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
> >  
> >      mc->desc = "IBM PowerNV (Non-Virtualized) POWER8";
> >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > @@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
> >      xic->icp_get = pnv_icp_get;
> >      xic->ics_get = pnv_ics_get;
> >      xic->ics_resend = pnv_ics_resend;
> > +
> > +    pmc->compat = compat;
> > +    pmc->compat_size = sizeof(compat);
> >  }
> >  
> >  static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> >      XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
> > +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > +    static const char compat[] = "qemu,powernv9\0ibm,powernv";
> >  
> >      mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
> >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> >      xfc->match_nvt = pnv_match_nvt;
> >  
> >      mc->alias = "powernv";
> > +
> > +    pmc->compat = compat;
> > +    pmc->compat_size = sizeof(compat);
> >  }
> >  
> >  static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> >  {
> >      MachineClass *mc = MACHINE_CLASS(oc);
> > +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > +    static const char compat[] = "qemu,powernv10\0ibm,powernv";
> >  
> >      mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
> >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0");
> > +
> > +    pmc->compat = compat;
> > +    pmc->compat_size = sizeof(compat);
> >  }
> >  
> >  static void pnv_machine_class_init(ObjectClass *oc, void *data)
> > @@ -1796,6 +1800,7 @@ static const TypeInfo types[] = {
> >          .instance_size = sizeof(PnvMachineState),
> >          .instance_init = pnv_machine_instance_init,
> >          .class_init    = pnv_machine_class_init,
> > +        .class_size    = sizeof(PnvMachineClass),
> >          .interfaces = (InterfaceInfo[]) {
> >              { TYPE_INTERRUPT_STATS_PROVIDER },
> >              { },
> > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > index 92f80b1ccead..d534746bd493 100644
> > --- a/include/hw/ppc/pnv.h
> > +++ b/include/hw/ppc/pnv.h
> > @@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> >  #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
> >  #define PNV_MACHINE(obj) \
> >      OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE)
> > +#define PNV_MACHINE_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE)
> > +#define PNV_MACHINE_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE)
> > +
> > +typedef struct PnvMachineClass {
> > +    /*< private >*/
> > +    MachineClass parent_class;
> > +
> > +    /*< public >*/
> > +    const char *compat;
> > +    int compat_size;
> > +} PnvMachineClass;
> >  
> >  typedef struct PnvMachineState {
> >      /*< private >*/
> > 
> 
> 

Re: [PATCH 04/13] ppc/pnv: Introduce PnvMachineClass and PnvMachineClass::compat

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6698 bytes --]

On Mon, Dec 16, 2019 at 07:07:43PM +0100, Greg Kurz wrote:
> On Fri, 13 Dec 2019 13:44:48 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 13/12/2019 12:59, Greg Kurz wrote:
> > > The pnv_dt_create() function generates different contents for the
> > > "compatible" property of the root node in the DT, depending on the
> > > CPU type. This is open coded with multiple ifs using pnv_is_powerXX()
> > > helpers.
> > > 
> > > It seems cleaner to achieve with QOM. Introduce a base class for the
> > > powernv machine and a compat attribute that each child class can use
> > > to provide the value for the "compatible" property.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > 
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > 
> > 
> > > ---
> > >  hw/ppc/pnv.c         |   33 +++++++++++++++++++--------------
> > >  include/hw/ppc/pnv.h |   13 +++++++++++++
> > >  2 files changed, 32 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > > index 0be0b6b411c3..5ac149b149d8 100644
> > > --- a/hw/ppc/pnv.c
> > > +++ b/hw/ppc/pnv.c
> > > @@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt)
> > >  
> > >  static void *pnv_dt_create(MachineState *machine)
> > >  {
> > > -    const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
> > > -    const char plat_compat9[] = "qemu,powernv9\0ibm,powernv";
> > > -    const char plat_compat10[] = "qemu,powernv10\0ibm,powernv";
> > > +    PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine);
> > >      PnvMachineState *pnv = PNV_MACHINE(machine);
> > >      void *fdt;
> > >      char *buf;
> > > @@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine)
> > >      _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2)));
> > >      _FDT((fdt_setprop_string(fdt, 0, "model",
> > >                               "IBM PowerNV (emulated by qemu)")));
> > > -    if (pnv_is_power10(pnv)) {
> > > -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10,
> > > -                          sizeof(plat_compat10))));
> > > -    } else if (pnv_is_power9(pnv)) {
> > > -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9,
> > > -                          sizeof(plat_compat9))));
> > > -    } else {
> > > -        _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8,
> > > -                          sizeof(plat_compat8))));
> > > -    }
> > > -
> > > +    _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat,
> > > +                      sizeof(pmc->compat))));
> 
> Of course the size should be pmc->compat_size ... David, can you fix this
> in your tree or should I post a v2 ?

Ah, yes.  This message came just barely in time - I've folded in the
fix as I've been setting up to to a pull request including it.

> 
> > >  
> > >      buf =  qemu_uuid_unparse_strdup(&qemu_uuid);
> > >      _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf)));
> > > @@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      MachineClass *mc = MACHINE_CLASS(oc);
> > >      XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> > > +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > > +    static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv";
> > >  
> > >      mc->desc = "IBM PowerNV (Non-Virtualized) POWER8";
> > >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
> > > @@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data)
> > >      xic->icp_get = pnv_icp_get;
> > >      xic->ics_get = pnv_ics_get;
> > >      xic->ics_resend = pnv_ics_resend;
> > > +
> > > +    pmc->compat = compat;
> > > +    pmc->compat_size = sizeof(compat);
> > >  }
> > >  
> > >  static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      MachineClass *mc = MACHINE_CLASS(oc);
> > >      XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc);
> > > +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > > +    static const char compat[] = "qemu,powernv9\0ibm,powernv";
> > >  
> > >      mc->desc = "IBM PowerNV (Non-Virtualized) POWER9";
> > >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> > >      xfc->match_nvt = pnv_match_nvt;
> > >  
> > >      mc->alias = "powernv";
> > > +
> > > +    pmc->compat = compat;
> > > +    pmc->compat_size = sizeof(compat);
> > >  }
> > >  
> > >  static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      MachineClass *mc = MACHINE_CLASS(oc);
> > > +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
> > > +    static const char compat[] = "qemu,powernv10\0ibm,powernv";
> > >  
> > >      mc->desc = "IBM PowerNV (Non-Virtualized) POWER10";
> > >      mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0");
> > > +
> > > +    pmc->compat = compat;
> > > +    pmc->compat_size = sizeof(compat);
> > >  }
> > >  
> > >  static void pnv_machine_class_init(ObjectClass *oc, void *data)
> > > @@ -1796,6 +1800,7 @@ static const TypeInfo types[] = {
> > >          .instance_size = sizeof(PnvMachineState),
> > >          .instance_init = pnv_machine_instance_init,
> > >          .class_init    = pnv_machine_class_init,
> > > +        .class_size    = sizeof(PnvMachineClass),
> > >          .interfaces = (InterfaceInfo[]) {
> > >              { TYPE_INTERRUPT_STATS_PROVIDER },
> > >              { },
> > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > > index 92f80b1ccead..d534746bd493 100644
> > > --- a/include/hw/ppc/pnv.h
> > > +++ b/include/hw/ppc/pnv.h
> > > @@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
> > >  #define TYPE_PNV_MACHINE       MACHINE_TYPE_NAME("powernv")
> > >  #define PNV_MACHINE(obj) \
> > >      OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE)
> > > +#define PNV_MACHINE_GET_CLASS(obj) \
> > > +    OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE)
> > > +#define PNV_MACHINE_CLASS(klass) \
> > > +    OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE)
> > > +
> > > +typedef struct PnvMachineClass {
> > > +    /*< private >*/
> > > +    MachineClass parent_class;
> > > +
> > > +    /*< public >*/
> > > +    const char *compat;
> > > +    int compat_size;
> > > +} PnvMachineClass;
> > >  
> > >  typedef struct PnvMachineState {
> > >      /*< private >*/
> > > 
> > 
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 05/13] ppc/pnv: Introduce PnvMachineClass::dt_power_mgt()

[Greg Kurz]

We add an extra node to advertise power management on some machines,
namely powernv9 and powernv10. This is achieved by using the
pnv_is_power9() and pnv_is_power10() helpers.

This can be achieved with QOM. Add a method to the base class for
powernv machines and have it implemented by machine types that
support power management instead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c         |   10 ++++++----
 include/hw/ppc/pnv.h |    8 ++++++--
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 5ac149b149d8..efc00f4cb67a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -472,7 +472,7 @@ static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
                        &args);
 }
 
-static void pnv_dt_power_mgt(void *fdt)
+static void pnv_dt_power_mgt(PnvMachineState *pnv, void *fdt)
 {
     int off;
 
@@ -540,9 +540,9 @@ static void *pnv_dt_create(MachineState *machine)
         pnv_dt_bmc_sensors(pnv->bmc, fdt);
     }
 
-    /* Create an extra node for power management on Power9 and Power10 */
-    if (pnv_is_power9(pnv) || pnv_is_power10(pnv)) {
-        pnv_dt_power_mgt(fdt);
+    /* Create an extra node for power management on machines that support it */
+    if (pmc->dt_power_mgt) {
+        pmc->dt_power_mgt(pnv, fdt);
     }
 
     return fdt;
@@ -1710,6 +1710,7 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
 
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
+    pmc->dt_power_mgt = pnv_dt_power_mgt;
 }
 
 static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
@@ -1723,6 +1724,7 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
 
     pmc->compat = compat;
     pmc->compat_size = sizeof(compat);
+    pmc->dt_power_mgt = pnv_dt_power_mgt;
 }
 
 static void pnv_machine_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index d534746bd493..8a42c199b65c 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -190,6 +190,8 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
 #define PNV_MACHINE_CLASS(klass) \
     OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE)
 
+typedef struct PnvMachineState PnvMachineState;
+
 typedef struct PnvMachineClass {
     /*< private >*/
     MachineClass parent_class;
@@ -197,9 +199,11 @@ typedef struct PnvMachineClass {
     /*< public >*/
     const char *compat;
     int compat_size;
+
+    void (*dt_power_mgt)(PnvMachineState *pnv, void *fdt);
 } PnvMachineClass;
 
-typedef struct PnvMachineState {
+struct PnvMachineState {
     /*< private >*/
     MachineState parent_obj;
 
@@ -216,7 +220,7 @@ typedef struct PnvMachineState {
     Notifier     powerdown_notifier;
 
     PnvPnor      *pnor;
-} PnvMachineState;
+};
 
 static inline bool pnv_chip_is_power9(const PnvChip *chip)
 {

Re: [PATCH 05/13] ppc/pnv: Introduce PnvMachineClass::dt_power_mgt()

[Cédric Le Goater]

On 13/12/2019 12:59, Greg Kurz wrote:
> We add an extra node to advertise power management on some machines,
> namely powernv9 and powernv10. This is achieved by using the
> pnv_is_power9() and pnv_is_power10() helpers.
> 
> This can be achieved with QOM. Add a method to the base class for
> powernv machines and have it implemented by machine types that
> support power management instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv.c         |   10 ++++++----
>  include/hw/ppc/pnv.h |    8 ++++++--
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 5ac149b149d8..efc00f4cb67a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -472,7 +472,7 @@ static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
>                         &args);
>  }
>  
> -static void pnv_dt_power_mgt(void *fdt)
> +static void pnv_dt_power_mgt(PnvMachineState *pnv, void *fdt)
>  {
>      int off;
>  
> @@ -540,9 +540,9 @@ static void *pnv_dt_create(MachineState *machine)
>          pnv_dt_bmc_sensors(pnv->bmc, fdt);
>      }
>  
> -    /* Create an extra node for power management on Power9 and Power10 */
> -    if (pnv_is_power9(pnv) || pnv_is_power10(pnv)) {
> -        pnv_dt_power_mgt(fdt);
> +    /* Create an extra node for power management on machines that support it */
> +    if (pmc->dt_power_mgt) {
> +        pmc->dt_power_mgt(pnv, fdt);
>      }
>  
>      return fdt;
> @@ -1710,6 +1710,7 @@ static void pnv_machine_power9_class_init(ObjectClass *oc, void *data)
>  
>      pmc->compat = compat;
>      pmc->compat_size = sizeof(compat);
> +    pmc->dt_power_mgt = pnv_dt_power_mgt;
>  }
>  
>  static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
> @@ -1723,6 +1724,7 @@ static void pnv_machine_power10_class_init(ObjectClass *oc, void *data)
>  
>      pmc->compat = compat;
>      pmc->compat_size = sizeof(compat);
> +    pmc->dt_power_mgt = pnv_dt_power_mgt;
>  }
>  
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index d534746bd493..8a42c199b65c 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -190,6 +190,8 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir);
>  #define PNV_MACHINE_CLASS(klass) \
>      OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE)
>  
> +typedef struct PnvMachineState PnvMachineState;
> +
>  typedef struct PnvMachineClass {
>      /*< private >*/
>      MachineClass parent_class;
> @@ -197,9 +199,11 @@ typedef struct PnvMachineClass {
>      /*< public >*/
>      const char *compat;
>      int compat_size;
> +
> +    void (*dt_power_mgt)(PnvMachineState *pnv, void *fdt);
>  } PnvMachineClass;
>  
> -typedef struct PnvMachineState {
> +struct PnvMachineState {
>      /*< private >*/
>      MachineState parent_obj;
>  
> @@ -216,7 +220,7 @@ typedef struct PnvMachineState {
>      Notifier     powerdown_notifier;
>  
>      PnvPnor      *pnor;
> -} PnvMachineState;
> +};
>  
>  static inline bool pnv_chip_is_power9(const PnvChip *chip)
>  {
> 

[PATCH 06/13] ppc/pnv: Drop pnv_is_power9() and pnv_is_power10() helpers

[Greg Kurz]

They aren't used anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/pnv.h |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 8a42c199b65c..c213bdd5ecd3 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -227,11 +227,6 @@ static inline bool pnv_chip_is_power9(const PnvChip *chip)
     return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER9;
 }
 
-static inline bool pnv_is_power9(PnvMachineState *pnv)
-{
-    return pnv_chip_is_power9(pnv->chips[0]);
-}
-
 PnvChip *pnv_get_chip(uint32_t chip_id);
 
 #define PNV_FDT_ADDR          0x01000000
@@ -242,11 +237,6 @@ static inline bool pnv_chip_is_power10(const PnvChip *chip)
     return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER10;
 }
 
-static inline bool pnv_is_power10(PnvMachineState *pnv)
-{
-    return pnv_chip_is_power10(pnv->chips[0]);
-}
-
 /*
  * BMC helpers
  */

Re: [PATCH 06/13] ppc/pnv: Drop pnv_is_power9() and pnv_is_power10() helpers

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> They aren't used anymore.

Good ! 

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>


> ---
>  include/hw/ppc/pnv.h |   10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 8a42c199b65c..c213bdd5ecd3 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -227,11 +227,6 @@ static inline bool pnv_chip_is_power9(const PnvChip *chip)
>      return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER9;
>  }
>  
> -static inline bool pnv_is_power9(PnvMachineState *pnv)
> -{
> -    return pnv_chip_is_power9(pnv->chips[0]);
> -}
> -
>  PnvChip *pnv_get_chip(uint32_t chip_id);
>  
>  #define PNV_FDT_ADDR          0x01000000
> @@ -242,11 +237,6 @@ static inline bool pnv_chip_is_power10(const PnvChip *chip)
>      return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER10;
>  }
>  
> -static inline bool pnv_is_power10(PnvMachineState *pnv)
> -{
> -    return pnv_chip_is_power10(pnv->chips[0]);
> -}
> -
>  /*
>   * BMC helpers
>   */
> 

[PATCH 07/13] ppc/pnv: Introduce PnvChipClass::intc_print_info() method

[Greg Kurz]

The pnv_pic_print_info() callback checks the type of the chip in order
to forward to the request appropriate interrupt controller. This can
be achieved with QOM. Introduce a method for this in the base chip class
and implement it in child classes.

This also prepares ground for the upcoming interrupt controller of POWER10
chips.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c         |   30 +++++++++++++++++++++++++-----
 include/hw/ppc/pnv.h |    1 +
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index efc00f4cb67a..2a53e99bda2e 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -832,6 +832,12 @@ static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
     pnv_cpu->intc = NULL;
 }
 
+static void pnv_chip_power8_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
+                                            Monitor *mon)
+{
+    icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
+}
+
 /*
  *    0:48  Reserved - Read as zeroes
  *   49:52  Node ID
@@ -889,6 +895,12 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
     pnv_cpu->intc = NULL;
 }
 
+static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
+                                            Monitor *mon)
+{
+    xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
+}
+
 static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu,
                                         Error **errp)
 {
@@ -910,6 +922,11 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
     pnv_cpu->intc = NULL;
 }
 
+static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
+                                             Monitor *mon)
+{
+}
+
 /*
  * Allowed core identifiers on a POWER8 Processor Chip :
  *
@@ -1086,6 +1103,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->intc_create = pnv_chip_power8_intc_create;
     k->intc_reset = pnv_chip_power8_intc_reset;
     k->intc_destroy = pnv_chip_power8_intc_destroy;
+    k->intc_print_info = pnv_chip_power8_intc_print_info;
     k->isa_create = pnv_chip_power8_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
@@ -1107,6 +1125,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->intc_create = pnv_chip_power8_intc_create;
     k->intc_reset = pnv_chip_power8_intc_reset;
     k->intc_destroy = pnv_chip_power8_intc_destroy;
+    k->intc_print_info = pnv_chip_power8_intc_print_info;
     k->isa_create = pnv_chip_power8_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
@@ -1128,6 +1147,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->intc_create = pnv_chip_power8_intc_create;
     k->intc_reset = pnv_chip_power8_intc_reset;
     k->intc_destroy = pnv_chip_power8_intc_destroy;
+    k->intc_print_info = pnv_chip_power8_intc_print_info;
     k->isa_create = pnv_chip_power8nvl_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
@@ -1299,6 +1319,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->intc_create = pnv_chip_power9_intc_create;
     k->intc_reset = pnv_chip_power9_intc_reset;
     k->intc_destroy = pnv_chip_power9_intc_destroy;
+    k->intc_print_info = pnv_chip_power9_intc_print_info;
     k->isa_create = pnv_chip_power9_isa_create;
     k->dt_populate = pnv_chip_power9_dt_populate;
     k->pic_print_info = pnv_chip_power9_pic_print_info;
@@ -1379,6 +1400,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
     k->intc_create = pnv_chip_power10_intc_create;
     k->intc_reset = pnv_chip_power10_intc_reset;
     k->intc_destroy = pnv_chip_power10_intc_destroy;
+    k->intc_print_info = pnv_chip_power10_intc_print_info;
     k->isa_create = pnv_chip_power10_isa_create;
     k->dt_populate = pnv_chip_power10_dt_populate;
     k->pic_print_info = pnv_chip_power10_pic_print_info;
@@ -1575,11 +1597,9 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        if (pnv_chip_is_power9(pnv->chips[0])) {
-            xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
-        } else {
-            icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
-        }
+        /* XXX: loop on each chip/core/thread instead of CPU_FOREACH() */
+        PNV_CHIP_GET_CLASS(pnv->chips[0])->intc_print_info(pnv->chips[0], cpu,
+                                                           mon);
     }
 
     for (i = 0; i < pnv->num_chips; i++) {
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index c213bdd5ecd3..7d2402784d4b 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -133,6 +133,7 @@ typedef struct PnvChipClass {
     void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
     void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
     void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu);
+    void (*intc_print_info)(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon);
     ISABus *(*isa_create)(PnvChip *chip, Error **errp);
     void (*dt_populate)(PnvChip *chip, void *fdt);
     void (*pic_print_info)(PnvChip *chip, Monitor *mon);

Re: [PATCH 07/13] ppc/pnv: Introduce PnvChipClass::intc_print_info() method

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> The pnv_pic_print_info() callback checks the type of the chip in order
> to forward to the request appropriate interrupt controller. This can
> be achieved with QOM. Introduce a method for this in the base chip class
> and implement it in child classes.
> 
> This also prepares ground for the upcoming interrupt controller of POWER10
> chips.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

One comment below.

> ---
>  hw/ppc/pnv.c         |   30 +++++++++++++++++++++++++-----
>  include/hw/ppc/pnv.h |    1 +
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index efc00f4cb67a..2a53e99bda2e 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -832,6 +832,12 @@ static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
>      pnv_cpu->intc = NULL;
>  }
>  
> +static void pnv_chip_power8_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> +                                            Monitor *mon)
> +{
> +    icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
> +}
> +
>  /*
>   *    0:48  Reserved - Read as zeroes
>   *   49:52  Node ID
> @@ -889,6 +895,12 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
>      pnv_cpu->intc = NULL;
>  }
>  
> +static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> +                                            Monitor *mon)
> +{
> +    xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
> +}
> +
>  static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>                                          Error **errp)
>  {
> @@ -910,6 +922,11 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
>      pnv_cpu->intc = NULL;
>  }
>  
> +static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> +                                             Monitor *mon)
> +{
> +}
> +
>  /*
>   * Allowed core identifiers on a POWER8 Processor Chip :
>   *
> @@ -1086,6 +1103,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->intc_create = pnv_chip_power8_intc_create;
>      k->intc_reset = pnv_chip_power8_intc_reset;
>      k->intc_destroy = pnv_chip_power8_intc_destroy;
> +    k->intc_print_info = pnv_chip_power8_intc_print_info;
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1107,6 +1125,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->intc_create = pnv_chip_power8_intc_create;
>      k->intc_reset = pnv_chip_power8_intc_reset;
>      k->intc_destroy = pnv_chip_power8_intc_destroy;
> +    k->intc_print_info = pnv_chip_power8_intc_print_info;
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1128,6 +1147,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->intc_create = pnv_chip_power8_intc_create;
>      k->intc_reset = pnv_chip_power8_intc_reset;
>      k->intc_destroy = pnv_chip_power8_intc_destroy;
> +    k->intc_print_info = pnv_chip_power8_intc_print_info;
>      k->isa_create = pnv_chip_power8nvl_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1299,6 +1319,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->intc_create = pnv_chip_power9_intc_create;
>      k->intc_reset = pnv_chip_power9_intc_reset;
>      k->intc_destroy = pnv_chip_power9_intc_destroy;
> +    k->intc_print_info = pnv_chip_power9_intc_print_info;
>      k->isa_create = pnv_chip_power9_isa_create;
>      k->dt_populate = pnv_chip_power9_dt_populate;
>      k->pic_print_info = pnv_chip_power9_pic_print_info;
> @@ -1379,6 +1400,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
>      k->intc_create = pnv_chip_power10_intc_create;
>      k->intc_reset = pnv_chip_power10_intc_reset;
>      k->intc_destroy = pnv_chip_power10_intc_destroy;
> +    k->intc_print_info = pnv_chip_power10_intc_print_info;
>      k->isa_create = pnv_chip_power10_isa_create;
>      k->dt_populate = pnv_chip_power10_dt_populate;
>      k->pic_print_info = pnv_chip_power10_pic_print_info;
> @@ -1575,11 +1597,9 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        if (pnv_chip_is_power9(pnv->chips[0])) {
> -            xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
> -        } else {
> -            icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
> -        }
> +        /* XXX: loop on each chip/core/thread instead of CPU_FOREACH() */

May be we should introduce a helper such as : 

int pnv_chip_cpu_foreach(PnvChip *chip,
	  int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void *opaque)
{
    int i, j;
    int ret = 0;

    for (i = 0; i < chip->nr_cores; i++) {
        PnvCore *pc = chip->cores[i];
        CPUCore *cc = CPU_CORE(pc);

        for (j = 0; j < cc->nr_threads; j++) {
            PowerPCCPU *cpu = pc->threads[j];
            ret = doit(chip, cpu, opaque);
            if (ret) { 
                break;
            }
        }
    }
    return ret;
}

> +        PNV_CHIP_GET_CLASS(pnv->chips[0])->intc_print_info(pnv->chips[0], cpu,
> +                                                           mon);
>      }
>  
>      for (i = 0; i < pnv->num_chips; i++) {
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index c213bdd5ecd3..7d2402784d4b 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -133,6 +133,7 @@ typedef struct PnvChipClass {
>      void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
>      void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
>      void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu);
> +    void (*intc_print_info)(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon);
>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>      void (*dt_populate)(PnvChip *chip, void *fdt);
>      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> 

Re: [PATCH 07/13] ppc/pnv: Introduce PnvChipClass::intc_print_info() method

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7403 bytes --]

On Fri, Dec 13, 2019 at 02:00:53PM +0100, Cédric Le Goater wrote:
> On 13/12/2019 13:00, Greg Kurz wrote:
> > The pnv_pic_print_info() callback checks the type of the chip in order
> > to forward to the request appropriate interrupt controller. This can
> > be achieved with QOM. Introduce a method for this in the base chip class
> > and implement it in child classes.
> > 
> > This also prepares ground for the upcoming interrupt controller of POWER10
> > chips.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> One comment below.
> 
> > ---
> >  hw/ppc/pnv.c         |   30 +++++++++++++++++++++++++-----
> >  include/hw/ppc/pnv.h |    1 +
> >  2 files changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index efc00f4cb67a..2a53e99bda2e 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -832,6 +832,12 @@ static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> >      pnv_cpu->intc = NULL;
> >  }
> >  
> > +static void pnv_chip_power8_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> > +                                            Monitor *mon)
> > +{
> > +    icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
> > +}
> > +
> >  /*
> >   *    0:48  Reserved - Read as zeroes
> >   *   49:52  Node ID
> > @@ -889,6 +895,12 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> >      pnv_cpu->intc = NULL;
> >  }
> >  
> > +static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> > +                                            Monitor *mon)
> > +{
> > +    xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
> > +}
> > +
> >  static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu,
> >                                          Error **errp)
> >  {
> > @@ -910,6 +922,11 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu)
> >      pnv_cpu->intc = NULL;
> >  }
> >  
> > +static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
> > +                                             Monitor *mon)
> > +{
> > +}
> > +
> >  /*
> >   * Allowed core identifiers on a POWER8 Processor Chip :
> >   *
> > @@ -1086,6 +1103,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >      k->intc_create = pnv_chip_power8_intc_create;
> >      k->intc_reset = pnv_chip_power8_intc_reset;
> >      k->intc_destroy = pnv_chip_power8_intc_destroy;
> > +    k->intc_print_info = pnv_chip_power8_intc_print_info;
> >      k->isa_create = pnv_chip_power8_isa_create;
> >      k->dt_populate = pnv_chip_power8_dt_populate;
> >      k->pic_print_info = pnv_chip_power8_pic_print_info;
> > @@ -1107,6 +1125,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
> >      k->intc_create = pnv_chip_power8_intc_create;
> >      k->intc_reset = pnv_chip_power8_intc_reset;
> >      k->intc_destroy = pnv_chip_power8_intc_destroy;
> > +    k->intc_print_info = pnv_chip_power8_intc_print_info;
> >      k->isa_create = pnv_chip_power8_isa_create;
> >      k->dt_populate = pnv_chip_power8_dt_populate;
> >      k->pic_print_info = pnv_chip_power8_pic_print_info;
> > @@ -1128,6 +1147,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
> >      k->intc_create = pnv_chip_power8_intc_create;
> >      k->intc_reset = pnv_chip_power8_intc_reset;
> >      k->intc_destroy = pnv_chip_power8_intc_destroy;
> > +    k->intc_print_info = pnv_chip_power8_intc_print_info;
> >      k->isa_create = pnv_chip_power8nvl_isa_create;
> >      k->dt_populate = pnv_chip_power8_dt_populate;
> >      k->pic_print_info = pnv_chip_power8_pic_print_info;
> > @@ -1299,6 +1319,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >      k->intc_create = pnv_chip_power9_intc_create;
> >      k->intc_reset = pnv_chip_power9_intc_reset;
> >      k->intc_destroy = pnv_chip_power9_intc_destroy;
> > +    k->intc_print_info = pnv_chip_power9_intc_print_info;
> >      k->isa_create = pnv_chip_power9_isa_create;
> >      k->dt_populate = pnv_chip_power9_dt_populate;
> >      k->pic_print_info = pnv_chip_power9_pic_print_info;
> > @@ -1379,6 +1400,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
> >      k->intc_create = pnv_chip_power10_intc_create;
> >      k->intc_reset = pnv_chip_power10_intc_reset;
> >      k->intc_destroy = pnv_chip_power10_intc_destroy;
> > +    k->intc_print_info = pnv_chip_power10_intc_print_info;
> >      k->isa_create = pnv_chip_power10_isa_create;
> >      k->dt_populate = pnv_chip_power10_dt_populate;
> >      k->pic_print_info = pnv_chip_power10_pic_print_info;
> > @@ -1575,11 +1597,9 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
> >      CPU_FOREACH(cs) {
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > -        if (pnv_chip_is_power9(pnv->chips[0])) {
> > -            xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon);
> > -        } else {
> > -            icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon);
> > -        }
> > +        /* XXX: loop on each chip/core/thread instead of CPU_FOREACH() */
> 
> May be we should introduce a helper such as : 
> 
> int pnv_chip_cpu_foreach(PnvChip *chip,
> 	  int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void *opaque)
> {
>     int i, j;
>     int ret = 0;
> 
>     for (i = 0; i < chip->nr_cores; i++) {
>         PnvCore *pc = chip->cores[i];
>         CPUCore *cc = CPU_CORE(pc);
> 
>         for (j = 0; j < cc->nr_threads; j++) {
>             PowerPCCPU *cpu = pc->threads[j];
>             ret = doit(chip, cpu, opaque);
>             if (ret) { 
>                 break;
>             }
>         }
>     }
>     return ret;
> }

What I'd actually like to work towards is just having the interrupt
controllers themselves advertize TYPE_INTERRUPT_STATS_PROVIDER and not
needing anything specific at the machine level to locate them, just
let the generic code in hmp_info_pic handle it.

But this patch is fine in the meantime.

> 
> > +        PNV_CHIP_GET_CLASS(pnv->chips[0])->intc_print_info(pnv->chips[0], cpu,
> > +                                                           mon);
> >      }
> >  
> >      for (i = 0; i < pnv->num_chips; i++) {
> > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > index c213bdd5ecd3..7d2402784d4b 100644
> > --- a/include/hw/ppc/pnv.h
> > +++ b/include/hw/ppc/pnv.h
> > @@ -133,6 +133,7 @@ typedef struct PnvChipClass {
> >      void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
> >      void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
> >      void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu);
> > +    void (*intc_print_info)(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon);
> >      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
> >      void (*dt_populate)(PnvChip *chip, void *fdt);
> >      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 07/13] ppc/pnv: Introduce PnvChipClass::intc_print_info() method

[Cédric Le Goater]

>> May be we should introduce a helper such as : 
>>
>> int pnv_chip_cpu_foreach(PnvChip *chip,
>> 	  int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void *opaque)
>> {
>>     int i, j;
>>     int ret = 0;
>>
>>     for (i = 0; i < chip->nr_cores; i++) {
>>         PnvCore *pc = chip->cores[i];
>>         CPUCore *cc = CPU_CORE(pc);
>>
>>         for (j = 0; j < cc->nr_threads; j++) {
>>             PowerPCCPU *cpu = pc->threads[j];
>>             ret = doit(chip, cpu, opaque);
>>             if (ret) { 
>>                 break;
>>             }
>>         }
>>     }
>>     return ret;
>> }
> 
> What I'd actually like to work towards is just having the interrupt
> controllers themselves advertize TYPE_INTERRUPT_STATS_PROVIDER and not
> needing anything specific at the machine level to locate them, just
> let the generic code in hmp_info_pic handle it.

OK. It would good to at least loop on the chips, so that the output
of the possible TYPE_INTERRUPT_STATS_PROVIDER (IC, PSIHB, PHB, NPU) 
are ordered. 

C.

[PATCH 08/13] ppc/pnv: Introduce PnvChipClass::xscom_core_base() method

[Greg Kurz]

The pnv_chip_core_realize() function configures the XSCOM MMIO subregion
for each core of a single chip. The base address of the subregion depends
on the CPU type. Its computation is currently open-code using the
pnv_chip_is_powerXX() helpers. This can be achieved with QOM. Introduce
a method for this in the base chip class and implement it in child classes.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c         |   31 ++++++++++++++++++++++++-------
 include/hw/ppc/pnv.h |    1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 2a53e99bda2e..88efa755e611 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -616,6 +616,24 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon)
     pnv_psi_pic_print_info(&chip9->psi, mon);
 }
 
+static uint64_t pnv_chip_power8_xscom_core_base(PnvChip *chip,
+                                                uint32_t core_id)
+{
+    return PNV_XSCOM_EX_BASE(core_id);
+}
+
+static uint64_t pnv_chip_power9_xscom_core_base(PnvChip *chip,
+                                                uint32_t core_id)
+{
+    return PNV9_XSCOM_EC_BASE(core_id);
+}
+
+static uint64_t pnv_chip_power10_xscom_core_base(PnvChip *chip,
+                                                 uint32_t core_id)
+{
+    return PNV10_XSCOM_EC_BASE(core_id);
+}
+
 static bool pnv_match_cpu(const char *default_type, const char *cpu_type)
 {
     PowerPCCPUClass *ppc_default =
@@ -1107,6 +1125,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->isa_create = pnv_chip_power8_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
+    k->xscom_core_base = pnv_chip_power8_xscom_core_base;
     dc->desc = "PowerNV Chip POWER8E";
 
     device_class_set_parent_realize(dc, pnv_chip_power8_realize,
@@ -1129,6 +1148,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->isa_create = pnv_chip_power8_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
+    k->xscom_core_base = pnv_chip_power8_xscom_core_base;
     dc->desc = "PowerNV Chip POWER8";
 
     device_class_set_parent_realize(dc, pnv_chip_power8_realize,
@@ -1151,6 +1171,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->isa_create = pnv_chip_power8nvl_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
+    k->xscom_core_base = pnv_chip_power8_xscom_core_base;
     dc->desc = "PowerNV Chip POWER8NVL";
 
     device_class_set_parent_realize(dc, pnv_chip_power8_realize,
@@ -1323,6 +1344,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->isa_create = pnv_chip_power9_isa_create;
     k->dt_populate = pnv_chip_power9_dt_populate;
     k->pic_print_info = pnv_chip_power9_pic_print_info;
+    k->xscom_core_base = pnv_chip_power9_xscom_core_base;
     dc->desc = "PowerNV Chip POWER9";
 
     device_class_set_parent_realize(dc, pnv_chip_power9_realize,
@@ -1404,6 +1426,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
     k->isa_create = pnv_chip_power10_isa_create;
     k->dt_populate = pnv_chip_power10_dt_populate;
     k->pic_print_info = pnv_chip_power10_pic_print_info;
+    k->xscom_core_base = pnv_chip_power10_xscom_core_base;
     dc->desc = "PowerNV Chip POWER10";
 
     device_class_set_parent_realize(dc, pnv_chip_power10_realize,
@@ -1491,13 +1514,7 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
                                  &error_fatal);
 
         /* Each core has an XSCOM MMIO region */
-        if (pnv_chip_is_power10(chip)) {
-            xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid);
-        } else if (pnv_chip_is_power9(chip)) {
-            xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid);
-        } else {
-            xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid);
-        }
+        xscom_core_base = pcc->xscom_core_base(chip, core_hwid);
 
         pnv_xscom_add_subregion(chip, xscom_core_base,
                                 &pnv_core->xscom_regs);
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 7d2402784d4b..17ca9a14ac8f 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -137,6 +137,7 @@ typedef struct PnvChipClass {
     ISABus *(*isa_create)(PnvChip *chip, Error **errp);
     void (*dt_populate)(PnvChip *chip, void *fdt);
     void (*pic_print_info)(PnvChip *chip, Monitor *mon);
+    uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id);
 } PnvChipClass;
 
 #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP

Re: [PATCH 08/13] ppc/pnv: Introduce PnvChipClass::xscom_core_base() method

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> The pnv_chip_core_realize() function configures the XSCOM MMIO subregion
> for each core of a single chip. The base address of the subregion depends
> on the CPU type. Its computation is currently open-code using the
> pnv_chip_is_powerXX() helpers. This can be achieved with QOM. Introduce
> a method for this in the base chip class and implement it in child classes.

OK. We might need to introduce a PnvXscom model one day but this is fine
for now.

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv.c         |   31 ++++++++++++++++++++++++-------
>  include/hw/ppc/pnv.h |    1 +
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 2a53e99bda2e..88efa755e611 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -616,6 +616,24 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon)
>      pnv_psi_pic_print_info(&chip9->psi, mon);
>  }
>  
> +static uint64_t pnv_chip_power8_xscom_core_base(PnvChip *chip,
> +                                                uint32_t core_id)
> +{
> +    return PNV_XSCOM_EX_BASE(core_id);
> +}
> +
> +static uint64_t pnv_chip_power9_xscom_core_base(PnvChip *chip,
> +                                                uint32_t core_id)
> +{
> +    return PNV9_XSCOM_EC_BASE(core_id);
> +}
> +
> +static uint64_t pnv_chip_power10_xscom_core_base(PnvChip *chip,
> +                                                 uint32_t core_id)
> +{
> +    return PNV10_XSCOM_EC_BASE(core_id);
> +}
> +
>  static bool pnv_match_cpu(const char *default_type, const char *cpu_type)
>  {
>      PowerPCCPUClass *ppc_default =
> @@ -1107,6 +1125,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> +    k->xscom_core_base = pnv_chip_power8_xscom_core_base;
>      dc->desc = "PowerNV Chip POWER8E";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> @@ -1129,6 +1148,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> +    k->xscom_core_base = pnv_chip_power8_xscom_core_base;
>      dc->desc = "PowerNV Chip POWER8";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> @@ -1151,6 +1171,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->isa_create = pnv_chip_power8nvl_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> +    k->xscom_core_base = pnv_chip_power8_xscom_core_base;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> @@ -1323,6 +1344,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->isa_create = pnv_chip_power9_isa_create;
>      k->dt_populate = pnv_chip_power9_dt_populate;
>      k->pic_print_info = pnv_chip_power9_pic_print_info;
> +    k->xscom_core_base = pnv_chip_power9_xscom_core_base;
>      dc->desc = "PowerNV Chip POWER9";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power9_realize,
> @@ -1404,6 +1426,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
>      k->isa_create = pnv_chip_power10_isa_create;
>      k->dt_populate = pnv_chip_power10_dt_populate;
>      k->pic_print_info = pnv_chip_power10_pic_print_info;
> +    k->xscom_core_base = pnv_chip_power10_xscom_core_base;
>      dc->desc = "PowerNV Chip POWER10";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power10_realize,
> @@ -1491,13 +1514,7 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>                                   &error_fatal);
>  
>          /* Each core has an XSCOM MMIO region */
> -        if (pnv_chip_is_power10(chip)) {
> -            xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid);
> -        } else if (pnv_chip_is_power9(chip)) {
> -            xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid);
> -        } else {
> -            xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid);
> -        }
> +        xscom_core_base = pcc->xscom_core_base(chip, core_hwid);
>  
>          pnv_xscom_add_subregion(chip, xscom_core_base,
>                                  &pnv_core->xscom_regs);
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 7d2402784d4b..17ca9a14ac8f 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -137,6 +137,7 @@ typedef struct PnvChipClass {
>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>      void (*dt_populate)(PnvChip *chip, void *fdt);
>      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> +    uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> 

[PATCH 09/13] ppc/pnv: Pass XSCOM base address and address size to pnv_dt_xscom()

[Greg Kurz]

Since pnv_dt_xscom() is called from chip specific dt_populate() hooks,
it shouldn't have to guess the chip type in order to populate the "reg"
property. Just pass the base address and address size as arguments.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c               |   12 +++++++++---
 hw/ppc/pnv_xscom.c         |   16 +++-------------
 include/hw/ppc/pnv_xscom.h |    3 ++-
 3 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 88efa755e611..c532e98e752a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -282,7 +282,9 @@ static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
 {
     int i;
 
-    pnv_dt_xscom(chip, fdt, 0);
+    pnv_dt_xscom(chip, fdt, 0,
+                 cpu_to_be64(PNV_XSCOM_BASE(chip)),
+                 cpu_to_be64(PNV_XSCOM_SIZE));
 
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = chip->cores[i];
@@ -302,7 +304,9 @@ static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt)
 {
     int i;
 
-    pnv_dt_xscom(chip, fdt, 0);
+    pnv_dt_xscom(chip, fdt, 0,
+                 cpu_to_be64(PNV9_XSCOM_BASE(chip)),
+                 cpu_to_be64(PNV9_XSCOM_SIZE));
 
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = chip->cores[i];
@@ -321,7 +325,9 @@ static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt)
 {
     int i;
 
-    pnv_dt_xscom(chip, fdt, 0);
+    pnv_dt_xscom(chip, fdt, 0,
+                 cpu_to_be64(PNV10_XSCOM_BASE(chip)),
+                 cpu_to_be64(PNV10_XSCOM_SIZE));
 
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = chip->cores[i];
diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index df926003f2ba..8189767eb0bb 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -286,24 +286,14 @@ static const char compat_p8[] = "ibm,power8-xscom\0ibm,xscom";
 static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
 static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom";
 
-int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
+int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
+                 uint64_t xscom_base, uint64_t xscom_size)
 {
-    uint64_t reg[2];
+    uint64_t reg[] = { xscom_base, xscom_size };
     int xscom_offset;
     ForeachPopulateArgs args;
     char *name;
 
-    if (pnv_chip_is_power10(chip)) {
-        reg[0] = cpu_to_be64(PNV10_XSCOM_BASE(chip));
-        reg[1] = cpu_to_be64(PNV10_XSCOM_SIZE);
-    } else if (pnv_chip_is_power9(chip)) {
-        reg[0] = cpu_to_be64(PNV9_XSCOM_BASE(chip));
-        reg[1] = cpu_to_be64(PNV9_XSCOM_SIZE);
-    } else {
-        reg[0] = cpu_to_be64(PNV_XSCOM_BASE(chip));
-        reg[1] = cpu_to_be64(PNV_XSCOM_SIZE);
-    }
-
     name = g_strdup_printf("xscom@%" PRIx64, be64_to_cpu(reg[0]));
     xscom_offset = fdt_add_subnode(fdt, root_offset, name);
     _FDT(xscom_offset);
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index 2bdb7ae84fd3..ad53f788b44c 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -114,7 +114,8 @@ typedef struct PnvXScomInterfaceClass {
 #define PNV10_XSCOM_PSIHB_SIZE     0x100
 
 void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp);
-int pnv_dt_xscom(PnvChip *chip, void *fdt, int offset);
+int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
+                 uint64_t xscom_base, uint64_t xscom_size);
 
 void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset,
                              MemoryRegion *mr);

Re: [PATCH 09/13] ppc/pnv: Pass XSCOM base address and address size to pnv_dt_xscom()

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> Since pnv_dt_xscom() is called from chip specific dt_populate() hooks,
> it shouldn't have to guess the chip type in order to populate the "reg"
> property. Just pass the base address and address size as arguments.

Much better,
 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv.c               |   12 +++++++++---
>  hw/ppc/pnv_xscom.c         |   16 +++-------------
>  include/hw/ppc/pnv_xscom.h |    3 ++-
>  3 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 88efa755e611..c532e98e752a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -282,7 +282,9 @@ static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
>  {
>      int i;
>  
> -    pnv_dt_xscom(chip, fdt, 0);
> +    pnv_dt_xscom(chip, fdt, 0,
> +                 cpu_to_be64(PNV_XSCOM_BASE(chip)),
> +                 cpu_to_be64(PNV_XSCOM_SIZE));
>  
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = chip->cores[i];
> @@ -302,7 +304,9 @@ static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt)
>  {
>      int i;
>  
> -    pnv_dt_xscom(chip, fdt, 0);
> +    pnv_dt_xscom(chip, fdt, 0,
> +                 cpu_to_be64(PNV9_XSCOM_BASE(chip)),
> +                 cpu_to_be64(PNV9_XSCOM_SIZE));
>  
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = chip->cores[i];
> @@ -321,7 +325,9 @@ static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt)
>  {
>      int i;
>  
> -    pnv_dt_xscom(chip, fdt, 0);
> +    pnv_dt_xscom(chip, fdt, 0,
> +                 cpu_to_be64(PNV10_XSCOM_BASE(chip)),
> +                 cpu_to_be64(PNV10_XSCOM_SIZE));
>  
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = chip->cores[i];
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index df926003f2ba..8189767eb0bb 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -286,24 +286,14 @@ static const char compat_p8[] = "ibm,power8-xscom\0ibm,xscom";
>  static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
>  static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom";
>  
> -int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
> +int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
> +                 uint64_t xscom_base, uint64_t xscom_size)
>  {
> -    uint64_t reg[2];
> +    uint64_t reg[] = { xscom_base, xscom_size };
>      int xscom_offset;
>      ForeachPopulateArgs args;
>      char *name;
>  
> -    if (pnv_chip_is_power10(chip)) {
> -        reg[0] = cpu_to_be64(PNV10_XSCOM_BASE(chip));
> -        reg[1] = cpu_to_be64(PNV10_XSCOM_SIZE);
> -    } else if (pnv_chip_is_power9(chip)) {
> -        reg[0] = cpu_to_be64(PNV9_XSCOM_BASE(chip));
> -        reg[1] = cpu_to_be64(PNV9_XSCOM_SIZE);
> -    } else {
> -        reg[0] = cpu_to_be64(PNV_XSCOM_BASE(chip));
> -        reg[1] = cpu_to_be64(PNV_XSCOM_SIZE);
> -    }
> -
>      name = g_strdup_printf("xscom@%" PRIx64, be64_to_cpu(reg[0]));
>      xscom_offset = fdt_add_subnode(fdt, root_offset, name);
>      _FDT(xscom_offset);
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> index 2bdb7ae84fd3..ad53f788b44c 100644
> --- a/include/hw/ppc/pnv_xscom.h
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -114,7 +114,8 @@ typedef struct PnvXScomInterfaceClass {
>  #define PNV10_XSCOM_PSIHB_SIZE     0x100
>  
>  void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp);
> -int pnv_dt_xscom(PnvChip *chip, void *fdt, int offset);
> +int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
> +                 uint64_t xscom_base, uint64_t xscom_size);
>  
>  void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset,
>                               MemoryRegion *mr);
> 

[PATCH 10/13] ppc/pnv: Pass content of the "compatible" property to pnv_dt_xscom()

[Greg Kurz]

Since pnv_dt_xscom() is called from chip specific dt_populate() hooks,
it shouldn't have to guess the chip type in order to populate the
"compatible" property. Just pass the compat string and its size as
arguments.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c               |   12 +++++++++---
 hw/ppc/pnv_xscom.c         |   20 +++-----------------
 include/hw/ppc/pnv_xscom.h |    3 ++-
 3 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index c532e98e752a..0447b534b8c5 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -280,11 +280,13 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
 
 static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
 {
+    static const char compat[] = "ibm,power8-xscom\0ibm,xscom";
     int i;
 
     pnv_dt_xscom(chip, fdt, 0,
                  cpu_to_be64(PNV_XSCOM_BASE(chip)),
-                 cpu_to_be64(PNV_XSCOM_SIZE));
+                 cpu_to_be64(PNV_XSCOM_SIZE),
+                 compat, sizeof(compat));
 
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = chip->cores[i];
@@ -302,11 +304,13 @@ static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
 
 static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt)
 {
+    static const char compat[] = "ibm,power9-xscom\0ibm,xscom";
     int i;
 
     pnv_dt_xscom(chip, fdt, 0,
                  cpu_to_be64(PNV9_XSCOM_BASE(chip)),
-                 cpu_to_be64(PNV9_XSCOM_SIZE));
+                 cpu_to_be64(PNV9_XSCOM_SIZE),
+                 compat, sizeof(compat));
 
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = chip->cores[i];
@@ -323,11 +327,13 @@ static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt)
 
 static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt)
 {
+    static const char compat[] = "ibm,power10-xscom\0ibm,xscom";
     int i;
 
     pnv_dt_xscom(chip, fdt, 0,
                  cpu_to_be64(PNV10_XSCOM_BASE(chip)),
-                 cpu_to_be64(PNV10_XSCOM_SIZE));
+                 cpu_to_be64(PNV10_XSCOM_SIZE),
+                 compat, sizeof(compat));
 
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = chip->cores[i];
diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index 8189767eb0bb..5ae9dfbb88ad 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -282,12 +282,9 @@ static int xscom_dt_child(Object *child, void *opaque)
     return 0;
 }
 
-static const char compat_p8[] = "ibm,power8-xscom\0ibm,xscom";
-static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
-static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom";
-
 int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
-                 uint64_t xscom_base, uint64_t xscom_size)
+                 uint64_t xscom_base, uint64_t xscom_size,
+                 const char *compat, int compat_size)
 {
     uint64_t reg[] = { xscom_base, xscom_size };
     int xscom_offset;
@@ -302,18 +299,7 @@ int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
     _FDT((fdt_setprop_cell(fdt, xscom_offset, "#address-cells", 1)));
     _FDT((fdt_setprop_cell(fdt, xscom_offset, "#size-cells", 1)));
     _FDT((fdt_setprop(fdt, xscom_offset, "reg", reg, sizeof(reg))));
-
-    if (pnv_chip_is_power10(chip)) {
-        _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p10,
-                          sizeof(compat_p10))));
-    } else if (pnv_chip_is_power9(chip)) {
-        _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p9,
-                          sizeof(compat_p9))));
-    } else {
-        _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p8,
-                          sizeof(compat_p8))));
-    }
-
+    _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat, compat_size)));
     _FDT((fdt_setprop(fdt, xscom_offset, "scom-controller", NULL, 0)));
 
     args.fdt = fdt;
diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
index ad53f788b44c..f74c81a980f3 100644
--- a/include/hw/ppc/pnv_xscom.h
+++ b/include/hw/ppc/pnv_xscom.h
@@ -115,7 +115,8 @@ typedef struct PnvXScomInterfaceClass {
 
 void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp);
 int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
-                 uint64_t xscom_base, uint64_t xscom_size);
+                 uint64_t xscom_base, uint64_t xscom_size,
+                 const char *compat, int compat_size);
 
 void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset,
                              MemoryRegion *mr);

Re: [PATCH 10/13] ppc/pnv: Pass content of the "compatible" property to pnv_dt_xscom()

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> Since pnv_dt_xscom() is called from chip specific dt_populate() hooks,
> it shouldn't have to guess the chip type in order to populate the
> "compatible" property. Just pass the compat string and its size as
> arguments.

Yeah. That is where I think a PnXscom model and class could be a little
cleaner. This is minor.

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv.c               |   12 +++++++++---
>  hw/ppc/pnv_xscom.c         |   20 +++-----------------
>  include/hw/ppc/pnv_xscom.h |    3 ++-
>  3 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index c532e98e752a..0447b534b8c5 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -280,11 +280,13 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>  
>  static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
>  {
> +    static const char compat[] = "ibm,power8-xscom\0ibm,xscom";
>      int i;
>  
>      pnv_dt_xscom(chip, fdt, 0,
>                   cpu_to_be64(PNV_XSCOM_BASE(chip)),
> -                 cpu_to_be64(PNV_XSCOM_SIZE));
> +                 cpu_to_be64(PNV_XSCOM_SIZE),
> +                 compat, sizeof(compat));
>  
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = chip->cores[i];
> @@ -302,11 +304,13 @@ static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt)
>  
>  static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt)
>  {
> +    static const char compat[] = "ibm,power9-xscom\0ibm,xscom";
>      int i;
>  
>      pnv_dt_xscom(chip, fdt, 0,
>                   cpu_to_be64(PNV9_XSCOM_BASE(chip)),
> -                 cpu_to_be64(PNV9_XSCOM_SIZE));
> +                 cpu_to_be64(PNV9_XSCOM_SIZE),
> +                 compat, sizeof(compat));
>  
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = chip->cores[i];
> @@ -323,11 +327,13 @@ static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt)
>  
>  static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt)
>  {
> +    static const char compat[] = "ibm,power10-xscom\0ibm,xscom";
>      int i;
>  
>      pnv_dt_xscom(chip, fdt, 0,
>                   cpu_to_be64(PNV10_XSCOM_BASE(chip)),
> -                 cpu_to_be64(PNV10_XSCOM_SIZE));
> +                 cpu_to_be64(PNV10_XSCOM_SIZE),
> +                 compat, sizeof(compat));
>  
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = chip->cores[i];
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index 8189767eb0bb..5ae9dfbb88ad 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -282,12 +282,9 @@ static int xscom_dt_child(Object *child, void *opaque)
>      return 0;
>  }
>  
> -static const char compat_p8[] = "ibm,power8-xscom\0ibm,xscom";
> -static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
> -static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom";
> -
>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
> -                 uint64_t xscom_base, uint64_t xscom_size)
> +                 uint64_t xscom_base, uint64_t xscom_size,
> +                 const char *compat, int compat_size)
>  {
>      uint64_t reg[] = { xscom_base, xscom_size };
>      int xscom_offset;
> @@ -302,18 +299,7 @@ int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
>      _FDT((fdt_setprop_cell(fdt, xscom_offset, "#address-cells", 1)));
>      _FDT((fdt_setprop_cell(fdt, xscom_offset, "#size-cells", 1)));
>      _FDT((fdt_setprop(fdt, xscom_offset, "reg", reg, sizeof(reg))));
> -
> -    if (pnv_chip_is_power10(chip)) {
> -        _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p10,
> -                          sizeof(compat_p10))));
> -    } else if (pnv_chip_is_power9(chip)) {
> -        _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p9,
> -                          sizeof(compat_p9))));
> -    } else {
> -        _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p8,
> -                          sizeof(compat_p8))));
> -    }
> -
> +    _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat, compat_size)));
>      _FDT((fdt_setprop(fdt, xscom_offset, "scom-controller", NULL, 0)));
>  
>      args.fdt = fdt;
> diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h
> index ad53f788b44c..f74c81a980f3 100644
> --- a/include/hw/ppc/pnv_xscom.h
> +++ b/include/hw/ppc/pnv_xscom.h
> @@ -115,7 +115,8 @@ typedef struct PnvXScomInterfaceClass {
>  
>  void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp);
>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset,
> -                 uint64_t xscom_base, uint64_t xscom_size);
> +                 uint64_t xscom_base, uint64_t xscom_size,
> +                 const char *compat, int compat_size);
>  
>  void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset,
>                               MemoryRegion *mr);
> 

[PATCH 11/13] ppc/pnv: Drop pnv_chip_is_power9() and pnv_chip_is_power10() helpers

[Greg Kurz]

They aren't used anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/pnv.h |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 17ca9a14ac8f..7a134a15d3b5 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -224,21 +224,11 @@ struct PnvMachineState {
     PnvPnor      *pnor;
 };
 
-static inline bool pnv_chip_is_power9(const PnvChip *chip)
-{
-    return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER9;
-}
-
 PnvChip *pnv_get_chip(uint32_t chip_id);
 
 #define PNV_FDT_ADDR          0x01000000
 #define PNV_TIMEBASE_FREQ     512000000ULL
 
-static inline bool pnv_chip_is_power10(const PnvChip *chip)
-{
-    return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER10;
-}
-
 /*
  * BMC helpers
  */

Re: [PATCH 11/13] ppc/pnv: Drop pnv_chip_is_power9() and pnv_chip_is_power10() helpers

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> They aren't used anymore.

Good !

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  include/hw/ppc/pnv.h |   10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 17ca9a14ac8f..7a134a15d3b5 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -224,21 +224,11 @@ struct PnvMachineState {
>      PnvPnor      *pnor;
>  };
>  
> -static inline bool pnv_chip_is_power9(const PnvChip *chip)
> -{
> -    return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER9;
> -}
> -
>  PnvChip *pnv_get_chip(uint32_t chip_id);
>  
>  #define PNV_FDT_ADDR          0x01000000
>  #define PNV_TIMEBASE_FREQ     512000000ULL
>  
> -static inline bool pnv_chip_is_power10(const PnvChip *chip)
> -{
> -    return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER10;
> -}
> -
>  /*
>   * BMC helpers
>   */
> 

[PATCH 12/13] ppc/pnv: Introduce PnvChipClass::xscom_pcba() method

[Greg Kurz]

The XSCOM bus is implemented with a QOM interface, which is mostly
generic from a CPU type standpoint, except for the computation of
addresses on the Pervasize Connect Bus (PCB) network. This is handled
by the pnv_xscom_pcba() function with a switch statement based on
the chip_type class level attribute of the CPU chip.

This can be achieved using QOM. Also the address argument is masked with
PNV_XSCOM_SIZE - 1, which is for POWER8 only. Addresses may have different
sizes with other CPU types. Have each CPU chip type handle the appropriate
computation with a QOM xscom_pcba() method.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c         |   23 +++++++++++++++++++++++
 hw/ppc/pnv_xscom.c   |   14 +-------------
 include/hw/ppc/pnv.h |    1 +
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0447b534b8c5..cc40b90e9cd2 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1121,6 +1121,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
                                 &chip8->homer.regs);
 }
 
+static uint32_t pnv_chip_power8_xscom_pcba(PnvChip *chip, uint64_t addr)
+{
+    addr &= (PNV_XSCOM_SIZE - 1);
+    return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
+}
+
 static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1138,6 +1144,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
     k->xscom_core_base = pnv_chip_power8_xscom_core_base;
+    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
     dc->desc = "PowerNV Chip POWER8E";
 
     device_class_set_parent_realize(dc, pnv_chip_power8_realize,
@@ -1161,6 +1168,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
     k->xscom_core_base = pnv_chip_power8_xscom_core_base;
+    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
     dc->desc = "PowerNV Chip POWER8";
 
     device_class_set_parent_realize(dc, pnv_chip_power8_realize,
@@ -1184,6 +1192,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
     k->xscom_core_base = pnv_chip_power8_xscom_core_base;
+    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
     dc->desc = "PowerNV Chip POWER8NVL";
 
     device_class_set_parent_realize(dc, pnv_chip_power8_realize,
@@ -1340,6 +1349,12 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
                                 &chip9->homer.regs);
 }
 
+static uint32_t pnv_chip_power9_xscom_pcba(PnvChip *chip, uint64_t addr)
+{
+    addr &= (PNV9_XSCOM_SIZE - 1);
+    return addr >> 3;
+}
+
 static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1357,6 +1372,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->dt_populate = pnv_chip_power9_dt_populate;
     k->pic_print_info = pnv_chip_power9_pic_print_info;
     k->xscom_core_base = pnv_chip_power9_xscom_core_base;
+    k->xscom_pcba = pnv_chip_power9_xscom_pcba;
     dc->desc = "PowerNV Chip POWER9";
 
     device_class_set_parent_realize(dc, pnv_chip_power9_realize,
@@ -1422,6 +1438,12 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
                                             (uint64_t) PNV10_LPCM_BASE(chip));
 }
 
+static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
+{
+    addr &= (PNV10_XSCOM_SIZE - 1);
+    return addr >> 3;
+}
+
 static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1439,6 +1461,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
     k->dt_populate = pnv_chip_power10_dt_populate;
     k->pic_print_info = pnv_chip_power10_pic_print_info;
     k->xscom_core_base = pnv_chip_power10_xscom_core_base;
+    k->xscom_pcba = pnv_chip_power10_xscom_pcba;
     dc->desc = "PowerNV Chip POWER10";
 
     device_class_set_parent_realize(dc, pnv_chip_power10_realize,
diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index 5ae9dfbb88ad..b681c72575b2 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -57,19 +57,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
 
 static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
 {
-    addr &= (PNV_XSCOM_SIZE - 1);
-
-    switch (PNV_CHIP_GET_CLASS(chip)->chip_type) {
-    case PNV_CHIP_POWER8E:
-    case PNV_CHIP_POWER8:
-    case PNV_CHIP_POWER8NVL:
-        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
-    case PNV_CHIP_POWER9:
-    case PNV_CHIP_POWER10:
-        return addr >> 3;
-    default:
-        g_assert_not_reached();
-    }
+    return PNV_CHIP_GET_CLASS(chip)->xscom_pcba(chip, addr);
 }
 
 static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 7a134a15d3b5..4972e93c2619 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -138,6 +138,7 @@ typedef struct PnvChipClass {
     void (*dt_populate)(PnvChip *chip, void *fdt);
     void (*pic_print_info)(PnvChip *chip, Monitor *mon);
     uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id);
+    uint32_t (*xscom_pcba)(PnvChip *chip, uint64_t addr);
 } PnvChipClass;
 
 #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP

Re: [PATCH 12/13] ppc/pnv: Introduce PnvChipClass::xscom_pcba() method

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> The XSCOM bus is implemented with a QOM interface, which is mostly
> generic from a CPU type standpoint, except for the computation of
> addresses on the Pervasize Connect Bus (PCB) network. This is handled

Pervasive

> by the pnv_xscom_pcba() function with a switch statement based on
> the chip_type class level attribute of the CPU chip.
> 
> This can be achieved using QOM. Also the address argument is masked with
> PNV_XSCOM_SIZE - 1, which is for POWER8 only. Addresses may have different
> sizes with other CPU types. Have each CPU chip type handle the appropriate
> computation with a QOM xscom_pcba() method.

PnvXscom model ? :)

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/ppc/pnv.c         |   23 +++++++++++++++++++++++
>  hw/ppc/pnv_xscom.c   |   14 +-------------
>  include/hw/ppc/pnv.h |    1 +
>  3 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0447b534b8c5..cc40b90e9cd2 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1121,6 +1121,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
>                                  &chip8->homer.regs);
>  }
>  
> +static uint32_t pnv_chip_power8_xscom_pcba(PnvChip *chip, uint64_t addr)
> +{
> +    addr &= (PNV_XSCOM_SIZE - 1);
> +    return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> +}
> +
>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1138,6 +1144,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
>      k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> +    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
>      dc->desc = "PowerNV Chip POWER8E";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> @@ -1161,6 +1168,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
>      k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> +    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
>      dc->desc = "PowerNV Chip POWER8";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> @@ -1184,6 +1192,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
>      k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> +    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> @@ -1340,6 +1349,12 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
>                                  &chip9->homer.regs);
>  }
>  
> +static uint32_t pnv_chip_power9_xscom_pcba(PnvChip *chip, uint64_t addr)
> +{
> +    addr &= (PNV9_XSCOM_SIZE - 1);
> +    return addr >> 3;
> +}
> +
>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1357,6 +1372,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->dt_populate = pnv_chip_power9_dt_populate;
>      k->pic_print_info = pnv_chip_power9_pic_print_info;
>      k->xscom_core_base = pnv_chip_power9_xscom_core_base;
> +    k->xscom_pcba = pnv_chip_power9_xscom_pcba;
>      dc->desc = "PowerNV Chip POWER9";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power9_realize,
> @@ -1422,6 +1438,12 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
>                                              (uint64_t) PNV10_LPCM_BASE(chip));
>  }
>  
> +static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
> +{
> +    addr &= (PNV10_XSCOM_SIZE - 1);
> +    return addr >> 3;
> +}
> +
>  static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -1439,6 +1461,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
>      k->dt_populate = pnv_chip_power10_dt_populate;
>      k->pic_print_info = pnv_chip_power10_pic_print_info;
>      k->xscom_core_base = pnv_chip_power10_xscom_core_base;
> +    k->xscom_pcba = pnv_chip_power10_xscom_pcba;
>      dc->desc = "PowerNV Chip POWER10";
>  
>      device_class_set_parent_realize(dc, pnv_chip_power10_realize,
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index 5ae9dfbb88ad..b681c72575b2 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -57,19 +57,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>  
>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>  {
> -    addr &= (PNV_XSCOM_SIZE - 1);
> -
> -    switch (PNV_CHIP_GET_CLASS(chip)->chip_type) {
> -    case PNV_CHIP_POWER8E:
> -    case PNV_CHIP_POWER8:
> -    case PNV_CHIP_POWER8NVL:
> -        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> -    case PNV_CHIP_POWER9:
> -    case PNV_CHIP_POWER10:
> -        return addr >> 3;
> -    default:
> -        g_assert_not_reached();
> -    }
> +    return PNV_CHIP_GET_CLASS(chip)->xscom_pcba(chip, addr);
>  }
>  
>  static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 7a134a15d3b5..4972e93c2619 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -138,6 +138,7 @@ typedef struct PnvChipClass {
>      void (*dt_populate)(PnvChip *chip, void *fdt);
>      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
>      uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id);
> +    uint32_t (*xscom_pcba)(PnvChip *chip, uint64_t addr);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> 

Re: [PATCH 12/13] ppc/pnv: Introduce PnvChipClass::xscom_pcba() method

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6772 bytes --]

On Fri, Dec 13, 2019 at 02:06:31PM +0100, Cédric Le Goater wrote:
> On 13/12/2019 13:00, Greg Kurz wrote:
> > The XSCOM bus is implemented with a QOM interface, which is mostly
> > generic from a CPU type standpoint, except for the computation of
> > addresses on the Pervasize Connect Bus (PCB) network. This is handled
> 
> Pervasive

I've fixed this typo in transit.

> 
> > by the pnv_xscom_pcba() function with a switch statement based on
> > the chip_type class level attribute of the CPU chip.
> > 
> > This can be achieved using QOM. Also the address argument is masked with
> > PNV_XSCOM_SIZE - 1, which is for POWER8 only. Addresses may have different
> > sizes with other CPU types. Have each CPU chip type handle the appropriate
> > computation with a QOM xscom_pcba() method.
> 
> PnvXscom model ? :)
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> > ---
> >  hw/ppc/pnv.c         |   23 +++++++++++++++++++++++
> >  hw/ppc/pnv_xscom.c   |   14 +-------------
> >  include/hw/ppc/pnv.h |    1 +
> >  3 files changed, 25 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0447b534b8c5..cc40b90e9cd2 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1121,6 +1121,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp)
> >                                  &chip8->homer.regs);
> >  }
> >  
> > +static uint32_t pnv_chip_power8_xscom_pcba(PnvChip *chip, uint64_t addr)
> > +{
> > +    addr &= (PNV_XSCOM_SIZE - 1);
> > +    return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> > +}
> > +
> >  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -1138,6 +1144,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >      k->dt_populate = pnv_chip_power8_dt_populate;
> >      k->pic_print_info = pnv_chip_power8_pic_print_info;
> >      k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> > +    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
> >      dc->desc = "PowerNV Chip POWER8E";
> >  
> >      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> > @@ -1161,6 +1168,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
> >      k->dt_populate = pnv_chip_power8_dt_populate;
> >      k->pic_print_info = pnv_chip_power8_pic_print_info;
> >      k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> > +    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
> >      dc->desc = "PowerNV Chip POWER8";
> >  
> >      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> > @@ -1184,6 +1192,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
> >      k->dt_populate = pnv_chip_power8_dt_populate;
> >      k->pic_print_info = pnv_chip_power8_pic_print_info;
> >      k->xscom_core_base = pnv_chip_power8_xscom_core_base;
> > +    k->xscom_pcba = pnv_chip_power8_xscom_pcba;
> >      dc->desc = "PowerNV Chip POWER8NVL";
> >  
> >      device_class_set_parent_realize(dc, pnv_chip_power8_realize,
> > @@ -1340,6 +1349,12 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp)
> >                                  &chip9->homer.regs);
> >  }
> >  
> > +static uint32_t pnv_chip_power9_xscom_pcba(PnvChip *chip, uint64_t addr)
> > +{
> > +    addr &= (PNV9_XSCOM_SIZE - 1);
> > +    return addr >> 3;
> > +}
> > +
> >  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -1357,6 +1372,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >      k->dt_populate = pnv_chip_power9_dt_populate;
> >      k->pic_print_info = pnv_chip_power9_pic_print_info;
> >      k->xscom_core_base = pnv_chip_power9_xscom_core_base;
> > +    k->xscom_pcba = pnv_chip_power9_xscom_pcba;
> >      dc->desc = "PowerNV Chip POWER9";
> >  
> >      device_class_set_parent_realize(dc, pnv_chip_power9_realize,
> > @@ -1422,6 +1438,12 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp)
> >                                              (uint64_t) PNV10_LPCM_BASE(chip));
> >  }
> >  
> > +static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr)
> > +{
> > +    addr &= (PNV10_XSCOM_SIZE - 1);
> > +    return addr >> 3;
> > +}
> > +
> >  static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -1439,6 +1461,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
> >      k->dt_populate = pnv_chip_power10_dt_populate;
> >      k->pic_print_info = pnv_chip_power10_pic_print_info;
> >      k->xscom_core_base = pnv_chip_power10_xscom_core_base;
> > +    k->xscom_pcba = pnv_chip_power10_xscom_pcba;
> >      dc->desc = "PowerNV Chip POWER10";
> >  
> >      device_class_set_parent_realize(dc, pnv_chip_power10_realize,
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index 5ae9dfbb88ad..b681c72575b2 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -57,19 +57,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
> >  
> >  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
> >  {
> > -    addr &= (PNV_XSCOM_SIZE - 1);
> > -
> > -    switch (PNV_CHIP_GET_CLASS(chip)->chip_type) {
> > -    case PNV_CHIP_POWER8E:
> > -    case PNV_CHIP_POWER8:
> > -    case PNV_CHIP_POWER8NVL:
> > -        return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
> > -    case PNV_CHIP_POWER9:
> > -    case PNV_CHIP_POWER10:
> > -        return addr >> 3;
> > -    default:
> > -        g_assert_not_reached();
> > -    }
> > +    return PNV_CHIP_GET_CLASS(chip)->xscom_pcba(chip, addr);
> >  }
> >  
> >  static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > index 7a134a15d3b5..4972e93c2619 100644
> > --- a/include/hw/ppc/pnv.h
> > +++ b/include/hw/ppc/pnv.h
> > @@ -138,6 +138,7 @@ typedef struct PnvChipClass {
> >      void (*dt_populate)(PnvChip *chip, void *fdt);
> >      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> >      uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id);
> > +    uint32_t (*xscom_pcba)(PnvChip *chip, uint64_t addr);
> >  } PnvChipClass;
> >  
> >  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 13/13] ppc/pnv: Drop PnvChipClass::type

[Greg Kurz]

It isn't used anymore.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv.c         |    5 -----
 include/hw/ppc/pnv.h |    9 ---------
 2 files changed, 14 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index cc40b90e9cd2..232b4a25603c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1132,7 +1132,6 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvChipClass *k = PNV_CHIP_CLASS(klass);
 
-    k->chip_type = PNV_CHIP_POWER8E;
     k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
     k->cores_mask = POWER8E_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
@@ -1156,7 +1155,6 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvChipClass *k = PNV_CHIP_CLASS(klass);
 
-    k->chip_type = PNV_CHIP_POWER8;
     k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
@@ -1180,7 +1178,6 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvChipClass *k = PNV_CHIP_CLASS(klass);
 
-    k->chip_type = PNV_CHIP_POWER8NVL;
     k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
@@ -1360,7 +1357,6 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvChipClass *k = PNV_CHIP_CLASS(klass);
 
-    k->chip_type = PNV_CHIP_POWER9;
     k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
     k->cores_mask = POWER9_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p9;
@@ -1449,7 +1445,6 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PnvChipClass *k = PNV_CHIP_CLASS(klass);
 
-    k->chip_type = PNV_CHIP_POWER10;
     k->chip_cfam_id = 0x120da04900008000ull; /* P10 DD1.0 (with NX) */
     k->cores_mask = POWER10_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p10;
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 4972e93c2619..f78fd0dd967c 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -38,14 +38,6 @@
 #define PNV_CHIP_GET_CLASS(obj) \
      OBJECT_GET_CLASS(PnvChipClass, (obj), TYPE_PNV_CHIP)
 
-typedef enum PnvChipType {
-    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
-    PNV_CHIP_POWER8,      /* AKA Venice */
-    PNV_CHIP_POWER8NVL,   /* AKA Naples */
-    PNV_CHIP_POWER9,      /* AKA Nimbus */
-    PNV_CHIP_POWER10,     /* AKA TBD */
-} PnvChipType;
-
 typedef struct PnvChip {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -123,7 +115,6 @@ typedef struct PnvChipClass {
     SysBusDeviceClass parent_class;
 
     /*< public >*/
-    PnvChipType  chip_type;
     uint64_t     chip_cfam_id;
     uint64_t     cores_mask;
 

Re: [PATCH 13/13] ppc/pnv: Drop PnvChipClass::type

[Cédric Le Goater]

On 13/12/2019 13:00, Greg Kurz wrote:
> It isn't used anymore.

Fantastic ! 

> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks, 

C.

> ---
>  hw/ppc/pnv.c         |    5 -----
>  include/hw/ppc/pnv.h |    9 ---------
>  2 files changed, 14 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index cc40b90e9cd2..232b4a25603c 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1132,7 +1132,6 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvChipClass *k = PNV_CHIP_CLASS(klass);
>  
> -    k->chip_type = PNV_CHIP_POWER8E;
>      k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>      k->cores_mask = POWER8E_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> @@ -1156,7 +1155,6 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvChipClass *k = PNV_CHIP_CLASS(klass);
>  
> -    k->chip_type = PNV_CHIP_POWER8;
>      k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> @@ -1180,7 +1178,6 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvChipClass *k = PNV_CHIP_CLASS(klass);
>  
> -    k->chip_type = PNV_CHIP_POWER8NVL;
>      k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> @@ -1360,7 +1357,6 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvChipClass *k = PNV_CHIP_CLASS(klass);
>  
> -    k->chip_type = PNV_CHIP_POWER9;
>      k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
>      k->cores_mask = POWER9_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p9;
> @@ -1449,7 +1445,6 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PnvChipClass *k = PNV_CHIP_CLASS(klass);
>  
> -    k->chip_type = PNV_CHIP_POWER10;
>      k->chip_cfam_id = 0x120da04900008000ull; /* P10 DD1.0 (with NX) */
>      k->cores_mask = POWER10_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p10;
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 4972e93c2619..f78fd0dd967c 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -38,14 +38,6 @@
>  #define PNV_CHIP_GET_CLASS(obj) \
>       OBJECT_GET_CLASS(PnvChipClass, (obj), TYPE_PNV_CHIP)
>  
> -typedef enum PnvChipType {
> -    PNV_CHIP_POWER8E,     /* AKA Murano (default) */
> -    PNV_CHIP_POWER8,      /* AKA Venice */
> -    PNV_CHIP_POWER8NVL,   /* AKA Naples */
> -    PNV_CHIP_POWER9,      /* AKA Nimbus */
> -    PNV_CHIP_POWER10,     /* AKA TBD */
> -} PnvChipType;
> -
>  typedef struct PnvChip {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -123,7 +115,6 @@ typedef struct PnvChipClass {
>      SysBusDeviceClass parent_class;
>  
>      /*< public >*/
> -    PnvChipType  chip_type;
>      uint64_t     chip_cfam_id;
>      uint64_t     cores_mask;
>  
> 

Re: [PATCH 00/13] ppc/pnv: Get rid of chip_type attributes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

On Fri, Dec 13, 2019 at 12:59:28PM +0100, Greg Kurz wrote:
> The PnvChipClass type has a chip_type attribute which identifies various
> POWER CPU chip types that can be used in a powernv machine.
> 
> typedef enum PnvChipType {
>     PNV_CHIP_POWER8E,     /* AKA Murano (default) */
>     PNV_CHIP_POWER8,      /* AKA Venice */
>     PNV_CHIP_POWER8NVL,   /* AKA Naples */
>     PNV_CHIP_POWER9,      /* AKA Nimbus */
>     PNV_CHIP_POWER10,     /* AKA TBD */
> } PnvChipType;
> 
> This attribute is used in many places where we want a different behaviour
> depending on the CPU type, either directly like:
> 
>     switch (PNV_CHIP_GET_CLASS(chip)->chip_type) {
>     case PNV_CHIP_POWER8E:
>     case PNV_CHIP_POWER8:
>     case PNV_CHIP_POWER8NVL:
>         return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf);
>     case PNV_CHIP_POWER9:
>     case PNV_CHIP_POWER10:
>         return addr >> 3;
>     default:
>         g_assert_not_reached();
>     }
> 
> or through various helpers that rely on it:
> 
>         /* Each core has an XSCOM MMIO region */
>         if (pnv_chip_is_power10(chip)) {
>             xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid);
>         } else if (pnv_chip_is_power9(chip)) {
>             xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid);
>         } else {
>             xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid);
>         }
> 
> The chip_type is also duplicated in the PnvPsiClass type.
> 
> It looks a bit unfortunate to implement manually something that falls into
> the scope of QOM. Especially since we don't seem to need a finer grain than
> the CPU familly, ie. POWER8, POWER9, POWER10, ..., and we already have
> specialized versions of PnvChipClass and PnvPsiClass for these.
> 
> This series basically QOM-ifies all the places where we check on the chip
> type, and gets rid of the chip_type attributes and the is_powerXX() helpers.
> 
> Patch 1 was recently posted to the list but it isn't available in David's
> ppc-for-5.0 tree yet, so I include it in this series for convenience.

Applied to ppc-for-5.0.  I had been anticipating just replacing a
bunch of tests on ->chip_type with object_dynamic_cast() checks, but
this is better still.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]