Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/intc/heathrow_pic.c | 31 ++++++++++++-------------------
hw/intc/trace-events | 5 +++++
include/hw/intc/heathrow_pic.h | 1 +
3 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
index 7bf44e0d86..afe0b08cbb 100644
--- a/hw/intc/heathrow_pic.c
+++ b/hw/intc/heathrow_pic.c
@@ -26,16 +26,7 @@
#include "hw/hw.h"
#include "hw/ppc/mac.h"
#include "hw/intc/heathrow_pic.h"
-
-/* debug PIC */
-//#define DEBUG_PIC
-
-#ifdef DEBUG_PIC
-#define PIC_DPRINTF(fmt, ...) \
- do { printf("PIC: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define PIC_DPRINTF(fmt, ...)
-#endif
+#include "trace.h"
static inline int heathrow_check_irq(HeathrowPICState *pic)
{
@@ -61,7 +52,7 @@ static void heathrow_write(void *opaque, hwaddr addr,
unsigned int n;
n = ((addr & 0xfff) - 0x10) >> 4;
- PIC_DPRINTF("writel: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
+ trace_heathrow_write(addr, n, value);
if (n >= 2)
return;
pic = &s->pics[n];
@@ -109,7 +100,7 @@ static uint64_t heathrow_read(void *opaque, hwaddr addr,
break;
}
}
- PIC_DPRINTF("readl: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
+ trace_heathrow_read(addr, n, value);
return value;
}
@@ -124,16 +115,18 @@ static void heathrow_set_irq(void *opaque, int num, int level)
HeathrowState *s = opaque;
HeathrowPICState *pic;
unsigned int irq_bit;
+ int last_level = (s->last_levels & (num << 1UL)) ? 1 : 0;
-#if defined(DEBUG)
- {
- static int last_level[64];
- if (last_level[num] != level) {
- PIC_DPRINTF("set_irq: num=0x%02x level=%d\n", num, level);
- last_level[num] = level;
+ if (last_level != level) {
+ trace_heathrow_set_irq(num, level);
+
+ if (level) {
+ s->last_levels |= (num << 1UL);
+ } else {
+ s->last_levels &= ~(num << 1UL);
}
}
-#endif
+
pic = &s->pics[1 - (num >> 5)];
irq_bit = 1 << (num & 0x1f);
if (level) {
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 4092d2825e..55e8c2570c 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -186,3 +186,8 @@ nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
nvic_sysreg_write(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+
+# hw/intc/heathrow_pic.c
+heathrow_write(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" %u: 0x%"PRIx64
+heathrow_read(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" %u: 0x%"PRIx64
+heathrow_set_irq(int num, int level) "set_irq: num=0x%02x level=%d"
diff --git a/include/hw/intc/heathrow_pic.h b/include/hw/intc/heathrow_pic.h
index bc3ffaab87..878a0f7cdb 100644
--- a/include/hw/intc/heathrow_pic.h
+++ b/include/hw/intc/heathrow_pic.h
@@ -42,6 +42,7 @@ typedef struct HeathrowState {
MemoryRegion mem;
HeathrowPICState pics[2];
qemu_irq *irqs;
+ uint64_t last_levels;
} HeathrowState;
#define HEATHROW_NUM_IRQS 64
--
2.11.0
On Mon, Feb 19, 2018 at 06:19:15PM +0000, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/intc/heathrow_pic.c | 31 ++++++++++++-------------------
> hw/intc/trace-events | 5 +++++
> include/hw/intc/heathrow_pic.h | 1 +
> 3 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
> index 7bf44e0d86..afe0b08cbb 100644
> --- a/hw/intc/heathrow_pic.c
> +++ b/hw/intc/heathrow_pic.c
> @@ -26,16 +26,7 @@
> #include "hw/hw.h"
> #include "hw/ppc/mac.h"
> #include "hw/intc/heathrow_pic.h"
> -
> -/* debug PIC */
> -//#define DEBUG_PIC
> -
> -#ifdef DEBUG_PIC
> -#define PIC_DPRINTF(fmt, ...) \
> - do { printf("PIC: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define PIC_DPRINTF(fmt, ...)
> -#endif
> +#include "trace.h"
>
> static inline int heathrow_check_irq(HeathrowPICState *pic)
> {
> @@ -61,7 +52,7 @@ static void heathrow_write(void *opaque, hwaddr addr,
> unsigned int n;
>
> n = ((addr & 0xfff) - 0x10) >> 4;
> - PIC_DPRINTF("writel: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
> + trace_heathrow_write(addr, n, value);
> if (n >= 2)
> return;
> pic = &s->pics[n];
> @@ -109,7 +100,7 @@ static uint64_t heathrow_read(void *opaque, hwaddr addr,
> break;
> }
> }
> - PIC_DPRINTF("readl: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
> + trace_heathrow_read(addr, n, value);
> return value;
> }
>
> @@ -124,16 +115,18 @@ static void heathrow_set_irq(void *opaque, int num, int level)
> HeathrowState *s = opaque;
> HeathrowPICState *pic;
> unsigned int irq_bit;
> + int last_level = (s->last_levels & (num << 1UL)) ? 1 : 0;
>
> -#if defined(DEBUG)
> - {
> - static int last_level[64];
> - if (last_level[num] != level) {
> - PIC_DPRINTF("set_irq: num=0x%02x level=%d\n", num, level);
> - last_level[num] = level;
> + if (last_level != level) {
> + trace_heathrow_set_irq(num, level);
> +
> + if (level) {
> + s->last_levels |= (num << 1UL);
> + } else {
> + s->last_levels &= ~(num << 1UL);
Cleaning up this last_level stuff is a good idea, but is a bit more
involved than merely converting to trace-events.
It's also not really the "last" level - it's effectively maintaining
the current level of the input irqs. .. and isn't that information
already within the HeathrowPICState structure?
> }
> }
> -#endif
> +
> pic = &s->pics[1 - (num >> 5)];
> irq_bit = 1 << (num & 0x1f);
> if (level) {
> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> index 4092d2825e..55e8c2570c 100644
> --- a/hw/intc/trace-events
> +++ b/hw/intc/trace-events
> @@ -186,3 +186,8 @@ nvic_complete_irq(int irq, bool secure) "NVIC complete IRQ %d (secure %d)"
> nvic_set_irq_level(int irq, int level) "NVIC external irq %d level set to %d"
> nvic_sysreg_read(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
> nvic_sysreg_write(uint64_t addr, uint32_t value, unsigned size) "NVIC sysreg write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
> +
> +# hw/intc/heathrow_pic.c
> +heathrow_write(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" %u: 0x%"PRIx64
> +heathrow_read(uint64_t addr, unsigned int n, uint64_t value) "0x%"PRIx64" %u: 0x%"PRIx64
> +heathrow_set_irq(int num, int level) "set_irq: num=0x%02x level=%d"
> diff --git a/include/hw/intc/heathrow_pic.h b/include/hw/intc/heathrow_pic.h
> index bc3ffaab87..878a0f7cdb 100644
> --- a/include/hw/intc/heathrow_pic.h
> +++ b/include/hw/intc/heathrow_pic.h
> @@ -42,6 +42,7 @@ typedef struct HeathrowState {
> MemoryRegion mem;
> HeathrowPICState pics[2];
> qemu_irq *irqs;
> + uint64_t last_levels;
> } HeathrowState;
>
> #define HEATHROW_NUM_IRQS 64
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On 20/02/18 04:20, David Gibson wrote:
> On Mon, Feb 19, 2018 at 06:19:15PM +0000, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/intc/heathrow_pic.c | 31 ++++++++++++-------------------
>> hw/intc/trace-events | 5 +++++
>> include/hw/intc/heathrow_pic.h | 1 +
>> 3 files changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/intc/heathrow_pic.c b/hw/intc/heathrow_pic.c
>> index 7bf44e0d86..afe0b08cbb 100644
>> --- a/hw/intc/heathrow_pic.c
>> +++ b/hw/intc/heathrow_pic.c
>> @@ -26,16 +26,7 @@
>> #include "hw/hw.h"
>> #include "hw/ppc/mac.h"
>> #include "hw/intc/heathrow_pic.h"
>> -
>> -/* debug PIC */
>> -//#define DEBUG_PIC
>> -
>> -#ifdef DEBUG_PIC
>> -#define PIC_DPRINTF(fmt, ...) \
>> - do { printf("PIC: " fmt , ## __VA_ARGS__); } while (0)
>> -#else
>> -#define PIC_DPRINTF(fmt, ...)
>> -#endif
>> +#include "trace.h"
>>
>> static inline int heathrow_check_irq(HeathrowPICState *pic)
>> {
>> @@ -61,7 +52,7 @@ static void heathrow_write(void *opaque, hwaddr addr,
>> unsigned int n;
>>
>> n = ((addr & 0xfff) - 0x10) >> 4;
>> - PIC_DPRINTF("writel: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
>> + trace_heathrow_write(addr, n, value);
>> if (n >= 2)
>> return;
>> pic = &s->pics[n];
>> @@ -109,7 +100,7 @@ static uint64_t heathrow_read(void *opaque, hwaddr addr,
>> break;
>> }
>> }
>> - PIC_DPRINTF("readl: " TARGET_FMT_plx " %u: %08x\n", addr, n, value);
>> + trace_heathrow_read(addr, n, value);
>> return value;
>> }
>>
>> @@ -124,16 +115,18 @@ static void heathrow_set_irq(void *opaque, int num, int level)
>> HeathrowState *s = opaque;
>> HeathrowPICState *pic;
>> unsigned int irq_bit;
>> + int last_level = (s->last_levels & (num << 1UL)) ? 1 : 0;
>>
>> -#if defined(DEBUG)
>> - {
>> - static int last_level[64];
>> - if (last_level[num] != level) {
>> - PIC_DPRINTF("set_irq: num=0x%02x level=%d\n", num, level);
>> - last_level[num] = level;
>> + if (last_level != level) {
>> + trace_heathrow_set_irq(num, level);
>> +
>> + if (level) {
>> + s->last_levels |= (num << 1UL);
>> + } else {
>> + s->last_levels &= ~(num << 1UL);
>
> Cleaning up this last_level stuff is a good idea, but is a bit more
> involved than merely converting to trace-events.
>
> It's also not really the "last" level - it's effectively maintaining
> the current level of the input irqs. .. and isn't that information
> already within the HeathrowPICState structure?
Yeah actually you're right here - I was so focused on converting the
code as-is that I didn't really look at what was happening at the end of
heathrow_set_irq(). The motivation was that without this hack the trace
output is always outputting the IRQ status, presumably because we have a
level-triggered interrupt being asserted.
Let me rework this patch to use the last state information from
HeathrowPICState instead.
ATB,
Mark.
© 2016 - 2025 Red Hat, Inc.