Re: [PATCH] Raspberry PI GPIO interrupt support

[Philippe Mathieu-Daudé]

Hi Davide,
On 2/9/21 12:02 PM, Davide Berardi wrote:
> The bcm2835 GPIOs now generate interrupts.
> This modification enables QTEST clients to trigger interrupt-based
> interfaces.

Thanks for your patch!

Can you provide QTEST client example? Even better would be a qtest!

> 
> Signed-off-by: Davide Berardi <berardi.dav@gmail.com>
> ---
>   hw/arm/bcm2835_peripherals.c   |   2 +
>   hw/gpio/bcm2835_gpio.c         | 105 +++++++++++++++++++++++++++++++++
>   hw/intc/bcm2835_ic.c           |   2 -
>   include/hw/gpio/bcm2835_gpio.h |  12 ++++
>   4 files changed, 119 insertions(+), 2 deletions(-)
...

I wonder how you generated your patch, it doesn't apply:

Applying: Raspberry PI GPIO interrupt support
error: patch failed: hw/arm/bcm2835_peripherals.c:114
error: hw/arm/bcm2835_peripherals.c: patch does not apply
error: patch failed: hw/gpio/bcm2835_gpio.c:7
error: hw/gpio/bcm2835_gpio.c: patch does not apply
error: patch failed: hw/intc/bcm2835_ic.c:57
error: hw/intc/bcm2835_ic.c: patch does not apply
error: patch failed: include/hw/gpio/bcm2835_gpio.h:7
error: include/hw/gpio/bcm2835_gpio.h: patch does not apply
Patch failed at 0001 Raspberry PI GPIO interrupt support

You can find the guidelines here:
https://wiki.qemu.org/Contribute/SubmitAPatch#Submitting_your_Patches

>   +static inline int get_bit_2_u32(const uint32_t idx,
> +                                const uint32_t v1, const uint32_t v2)
> +{
> +    uint64_t v = v1 | ((uint64_t)v2) << 32;
> +    return !!(v & (1 << idx));
> +}
> +
> +static int ren_detect(BCM2835GpioState *s, int index)
> +{
> +    if (index >= 0 && index < 54) {
> +        return get_bit_2_u32(index, s->ren0, s->ren1);
> +    }
> +    return 0;
> +}
> +
> +static int fen_detect(BCM2835GpioState *s, int index)
> +{
> +    if (index >= 0 && index < 54) {
> +        return get_bit_2_u32(index, s->fen0, s->fen1);
> +    }
> +    return 0;
> +}

I suppose you can simplify by using 'uint64_t fen' and ren,
and the extract64() method from "qemu/bitops.h".

> @@ -137,6 +178,15 @@ static void gpclr(BCM2835GpioState *s,
>       for (i = 0; i < count; i++) {
>           if ((changes & cur) && (gpfsel_is_out(s, start + i))) {
>               qemu_set_irq(s->out[start + i], 0);
> +        } else if ((changes & cur) && fen_detect(s, start + i)) {
> +            /* If this is an input we must check falling edge */
> +            int irqline = 0;
> +            if (i > 27)
> +                irqline = 1;
> +            if (i > 45)
> +              irqline = 2;

Please respect QEMU's CODING_STYLE.rst.

Matter of taste, this seems easier to follow:

  int irqline;

  if (i > 45) {
    irqline = 2;
  } else if > 27
    irqline = 1;
  } else {
    irqline = 0;
  }

> +
> +            qemu_set_irq(s->irq[irqline], 1);
>           }
>           cur <<= 1;
>       }
> @@ -144,6 +194,34 @@ static void gpclr(BCM2835GpioState *s,
>       *lev &= ~val;
>   }
>   +static int gpio_from_value(uint64_t value, int bank)
> +{
> +    int i;
> +    for (i = 0 ; i < 32; ++i)
> +        if (value & (1 << i))

Please use extract32().

> +           return i + (32 * bank);
> +    return 0;
> +}
> +
> +static void eds_clear(BCM2835GpioState *s, uint64_t value, int bank)
> +{
> +    int gpio = 0;
> +    int irqline = 0;
> +    if (bank) {
> +        s->eds0 &= ~value;
> +    } else {
> +        s->eds1 &= (~value & 0x3f);
> +    }

Similarly, using 'uint64_t eds' should simplify this.

> +    gpio = gpio_from_value(value, bank);
> +
> +    if (gpio > 27)
> +       irqline = 1;
> +    if (gpio > 45)
> +       irqline = 2;
> +
> +    qemu_set_irq(s->irq[irqline], 0);
> +}
> +
>   static uint64_t bcm2835_gpio_read(void *opaque, hwaddr offset,
>           unsigned size)
>   {
> @@ -170,11 +248,17 @@ static uint64_t bcm2835_gpio_read(void *opaque,
> hwaddr offset,
>       case GPLEV1:
>           return s->lev1;
>       case GPEDS0:
> +        return s->eds0;
>       case GPEDS1:
> +        return s->eds1;

Using 'uint64_t eds' this becomes:

  case GPEDS0:
  case GPEDS1:
      return extract64(s->eds, offset == GPEDS1 ? 0 : 32, 32);

>   @@ -318,6 +415,14 @@ static void bcm2835_gpio_realize(DeviceState
> *dev, Error **errp)
>         obj = object_property_get_link(OBJECT(dev), "sdbus-sdhost",
> &error_abort);
>       s->sdbus_sdhost = SD_BUS(obj);
> +
> +    obj = object_property_get_link(OBJECT(dev), "ic", &error_abort);
> +    s->ic = BCM2835_IC(obj);
> +
> +    for (i = 0 ; i < 3; ++i) {

Please replace this magic 3 by a definition, maybe
BCM2835_EXTERNAL_IRQ_COUNT?

> +        s->irq[i] = qdev_get_gpio_in_named(DEVICE(obj),
> +                                           BCM2835_IC_GPU_IRQ, i + 49);
> +    }
>   }
>     static void bcm2835_gpio_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/bcm2835_ic.c b/hw/intc/bcm2835_ic.c
> index 9000d995e8..b381dc6603 100644
> --- a/hw/intc/bcm2835_ic.c
> +++ b/hw/intc/bcm2835_ic.c
> @@ -57,7 +57,6 @@ static void bcm2835_ic_update(BCM2835ICState *s)
>   static void bcm2835_ic_set_gpu_irq(void *opaque, int irq, int level)
>   {
>       BCM2835ICState *s = opaque;
> -

Spurious change.

>       assert(irq >= 0 && irq < 64);
>       trace_bcm2835_ic_set_gpu_irq(irq, level);
>       s->gpu_irq_level = deposit64(s->gpu_irq_level, irq, 1, level != 0);
> @@ -67,7 +66,6 @@ static void bcm2835_ic_set_gpu_irq(void *opaque, int
> irq, int level)
>   static void bcm2835_ic_set_arm_irq(void *opaque, int irq, int level)
>   {
>       BCM2835ICState *s = opaque;
> -

Ditto.

>       assert(irq >= 0 && irq < 8);
>       trace_bcm2835_ic_set_cpu_irq(irq, level);
>       s->arm_irq_level = deposit32(s->arm_irq_level, irq, 1, level != 0);
> diff --git a/include/hw/gpio/bcm2835_gpio.h
> b/include/hw/gpio/bcm2835_gpio.h
> index 1c53a05090..cad3e987d3 100644
> --- a/include/hw/gpio/bcm2835_gpio.h
> +++ b/include/hw/gpio/bcm2835_gpio.h
> @@ -7,6 +7,9 @@
>    *  Clement Deschamps <clement.deschamps@antfield.fr>
>    *  Luc Michel <luc.michel@antfield.fr>
>    *
> + * GPIO External support
> + *  Davide Berardi <berardi.dav@gmail.com>
> + *
>    * This work is licensed under the terms of the GNU GPL, version 2 or
> later.
>    * See the COPYING file in the top-level directory.
>    */
> @@ -17,6 +20,7 @@
>   #include "hw/sd/sd.h"
>   #include "hw/sysbus.h"
>   #include "qom/object.h"
> +#include "hw/intc/bcm2835_ic.h"

Here would go:

#define BCM2835_EXTERNAL_IRQ_COUNT 3

>     struct BCM2835GpioState {
>       SysBusDevice parent_obj;
> @@ -27,11 +31,19 @@ struct BCM2835GpioState {
>       SDBus sdbus;
>       SDBus *sdbus_sdhci;
>       SDBus *sdbus_sdhost;
> +    BCM2835ICState *ic;
>         uint8_t fsel[54];
>       uint32_t lev0, lev1;
> +    /* Event detection */
> +    uint32_t eds0, eds1;
> +    /* Edge selector */
> +    uint32_t ren0, ren1;
> +    uint32_t fen0, fen1;
> +
>       uint8_t sd_fsel;
>       qemu_irq out[54];
> +    qemu_irq irq[3];

and here 3 -> BCM2835_EXTERNAL_IRQ_COUNT.

>   };
>     #define TYPE_BCM2835_GPIO "bcm2835_gpio"

Regards,

Phil.