Hi Arnaud,
On 11/11/23 15:33, ~aminier wrote:
From: Arnaud Minier <arnaud.min...@telecom-paris.fr>
Signed-off-by: Arnaud Minier <arnaud.min...@telecom-paris.fr>
Signed-off-by: Inès Varhol <ines.var...@telecom-paris.fr>
---
hw/arm/Kconfig | 1 +
hw/arm/stm32l4x5_soc.c | 65 +++++-
hw/misc/Kconfig | 3 +
hw/misc/meson.build | 1 +
hw/misc/stm32l4x5_exti.c | 329 ++++++++++++++++++++++++++++++
hw/misc/trace-events | 5 +
include/hw/arm/stm32l4x5_soc.h | 3 +
include/hw/misc/stm32l4x5_exti.h | 64 ++++++
tests/qtest/meson.build | 5 +
tests/qtest/stm32l4x5_exti-test.c | 102 +++++++++
10 files changed, 576 insertions(+), 2 deletions(-)
create mode 100644 hw/misc/stm32l4x5_exti.c
create mode 100644 include/hw/misc/stm32l4x5_exti.h
create mode 100644 tests/qtest/stm32l4x5_exti-test.c
diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
index 198d3f6d3e..6f2a1b34b3 100644
--- a/hw/arm/stm32l4x5_soc.c
+++ b/hw/arm/stm32l4x5_soc.c
@@ -43,10 +43,51 @@
#define SRAM2_BASE_ADDRESS 0x10000000
#define SRAM2_SIZE (32 * KiB)
+static const hwaddr exti_addr = 0x40010400;
Why not a #define?
+#define NUM_EXTI_IRQ 40
diff --git a/hw/misc/stm32l4x5_exti.c b/hw/misc/stm32l4x5_exti.c
+static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
+{
+ Stm32l4x5ExtiState *s = opaque;
+
+ trace_stm32l4x5_exti_set_irq(irq, level);
+
+ if (irq >= NUM_INTERRUPT_OUT_LINES) {
+ return;
This can not happen. If you are unsure, this would be a programming
error, thus aborting is better, but nothing needed IMO.
+ }
+
+ if (irq < 32) {
+ if (((1 << irq) & s->exti_rtsr1) && level) {
+ /* Rising Edge */
+ s->exti_pr1 |= 1 << irq;
+ }
+
+ if (((1 << irq) & s->exti_ftsr1) && !level) {
+ /* Falling Edge */
+ s->exti_pr1 |= 1 << irq;
+ }
+
+ if (!((1 << irq) & s->exti_imr1)) {
+ /* Interrupt is masked */
+ return;
+ }
+ } else {
+ /* Shift the value to enable access in x2 registers*/
+ int irq_x2 = irq - 32;
+ if (((1 << irq_x2) & s->exti_rtsr2) && level) {
+ /* Rising Edge */
+ s->exti_pr2 |= 1 << irq_x2;
+ }
+
+ if (((1 << irq_x2) & s->exti_ftsr2) && !level) {
+ /* Falling Edge */
+ s->exti_pr2 |= 1 << irq_x2;
+ }
+
+ if (!((1 << irq_x2) & s->exti_imr2)) {
+ /* Interrupt is masked */
+ return;
+ }
+ }
+ qemu_irq_pulse(s->irq[irq]);
+}
Could be simpler avoiding duplication, as:
---
static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
{
Stm32l4x5ExtiState *s = opaque;
int oirq = irq;
uint32_t *rtsr;
uint32_t *ftsr;
uint32_t *pr;
uint32_t *imr;
trace_stm32l4x5_exti_set_irq(irq, level);
if (irq < 32) {
rtsr = &s->exti_rtsr1;
ftsr = &s->exti_ftsr1;
pr = &s->exti_pr1;
imr = &s->exti_imr1;
} else {
rtsr = &s->exti_rtsr2;
ftsr = &s->exti_ftsr2;
pr = &s->exti_pr2;
imr = &s->exti_imr2;
/* Shift the value to enable access in x2 registers. */
irq -= 32;
}
if (((1 << irq) & *rtsr) && level) {
/* Rising Edge */
*pr |= 1 << irq;
}
if (((1 << irq) & *ftsr) && !level) {
/* Falling Edge */
*pr |= 1 << irq;
}
if (!((1 << irq) & *imr)) {
/* Interrupt is masked */
return;
}
qemu_irq_pulse(s->irq[oirq]);
}
---
But changing Stm32l4x5ExtiState as:
---
struct Stm32l4x5ExtiState {
SysBusDevice parent_obj;
MemoryRegion mmio;
uint32_t imr[2];
uint32_t emr[2];
uint32_t rtsr[2];
uint32_t ftsr[2];
uint32_t swier[2];
uint32_t pr[2];
qemu_irq irq[NUM_INTERRUPT_OUT_LINES];
};
---
We get even simpler:
---
static void stm32l4x5_exti_set_irq(void *opaque, int irq, int level)
{
Stm32l4x5ExtiState *s = opaque;
unsigned n = irq >= 32;
int oirq = irq;
trace_stm32l4x5_exti_set_irq(irq, level);
if (irq >= 32) {
/* Shift the value to enable access in x2 registers. */
irq -= 32;
}
if (((1 << irq) & s->rtsr[n]) && level) {
/* Rising Edge */
s->pr[n] |= 1 << irq;
}
if (((1 << irq) & s->ftsr[n]) && !level) {
/* Falling Edge */
s->pr[n] |= 1 << irq;
}
if (!((1 << irq) & s->imr[n])) {
/* Interrupt is masked */
return;
}
qemu_irq_pulse(s->irq[oirq]);
}
---
(code untested).
+
+static uint64_t stm32l4x5_exti_read(void *opaque, hwaddr addr,
+ unsigned int size)
+{
+ Stm32l4x5ExtiState *s = opaque;
+ uint32_t r = 0;
unsigned n = addr >= EXTI_IMR2;
+
+ switch (addr) {
+ case EXTI_IMR1:
+ r = s->exti_imr1;
+ break;
This becomes:
case EXTI_IMR1:
case EXTI_IMR2:
r = s->exti_imr[n];
break;
...
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "STM32L4X5_exti_read: Bad offset %x\n", (int)addr);
Please use '0x' prefix for hexadecimal.
+ break;
+ }
+
+ trace_stm32l4x5_exti_read(addr, r);
+
+ return r;
+}
+static const MemoryRegionOps stm32l4x5_exti_ops = {
+ .read = stm32l4x5_exti_read,
+ .write = stm32l4x5_exti_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
Your implementation is 32-bit wide (all your registers are),
so:
.impl.min_access_size = 4,
.impl.max_access_size = 4,
What are the allowed accesses? any 8/16/32/64 bits?
(This is what happens when .valid fields aren't set).
+};
+static void stm32l4x5_exti_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->reset = stm32l4x5_exti_reset;
Better set a ResettableClass handler. DeviceClass::reset will soon
be deprecated.
+ dc->vmsd = &vmstate_stm32l4x5_exti;
+}
+static void stm32l4x5_exti_register_types(void)
+{
+ type_register_static(&stm32l4x5_exti_info);
+}
Preferably use the DEFINE_TYPES() macro.
+type_init(stm32l4x5_exti_register_types)
diff --git a/include/hw/misc/stm32l4x5_exti.h b/include/hw/misc/stm32l4x5_exti.h
new file mode 100644
index 0000000000..4305e7fcbb
--- /dev/null
+++ b/include/hw/misc/stm32l4x5_exti.h
@@ -0,0 +1,64 @@
+/*
+ * STM32L4x5 SoC family EXTI
"STM32L4x5 EXTI (Extended interrupts and events controller)"
+#define NUM_GPIO_EVENT_IN_LINES 16
Since not used externally, NUM_GPIO_EVENT_IN_LINES could be
restricted to the source.
+#define NUM_INTERRUPT_OUT_LINES 40
+
+struct Stm32l4x5ExtiState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+
+ uint32_t exti_imr1;
+ uint32_t exti_emr1;
+ uint32_t exti_rtsr1;
+ uint32_t exti_ftsr1;
+ uint32_t exti_swier1;
+ uint32_t exti_pr1;
+ uint32_t exti_imr2;
+ uint32_t exti_emr2;
+ uint32_t exti_rtsr2;
+ uint32_t exti_ftsr2;
+ uint32_t exti_swier2;
+ uint32_t exti_pr2;
See previous suggestion for Stm32l4x5ExtiState. Besides, no
need to use the cumbersome 'exti_' prefix.
+ qemu_irq irq[NUM_INTERRUPT_OUT_LINES];
+};
+
+#endif
Good patch quality, looking forward for v2!
Regards,
Phil.
