Re: [PATCH v2 04/13] hw/arm/raspi: Introduce RaspiProcessorId enum

[Luc Michel]

Hi Phil,

On 2/17/20 12:45 PM, Philippe Mathieu-Daudé wrote:
> As we only support a reduced set of the REV_CODE_PROCESSOR id
> encoded in the board revision, define the PROCESSOR_ID values
> as an enum. We can simplify the board_soc_type and cores_count
> methods.
> 
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/arm/raspi.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index d9e8acfe3b..b628dadf34 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -69,16 +69,33 @@ FIELD(REV_CODE, MANUFACTURER,      16, 4);
>  FIELD(REV_CODE, MEMORY_SIZE,       20, 3);
>  FIELD(REV_CODE, STYLE,             23, 1);
>  
> +typedef enum RaspiProcessorId {
> +    PROCESSOR_ID_BCM2836 = 1,
> +    PROCESSOR_ID_BCM2837 = 2,
> +} RaspiProcessorId;
> +
> +static const struct {
> +    const char *type;
> +    int cores_count;
> +} soc_property[] = {
> +    [PROCESSOR_ID_BCM2836] = {TYPE_BCM2836, BCM283X_NCPUS},
> +    [PROCESSOR_ID_BCM2837] = {TYPE_BCM2837, BCM283X_NCPUS},
> +};
> +
>  static uint64_t board_ram_size(uint32_t board_rev)
>  {
>      assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
>      return 256 * MiB << FIELD_EX32(board_rev, REV_CODE, MEMORY_SIZE);
>  }
>  
> -static int board_processor_id(uint32_t board_rev)
> +static RaspiProcessorId board_processor_id(uint32_t board_rev)
>  {
> +    int proc_id = FIELD_EX32(board_rev, REV_CODE, PROCESSOR);;
You have a superfluous semicolon here.

Apart from that:

Reviewed-by: Luc Michel <address@hidden>

> +
>      assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
> -    return FIELD_EX32(board_rev, REV_CODE, PROCESSOR);
> +    assert(proc_id < ARRAY_SIZE(soc_property) && soc_property[proc_id].type);
> +
> +    return proc_id;
>  }
>  
>  static int board_version(uint32_t board_rev)
> @@ -88,32 +105,12 @@ static int board_version(uint32_t board_rev)
>  
>  static const char *board_soc_type(uint32_t board_rev)
>  {
> -    static const char *soc_types[] = {
> -        NULL, TYPE_BCM2836, TYPE_BCM2837,
> -    };
> -    int proc_id = board_processor_id(board_rev);
> -
> -    if (proc_id >= ARRAY_SIZE(soc_types) || !soc_types[proc_id]) {
> -        error_report("Unsupported processor id '%d' (board revision: 0x%x)",
> -                     proc_id, board_rev);
> -        exit(1);
> -    }
> -    return soc_types[proc_id];
> +    return soc_property[board_processor_id(board_rev)].type;
>  }
>  
>  static int cores_count(uint32_t board_rev)
>  {
> -    static const int soc_cores_count[] = {
> -        0, BCM283X_NCPUS, BCM283X_NCPUS,
> -    };
> -    int proc_id = board_processor_id(board_rev);
> -
> -    if (proc_id >= ARRAY_SIZE(soc_cores_count) || !soc_cores_count[proc_id]) 
> {
> -        error_report("Unsupported processor id '%d' (board revision: 0x%x)",
> -                     proc_id, board_rev);
> -        exit(1);
> -    }
> -    return soc_cores_count[proc_id];
> +    return soc_property[board_processor_id(board_rev)].cores_count;
>  }
>  
>  static const char *board_type(uint32_t board_rev)
>