Re: [PATCH v3 08/13] hw/arm/raspi: Make board_rev a field of RaspiMachineClass

[Igor Mammedov]

On Sat,  8 Feb 2020 17:56:40 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

> We want to have a common class_init(). The only value that
> matters (and changes) is the board revision.
> Pass the board_rev as class_data to class_init().
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/arm/raspi.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 62b8df3c2e..fbfcd29732 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -46,6 +46,7 @@ typedef struct RaspiMachineClass {
>      /*< private >*/
>      MachineClass parent_obj;
>      /*< public >*/
> +    uint32_t board_rev;
>  } RaspiMachineClass;
>  
>  #define TYPE_RASPI_MACHINE       MACHINE_TYPE_NAME("raspi-common")
> @@ -227,9 +228,11 @@ static void setup_boot(MachineState *machine, int 
> version, size_t ram_size)
>      arm_load_kernel(ARM_CPU(first_cpu), machine, &binfo);
>  }
>  
> -static void raspi_init(MachineState *machine, uint32_t board_rev)
> +static void raspi_init(MachineState *machine)
>  {
> +    RaspiMachineClass *mc = RASPI_MACHINE_GET_CLASS(machine);
>      RaspiMachineState *s = RASPI_MACHINE(machine);
> +    uint32_t board_rev = mc->board_rev;
>      int version = board_version(board_rev);
>      uint64_t ram_size = board_ram_size(board_rev);
>      uint32_t vcram_size;
> @@ -279,13 +282,16 @@ static void raspi_init(MachineState *machine, uint32_t 
> board_rev)
>  
>  static void raspi2_init(MachineState *machine)
>  {
> -    raspi_init(machine, 0xa21041);
> +    raspi_init(machine);
>  }
>  
>  static void raspi2_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>  
> +    rmc->board_rev = board_rev;

instead of doing a bit obscure ".class_data     = (void *)0xa21041," and
using it here, I'd just do

       rmc->board_rev = 0xa21041;

using value specific for each leaf class

with this change
 Reviewed-by: Igor Mammedov <address@hidden>

>      mc->desc = "Raspberry Pi 2B";
>      mc->init = raspi2_init;
>      mc->block_default_type = IF_SD;
> @@ -302,13 +308,16 @@ static void raspi2_machine_class_init(ObjectClass *oc, 
> void *data)
>  #ifdef TARGET_AARCH64
>  static void raspi3_init(MachineState *machine)
>  {
> -    raspi_init(machine, 0xa02082);
> +    raspi_init(machine);
>  }
>  
>  static void raspi3_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    RaspiMachineClass *rmc = RASPI_MACHINE_CLASS(oc);
> +    uint32_t board_rev = (uint32_t)(uintptr_t)data;
>  
> +    rmc->board_rev = board_rev;
>      mc->desc = "Raspberry Pi 3B";
>      mc->init = raspi3_init;
>      mc->block_default_type = IF_SD;
> @@ -327,11 +336,13 @@ static const TypeInfo raspi_machine_types[] = {
>          .name           = MACHINE_TYPE_NAME("raspi2"),
>          .parent         = TYPE_RASPI_MACHINE,
>          .class_init     = raspi2_machine_class_init,
> +        .class_data     = (void *)0xa21041,
>  #ifdef TARGET_AARCH64
>      }, {
>          .name           = MACHINE_TYPE_NAME("raspi3"),
>          .parent         = TYPE_RASPI_MACHINE,
>          .class_init     = raspi3_machine_class_init,
> +        .class_data     = (void *)0xa02082,
>  #endif
>      }, {
>          .name           = TYPE_RASPI_MACHINE,