Re: [Qemu-arm] [Qemu-devel] [PATCH 1/7] hw/arm/mps2: Implement skeleton mps2-an385 and mps2-an511 board models

[Peter Maydell]

On 11 July 2017 at 15:33, Alistair Francis <address@hidden> wrote:
> On Tue, Jul 11, 2017 at 1:17 PM, Peter Maydell <address@hidden> wrote:
>> Model the ARM MPS2/MPS2+ FPGA based development board.
>>
>> The MPS2 and MPS2+ dev boards are FPGA based (the 2+ has a bigger
>> FPGA but is otherwise the same as the 2). Since the CPU itself
>> and most of the devices are in the FPGA, the details of the board
>> as seen by the guest depend significantly on the FPGA image.
>>
>> We model the following FPGA images:
>>  "mps2_an385" -- Cortex-M3 as documented in ARM Application Note AN385
>>  "mps2_an511" -- Cortex-M3 'DesignStart' as documented in AN511
>>
>> They are fairly similar but differ in the details for some
>> peripherals.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/arm/Makefile.objs            |   1 +
>>  hw/arm/mps2.c                   | 273 
>> ++++++++++++++++++++++++++++++++++++++++
>>  default-configs/arm-softmmu.mak |   1 +
>>  3 files changed, 275 insertions(+)
>>  create mode 100644 hw/arm/mps2.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 4c5c4ee..a2e56ec 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_FSL_IMX25) += fsl-imx25.o imx25_pdk.o
>>  obj-$(CONFIG_FSL_IMX31) += fsl-imx31.o kzm.o
>>  obj-$(CONFIG_FSL_IMX6) += fsl-imx6.o sabrelite.o
>>  obj-$(CONFIG_ASPEED_SOC) += aspeed_soc.o aspeed.o
>> +obj-$(CONFIG_MPS2) += mps2.o
>
> Is this file name possibly too generic? Should it be arm-mps2 instead.

It's already in hw/arm/, and we don't ever do anything that
confuses foo.o in one directory with foo.o in another.
(Also none of the other board or SoCs have 'arm' prepended.)

> I don't see any other boards with the same name from a Google search,
> but it just seems a bit vague having a three letter acronym.

>> +#define TYPE_MPS2_MACHINE "mps2"
>
> This public name seems too common as well. I am just worried it'll
> conflict with something one day.

I can see the issue, but on the other hand the type name is not
external ABI, so we can always rename it later if we have to.
The board names themselves (mps2-an385 and mps2-an511) on the
other hand we have to get right from the start. I'm not sure
tacking an arm- on the front of those helps; they're already
pretty ungainly.

The other possible name floating around here is "v2m-mps",
which at least some of the docs refer to as the name of
the motherboard. There's a lot of just of just plain "MPS2", though.

(I note that we've had 'kzm' and 'z2' since forever and they
haven't ever conflicted with anything despite being pretty short.)

I dunno; I'll have a think about it. Naming is hard...

>> +
>> +    MemoryRegion *system_memory = get_system_memory();
>> +
>> +    DeviceState *armv7m;
>
> These should be at the top of the function.

Fixed (along with the 'sccdev' added to the DeviceState
line in a later patch).

>> +    create_unimplemented_device("CMSDK APB peripheral region @0x40000000",
>> +                                0x40000000, 0x00010000);
>> +    create_unimplemented_device("CMSDK peripheral region @0x40010000",
>> +                                0x40010000, 0x00010000);
>> +    create_unimplemented_device("Extra peripheral region @0x40020000",
>> +                                0x40020000, 0x00010000);
>> +    create_unimplemented_device("RESERVED 4", 0x40030000, 0x001D0000);
>> +    create_unimplemented_device("Ethernet", 0x40200000, 0x00100000);
>> +    create_unimplemented_device("VGA", 0x41000000, 0x0200000);
>
> I did not know this was an option, this is pretty cool.

Yeah. It's really nice for bringing up a board model because
you can just add -d unimp to your command line and get the
info about what devices the guest is prodding that you need
to implement next.

thanks
-- PMM