[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 3/3] arm: Add BBC micro:bit machine
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v4 3/3] arm: Add BBC micro:bit machine |
Date: |
Thu, 16 Aug 2018 15:11:59 +0100 |
On 3 August 2018 at 06:21, Joel Stanley <address@hidden> wrote:
> This adds the base for a machine model of the BBC micro:bit:
>
> https://en.wikipedia.org/wiki/Micro_Bit
>
> This is a system with a nRF51 SoC containing the main processor, with
> various peripherals on board.
>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Joel Stanley <address@hidden>
> ---
> v2:
> - Instead of setting kernel filename property, load the image directly
> - Add link to hardware overview website
> v3:
> - Rebase microbit on m0 changes
> - Remove hard-coded flash size and retrieve from the soc
> - Add Stefan's reviewed-by
> ---
> hw/arm/Makefile.objs | 2 +-
> hw/arm/microbit.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 55 insertions(+), 1 deletion(-)
> create mode 100644 hw/arm/microbit.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index e31875ec69bc..2798a257921d 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -36,4 +36,4 @@ obj-$(CONFIG_MSF2) += msf2-soc.o msf2-som.o
> obj-$(CONFIG_IOTKIT) += iotkit.o
> obj-$(CONFIG_FSL_IMX7) += fsl-imx7.o mcimx7d-sabre.o
> obj-$(CONFIG_ARM_SMMUV3) += smmu-common.o smmuv3.o
> -obj-$(CONFIG_NRF51_SOC) += nrf51_soc.o
> +obj-$(CONFIG_NRF51_SOC) += nrf51_soc.o microbit.o
> diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
> new file mode 100644
> index 000000000000..ecf64e883f4f
> --- /dev/null
> +++ b/hw/arm/microbit.c
> @@ -0,0 +1,54 @@
> +/*
> + * BBC micro:bit machine
> + * http://tech.microbit.org/hardware/
> + *
> + * Copyright 2018 Joel Stanley <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "hw/arm/arm.h"
> +#include "exec/address-spaces.h"
> +
> +#include "hw/arm/nrf51_soc.h"
> +
> +typedef struct {
> + MachineState parent;
> +
> + NRF51State nrf51;
> +} MICROBITMachineState;
Can we call this "MicrobitMachineState", please? I don't
think the board name is all-caps, and our convention
for struct type names is CamelCase.
> +
> +#define TYPE_MICROBIT_MACHINE "microbit"
Should be MACHINE_TYPE_NAME("microbit")
> +
> +#define MICROBIT_MACHINE(obj) \
> + OBJECT_CHECK(MICROBITMachineState, obj, TYPE_MICROBIT_MACHINE)
> +
> +static void microbit_init(MachineState *machine)
> +{
> + MICROBITMachineState *s = g_new(MICROBITMachineState, 1);
This is odd. The MICROBITMachineState is the state struct
for your subclass of MachineState, and that object has
already been allocated (you get a pointer to it as the
argument to the init function here). So all you need to do
is cast it to the right type:
MICROBITMachineState *s = MICROBIT_MACHINE(machine);
You don't need to allocate a second copy. (You do need to
get the type registration right so that you declare that the
type is of size sizeof(MICROBITMachineState) rather than
just sizeof(MachineState), though -- see below.)
> + MemoryRegion *system_memory = get_system_memory();
> + Object *soc;
> +
> + object_initialize(&s->nrf51, sizeof(s->nrf51), TYPE_NRF51_SOC);
> + soc = OBJECT(&s->nrf51);
> + object_property_add_child(OBJECT(machine), "nrf51", soc, &error_fatal);
You should use the new function init_sysbus_child() here
rather than doing separate initialize and add_child steps
(we realised late in the 3.0 cycle that we had refcount
leaks as a result of doing this as a 2-step process, hence
the new function).
> + object_property_set_link(soc, OBJECT(system_memory),
> + "memory", &error_abort);
> +
> + object_property_set_bool(soc, true, "realized", &error_abort);
Better to use error_fatal rather than error_abort in these two calls,
I think.
> +
> + arm_m_profile_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> + NRF51_SOC(soc)->flash_size);
> +}
> +
> +static void microbit_machine_init(MachineClass *mc)
> +{
> + mc->desc = "BBC micro:bit";
> + mc->init = microbit_init;
> + mc->max_cpus = 1;
> +}
> +DEFINE_MACHINE("microbit", microbit_machine_init);
Your subclass of TYPE_MACHINE has extra state, so it can't
use DEFINE_MACHINE (which creates a subclass whose instance_size
is the same as the parent TYPE_MACHINE). You need to do this
longhand:
static void machine_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
mc->desc = "BBC micro:bit";
mc->init = microbit_init;
mc->max_cpus = 1;
}
static const TypeInfo microbit_info = {
.name = TYPE_MICROBIT_MACHINE,
.parent = TYPE_MACHINE,
.instance_size = sizeof(MICROBITMachineState),
.class_init = microbit_class_init,
};
static void microbit_machine_init(void)
{
type_register_static(µbit_info);
}
type_init(microbit_machine_init);
(code untested but should be correct; compare against other boards.)
thanks
-- PMM