Message ID | 20210209110252.GA14409@carpenter.lan |
---|---|
State | New |
Headers | show |
Series | Raspberry PI GPIO interrupt support | expand |
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.
Dear Philippe, thank you for your kind response. I will send a new version for the patch with the modifications you've highlighted, but first I've some question on the best way to implement them. On Tue, Feb 09, 2021 at 12:30:32PM +0100, Philippe Mathieu-Daudé wrote: >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! > The minimal client I've developed can be found at https://github.com/berdav/qemu-rpi-gpio As I have introduced, I'm building a qtest but I've found a problem: In my test, I want to check if the IRQs of the named GPIOs /machine/soc/peripherals/ic/gpu-irq are correctly handled by the controller. I was thinking to use irq_intercept_in to get the status of the IRQ lines using something similar to the following: qtest_irq_intercept_in(s, "/machine/soc/peripherals/ic"); And then check it using qtest_get_irq(s, 49 + irqline); But it isn't triggering. Looking in the source of qtest I've found this check in the qtest_irq_intercept_in function: QLIST_FOREACH(ngl, &dev->gpios, node) { /* We don't support intercept of named GPIOs yet */ if (ngl->name) { continue; } I've removed the if and the qtests seems to work just fine. Is there particular caveats or details to implement this part of the framework? I can send separated patches if you have some details on the desiderata of the support, or maybe start a new thread with this question. Otherwise, I can remove the part of my qtest to not check the IRQs, but I don't know if it would be the best choice. > >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 > I have just created the patch using git format patch -s , it does not give me any error applying it over the master branch on https://github.com/qemu/QEMU . I will take more care and apply it on the master branch of https://git.qemu.org/git/qemu.git/ , thank you. > [... cut ...] > >Regards, > >Phil. I'm adapting the patch using the Coding style script now. I've also introduced a new costant for the number of GPIOs. Thank you for your time, D.
diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c index dcff13433e..101c3e6fba 100644 --- a/hw/arm/bcm2835_peripherals.c +++ b/hw/arm/bcm2835_peripherals.c @@ -114,6 +114,8 @@ static void bcm2835_peripherals_init(Object *obj) OBJECT(&s->sdhci.sdbus)); object_property_add_const_link(OBJECT(&s->gpio), "sdbus-sdhost", OBJECT(&s->sdhost.sdbus)); + object_property_add_const_link(OBJECT(&s->gpio), "ic", + OBJECT(&s->ic)); /* Mphi */ object_initialize_child(obj, "mphi", &s->mphi, TYPE_BCM2835_MPHI); diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c index abdddbc67c..2952ebe1da 100644 --- a/hw/gpio/bcm2835_gpio.c +++ b/hw/gpio/bcm2835_gpio.c @@ -7,6 +7,9 @@ * Clement Deschamps <clement.deschamps@antfield.fr> * Luc Michel <luc.michel@antfield.fr> * + * GPIO Interrupt 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. */ @@ -20,6 +23,7 @@ #include "migration/vmstate.h" #include "hw/sd/sd.h" #include "hw/gpio/bcm2835_gpio.h" +#include "hw/intc/bcm2835_ic.h" #include "hw/irq.h" #define GPFSEL0 0x00 @@ -110,6 +114,29 @@ static int gpfsel_is_out(BCM2835GpioState *s, int index) return 0; } +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; +} + static void gpset(BCM2835GpioState *s, uint32_t val, uint8_t start, uint8_t count, uint32_t *lev) { @@ -120,6 +147,20 @@ static void gpset(BCM2835GpioState *s, for (i = 0; i < count; i++) { if ((changes & cur) && (gpfsel_is_out(s, start + i))) { qemu_set_irq(s->out[start + i], 1); + } else if ((changes & cur) && ren_detect(s, start + i)) { + /* If this is an input and must check rising edge */ + int irqline = 0; + if (i > 27) + irqline = 1; + if (i > 45) + irqline = 2; + + /* Set the bit in the events */ + if (i < 32) + s->eds0 |= cur; + else + s->eds1 |= cur; + qemu_set_irq(s->irq[irqline], 1); } cur <<= 1; } @@ -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; + + 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)) + 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); + } + 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; case GPREN0: + return s->ren0; case GPREN1: + return s->ren1; case GPFEN0: + return s->fen0; case GPFEN1: + return s->fen1; case GPHEN0: case GPHEN1: case GPLEN0: @@ -228,11 +312,23 @@ static void bcm2835_gpio_write(void *opaque, hwaddr offset, /* Read Only */ break; case GPEDS0: + eds_clear(s, value, 0); + break; case GPEDS1: + eds_clear(s, value, 1); + break; case GPREN0: + s->ren0 = value; + break; case GPREN1: + s->ren1 = value; + break; case GPFEN0: + s->fen0 = value; + break; case GPFEN1: + s->fen1 = value; + break; case GPHEN0: case GPHEN1: case GPLEN0: @@ -310,6 +406,7 @@ static void bcm2835_gpio_init(Object *obj) static void bcm2835_gpio_realize(DeviceState *dev, Error **errp) { + int i; BCM2835GpioState *s = BCM2835_GPIO(dev); Object *obj; @@ -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) { + 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; - 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; - 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" 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]; }; #define TYPE_BCM2835_GPIO "bcm2835_gpio"
The bcm2835 GPIOs now generate interrupts. This modification enables QTEST clients to trigger interrupt-based interfaces. 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(-)