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....@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.descha...@antfield.fr> > * Luc Michel <luc.mic...@antfield.fr> > * > + * GPIO External support > + * Davide Berardi <berardi....@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.
