Re: [Qemu-devel] [RFC] generic-gpio-led & stm32-gpio-led

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 7:15 AM, Liviu Ionescu <address@hidden> wrote:
> # Intro
>
> For my multiple Cortex-M boards, I need a LED device that will display a 
> message when a GPIO bit is written.
>
> # Goal
>
> The goal is to pack the LED into an object that is very easy to instantiate 
> and configure.
>
> # Solution
>
> My solution is an abstract class "generic-gpio-led" and derived classes like 
> "stm32-gpio-led" that implements the specifics of a certain Cortex-M family.
>
> The derived class doesn't have to do much, just to return the desired GPIO 
> device from the specific MCU.
>
> The abstract class includes the construction logic, the connection logic and 
> the parametrised handler used to blink the LED.
>
> # Usage
>
> The LED instantiation is very simple, the object is created using a structure 
> that packs all LED specifics and a reference to the MCU:


Commit message should wrap to 80 chars.

>
> GenericGPIOLEDInfo h103_machine_green_led = {
>     .desc = "STM32-H103 Green LED, GPIOC[12], active low",
>     .port_index = STM32_GPIO_PORT_C,
>     .port_bit = 12,
>     .active_low = true,
>     .on_message = "[Green LED On]\n",
>     .off_message = "[Green LED Off]\n",
>     .use_stderr = true };
>
> static void stm32_h103_board_init_callback(MachineState *machine)
> {
>     ...
>
>     DeviceState *led = qdev_alloc(NULL, TYPE_STM32_GPIO_LED);
>     {
>         STM32_GPIO_LED_GET_CLASS(led)->construct(OBJECT(led),
>                 &h103_machine_green_led, mcu);
>     }
>     qdev_realize(led);
> }
>
>
>

If you use git send-email to do patches you should get a diffstat here.

>
>
> diff --git a/include/hw/display/generic-gpio-led.h 
> b/include/hw/display/generic-gpio-led.h
> new file mode 100644
> index 0000000..e84ec0e
> --- /dev/null
> +++ b/include/hw/display/generic-gpio-led.h
> @@ -0,0 +1,96 @@
> +/*
> + * Generic GPIO connected LED device emulation.
> + *
> + * Copyright (c) 2015 Liviu Ionescu
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef GENERIC_GPIO_LED_H_
> +#define GENERIC_GPIO_LED_H_
> +
> +#include "hw/qdev.h"
> +#include "qemu/typedefs.h"
> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#define DEFINE_PROP_GENERIC_GPIO_LED_PTR(_n, _s, _f) \
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, GenericGPIOLEDInfo*)
> +
> +/**
> + * This structure must be passed via the constructor, to configure the
> + * LED connectivity, logic and message.
> + */
> +typedef struct {
> +    const char *desc;
> +    int port_index;
> +    int port_bit; //
> +    bool active_low;
> +    const char *on_message;
> +    const char *off_message; //
> +    bool use_stderr;

Why do you want to switch between stderr and stdout?

I think stderr is more correct, or should at least be forced in
nographic QEMU where stdout may be owned by a terminal mux. Otherwise
you should probably talk to the monitor.

> +} GenericGPIOLEDInfo;
> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#define TYPE_GENERIC_GPIO_LED "generic-gpio-led"
> +
> +#define GENERIC_GPIO_LED_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(GenericGPIOLEDClass, (obj), TYPE_GENERIC_GPIO_LED)
> +#define GENERIC_GPIO_LED_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(GenericGPIOLEDClass, (klass), TYPE_GENERIC_GPIO_LED)
> +
> +typedef struct {
> +    /*< private >*/
> +    DeviceClass parent_class;
> +    /*< public >*/
> +
> +    /**
> +     * Constructor; it uses the Info structure and a pointer to the MCU
> +     * to get the GPIO(n) port and to connect to the pin.
> +     */
> +    void (*construct)(Object *obj, GenericGPIOLEDInfo* info, DeviceState 
> *mcu);
> +
> +    /**
> +     * Abstract function, to be implemented by all derived classes.
> +     */
> +    DeviceState *(*get_gpio_dev)(DeviceState *dev, int port_index);
> +} GenericGPIOLEDClass;


Drop the "Generic"

> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#define GENERIC_GPIO_LED_STATE(obj) \
> +    OBJECT_CHECK(GenericGPIOLEDState, (obj), TYPE_GENERIC_GPIO_LED)
> +
> +typedef struct {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +    /*< public >*/
> +
> +    GenericGPIOLEDInfo *info;
> +
> +    DeviceState *mcu;
> +    DeviceState *gpio;
> +
> +    /**
> +     * The actual irq used to blink the LED. It works connected to
> +     * a GPIO device outgoing irq.
> +     */
> +    qemu_irq irq;
> +
> +} GenericGPIOLEDState;
> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#endif /* GENERIC_GPIO_LED_H_ */
>
>
>
> diff --git a/hw/display/generic-gpio-led.c b/hw/display/generic-gpio-led.c
> new file mode 100644
> index 0000000..a5875ea
> --- /dev/null
> +++ b/hw/display/generic-gpio-led.c
> @@ -0,0 +1,122 @@
> +/*
> + * Generic GPIO connected LED device emulation.
> + *
> + * Copyright (c) 2015 Liviu Ionescu
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/display/generic-gpio-led.h"
> +
> +/**
> + * This is an abstract class that implements a generic LED connected to
> + * a GPIO device. It requires a derived class to implement the get_gpio_dev()
> + * virtual call, to access the actual GPIO device specific to a family.
> + */
> +
> +static void generic_gpio_led_irq_handler(void *opaque, int n, int level)
> +{
> +    GenericGPIOLEDInfo *info = GENERIC_GPIO_LED_STATE(opaque)->info;
> +
> +    /* There should be only one IRQ for the LED */
> +    assert(n == 0);
> +
> +    FILE *file;
> +    if (info->use_stderr) {
> +        file = stderr;
> +    } else {
> +        file = stdout;
> +    }
> +    /*
> +     * Assume that the IRQ is only triggered if the LED has changed state.
> +     * If this is not correct, we may get multiple LED Offs or Ons in a row.
> +     */
> +    switch (level) {
> +    case 0:
> +        fprintf(file, "%s",
> +                info->active_low ? info->on_message : info->off_message);
> +        break;
> +
> +    case 1:
> +        fprintf(file, "%s",
> +                info->active_low ? info->off_message : info->on_message);
> +        break;

Can you do an ^ or != between level and active low to remove dup?

> +    }
> +}
> +
> +static void generic_gpio_led_construct_callback(Object *obj,
> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
> +{
> +    qemu_log_function_name();

Function call after QOM casts.

> +
> +    GenericGPIOLEDState *state = GENERIC_GPIO_LED_STATE(obj);
> +    GenericGPIOLEDClass *klass = GENERIC_GPIO_LED_GET_CLASS(state);
> +

As we don't strictly forbid C99 decls but try and avoid them.

> +    state->info = info;
> +    state->mcu = mcu;
> +
> +    DeviceState *gpio_dev = klass->get_gpio_dev(DEVICE(obj), 
> info->port_index);
> +    assert(gpio_dev);
> +
> +    /*
> +     * Allocate 1 single incoming irq, and attach handler, this device
> +     * and n=0. (n==0 is checked in the handler by an assert)
> +     */
> +    state->irq = qemu_allocate_irq(generic_gpio_led_irq_handler, obj, 0);
> +
> +    /*
> +     * Connect the LED interrupt to the originating GPIO pin.
> +     * This will finally create a link inside the STM32 GPIO device.
> +     */
> +    qdev_connect_gpio_out(gpio_dev, info->port_bit, state->irq);

I think this is where the big QOM issue is. You are creating your own
GPIO connection mechanism on the abstract device level. Why can't you
do the GPIO connections on the machine level? Instead, this would be a
call to qdev_init_gpio_in() and you can remove all the refs to
gpio_dev.

> +}
> +
> +static void generic_gpio_led_realize_callback(DeviceState *dev, Error **errp)
> +{
> +    qemu_log_function_name();
> +}
> +

Don't worry about this, just leave the reset hook unset in the class
and nothing will happen.

> +#define DEFINE_PROP_DEVICE_STATE_PTR(_n, _s, _f) \
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, DeviceState*)
> +
> +static Property generic_gpio_led_properties[] = {
> +        DEFINE_PROP_GENERIC_GPIO_LED_PTR("info", GenericGPIOLEDState, info),
> +        DEFINE_PROP_DEVICE_STATE_PTR("mcu", GenericGPIOLEDState, mcu),

With my proposal to remove connectivity info from the struct, the
struct simple enough to just list out the remaining props one-by-one
rather than having to QOMify a struct for parameters.

> +    DEFINE_PROP_END_OF_LIST() };
> +
> +static void generic_gpio_led_class_init_callback(ObjectClass *klass, void 
> *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->props = generic_gpio_led_properties;
> +    dc->realize = generic_gpio_led_realize_callback;
> +
> +    GenericGPIOLEDClass *gl_class = GENERIC_GPIO_LED_CLASS(klass);

Move up top of fn.

> +    gl_class->construct = generic_gpio_led_construct_callback;
> +}
> +
> +static const TypeInfo generic_gpio_led_type_info = {
> +    .abstract = true,
> +    .name = TYPE_GENERIC_GPIO_LED,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(GenericGPIOLEDState),
> +    .class_init = generic_gpio_led_class_init_callback,
> +    .class_size = sizeof(GenericGPIOLEDClass) };

newline before }

> +
> +static void generic_gpio_led_type_init()
> +{
> +    type_register_static(&generic_gpio_led_type_info);
> +}
> +
> +type_init(generic_gpio_led_type_init);
>
>
>
> diff --git a/include/hw/display/stm32-gpio-led.h 
> b/include/hw/display/stm32-gpio-led.h
> new file mode 100644
> index 0000000..f1bb581
> --- /dev/null
> +++ b/include/hw/display/stm32-gpio-led.h
> @@ -0,0 +1,61 @@
> +/*
> + * STM32 GPIO connected LED device emulation.
> + *
> + * Copyright (c) 2015 Liviu Ionescu
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef STM32_GPIO_LED_H_
> +#define STM32_GPIO_LED_H_
> +
> +#include "hw/display/generic-gpio-led.h"
> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#define TYPE_STM32_GPIO_LED "stm32-gpio-led"
> +
> +#define STM32_GPIO_LED_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(STM32GPIOLEDClass, (obj), TYPE_STM32_GPIO_LED)
> +#define STM32_GPIO_LED_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(STM32GPIOLEDClass, (klass), TYPE_STM32_GPIO_LED)
> +
> +typedef struct {
> +    /*< private >*/
> +    GenericGPIOLEDClass parent_class;
> +    /*< public >*/
> +
> +    /**
> +     * Constructor; does not much, just passes the Info structure
> +     * and a pointer to the MCU to the base class.
> +     */
> +    void (*construct)(Object *obj, GenericGPIOLEDInfo* info, DeviceState 
> *mcu);
> +} STM32GPIOLEDClass;
> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#define STM32_GPIO_LED_STATE(obj) \
> +    OBJECT_CHECK(STM32GPIOState, (obj), TYPE_GENERIC_GPIO_LED)
> +
> +typedef struct {
> +    /*< private >*/
> +    GenericGPIOLEDState parent_obj;
> +    /*< public >*/
> +
> +    /* Currently there is nothing new here. */
> +} STM32GPIOLEDState;
> +
> +/* ------------------------------------------------------------------------- 
> */
> +
> +#endif /* STM32_GPIO_LED_H_ */
>
>
> diff --git a/hw/display/stm32-gpio-led.c b/hw/display/stm32-gpio-led.c
> new file mode 100644
> index 0000000..9438efe
> --- /dev/null
> +++ b/hw/display/stm32-gpio-led.c
> @@ -0,0 +1,76 @@
> +/*
> + * STM32 LED device emulation.
> + *
> + * Copyright (c) 2015 Liviu Ionescu
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/display/stm32-gpio-led.h"
> +#include "hw/arm/stm32-mcu.h"
> +
> +/**
> + * Implementation of the "generic-gpio-led" base class for the STM32 family.
> + */
> +
> +/**
> + * Implementation of the parent class virtual function.
> + * It mainly returns a pointer to the device GPIO[i].
> + */
> +static DeviceState *stm32_gpio_led_get_gpio_dev(DeviceState *dev,
> +        int port_index)
> +{
> +    GenericGPIOLEDState *gl_state = GENERIC_GPIO_LED_STATE(dev);
> +
> +    /**
> +     * Ask the MCU for the i-th (port_index) GPIO device.
> +     */
> +    return stm32_mcu_get_gpio_dev(gl_state->mcu, port_index);

In my proposal the machine model would do this.

qdev_connect_gpio_out_named(mcu, "name", index, qdev_get_gpio_in(gpio_dev, 0));

Or something like that.

As you are trying to create a large number of machines, SoC and GPIO
messages you may want to do some tabulation of these connections and
rip through it in a loop. The same table may contain the on/off
messages and drive the creation of the GPIOs (and you may need to
parse the table multiple times if there are construction pass
inter-deps).

> +}
> +
> +static void stm_gpio_led_construct_callback(Object *obj,
> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
> +{
> +    qemu_log_function_name();
> +

What would you want to put here?

Regards,
Peter

> +    /* Explicitly call the parent constructor. */
> +    GENERIC_GPIO_LED_GET_CLASS(obj)->construct(obj, info, mcu);
> +}
> +
> +static void stm32_gpio_led_class_init_callback(ObjectClass *klass, void 
> *data)
> +{
> +    STM32GPIOLEDClass *st_class = STM32_GPIO_LED_CLASS(klass);
> +    st_class->construct = stm_gpio_led_construct_callback;
> +
> +    /* Set virtual in parent class */
> +    GenericGPIOLEDClass *gl_class = GENERIC_GPIO_LED_CLASS(klass);
> +    gl_class->get_gpio_dev = stm32_gpio_led_get_gpio_dev;
> +}
> +
> +/*
> + * The realize() would be empty here, and its pointer is not initialised.
> + */
> +static const TypeInfo stm32_gpio_led_type_info = {
> +    .name = TYPE_STM32_GPIO_LED,
> +    .parent = TYPE_GENERIC_GPIO_LED,
> +    .instance_size = sizeof(STM32GPIOLEDState),
> +    .class_init = stm32_gpio_led_class_init_callback,
> +    .class_size = sizeof(STM32GPIOLEDClass) };
> +
> +static void stm32_gpio_led_type_init()
> +{
> +    type_register_static(&stm32_gpio_led_type_info);
> +}
> +
> +type_init(stm32_gpio_led_type_init);
>
>
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index e73cb7d..3f76c4e 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -34,3 +34,8 @@ obj-$(CONFIG_CG3) += cg3.o
>  obj-$(CONFIG_VGA) += vga.o
>
>  common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
> +
> +# [GNU ARM Eclipse]
> +obj-$(CONFIG_GNU_ARM_ECLIPSE) += generic-gpio-led.o
> +obj-$(CONFIG_STM32) += stm32-gpio-led.o
> +# [GNU ARM Eclipse]
>
>
>
>