Re: [Qemu-arm] [PATCH 7/7] arm: Instantiate Microbit board-level devices

[Peter Maydell]

On 11 August 2018 at 10:08, Steffen Görtz <address@hidden> wrote:
> Instantiate the LED matrix and set board-level
> pull-up default values to buttons A and B.
> This is necessary to calm down the microsoft pxt
> javascript runtime available for the micro:bit.
>
> Signed-off-by: Steffen Görtz <address@hidden>

> +static void microbit_reset(void)
> +{
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    MicrobitMachineState *s = MICROBIT_MACHINE(machine);
> +
> +    qemu_devices_reset();
> +
> +    /* Board level pull-up */
> +    if (!qtest_enabled()) {
> +        qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->nrf51), BUTTON_A_PIN), 1);
> +        qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->nrf51), BUTTON_B_PIN), 1);
> +    }

Reset functions should not call qemu_set_irq().

The problem is that there's no ordering on device reset functions,
so there's no guarantee that the reset function on this end is
called after the reset function for the device it's trying to set
the input GPIO line for. If the two resets happen in the other order
then the device will forget that the input line was high.

There is no good way to handle a line that is supposed to be
pulled high on reset, I'm afraid. You just have to try to avoid
modelling things in a way that requires it.

(Yes, our modelling of reset is basically broken. It's a
hard design problem that nobody's found the time to tackle
properly. You'll see various more-or-less dubious workarounds
if you dig in the right parts of the code base...)

> +}
> +
>
>  static void microbit_machine_init(MachineClass *mc)
>  {
>      mc->desc = "BBC micro:bit";
>      mc->init = microbit_init;
>      mc->max_cpus = 1;
> +    mc->reset = microbit_reset;
>  }
>
>  static void microbit_machine_init_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/arm/microbit.h b/include/hw/arm/microbit.h
> index 89f0c6bc07..094326b4ae 100644
> --- a/include/hw/arm/microbit.h
> +++ b/include/hw/arm/microbit.h
> @@ -24,6 +24,7 @@ typedef struct MicrobitMachineState {
>      MachineState parent_obj;
>
>      NRF51State nrf51;
> +    LEDMatrixState matrix;
>  } MicrobitMachineState;

thanks
-- PMM