Re: [PATCH v3 03/13] hw/arm/raspi: Extract the version from the board revision

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 2/13/20 2:40 PM, Peter Maydell wrote:
> On Sat, 8 Feb 2020 at 16:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The board revision encode the board version. Add a helper
>> to extract the version, and use it.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/arm/raspi.c | 31 +++++++++++++++++++++++++++----
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 818146fdbb..f285e2988f 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -16,6 +16,7 @@
>>   #include "qapi/error.h"
>>   #include "cpu.h"
>>   #include "hw/arm/bcm2836.h"
>> +#include "hw/registerfields.h"
>>   #include "qemu/error-report.h"
>>   #include "hw/boards.h"
>>   #include "hw/loader.h"
>> @@ -37,6 +38,28 @@ typedef struct RasPiState {
>>       MemoryRegion ram;
>>   } RasPiState;
>>
>> +/*
>> + * Board revision codes:
>> + * www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/
>> + */
>> +FIELD(REV_CODE, REVISION,           0, 4);
>> +FIELD(REV_CODE, TYPE,               4, 8);
>> +FIELD(REV_CODE, PROCESSOR,         12, 4);
>> +FIELD(REV_CODE, MANUFACTURER,      16, 4);
>> +FIELD(REV_CODE, MEMORY_SIZE,       20, 3);
>> +FIELD(REV_CODE, STYLE,             23, 1);
>> +
>> +static int board_processor_id(uint32_t board_rev)
>> +{
>> +    assert(FIELD_EX32(board_rev, REV_CODE, STYLE)); /* Only new style */
>> +    return FIELD_EX32(board_rev, REV_CODE, PROCESSOR);
>> +}
>> +
>> +static int board_version(uint32_t board_rev)
>> +{
>> +    return board_processor_id(board_rev) + 1;
> 
> This uses the 'processor' field, which basically means the SoC
> (0 for BCM2835, 1 for BCM2836, 2 for BCMM2837, 3 for BCM2711).
> We use 'version' for a wider range of things in our code here:
>   * do we need SMP setup?
>   * which address does the firmware image go?
>   * do we need to set up SMC vectors so no-op SMC works?
>   * as well as "which SoC do we instantiate"?
> 
> We think of 'version' as basically "raspi 2 or 3?", but
> according to the table in your url you can get a version of
> the raspi 2b with a BCM2837 SoC, which confuses this idea.
> 
> Anyway, since what we have in this patch works OK for the set
> of board models we support, I'm happy to leave the patch as-is,
> but maybe worth checking and considering what in our code we
> should really be making conditional on "actually the SoC type"
> and what on something else...

Yes you are right, version = (2, 3) was too simple, I replaced the 
'version' check by 'processor_id' in the series introducing the raspi4 
(with other cleanups).
Eventually this file will only use the board_rev fields directly.