Re: [Qemu-devel] [Qemu-arm] [PATCH v1 3/3] raspi: Add "raspi3" machine type

[Philippe Mathieu-Daudé]

On 02/15/2018 10:18 AM, Peter Maydell wrote:
> On 15 February 2018 at 13:14, Philippe Mathieu-Daudé  wrote:
>> On 02/15/2018 09:49 AM, Philippe Mathieu-Daudé wrote:
>>> On 02/15/2018 09:39 AM, Peter Maydell wrote:
 On 8 February 2018 at 05:50, Pekka Enberg  wrote:
>>
>> Now I remember why I hesitated with this patch,
>>
>> This part {
>>
> +mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
> +mc->max_cpus = BCM2836_NCPUS;
> +mc->min_cpus = BCM2836_NCPUS;
> +mc->default_cpus = BCM2836_NCPUS;
>>
>> } is the BCM2837 SoC, very similar to the BCM2836.
> 
> Yeah, we had a whole go-around about whether we should have a
> BCM2837 object or just make the BCM2836 object have a configurable
> CPU type. You could argue either way...

Since both SoCs are clocked at the same freq (and we don't model the L2
cache, the only diff) your suggestion (#ifdef TARGET_AARCH64) is the
easiest/cleaner way to go and I'm happy with it :)

A one-line comment would be worthful although.

Regards,

Phil.

Re: [Qemu-devel] [Qemu-arm] [PATCH v1 3/3] raspi: Add "raspi3" machine type

[Peter Maydell]

On 15 February 2018 at 13:14, Philippe Mathieu-Daudé  wrote:
> On 02/15/2018 09:49 AM, Philippe Mathieu-Daudé wrote:
>> On 02/15/2018 09:39 AM, Peter Maydell wrote:
>>> On 8 February 2018 at 05:50, Pekka Enberg  wrote:
>
> Now I remember why I hesitated with this patch,
>
> This part {
>
 +mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
 +mc->max_cpus = BCM2836_NCPUS;
 +mc->min_cpus = BCM2836_NCPUS;
 +mc->default_cpus = BCM2836_NCPUS;
>
> } is the BCM2837 SoC, very similar to the BCM2836.

Yeah, we had a whole go-around about whether we should have a
BCM2837 object or just make the BCM2836 object have a configurable
CPU type. You could argue either way...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH v1 3/3] raspi: Add "raspi3" machine type

[Philippe Mathieu-Daudé]

On 02/15/2018 09:49 AM, Philippe Mathieu-Daudé wrote:
> On 02/15/2018 09:39 AM, Peter Maydell wrote:
>> On 8 February 2018 at 05:50, Pekka Enberg  wrote:
>>> This patch adds a "raspi3" machine type, which can now be selected as
>>> the machine to run on by users via the "-M" command line option to QEMU.
>>>
>>> The machine type does *not* ignore memory transaction failures so we
>>> likely need to add some dummy devices later when people run something
>>> more complicated than what I'm using for testing.
>>>
>>> Signed-off-by: Pekka Enberg 
>>> ---
>>>  hw/arm/raspi.c | 21 +
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>>> index 66fe10e376..048ff23a51 100644
>>> --- a/hw/arm/raspi.c
>>> +++ b/hw/arm/raspi.c
>>> @@ -187,3 +187,24 @@ static void raspi2_machine_init(MachineClass *mc)
>>>  mc->ignore_memory_transaction_failures = true;
>>>  };
>>>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
>>> +
>>> +static void raspi3_init(MachineState *machine)
>>> +{
>>> +raspi_init(machine, 3);
>>> +}
>>> +
>>> +static void raspi3_machine_init(MachineClass *mc)
>>> +{
>>> +mc->desc = "Raspberry Pi 3";
>>> +mc->init = raspi3_init;
>>> +mc->block_default_type = IF_SD;
>>> +mc->no_parallel = 1;
>>> +mc->no_floppy = 1;
>>> +mc->no_cdrom = 1;

Now I remember why I hesitated with this patch,

This part {

>>> +mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>>> +mc->max_cpus = BCM2836_NCPUS;
>>> +mc->min_cpus = BCM2836_NCPUS;
>>> +mc->default_cpus = BCM2836_NCPUS;

} is the BCM2837 SoC, very similar to the BCM2836.

>>> +mc->default_ram_size = 1024 * 1024 * 1024;
>>> +}
>>> +DEFINE_MACHINE("raspi3", raspi3_machine_init)
>>
>> Hi. This patch breaks "make check", because it adds the raspi3
>> to the arm-softmmu (32-bit guest CPUs only) build, where the
>> cortex-a53 CPU doesn't exist:
>>
>> e104462:xenial:qemu$ ./build/x86/arm-softmmu/qemu-system-arm -M raspi3
>> **
>> ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:372:object_initialize_with_type:
>> assertion failed: (type != NULL)
>> Aborted (core dumped)
>>
>> The usual way we avoid this is that 64-bit only boards are
>> in their own source file, which is only compiled if the right
>> CONFIG_FOO is set by default-configs/aarch64-softmmu.mak.
>> In this case splitting the 64-bit board into its own source
>> file would be weird and awkward, so the simple thing is to
>> guard the raspi3 bits with #ifdef TARGET_AARCH64.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 
>>
>> (You might think we could define a CONFIG_RASPI3 in
>> aarch64-softmmu.mak and #ifdef on it, but for some reason
>> we don't expose those CONFIG_* to C code, possibly just because
>> we've never needed to in the past...)
>>
>> Since this was the only code change needed, I'm just going to make
>> it and apply the patchset to target-arm.next, rather than ask
>> you to do a respin. (There was also a stray space-at-end-of-line
>> in patch 2 which checkpatch grumbles about; I'll fix that up too.)
>>
>> thanks
>> -- PMM
>>

Re: [Qemu-devel] [Qemu-arm] [PATCH v1 3/3] raspi: Add "raspi3" machine type

[Philippe Mathieu-Daudé]

On 02/15/2018 09:39 AM, Peter Maydell wrote:
> On 8 February 2018 at 05:50, Pekka Enberg  wrote:
>> This patch adds a "raspi3" machine type, which can now be selected as
>> the machine to run on by users via the "-M" command line option to QEMU.
>>
>> The machine type does *not* ignore memory transaction failures so we
>> likely need to add some dummy devices later when people run something
>> more complicated than what I'm using for testing.
>>
>> Signed-off-by: Pekka Enberg 
>> ---
>>  hw/arm/raspi.c | 21 +
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index 66fe10e376..048ff23a51 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -187,3 +187,24 @@ static void raspi2_machine_init(MachineClass *mc)
>>  mc->ignore_memory_transaction_failures = true;
>>  };
>>  DEFINE_MACHINE("raspi2", raspi2_machine_init)
>> +
>> +static void raspi3_init(MachineState *machine)
>> +{
>> +raspi_init(machine, 3);
>> +}
>> +
>> +static void raspi3_machine_init(MachineClass *mc)
>> +{
>> +mc->desc = "Raspberry Pi 3";
>> +mc->init = raspi3_init;
>> +mc->block_default_type = IF_SD;
>> +mc->no_parallel = 1;
>> +mc->no_floppy = 1;
>> +mc->no_cdrom = 1;
>> +mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
>> +mc->max_cpus = BCM2836_NCPUS;
>> +mc->min_cpus = BCM2836_NCPUS;
>> +mc->default_cpus = BCM2836_NCPUS;
>> +mc->default_ram_size = 1024 * 1024 * 1024;
>> +}
>> +DEFINE_MACHINE("raspi3", raspi3_machine_init)
> 
> Hi. This patch breaks "make check", because it adds the raspi3
> to the arm-softmmu (32-bit guest CPUs only) build, where the
> cortex-a53 CPU doesn't exist:
> 
> e104462:xenial:qemu$ ./build/x86/arm-softmmu/qemu-system-arm -M raspi3
> **
> ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/qom/object.c:372:object_initialize_with_type:
> assertion failed: (type != NULL)
> Aborted (core dumped)
> 
> The usual way we avoid this is that 64-bit only boards are
> in their own source file, which is only compiled if the right
> CONFIG_FOO is set by default-configs/aarch64-softmmu.mak.
> In this case splitting the 64-bit board into its own source
> file would be weird and awkward, so the simple thing is to
> guard the raspi3 bits with #ifdef TARGET_AARCH64.

Reviewed-by: Philippe Mathieu-Daudé 

> 
> (You might think we could define a CONFIG_RASPI3 in
> aarch64-softmmu.mak and #ifdef on it, but for some reason
> we don't expose those CONFIG_* to C code, possibly just because
> we've never needed to in the past...)
> 
> Since this was the only code change needed, I'm just going to make
> it and apply the patchset to target-arm.next, rather than ask
> you to do a respin. (There was also a stray space-at-end-of-line
> in patch 2 which checkpatch grumbles about; I'll fix that up too.)
> 
> thanks
> -- PMM
>