[RFC] generic-gpio-led & stm32-gpio-led

# 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:

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);
}

On Tue, Jun 16, 2015 at 7:15 AM, Liviu Ionescu <ilg@livius.net> 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]
>
>
>
>

> On 16 Jun 2015, at 19:10, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
>> +    bool use_stderr;
> 
> Why do you want to switch between stderr and stdout?

I wasn't sure which one is more appropriate, I first used stdout then tried stderr.

> I think stderr is more correct,

yes, that was my conclusion too. I'll make it default.

> 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.

most of my use cases are running with semihosting active, and the application may printf() whatever it likes, so it is not realistic to expect no traffic on stdout.

but I got your point.

>> +} GenericGPIOLEDClass;
> 
> Drop the "Generic"

from the typedef or from the string type name? what about the file name?

>> +    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?

yes, but code readability decreases.

>> +static void generic_gpio_led_construct_callback(Object *obj,
>> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
>> +{
>> +    qemu_log_function_name();
> 
> Function call after QOM casts.

could you be more explicit?

> 
>> +
>> +    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.

come on, we're in 2015, 16 years were not enough to get used to C99? ;-)
 
>> +    /*
>> +     * 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.

I'll try to see how the code looks like.

the main idea was to automate all these connection details.

but if it is just a single additional line, I can live with it.

>> +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.

right, just that it requires some extra typing...

>> +    .class_size = sizeof(GenericGPIOLEDClass) };
> 
> newline before }

unfortunately this is way too complicated to fix. I'm not doing any formatting manually, I use the Eclipse auto formatter, which generally is ok, but when asked to do the antiquated R&K formatting, it uses this peculiar closing brace syntax. (except for QEMU, in my all other sources I use the GNU formatting style, which is ok in Eclipse).

so manually fixing this is useless, since I auto-format after entering every few lines and the manual formatting would be lost.

> 
> 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.

aha! I saw some examples where such names were used, but had no idea where the names came from.

what names do you suggest to use for GPIOs? I guess some nice aliases, since the internal names are not for humans. could you give me a hint how these aliases should be defined and used?

> 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).

I'm using tables and loop type creation when defining MCUs (for example about a dozen STM32 MCUs are created like this), but for machines things are not as uniform, and are anyway grouped by board vendor, so not all boards are in a file.

>> +static void stm_gpio_led_construct_callback(Object *obj,
>> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
>> +{
>> +    qemu_log_function_name();
>> +
> 
> What would you want to put here?

>> +    /* Explicitly call the parent constructor. */
>> +    GENERIC_GPIO_LED_GET_CLASS(obj)->construct(obj, info, mcu);

in this case nothing, because the derived class just implements a base abstract.

but the constructor was kept to call the parent constructor, like usual.


thank you,

Liviu

On Tue, Jun 16, 2015 at 10:16 AM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 16 Jun 2015, at 19:10, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>>> +    bool use_stderr;
>>
>> Why do you want to switch between stderr and stdout?
>
> I wasn't sure which one is more appropriate, I first used stdout then tried stderr.
>
>> I think stderr is more correct,
>
> yes, that was my conclusion too. I'll make it default.
>
>> 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.
>
> most of my use cases are running with semihosting active, and the application may printf() whatever it likes, so it is not realistic to expect no traffic on stdout.
>
> but I got your point.
>
>>> +} GenericGPIOLEDClass;
>>
>> Drop the "Generic"
>
> from the typedef or from the string type name? what about the file name?
>

All. Generic is implicit from the fact there is no specifics.

>>> +    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?
>
> yes, but code readability decreases.
>
>>> +static void generic_gpio_led_construct_callback(Object *obj,
>>> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
>>> +{
>>> +    qemu_log_function_name();
>>
>> Function call after QOM casts.
>
> could you be more explicit?
>

Just the C99 thing.

>>
>>> +
>>> +    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.
>
> come on, we're in 2015, 16 years were not enough to get used to C99? ;-)
>

Don't shoot the messenger :) It is not a hard rule, but you need a
solid reason to break it (#ifdeffery being the only one I can think
of).

>>> +    /*
>>> +     * 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.
>
> I'll try to see how the code looks like.
>
> the main idea was to automate all these connection details.
>
> but if it is just a single additional line, I can live with it.
>
>>> +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.
>
> right, just that it requires some extra typing...
>

Wont a machine level table parse make this a one-liner.

>>> +    .class_size = sizeof(GenericGPIOLEDClass) };
>>
>> newline before }
>
> unfortunately this is way too complicated to fix. I'm not doing any formatting manually, I use the Eclipse auto formatter, which generally is ok, but when asked to do the antiquated R&K formatting, it uses this peculiar closing brace syntax. (except for QEMU, in my all other sources I use the GNU formatting style, which is ok in Eclipse).
>
> so manually fixing this is useless, since I auto-format after entering every few lines and the manual formatting would be lost.
>

Your autoformatter does a surprisingly good job of getting close to
qemu coding style. Can the rules just be tweaked any maybe QEMU coding
style can be added to eclipse?

To check your coding style, commit your work to git and:

git show | ./scripts/checkpatch.pl -

>>
>> 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.
>
> aha! I saw some examples where such names were used, but had no idea where the names came from.
>
> what names do you suggest to use for GPIOs? I guess some nice aliases, since the internal names are not for humans. could you give me a hint how these aliases should be defined and used?
>

They are not really aliases, they are the GPIO names proper.
qdev_[init|get|connect]_gpio_[in|out]_named are direct replacements
for the non named variants. The non-human internal names should go
away if you use them. The name of the GPIO should match something in
the TRM for the device. E.g. if the SoC define "bank A" gpios, then a
suitable string name would be "bank-A". This should match the SoC
level, not the board level so it wouldn't be names like "green-led".
Your new code handles that information. For your generic LED however,
that is a QEMU invention which is why I propose a single unnamed GPIO
in that case.

>> 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).
>
> I'm using tables and loop type creation when defining MCUs (for example about a dozen STM32 MCUs are created like this), but for machines things are not as uniform, and are anyway grouped by board vendor, so not all boards are in a file.
>

The struct definition and some helper functions to do the iteration
can go in common code. Just the tables go in each file.

I have to wonder though if the printfery is the right approach. Can
eclipse use QEMUs existing GPIO introspection capabilites to get the
LED states and do something with them? Then the LED definition is pure
GPIO passing and these is no need for new devices at all.

Regards,
Peter

>>> +static void stm_gpio_led_construct_callback(Object *obj,
>>> +        GenericGPIOLEDInfo* info, DeviceState *mcu)
>>> +{
>>> +    qemu_log_function_name();
>>> +
>>
>> What would you want to put here?
>
>>> +    /* Explicitly call the parent constructor. */
>>> +    GENERIC_GPIO_LED_GET_CLASS(obj)->construct(obj, info, mcu);
>
> in this case nothing, because the derived class just implements a base abstract.
>
> but the constructor was kept to call the parent constructor, like usual.
>
>
> thank you,
>
> Liviu
>
>
>
>

> On 16 Jun 2015, at 21:19, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> Your autoformatter does a surprisingly good job of getting close to
> qemu coding style. Can the rules just be tweaked any maybe QEMU coding
> style can be added to eclipse?

this is exactly what I did, I took the K&R and tweaked it where possible. unfortunately the closing brace position is not configurable.
 
> They are not really aliases, they are the GPIO names proper.
> qdev_[init|get|connect]_gpio_[in|out]_named are direct replacements
> for the non named variants. The non-human internal names should go
> away if you use them. The name of the GPIO should match something in
> the TRM for the device. E.g. if the SoC define "bank A" gpios, then a
> suitable string name would be "bank-A". This should match the SoC
> level, not the board level so it wouldn't be names like "green-led".
> Your new code handles that information. For your generic LED however,
> that is a QEMU invention which is why I propose a single unnamed GPIO
> in that case.

oops... you lost me... I need to dig a bit to understand this.

> I have to wonder though if the printfery is the right approach.

blinking leds with printf() is just the first step, or the backup solution if graphics are not available. the plan was to enable graphics, present a picture of the board, and blink the led by painting a saturated square in the led location. however I don't know yet how much effort this may take, and if it is worth doing it, but it would definitely be a good marketing tool ;-)

> Can
> eclipse use QEMUs existing GPIO introspection capabilites to get the
> LED states and do something with them? Then the LED definition is pure
> GPIO passing and these is no need for new devices at all.

I do not know the QEMU introspection capabilities (hint?) but my Eclipse plug-in can be extended to connect to any reasonable protocol.

---

I'm currently reworking the (generic-)gpio-led to be standalone and accommodate your suggestions, but it'll take me some time to figure out the details of nodes naming.

I also had a small problem with the objects tree. initially my gpio-led object was not derived from SysBusDevice, since I considered it a separate object. it was functional, but made 'info qtree' fail. I changed the node parent to SysBusDevice but I'm not sure what would be the best topology for my object tree.


regards,

Liviu

On Tue, Jun 16, 2015 at 12:22 PM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 16 Jun 2015, at 21:19, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>> Your autoformatter does a surprisingly good job of getting close to
>> qemu coding style. Can the rules just be tweaked any maybe QEMU coding
>> style can be added to eclipse?
>
> this is exactly what I did, I took the K&R and tweaked it where possible. unfortunately the closing brace position is not configurable.
>
>> They are not really aliases, they are the GPIO names proper.
>> qdev_[init|get|connect]_gpio_[in|out]_named are direct replacements
>> for the non named variants. The non-human internal names should go
>> away if you use them. The name of the GPIO should match something in
>> the TRM for the device. E.g. if the SoC define "bank A" gpios, then a
>> suitable string name would be "bank-A". This should match the SoC
>> level, not the board level so it wouldn't be names like "green-led".
>> Your new code handles that information. For your generic LED however,
>> that is a QEMU invention which is why I propose a single unnamed GPIO
>> in that case.
>
> oops... you lost me... I need to dig a bit to understand this.
>
>> I have to wonder though if the printfery is the right approach.
>
> blinking leds with printf() is just the first step, or the backup solution if graphics are not available. the plan was to enable graphics, present a picture of the board, and blink the led by painting a saturated square in the led location. however I don't know yet how much effort this may take, and if it is worth doing it, but it would definitely be a good marketing tool ;-)
>
>> Can
>> eclipse use QEMUs existing GPIO introspection capabilites to get the
>> LED states and do something with them? Then the LED definition is pure
>> GPIO passing and these is no need for new devices at all.
>
> I do not know the QEMU introspection capabilities (hint?) but my Eclipse plug-in can be extended to connect to any reasonable protocol.
>

qtest protocol I think. I would grep qdev_intercept_gpio_out and go from there.

Regards,
Peter

> ---
>
> I'm currently reworking the (generic-)gpio-led to be standalone and accommodate your suggestions, but it'll take me some time to figure out the details of nodes naming.
>
> I also had a small problem with the objects tree. initially my gpio-led object was not derived from SysBusDevice, since I considered it a separate object. it was functional, but made 'info qtree' fail. I changed the node parent to SysBusDevice but I'm not sure what would be the best topology for my object tree.
>

That SysBus parenting will lead to a false topology anyways. The
correct parent for the LED is TYPE_DEVICE. don't worry about the
qtree, Andreas was working on a correct solution for the tree-dump
that uses QOM rather than the legacy qbus tree (qtree).

Regards,
Peter

>
> regards,
>
> Liviu
>
>

> On 16 Jun 2015, at 19:10, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> ... 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.

connecting a gpio_out to a gpio_in seems not possible, gpio_in irqs are parented to the device and parenting them to the gpio_out fail with the parent != 0 assertion.

however I was able to connect_gpio_out for standalone irqs.

the actual connection sequence is:

	qemu_irq irq = GENERIC_GPIO_LED_STATE(led)->irq; 
	qdev_connect_gpio_out(DEVICE(object_resolve_path("/machine/stm32/gpio[c]", NULL)), 12, irq); 

I'm not very happy with it, since it is quite long.

any suggestions how to make it more readable?

> E.g. if the SoC define "bank A" gpios, then a
> suitable string name would be "bank-A". This should match the SoC
> level, not the board level so it wouldn't be names like "green-led".

this is still too tricky for me.

since the default names were unusable (like device[7], etc), I created two containers (cortexm and stm32) and placed the peripherals inside, with names like /cortexm/nvic, cortexm/itm, stm32/rcc, stm32/gpio[%c], etc.

I used calls like:

    state->container = container_get(qdev_get_machine(), "/stm32");

    object_property_add_child(state->container, "rcc", OBJECT(state->rcc), NULL);


would this be acceptable?


regards,

Liviu

On Tue, Jun 16, 2015 at 3:25 PM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 16 Jun 2015, at 19:10, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>> ... 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.
>
> connecting a gpio_out to a gpio_in seems not possible, gpio_in irqs are parented to the device and parenting them to the gpio_out fail with the parent != 0 assertion.
>

This should work. Can I see the backtrace of that abort?

Regards.
Peter

> however I was able to connect_gpio_out for standalone irqs.
>
> the actual connection sequence is:
>
>         qemu_irq irq = GENERIC_GPIO_LED_STATE(led)->irq;
>         qdev_connect_gpio_out(DEVICE(object_resolve_path("/machine/stm32/gpio[c]", NULL)), 12, irq);
>
> I'm not very happy with it, since it is quite long.
>
> any suggestions how to make it more readable?
>
>> E.g. if the SoC define "bank A" gpios, then a
>> suitable string name would be "bank-A". This should match the SoC
>> level, not the board level so it wouldn't be names like "green-led".
>
> this is still too tricky for me.
>
> since the default names were unusable (like device[7], etc), I created two containers (cortexm and stm32) and placed the peripherals inside, with names like /cortexm/nvic, cortexm/itm, stm32/rcc, stm32/gpio[%c], etc.
>
> I used calls like:
>
>     state->container = container_get(qdev_get_machine(), "/stm32");
>
>     object_property_add_child(state->container, "rcc", OBJECT(state->rcc), NULL);
>
>
> would this be acceptable?
>
>
> regards,
>
> Liviu
>
>
>
>
>

> On 17 Jun 2015, at 03:25, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
>> ...
>> connecting a gpio_out to a gpio_in seems not possible, gpio_in irqs are parented to the device and parenting them to the gpio_out fail with the parent != 0 assertion.
>> 
> 
> This should work. Can I see the backtrace of that abort?

**
ERROR:/Users/ilg/Work/qemu/gnuarmeclipse-qemu.git/qom/object.c:1274:object_get_canonical_path_component: assertion failed: (obj->parent != NULL)

gpio-led

    // state->irq = qemu_allocate_irq(gpio_led_irq_handler, obj, 0);
    qdev_init_gpio_in(DEVICE(obj), gpio_led_irq_handler, 1);


mchine creation

    // qemu_irq irq = GPIO_LED_STATE(led)->irq;
    qemu_irq irq = qdev_get_gpio_in(led, 0);
    qdev_connect_gpio_out(
                DEVICE(object_resolve_path("/machine/stm32/gpio[c]", NULL)), 12,
                irq);

the functional solution is commented out.


regards,

Liviu