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

Greg Kurz posted 13 patches 4 years, 3 months ago
Failed in applying to current master (apply log)
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 00/13] ppc/pnv: Get rid of chip_type attributes
Posted by Greg Kurz 4 years, 3 months ago
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(-)


Re: [PATCH 00/13] ppc/pnv: Get rid of chip_type attributes
Posted by David Gibson 4 years, 3 months ago
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