diff mbox series

[v4,3/3] arm: Add BBC micro:bit machine

Message ID 20180803052137.10602-4-joel@jms.id.au (mailing list archive)
State New, archived
Headers show
Series arm: Add nRF51 SoC and micro:bit machine | expand

Commit Message

Joel Stanley Aug. 3, 2018, 5:21 a.m. UTC
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 <stefanha@redhat.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
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

Comments

Peter Maydell Aug. 16, 2018, 2:11 p.m. UTC | #1
On 3 August 2018 at 06:21, Joel Stanley <joel@jms.id.au> 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 <stefanha@redhat.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> 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 <joel@jms.id.au>
> + *
> + * 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(&microbit_info);
}

type_init(microbit_machine_init);


(code untested but should be correct; compare against other boards.)

thanks
-- PMM
Steffen Görtz Aug. 16, 2018, 2:19 p.m. UTC | #2
>> +
>> +    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:
> 

Hi Peter,

this is covered in <20180811090836.4024-1-contrib@steffen-goertz.de>

Steffen
Peter Maydell Aug. 16, 2018, 2:26 p.m. UTC | #3
On 16 August 2018 at 15:19, Steffen Görtz <mail@steffen-goertz.de> wrote:
>
>>> +
>>> +    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:
>>
>
> Hi Peter,
>
> this is covered in <20180811090836.4024-1-contrib@steffen-goertz.de>

So it is. This patch is wrong though, so the fix needs to be
made by folding the relevant changes into this patch, not as
a separate commit later.

PS: you don't really need a header file for the machine's
struct, because nothing except the machine's .c file will
ever need to include it.

thanks
-- PMM
Joel Stanley Aug. 26, 2018, 1:14 a.m. UTC | #4
On Thu, 16 Aug 2018 at 07:12, Peter Maydell <peter.maydell@linaro.org> wrote:

> > +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(&microbit_info);
> }
>
> type_init(microbit_machine_init);
>
>
> (code untested but should be correct; compare against other boards.)

Thanks for spelling it out. I spent a decent chunk of time looking at
other boards, and this aspect of Qemu still baffles me.
diff mbox series

Patch

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 <joel@jms.id.au>
+ *
+ * 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;
+
+#define TYPE_MICROBIT_MACHINE "microbit"
+
+#define MICROBIT_MACHINE(obj) \
+    OBJECT_CHECK(MICROBITMachineState, obj, TYPE_MICROBIT_MACHINE)
+
+static void microbit_init(MachineState *machine)
+{
+    MICROBITMachineState *s = g_new(MICROBITMachineState, 1);
+    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);
+    object_property_set_link(soc, OBJECT(system_memory),
+                             "memory", &error_abort);
+
+    object_property_set_bool(soc, true, "realized", &error_abort);
+
+    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);