Re: [PATCH v2 05/13] hw/arm/raspi: Remove use of the 'version' value in the board code

[Luc Michel]

On 2/17/20 12:45 PM, Philippe Mathieu-Daudé wrote:
> We expected the 'version' ID to match the board processor ID,
> but this is not always true (for example boards with revision
> id 0xa02042/0xa22042 are Raspberry Pi 2 with a BCM2837 SoC).
> This was not important because we were not modelling them, but
> since the recent refactor now allow to model these boards, it
> is safer to check the processor id directly. Remove the version
> check.
> 
> Suggested-by: Peter Maydell <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/arm/raspi.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index b628dadf34..fff501affb 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -98,11 +98,6 @@ static RaspiProcessorId board_processor_id(uint32_t 
> board_rev)
>      return proc_id;
>  }
>  
> -static int board_version(uint32_t board_rev)
> -{
> -    return board_processor_id(board_rev) + 1;
> -}
> -
>  static const char *board_soc_type(uint32_t board_rev)
>  {
>      return soc_property[board_processor_id(board_rev)].type;
> @@ -201,7 +196,8 @@ static void reset_secondary(ARMCPU *cpu, const struct 
> arm_boot_info *info)
>      cpu_set_pc(cs, info->smp_loader_start);
>  }
>  
> -static void setup_boot(MachineState *machine, int version, size_t ram_size)
> +static void setup_boot(MachineState *machine, RaspiProcessorId processor_id,
> +                       size_t ram_size)
>  {
>      static struct arm_boot_info binfo;
>      int r;
> @@ -210,12 +206,13 @@ static void setup_boot(MachineState *machine, int 
> version, size_t ram_size)
>      binfo.ram_size = ram_size;
>      binfo.nb_cpus = machine->smp.cpus;
>  
> -    if (version <= 2) {
> -        /* The rpi1 and 2 require some custom setup code to run in Secure
> -         * mode before booting a kernel (to set up the SMC vectors so
> -         * that we get a no-op SMC; this is used by Linux to call the
> +    if (processor_id <= PROCESSOR_ID_BCM2836) {
> +        /*
> +         * The BCM2835 and BCM2836 require some custom setup code to run
> +         * in Secure mode before booting a kernel (to set up the SMC vectors
> +         * so that we get a no-op SMC; this is used by Linux to call the
>           * firmware for some cache maintenance operations.
> -         * The rpi3 doesn't need this.
> +         * The BCM2837 doesn't need this.
>           */
>          binfo.board_setup_addr = BOARDSETUP_ADDR;
>          binfo.write_board_setup = write_board_setup;
> @@ -223,10 +220,10 @@ static void setup_boot(MachineState *machine, int 
> version, size_t ram_size)
>          binfo.secure_boot = true;
>      }
>  
> -    /* Pi2 and Pi3 requires SMP setup */
> -    if (version >= 2) {
> +    /* BCM2836 and BCM2837 requires SMP setup */
> +    if (processor_id >= PROCESSOR_ID_BCM2836) {
>          binfo.smp_loader_start = SMPBOOT_ADDR;
> -        if (version == 2) {
> +        if (processor_id == PROCESSOR_ID_BCM2836) {
>              binfo.write_secondary_boot = write_smpboot;
>          } else {
>              binfo.write_secondary_boot = write_smpboot64;
> @@ -238,7 +235,13 @@ static void setup_boot(MachineState *machine, int 
> version, size_t ram_size)
>       * the normal Linux boot process
>       */
>      if (machine->firmware) {
> -        hwaddr firmware_addr = version == 3 ? FIRMWARE_ADDR_3 : 
> FIRMWARE_ADDR_2;
> +        hwaddr firmware_addr;
> +
> +        if (processor_id == PROCESSOR_ID_BCM2837) {
> +            firmware_addr = FIRMWARE_ADDR_3;
> +        } else {
> +            firmware_addr = FIRMWARE_ADDR_2;
Maybe rename those constants too, because now that the version is gone,
we can wonder what those 2 and 3 mean. By the way since this firmware
address seems processor ID specific, maybe you can put them in your
soc_property structure?

Anyway with or without those modifications:

Reviewed-by: Luc Michel <address@hidden>

> +        }
>          /* load the firmware image (typically kernel.img) */
>          r = load_image_targphys(machine->firmware, firmware_addr,
>                                  ram_size - firmware_addr);
> @@ -259,7 +262,6 @@ static void raspi_machine_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;
>      DriveInfo *di;
> @@ -303,7 +305,8 @@ static void raspi_machine_init(MachineState *machine)
>  
>      vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
>                                            &error_abort);
> -    setup_boot(machine, version, machine->ram_size - vcram_size);
> +    setup_boot(machine, board_processor_id(mc->board_rev),
> +               machine->ram_size - vcram_size);
>  }
>  
>  static void raspi_machine_class_common_init(MachineClass *mc,
>