Re: [PATCH v2 02/10] ppc/pnv: Introduce 'PnvChipClass::chip_type'

[Cédric Le Goater]

On 4/26/24 13:00, Aditya Gupta wrote:
> Introduce 'PnvChipClass::chip_type' to easily get which Power chip is
> it.
> This helps generalise similar codes such as *_dt_populate, and removes
> duplication of code between Power11 and Power10 changes in following
> patches.
>
> Cc: Cédric Le Goater <clg@kaod.org>
> Cc: Frédéric Barrat <fbarrat@linux.ibm.com>
> Cc: Mahesh J Salgaonkar <mahesh@linux.ibm.com>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
> ---
>   hw/ppc/pnv.c              |  5 +++++
>   include/hw/ppc/pnv_chip.h | 10 ++++++++++
>   2 files changed, 15 insertions(+)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 6e3a5ccdec76..74e7908e5ffb 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1457,6 +1457,7 @@ static void pnv_chip_power8e_class_init(ObjectClass 
> *klass, void *data)
>       PnvChipClass *k = PNV_CHIP_CLASS(klass);
>   
>       k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
> +    k->chip_type = PNV_TYPE_POWER8E;
>       k->cores_mask = POWER8E_CORE_MASK;
>       k->num_phbs = 3;
>       k->chip_pir = pnv_chip_pir_p8;
> @@ -1481,6 +1482,7 @@ static void pnv_chip_power8_class_init(ObjectClass 
> *klass, void *data)
>       PnvChipClass *k = PNV_CHIP_CLASS(klass);
>   
>       k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
> +    k->chip_type = PNV_TYPE_POWER8;
>       k->cores_mask = POWER8_CORE_MASK;
>       k->num_phbs = 3;
>       k->chip_pir = pnv_chip_pir_p8;
> @@ -1505,6 +1507,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass 
> *klass, void *data)
>       PnvChipClass *k = PNV_CHIP_CLASS(klass);
>   
>       k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
> +    k->chip_type = PNV_TYPE_POWER8NVL;
>       k->cores_mask = POWER8_CORE_MASK;
>       k->num_phbs = 4;
>       k->chip_pir = pnv_chip_pir_p8;
> @@ -1779,6 +1782,7 @@ static void pnv_chip_power9_class_init(ObjectClass 
> *klass, void *data)
>       static const int i2c_ports_per_engine[PNV9_CHIP_MAX_I2C] = {2, 13, 2, 2};
>   
>       k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
> +    k->chip_type = PNV_TYPE_POWER9;
>       k->cores_mask = POWER9_CORE_MASK;
>       k->chip_pir = pnv_chip_pir_p9;
>       k->intc_create = pnv_chip_power9_intc_create;
> @@ -2091,6 +2095,7 @@ static void pnv_chip_power10_class_init(ObjectClass 
> *klass, void *data)
>       static const int i2c_ports_per_engine[PNV10_CHIP_MAX_I2C] = {14, 14, 2, 
> 16};
>   
>       k->chip_cfam_id = 0x120da04900008000ull; /* P10 DD1.0 (with NX) */
> +    k->chip_type = PNV_TYPE_POWER10;
>       k->cores_mask = POWER10_CORE_MASK;
>       k->chip_pir = pnv_chip_pir_p10;
>       k->intc_create = pnv_chip_power10_intc_create;
> diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
> index 8589f3291ed3..ebfe82b89537 100644
> --- a/include/hw/ppc/pnv_chip.h
> +++ b/include/hw/ppc/pnv_chip.h
> @@ -17,12 +17,21 @@
>   OBJECT_DECLARE_TYPE(PnvChip, PnvChipClass,
>                       PNV_CHIP)
>   
> +typedef enum PnvChipType {
> +    PNV_TYPE_POWER8E,     /* AKA Murano (default) */
> +    PNV_TYPE_POWER8,      /* AKA Venice */
> +    PNV_TYPE_POWER8NVL,   /* AKA Naples */
> +    PNV_TYPE_POWER9,      /* AKA Nimbus */
> +    PNV_TYPE_POWER10,
> +} PnvChipType;

Nope.

> +
>   struct PnvChip {
>       /*< private >*/
>       SysBusDevice parent_obj;
>   
>       /*< public >*/
>       uint32_t     chip_id;
> +
>       uint64_t     ram_start;
>       uint64_t     ram_size;
>   
> @@ -137,6 +146,7 @@ struct PnvChipClass {
>       SysBusDeviceClass parent_class;
>   
>       /*< public >*/
> +    PnvChipType  chip_type;
>       uint64_t     chip_cfam_id;
>       uint64_t     cores_mask;
>       uint32_t     num_pecs;

Adding an enum type under PnvChipClass which is a type already
looks wrong. Please find another way. It is possible I am sure.

Thanks,

C.