Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-13 Thread bzt 
Hi,

That's great, thank you! Looking forward to see the patch in the master branch!

Cheers,
bzt

On 3/12/19, Peter Maydell  wrote:
> On Sun, 10 Mar 2019 at 11:02, bzt  wrote:
>>
>> Hi,
>>
>> Okay, as you wish. My code works either way and on real hardware as
>> well, because I acknowledge the periodic IRQ as soon as possible, so
>> good for me.
>
> Thanks; I'll get this version of the patch into git master
> at some point in the next week.
>
> (I happened to have an rpi2 to hand, so I wrote a little test
> program to check what the behaviour of the h/w is, and it does
> indeed not clear the INTEN bit when it signals an IRQ,
> and it doesn't clear the IRQ line until the guest clears the
> interrupt flag: so if your IRQ handler doesn't clear the flag
> by writing to the register then the IRQ will 'scream' (fire
> continually) both on QEMU and on the real hardware. So I think
> the data sheet is correct here, though it certainly has its share
> of errors...)
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-12 Thread Peter Maydell 
On Sun, 10 Mar 2019 at 11:02, bzt  wrote:
>
> Hi,
>
> Okay, as you wish. My code works either way and on real hardware as
> well, because I acknowledge the periodic IRQ as soon as possible, so
> good for me.

Thanks; I'll get this version of the patch into git master
at some point in the next week.

(I happened to have an rpi2 to hand, so I wrote a little test
program to check what the behaviour of the h/w is, and it does
indeed not clear the INTEN bit when it signals an IRQ,
and it doesn't clear the IRQ line until the guest clears the
interrupt flag: so if your IRQ handler doesn't clear the flag
by writing to the register then the IRQ will 'scream' (fire
continually) both on QEMU and on the real hardware. So I think
the data sheet is correct here, though it certainly has its share
of errors...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-10 Thread bzt 
Hi,

Okay, as you wish. My code works either way and on real hardware as
well, because I acknowledge the periodic IRQ as soon as possible, so
good for me.

Sign-off-by: Zoltán Baldaszti 
Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer
diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
index cfa5bc7365..82d2f51ffe 100644
--- a/hw/intc/bcm2836_control.c
+++ b/hw/intc/bcm2836_control.c
@@ -7,7 +7,13 @@
  * This code is licensed under the GNU GPLv2 and later.
  *
  * At present, only implements interrupt routing, and mailboxes (i.e.,
- * not local timer, PMU interrupt, or AXI counters).
+ * not PMU interrupt, or AXI counters).
+ *
+ * ARM Local Timer IRQ Copyright (c) 2019. Zoltán Baldaszti
+ * The IRQ_TIMER support is still very basic, does not provide timer counter
+ * access and other timer features, it just generates periodic IRQs. But it
+ * still requires not only the interrupt enable, but the timer enable bit to
+ * be set.
  *
  * Ref:
  * 
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
@@ -18,6 +24,9 @@
 #include "qemu/log.h"

 #define REG_GPU_ROUTE   0x0c
+#define REG_LOCALTIMERROUTING   0x24
+#define REG_LOCALTIMERCONTROL   0x34
+#define REG_LOCALTIMERACK   0x38
 #define REG_TIMERCONTROL0x40
 #define REG_MBOXCONTROL 0x50
 #define REG_IRQSRC  0x60
@@ -43,6 +52,13 @@
 #define IRQ_TIMER   11
 #define IRQ_MAX IRQ_TIMER

+#define LOCALTIMER_FREQ  3840
+#define LOCALTIMER_INTFLAG   (1 << 31)
+#define LOCALTIMER_RELOAD(1 << 30)
+#define LOCALTIMER_INTENABLE (1 << 29)
+#define LOCALTIMER_ENABLE(1 << 28)
+#define LOCALTIMER_VALUE(x)  ((x) & 0xfff)
+
 static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
   uint32_t controlreg, uint8_t controlidx)
 {
@@ -78,6 +94,17 @@ static void bcm2836_control_update(BCM2836ControlState *s)
 s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
 }

+/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
+if ((s->local_timer_control & LOCALTIMER_INTENABLE) &&
+(s->local_timer_control & LOCALTIMER_INTFLAG)) {
+/* note: this will keep firing the IRQ as Peter asked */
+if (s->route_localtimer & 4) {
+s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+} else {
+s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+}
+}
+
 for (i = 0; i < BCM2836_NCORES; i++) {
 /* handle local timer interrupts for this core */
 if (s->timerirqs[i]) {
@@ -162,6 +189,54 @@ static void bcm2836_control_set_gpu_fiq(void
*opaque, int irq, int level)
 bcm2836_control_update(s);
 }

+static void bcm2836_control_local_timer_set_next(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+uint64_t next_event;
+
+assert(LOCALTIMER_VALUE(s->local_timer_control) > 0);
+
+next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+muldiv64(LOCALTIMER_VALUE(s->local_timer_control),
+NANOSECONDS_PER_SECOND, LOCALTIMER_FREQ);
+timer_mod(>timer, next_event);
+}
+
+static void bcm2836_control_local_timer_tick(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+
+bcm2836_control_local_timer_set_next(s);
+
+s->local_timer_control |= LOCALTIMER_INTFLAG;
+bcm2836_control_update(s);
+}
+
+static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+s->local_timer_control = val;
+if (val & LOCALTIMER_ENABLE) {
+bcm2836_control_local_timer_set_next(s);
+} else {
+timer_del(>timer);
+}
+}
+
+static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+if (val & LOCALTIMER_INTFLAG) {
+s->local_timer_control &= ~LOCALTIMER_INTFLAG;
+}
+if ((val & LOCALTIMER_RELOAD) &&
+(s->local_timer_control & LOCALTIMER_ENABLE)) {
+bcm2836_control_local_timer_set_next(s);
+}
+}
+
 static uint64_t bcm2836_control_read(void *opaque, hwaddr offset,
unsigned size)
 {
 BCM2836ControlState *s = opaque;
@@ -170,6 +245,12 @@ static uint64_t bcm2836_control_read(void
*opaque, hwaddr offset, unsigned size)
 assert(s->route_gpu_fiq < BCM2836_NCORES
&& s->route_gpu_irq < BCM2836_NCORES);
 return ((uint32_t)s->route_gpu_fiq << 2) | s->route_gpu_irq;
+} else if (offset == REG_LOCALTIMERROUTING) {
+return s->route_localtimer;
+} else if (offset == REG_LOCALTIMERCONTROL) {
+return s->local_timer_control;
+} else if (offset == REG_LOCALTIMERACK) {
+return 0;
 } else if (offset >= REG_TIMERCONTROL && offset < REG_MBOXCONTROL) {
 return s->timercontrol[(offset - REG_TIMERCONTROL) >> 2];
 } else if (offset >= REG_MBOXCONTROL && offset < REG_IRQSRC) {
@@ -195,6 +276,12 @@ static void

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-09 Thread Peter Maydell 
On Sat, 9 Mar 2019 at 01:03, bzt  wrote:
> Thanks for your answers. If I don't clear the INTENABLE flag, then the
> IRQ would keep firing constantly. This is not how the real hardware
> works: it triggers the IRQ once, and then it inhibits. The timer won't
> trigger the IRQ again until you acknowledge it by writing the INTFLAG
> into the ack register. My solution emulates this behaviour. That's
> what the triggered flag was for in my original patch. Should I bring
> that flag back or is the current solution acceptable knowing this?

Huh. The QA7 spec doc is pretty clear that the IRQ is kept high
until the guest acknowledges it (and that is how in general
IRQ/FIQ works for Arm -- it is level triggered and it stays high
until the guest silences the device):
"An interrupt is generated as long as the interrupt flag is set
and the interrupt-enable bit is set" and "The user must clear the
interrupt flag".


thanks
-- PMM

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-08 Thread bzt 
Hi,

Thanks for your answers. If I don't clear the INTENABLE flag, then the
IRQ would keep firing constantly. This is not how the real hardware
works: it triggers the IRQ once, and then it inhibits. The timer won't
trigger the IRQ again until you acknowledge it by writing the INTFLAG
into the ack register. My solution emulates this behaviour. That's
what the triggered flag was for in my original patch. Should I bring
that flag back or is the current solution acceptable knowing this?

About keeping the INTFLAG on control write that's to avoid an instant
IRQ, but that's OK. I'll modify that.

Thanks,
bzt


On 3/7/19, Peter Maydell  wrote:
> On Thu, 7 Mar 2019 at 15:57, bzt  wrote:
>>
>> Nope. I meant the second patch, sent on 4th Mar, which had all the
>> things fixed you listed in your review.
>>
>> But here's the modification for your latest query. This one controls
>> the timer depending on ENABLE bit. It sets the INTFLAG even if
>> INTENABLE is not set, and only raises the IRQ if both are set.
>
> Thanks. I've listed a couple of minor things below
> which I think are not quite handling the flags right,
> but the rest looks good.
>
>> @@ -78,6 +94,17 @@ static void bcm2836_control_update(BCM2836ControlState
>> *s)
>>  s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
>>  }
>>
>> +/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
>> +if ((s->local_timer_control & LOCALTIMER_INTENABLE) &&
>> +(s->local_timer_control & LOCALTIMER_INTFLAG)) {
>> +if (s->route_localtimer & 4) {
>> +s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 <<
>> IRQ_TIMER;
>> +} else {
>> +s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 <<
>> IRQ_TIMER;
>> +}
>> +s->local_timer_control &= ~LOCALTIMER_INTENABLE;
>
> This shouldn't clear the INTENABLE flag.
>
>> +}
>> +
>>  for (i = 0; i < BCM2836_NCORES; i++) {
>>  /* handle local timer interrupts for this core */
>>  if (s->timerirqs[i]) {
>> @@ -162,6 +189,55 @@ static void bcm2836_control_set_gpu_fiq(void
>> *opaque, int irq, int level)
>>  bcm2836_control_update(s);
>>  }
>
>> +static void bcm2836_control_local_timer_control(void *opaque, uint32_t
>> val)
>> +{
>> +BCM2836ControlState *s = opaque;
>> +
>> +s->local_timer_control = val & ~LOCALTIMER_INTFLAG;
>
> This will clear the LOCALTIMER_INTFLAG if it was already
> set -- you want to preserve it, something like
>s->local_timer_control = (s->local_timer_control & LOCALTIMER_INTFLAG) |
>  (val & ~LOCALTIMER_INTFLAG);
>
>> +if (val & LOCALTIMER_ENABLE) {
>> +bcm2836_control_local_timer_set_next(s);
>> +} else {
>> +timer_del(>timer);
>> +}
>> +}
>> +
>> +static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
>> +{
>> +BCM2836ControlState *s = opaque;
>> +
>> +if (val & LOCALTIMER_INTFLAG) {
>> +s->local_timer_control |= LOCALTIMER_INTENABLE;
>> +s->local_timer_control &= ~LOCALTIMER_INTFLAG;
>
> This should just clear the INTFLAG, it doesn't affect INTENABLE.
>
>> +}
>> +if ((val & LOCALTIMER_RELOAD) &&
>> +(s->local_timer_control & LOCALTIMER_ENABLE)) {
>> +bcm2836_control_local_timer_set_next(s);
>> +}
>> +}
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-07 Thread Peter Maydell 
On Thu, 7 Mar 2019 at 15:57, bzt  wrote:
>
> Nope. I meant the second patch, sent on 4th Mar, which had all the
> things fixed you listed in your review.
>
> But here's the modification for your latest query. This one controls
> the timer depending on ENABLE bit. It sets the INTFLAG even if
> INTENABLE is not set, and only raises the IRQ if both are set.

Thanks. I've listed a couple of minor things below
which I think are not quite handling the flags right,
but the rest looks good.

> @@ -78,6 +94,17 @@ static void bcm2836_control_update(BCM2836ControlState *s)
>  s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
>  }
>
> +/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
> +if ((s->local_timer_control & LOCALTIMER_INTENABLE) &&
> +(s->local_timer_control & LOCALTIMER_INTFLAG)) {
> +if (s->route_localtimer & 4) {
> +s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
> +} else {
> +s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
> +}
> +s->local_timer_control &= ~LOCALTIMER_INTENABLE;

This shouldn't clear the INTENABLE flag.

> +}
> +
>  for (i = 0; i < BCM2836_NCORES; i++) {
>  /* handle local timer interrupts for this core */
>  if (s->timerirqs[i]) {
> @@ -162,6 +189,55 @@ static void bcm2836_control_set_gpu_fiq(void
> *opaque, int irq, int level)
>  bcm2836_control_update(s);
>  }

> +static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
> +{
> +BCM2836ControlState *s = opaque;
> +
> +s->local_timer_control = val & ~LOCALTIMER_INTFLAG;

This will clear the LOCALTIMER_INTFLAG if it was already
set -- you want to preserve it, something like
   s->local_timer_control = (s->local_timer_control & LOCALTIMER_INTFLAG) |
 (val & ~LOCALTIMER_INTFLAG);

> +if (val & LOCALTIMER_ENABLE) {
> +bcm2836_control_local_timer_set_next(s);
> +} else {
> +timer_del(>timer);
> +}
> +}
> +
> +static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
> +{
> +BCM2836ControlState *s = opaque;
> +
> +if (val & LOCALTIMER_INTFLAG) {
> +s->local_timer_control |= LOCALTIMER_INTENABLE;
> +s->local_timer_control &= ~LOCALTIMER_INTFLAG;

This should just clear the INTFLAG, it doesn't affect INTENABLE.

> +}
> +if ((val & LOCALTIMER_RELOAD) &&
> +(s->local_timer_control & LOCALTIMER_ENABLE)) {
> +bcm2836_control_local_timer_set_next(s);
> +}
> +}

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-07 Thread Peter Maydell 
On Thu, 7 Mar 2019 at 15:27, bzt  wrote:
> Yes, could be. The QA7 spec is not really detailed, and calling both
> timers simply ARM timers can be misleading. But it's not relevant
> anyway from the IRQ's point of view. My latest patch checks both bits
> to be set to generate the IRQ, so it does not really matter. As I've
> said, this patch adds only the periodic IRQ, and not a full timer
> emulation.
>
> Do you want any modifications on the patch? If so, what exactly? Let
> me know and I'll update it.

I assume by "latest patch" you mean a planned v3
that you haven't sent yet?

I think it's probably best to go with my interpretation of
the specs, if you think it makes sense:
 * running and stopping the timer is controlled by the
   "timer enable" bit (and doesn't care about the
   "interrupt enable" bit)
 * the timer timing out always sets the "interrupt flag" bit
 * we set the destination core IRQ/FIQ if the "interrupt flag"
   and "interrupt enable" bits are both set (we don't care
   about whether the "timer enable" bit is set for this)

That should be only very minor changes from what you have now.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-07 Thread bzt 
Nope. I meant the second patch, sent on 4th Mar, which had all the
things fixed you listed in your review.

But here's the modification for your latest query. This one controls
the timer depending on ENABLE bit. It sets the INTFLAG even if
INTENABLE is not set, and only raises the IRQ if both are set.

Sign-off-by: Zoltán Baldaszti 
Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer
diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
index cfa5bc7365..d75dedbf20 100644
--- a/hw/intc/bcm2836_control.c
+++ b/hw/intc/bcm2836_control.c
@@ -7,7 +7,13 @@
  * This code is licensed under the GNU GPLv2 and later.
  *
  * At present, only implements interrupt routing, and mailboxes (i.e.,
- * not local timer, PMU interrupt, or AXI counters).
+ * not PMU interrupt, or AXI counters).
+ *
+ * ARM Local Timer IRQ Copyright (c) 2019. Zoltán Baldaszti
+ * The IRQ_TIMER support is still very basic, does not provide timer counter
+ * access and other timer features, it just generates periodic IRQs. But it
+ * still requires not only the interrupt enable, but the timer enable bit to
+ * be set.
  *
  * Ref:
  * 
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
@@ -18,6 +24,9 @@
 #include "qemu/log.h"

 #define REG_GPU_ROUTE   0x0c
+#define REG_LOCALTIMERROUTING   0x24
+#define REG_LOCALTIMERCONTROL   0x34
+#define REG_LOCALTIMERACK   0x38
 #define REG_TIMERCONTROL0x40
 #define REG_MBOXCONTROL 0x50
 #define REG_IRQSRC  0x60
@@ -43,6 +52,13 @@
 #define IRQ_TIMER   11
 #define IRQ_MAX IRQ_TIMER

+#define LOCALTIMER_FREQ  3840
+#define LOCALTIMER_INTFLAG   (1 << 31)
+#define LOCALTIMER_RELOAD(1 << 30)
+#define LOCALTIMER_INTENABLE (1 << 29)
+#define LOCALTIMER_ENABLE(1 << 28)
+#define LOCALTIMER_VALUE(x)  ((x) & 0xfff)
+
 static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
   uint32_t controlreg, uint8_t controlidx)
 {
@@ -78,6 +94,17 @@ static void bcm2836_control_update(BCM2836ControlState *s)
 s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
 }

+/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
+if ((s->local_timer_control & LOCALTIMER_INTENABLE) &&
+(s->local_timer_control & LOCALTIMER_INTFLAG)) {
+if (s->route_localtimer & 4) {
+s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+} else {
+s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+}
+s->local_timer_control &= ~LOCALTIMER_INTENABLE;
+}
+
 for (i = 0; i < BCM2836_NCORES; i++) {
 /* handle local timer interrupts for this core */
 if (s->timerirqs[i]) {
@@ -162,6 +189,55 @@ static void bcm2836_control_set_gpu_fiq(void
*opaque, int irq, int level)
 bcm2836_control_update(s);
 }

+static void bcm2836_control_local_timer_set_next(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+uint64_t next_event;
+
+assert(LOCALTIMER_VALUE(s->local_timer_control) > 0);
+
+next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+muldiv64(LOCALTIMER_VALUE(s->local_timer_control),
+NANOSECONDS_PER_SECOND, LOCALTIMER_FREQ);
+timer_mod(>timer, next_event);
+}
+
+static void bcm2836_control_local_timer_tick(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+
+bcm2836_control_local_timer_set_next(s);
+
+s->local_timer_control |= LOCALTIMER_INTFLAG;
+bcm2836_control_update(s);
+}
+
+static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+s->local_timer_control = val & ~LOCALTIMER_INTFLAG;
+if (val & LOCALTIMER_ENABLE) {
+bcm2836_control_local_timer_set_next(s);
+} else {
+timer_del(>timer);
+}
+}
+
+static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+if (val & LOCALTIMER_INTFLAG) {
+s->local_timer_control |= LOCALTIMER_INTENABLE;
+s->local_timer_control &= ~LOCALTIMER_INTFLAG;
+}
+if ((val & LOCALTIMER_RELOAD) &&
+(s->local_timer_control & LOCALTIMER_ENABLE)) {
+bcm2836_control_local_timer_set_next(s);
+}
+}
+
 static uint64_t bcm2836_control_read(void *opaque, hwaddr offset,
unsigned size)
 {
 BCM2836ControlState *s = opaque;
@@ -170,6 +246,12 @@ static uint64_t bcm2836_control_read(void
*opaque, hwaddr offset, unsigned size)
 assert(s->route_gpu_fiq < BCM2836_NCORES
&& s->route_gpu_irq < BCM2836_NCORES);
 return ((uint32_t)s->route_gpu_fiq << 2) | s->route_gpu_irq;
+} else if (offset == REG_LOCALTIMERROUTING) {
+return s->route_localtimer;
+} else if (offset == REG_LOCALTIMERCONTROL) {
+return s->local_timer_control;
+} else if (offset == REG_LOCALTIMERACK) {
+return 0;
 } else if (offset >=

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-07 Thread bzt 
Hi,

Yes, could be. The QA7 spec is not really detailed, and calling both
timers simply ARM timers can be misleading. But it's not relevant
anyway from the IRQ's point of view. My latest patch checks both bits
to be set to generate the IRQ, so it does not really matter. As I've
said, this patch adds only the periodic IRQ, and not a full timer
emulation.

Do you want any modifications on the patch? If so, what exactly? Let
me know and I'll update it.

Cheers,
bzt

On 3/5/19, Peter Maydell  wrote:
> On Mon, 4 Mar 2019 at 19:27, bzt  wrote:
>>
>> On 3/4/19, Peter Maydell  wrote:
>> >> + * The IRQ_TIMER support is still very basic, does not handle timer
>> >> access,
>> >> + * and such there's no point in enabling it without the interrupt
>> >> flag
>> >> set.
>> >
>> > Can you be more precise about what's missing here? I didn't
>>
>> The local timer (as per the referenced QA7 doc calls it, section
>> 4.11), means 3 registers in the ARM Control MMIO, at addresses 0x24,
>> 0x34, 0x38. Please note that I haven't named IRQ_TIMER bit in the
>> irqsrc and fiqsrc fields, that was already defined. This patch
>> implements that one.
>>
>> > see anything in the spec that looked like a register for
>> > reading the current timer value (though it would certainly
>> > be usual to provide one).
>>
>> Those are registers 0x1C and 0x20, called "Core timer access LS/MS",
>> section 4.3. I'll make the comment more clear. Those are not local
>> timer IRQ related, because they could use a different frequency than
>> the IRQ, just happen to be enabled with a bit in the local timer's
>> control register (and not in the core timer control register at 0x0),
>> and on real hardware triggering an IRQ requires both the timer enable
>> and interrupt enable bits to be set. The timer counter is a 64 bits
>> one, while the IRQ's counter is only 28 bit wide, which cannot be
>> directly read, does not use the prescaler, and it's reload value
>> stored in the local timer control register itself (unusal, but that's
>> how it is).
>
> OK, so that's just an entirely separate timer whose control bit
> happens to be in the same register? I'd assumed that the enable
> bit here was so you could have the local timer be running but
> not generate interrupts -- in which case when it expired it would
> set the "interrupt flag" (and reload), but it wouldn't assert
> the external interrupt line.
>
> In fact thinking about it that does seem like the more plausible
> reading of that specification:
>  * bit 28 (Timer enable) enables the timer
>  * when it gets to zero we set bit 31 "Interrupt flag"
>  * the outbound interrupt line is the logical OR of
>bits 31 ('interrupt flag') and 29 ('interrupt enable')
>
> Are you sure it doesn't work that way ?
>
> I'd have thought that any enable bit for the "core timer" would
> be in the core timer control register at 0x4000_. Since the
> "core timer" is (apparently) the source of the clock for all the
> per-CPU architected generic timers, it seems more likely that
> it just runs constantly without an enable bit.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-05 Thread Peter Maydell 
On Mon, 4 Mar 2019 at 19:27, bzt  wrote:
>
> On 3/4/19, Peter Maydell  wrote:
> >> + * The IRQ_TIMER support is still very basic, does not handle timer
> >> access,
> >> + * and such there's no point in enabling it without the interrupt flag
> >> set.
> >
> > Can you be more precise about what's missing here? I didn't
>
> The local timer (as per the referenced QA7 doc calls it, section
> 4.11), means 3 registers in the ARM Control MMIO, at addresses 0x24,
> 0x34, 0x38. Please note that I haven't named IRQ_TIMER bit in the
> irqsrc and fiqsrc fields, that was already defined. This patch
> implements that one.
>
> > see anything in the spec that looked like a register for
> > reading the current timer value (though it would certainly
> > be usual to provide one).
>
> Those are registers 0x1C and 0x20, called "Core timer access LS/MS",
> section 4.3. I'll make the comment more clear. Those are not local
> timer IRQ related, because they could use a different frequency than
> the IRQ, just happen to be enabled with a bit in the local timer's
> control register (and not in the core timer control register at 0x0),
> and on real hardware triggering an IRQ requires both the timer enable
> and interrupt enable bits to be set. The timer counter is a 64 bits
> one, while the IRQ's counter is only 28 bit wide, which cannot be
> directly read, does not use the prescaler, and it's reload value
> stored in the local timer control register itself (unusal, but that's
> how it is).

OK, so that's just an entirely separate timer whose control bit
happens to be in the same register? I'd assumed that the enable
bit here was so you could have the local timer be running but
not generate interrupts -- in which case when it expired it would
set the "interrupt flag" (and reload), but it wouldn't assert
the external interrupt line.

In fact thinking about it that does seem like the more plausible
reading of that specification:
 * bit 28 (Timer enable) enables the timer
 * when it gets to zero we set bit 31 "Interrupt flag"
 * the outbound interrupt line is the logical OR of
   bits 31 ('interrupt flag') and 29 ('interrupt enable')

Are you sure it doesn't work that way ?

I'd have thought that any enable bit for the "core timer" would
be in the core timer control register at 0x4000_. Since the
"core timer" is (apparently) the source of the clock for all the
per-CPU architected generic timers, it seems more likely that
it just runs constantly without an enable bit.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-04 Thread bzt 
Hi Peter,

Here's the modified patch. I've changed the comment, I hope now it
makes clear that dispite this patch handles the timer enable bit
(which is required for the interrupt), it only adds the periodic IRQ
feature, and not the full timer functionality.

Otherwise I've modified everything you asked for.
bzt

Sign-off-by: Zoltán Baldaszti 
Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer
diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
index cfa5bc7365..2157099b31 100644
--- a/hw/intc/bcm2836_control.c
+++ b/hw/intc/bcm2836_control.c
@@ -7,7 +7,13 @@
  * This code is licensed under the GNU GPLv2 and later.
  *
  * At present, only implements interrupt routing, and mailboxes (i.e.,
- * not local timer, PMU interrupt, or AXI counters).
+ * not PMU interrupt, or AXI counters).
+ *
+ * ARM Local Timer IRQ Copyright (c) 2019. Zoltán Baldaszti
+ * The IRQ_TIMER support is still very basic, does not provide timer counter
+ * access and other timer features, it just generates periodic IRQs. But it
+ * still requires not only the interrupt enable, but the timer enable bit to
+ * be set.
  *
  * Ref:
  * 
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
@@ -18,6 +24,9 @@
 #include "qemu/log.h"

 #define REG_GPU_ROUTE   0x0c
+#define REG_LOCALTIMERROUTING   0x24
+#define REG_LOCALTIMERCONTROL   0x34
+#define REG_LOCALTIMERACK   0x38
 #define REG_TIMERCONTROL0x40
 #define REG_MBOXCONTROL 0x50
 #define REG_IRQSRC  0x60
@@ -43,6 +52,13 @@
 #define IRQ_TIMER   11
 #define IRQ_MAX IRQ_TIMER

+#define LOCALTIMER_FREQ  3840
+#define LOCALTIMER_INTFLAG   (1 << 31)
+#define LOCALTIMER_RELOAD(1 << 30)
+#define LOCALTIMER_INTENABLE (1 << 29)
+#define LOCALTIMER_ENABLE(1 << 28)
+#define LOCALTIMER_VALUE(x)  ((x) & 0xfff)
+
 static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
   uint32_t controlreg, uint8_t controlidx)
 {
@@ -78,6 +94,15 @@ static void bcm2836_control_update(BCM2836ControlState *s)
 s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
 }

+/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
+if (s->local_timer_control & LOCALTIMER_INTFLAG) {
+if (s->route_localtimer & 4) {
+s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+} else {
+s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+}
+}
+
 for (i = 0; i < BCM2836_NCORES; i++) {
 /* handle local timer interrupts for this core */
 if (s->timerirqs[i]) {
@@ -162,6 +187,58 @@ static void bcm2836_control_set_gpu_fiq(void
*opaque, int irq, int level)
 bcm2836_control_update(s);
 }

+static void bcm2836_control_local_timer_set_next(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+uint64_t next_event;
+
+assert(LOCALTIMER_VALUE(s->local_timer_control) > 0);
+
+next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+muldiv64(LOCALTIMER_VALUE(s->local_timer_control),
+NANOSECONDS_PER_SECOND, LOCALTIMER_FREQ);
+timer_mod(>timer, next_event);
+}
+
+static void bcm2836_control_local_timer_tick(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+
+bcm2836_control_local_timer_set_next(s);
+
+if (!(s->local_timer_control & LOCALTIMER_INTFLAG)) {
+s->local_timer_control |= LOCALTIMER_INTFLAG;
+bcm2836_control_update(s);
+}
+}
+
+static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+s->local_timer_control = val;
+if ((val & LOCALTIMER_ENABLE) && (val & LOCALTIMER_INTENABLE)) {
+bcm2836_control_local_timer_set_next(s);
+} else {
+timer_del(>timer);
+s->local_timer_control &= ~LOCALTIMER_INTFLAG;
+}
+}
+
+static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+if (val & LOCALTIMER_INTFLAG) {
+s->local_timer_control &= ~LOCALTIMER_INTFLAG;
+}
+if ((val & LOCALTIMER_RELOAD) &&
+(s->local_timer_control & LOCALTIMER_ENABLE) &&
+(s->local_timer_control & LOCALTIMER_INTENABLE)) {
+bcm2836_control_local_timer_set_next(s);
+}
+}
+
 static uint64_t bcm2836_control_read(void *opaque, hwaddr offset,
unsigned size)
 {
 BCM2836ControlState *s = opaque;
@@ -170,6 +247,12 @@ static uint64_t bcm2836_control_read(void
*opaque, hwaddr offset, unsigned size)
 assert(s->route_gpu_fiq < BCM2836_NCORES
&& s->route_gpu_irq < BCM2836_NCORES);
 return ((uint32_t)s->route_gpu_fiq << 2) | s->route_gpu_irq;
+} else if (offset == REG_LOCALTIMERROUTING) {
+return s->route_localtimer;
+} else if (offset == REG_LOCALTIMERCONTROL) {
+return s->local_timer_control;
+} else if (offset == REG_LOCALTIMERACK) {
+

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-04 Thread bzt 
Hi,

Thanks for the review! Most of it makes sense, and I'll modify the
patch accordingly. There are few things though, mostly related to
emulating this unique timer.

On 3/4/19, Peter Maydell  wrote:
> OK, here are my substantive review comments.
>
>> diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
>> index cfa5bc7365..fbd31de0f1 100644
>> --- a/hw/intc/bcm2836_control.c
>> +++ b/hw/intc/bcm2836_control.c
>> @@ -7,7 +7,11 @@
>>   * This code is licensed under the GNU GPLv2 and later.
>>   *
>>   * At present, only implements interrupt routing, and mailboxes (i.e.,
>> - * not local timer, PMU interrupt, or AXI counters).
>> + * not PMU interrupt, or AXI counters).
>> + *
>> + * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti
>> + * The IRQ_TIMER support is still very basic, does not handle timer
>> access,
>> + * and such there's no point in enabling it without the interrupt flag
>> set.
>
> Can you be more precise about what's missing here? I didn't

The local timer (as per the referenced QA7 doc calls it, section
4.11), means 3 registers in the ARM Control MMIO, at addresses 0x24,
0x34, 0x38. Please note that I haven't named IRQ_TIMER bit in the
irqsrc and fiqsrc fields, that was already defined. This patch
implements that one.

> see anything in the spec that looked like a register for
> reading the current timer value (though it would certainly
> be usual to provide one).

Those are registers 0x1C and 0x20, called "Core timer access LS/MS",
section 4.3. I'll make the comment more clear. Those are not local
timer IRQ related, because they could use a different frequency than
the IRQ, just happen to be enabled with a bit in the local timer's
control register (and not in the core timer control register at 0x0),
and on real hardware triggering an IRQ requires both the timer enable
and interrupt enable bits to be set. The timer counter is a 64 bits
one, while the IRQ's counter is only 28 bit wide, which cannot be
directly read, does not use the prescaler, and it's reload value
stored in the local timer control register itself (unusal, but that's
how it is).

As I've said, this patch only provides the IRQ, and not the timer
part. That's a whole different story, specially beacuse of the
counters (calculating correct value for the selected clocksource
(crystal/APB), using divisor and prescaler value, the 32 bit register
locking mechanism etc.), that's for another patch.

On a real hardware the interrupt has to be implemented using the
timer. On qemu, we don't need to emulate the whole timer circuit and
counter, we can use QEMUTimer to calculate when to trigger a periodic
IRQ (which is always at fixed 38.4Mhz not using any divisor or
prescaler). But I think regadless we should emulate the control
register in that timer enable bit should be set when we generate local
timer IRQs. I hope this makes sense to you.

>
>>   *
>>   * Ref:
>>   *
>> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
>> @@ -18,6 +22,9 @@
>>  #include "qemu/log.h"
>>
>>  #define REG_GPU_ROUTE   0x0c
>> +#define REG_LOCALTIMERROUTING   0x24
>> +#define REG_LOCALTIMERCONTROL   0x34
>> +#define REG_LOCALTIMERACK   0x38
>>  #define REG_TIMERCONTROL0x40
>>  #define REG_MBOXCONTROL 0x50
>>  #define REG_IRQSRC  0x60
>> @@ -43,6 +50,13 @@
>>  #define IRQ_TIMER   11
>>  #define IRQ_MAX IRQ_TIMER
>>
>> +#define LOCALTIMER_FREQ  3840
>> +#define LOCALTIMER_INTFLAG   (1 << 31)
>> +#define LOCALTIMER_RELOAD(1 << 30)
>> +#define LOCALTIMER_INTENABLE (1 << 29)
>> +#define LOCALTIMER_ENABLE(1 << 28)
>> +#define LOCALTIMER_VALUE(x)  ((x) & 0xfff)
>> +
>>  static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t
>> irq,
>>uint32_t controlreg, uint8_t controlidx)
>>  {
>> @@ -78,6 +92,15 @@ static void bcm2836_control_update(BCM2836ControlState
>> *s)
>>  s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
>>  }
>>
>> +/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
>> +if (s->triggered) {
>> +if (s->route_localtimer & 4) {
>> +s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 <<
>> IRQ_TIMER;
>> +} else {
>> +s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 <<
>> IRQ_TIMER;
>> +}
>> +}
>> +
>>  for (i = 0; i < BCM2836_NCORES; i++) {
>>  /* handle local timer interrupts for this core */
>>  if (s->timerirqs[i]) {
>> @@ -162,6 +185,62 @@ static void bcm2836_control_set_gpu_fiq(void
>> *opaque, int irq, int level)
>>  bcm2836_control_update(s);
>>  }
>>
>> +static void bcm2836_control_local_timer_set_next(void *opaque)
>> +{
>> +BCM2836ControlState *s = opaque;
>> +uint64_t next_event;
>> +
>> +assert(s->period > 0);
>> +
>> +next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>> +(NANOSECONDS_PER_SECOND * s->period /

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-03-04 Thread Peter Maydell 
On Tue, 26 Feb 2019 at 11:38, bzt  wrote:
>
> Dear qemu developers,
>
> Honestly SubmitAPatch is a bit complicated for me, so I'm hoping I've
> done everything right. To be sure, you can find my patch here:
> https://github.com/bztsrc/qemu-local-timer and diff against the latest
> github repo here:
> https://github.com/bztsrc/qemu-local-timer/blob/patches/3.1.50.diff
>
> Currently the IRQ_TIMER in bcm2836_control is defined, but not
> implemented. This patch adds the basic functionality to qemu by
> implementing 3 new registers in bcm2836_control. You can route the
> interrupt to one of the cores' IRQ or FIQ line by writing 0x4024,
> and you can set up a periodic interrupt with 38.4MHz frequency by
> writing the divider into 0x4034. Prescaler are not taken into
> account. When the IRQ fired, you'll see it in 0x4040+4*N bit 11
> with the rest of the local IRQs, and you can acknowledge it by writing
> 1<<31 to 0x4038.
>
> The patch is pretty simple, should be easy to review: it does not
> create new classes, does not delete anything, does not change class
> interface, and only two files modified, the bcm2836_control.c and it's
> header.
>
> Sign-off-by: Zoltán Baldaszti 
> Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer

OK, here are my substantive review comments.

> diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
> index cfa5bc7365..fbd31de0f1 100644
> --- a/hw/intc/bcm2836_control.c
> +++ b/hw/intc/bcm2836_control.c
> @@ -7,7 +7,11 @@
>   * This code is licensed under the GNU GPLv2 and later.
>   *
>   * At present, only implements interrupt routing, and mailboxes (i.e.,
> - * not local timer, PMU interrupt, or AXI counters).
> + * not PMU interrupt, or AXI counters).
> + *
> + * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti
> + * The IRQ_TIMER support is still very basic, does not handle timer access,
> + * and such there's no point in enabling it without the interrupt flag set.

Can you be more precise about what's missing here? I didn't
see anything in the spec that looked like a register for
reading the current timer value (though it would certainly
be usual to provide one).

>   *
>   * Ref:
>   * 
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
> @@ -18,6 +22,9 @@
>  #include "qemu/log.h"
>
>  #define REG_GPU_ROUTE   0x0c
> +#define REG_LOCALTIMERROUTING   0x24
> +#define REG_LOCALTIMERCONTROL   0x34
> +#define REG_LOCALTIMERACK   0x38
>  #define REG_TIMERCONTROL0x40
>  #define REG_MBOXCONTROL 0x50
>  #define REG_IRQSRC  0x60
> @@ -43,6 +50,13 @@
>  #define IRQ_TIMER   11
>  #define IRQ_MAX IRQ_TIMER
>
> +#define LOCALTIMER_FREQ  3840
> +#define LOCALTIMER_INTFLAG   (1 << 31)
> +#define LOCALTIMER_RELOAD(1 << 30)
> +#define LOCALTIMER_INTENABLE (1 << 29)
> +#define LOCALTIMER_ENABLE(1 << 28)
> +#define LOCALTIMER_VALUE(x)  ((x) & 0xfff)
> +
>  static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
>uint32_t controlreg, uint8_t controlidx)
>  {
> @@ -78,6 +92,15 @@ static void bcm2836_control_update(BCM2836ControlState *s)
>  s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
>  }
>
> +/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
> +if (s->triggered) {
> +if (s->route_localtimer & 4) {
> +s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
> +} else {
> +s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
> +}
> +}
> +
>  for (i = 0; i < BCM2836_NCORES; i++) {
>  /* handle local timer interrupts for this core */
>  if (s->timerirqs[i]) {
> @@ -162,6 +185,62 @@ static void bcm2836_control_set_gpu_fiq(void
> *opaque, int irq, int level)
>  bcm2836_control_update(s);
>  }
>
> +static void bcm2836_control_local_timer_set_next(void *opaque)
> +{
> +BCM2836ControlState *s = opaque;
> +uint64_t next_event;
> +
> +assert(s->period > 0);
> +
> +next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +(NANOSECONDS_PER_SECOND * s->period / LOCALTIMER_FREQ);

This sort of X * Y / Z calculation for timers should
be done with muldiv64() which uses a larger intermediate
result to avoid overflow problems:

   next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
  muldiv64(s->period, NANOSECONDS_PER_SECOND, LOCALTIMER_FREQ);

> +timer_mod(s->timer, next_event);
> +}
> +
> +static void bcm2836_control_local_timer_tick(void *opaque)
> +{
> +BCM2836ControlState *s = opaque;
> +
> +bcm2836_control_local_timer_set_next(s);
> +
> +if (!s->triggered) {
> +s->triggered = 1;
> +bcm2836_control_update(s);
> +}
> +}
> +
> +static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
> +{
> +BCM2836ControlState *s = opaque;
> +
> +s->period =

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-02-28 Thread bzt 
Hi Andrew,

That's great news, I'll then bring those drivers up to the modern qemu API!

Maybe that's a Linux kernel module configuration issue as well, but
with the BCM System Timer small delays depend on polling the counter.
Without the qemu support that counter register remains zero, causing
an endless loop. TBH it was years ago when I last checked raspbian,
but I know for sure that many bare metal applications and libraries
(like https://github.com/rsta2/circle) fails to boot because of this,
and the community is very excited to have the BCM ST emulation in
qemu. The PM thing is mostly for me, I'd like to reboot and shut down
the vm gracefully from the guest without the semihosting hack :-)

Thank you again, I'll get to work!

Best wishes,
bzt


On 2/27/19, Andrew Baumann  wrote:
>> From: bzt 
>> Sent: Wednesday, 27 February 2019 03:54
>>
>> I'd like to add more drivers for the bcm283[567] too, and this question
>> goes to
>> Andrew Baumann mostly. I've seen implemented those missing drivers in his
>> repo, which aren't in the qemu mainline yet. I don't want to reinvent the
>> wheel,
>> so I'm willing to fix those classes to get them right, but I'd like to
>> know what's
>> wrong with them in the first place.
>
> Nothing's wrong with them per se, but when I was working on upstreaming the
> raspi board I prioritised the device support needed to boot Linux and
> Windows on raspi2. The remaining devices were always planned for eventual
> inclusion, but just fell off the end of my TODO list. They never went
> through code review on qemu-devel, so would probably need some work to bring
> them up to modern qemu APIs and standards.
>
>> I'm interested in fixing the
>> TYPE_BCM2835_POWER and TYPE_BCM2835_ST drivers, which I know many
>> hoppy OS developers lack and would improve the raspi emulation experience
>> remarkably.
>
> Sounds good to me!
>
>> It would be still a long way to boot a raspbian image, but still, a
>> small step towards that goal at least.
>
> That's interesting. Raspbian did seem to be working (on pi2) when I last
> worked on this. Perhaps it now depends on these devices, but didn't before?
>
> Cheers,
> Andrew
>

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-02-27 Thread Andrew Baumann via Qemu-devel 
> From: bzt 
> Sent: Wednesday, 27 February 2019 03:54
> 
> I'd like to add more drivers for the bcm283[567] too, and this question goes 
> to
> Andrew Baumann mostly. I've seen implemented those missing drivers in his
> repo, which aren't in the qemu mainline yet. I don't want to reinvent the 
> wheel,
> so I'm willing to fix those classes to get them right, but I'd like to know 
> what's
> wrong with them in the first place.

Nothing's wrong with them per se, but when I was working on upstreaming the 
raspi board I prioritised the device support needed to boot Linux and Windows 
on raspi2. The remaining devices were always planned for eventual inclusion, 
but just fell off the end of my TODO list. They never went through code review 
on qemu-devel, so would probably need some work to bring them up to modern qemu 
APIs and standards.

> I'm interested in fixing the
> TYPE_BCM2835_POWER and TYPE_BCM2835_ST drivers, which I know many
> hoppy OS developers lack and would improve the raspi emulation experience
> remarkably.

Sounds good to me! 

> It would be still a long way to boot a raspbian image, but still, a
> small step towards that goal at least.

That's interesting. Raspbian did seem to be working (on pi2) when I last worked 
on this. Perhaps it now depends on these devices, but didn't before?

Cheers,
Andrew

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-02-27 Thread bzt 
Hi Peter,

Thank you very much! Let me know if I need to modify anything! Btw you
should update the maintainer's list, currently it does not list you as
a reviewer :-)

I'd like to add more drivers for the bcm283[567] too, and this
question goes to Andrew Baumann mostly. I've seen implemented those
missing drivers in his repo, which aren't in the qemu mainline yet. I
don't want to reinvent the wheel, so I'm willing to fix those classes
to get them right, but I'd like to know what's wrong with them in the
first place. I'm interested in fixing the TYPE_BCM2835_POWER and
TYPE_BCM2835_ST drivers, which I know many hoppy OS developers lack
and would improve the raspi emulation experience remarkably. It would
be still a long way to boot a raspbian image, but still, a small step
towards that goal at least.

Thanks,
bzt

On 2/26/19, Peter Maydell  wrote:
> On Tue, 26 Feb 2019 at 11:38, bzt  wrote:
>>
>> Dear qemu developers,
>>
>> Honestly SubmitAPatch is a bit complicated for me, so I'm hoping I've
>> done everything right. To be sure, you can find my patch here:
>> https://github.com/bztsrc/qemu-local-timer and diff against the latest
>> github repo here:
>> https://github.com/bztsrc/qemu-local-timer/blob/patches/3.1.50.diff
>
> Thanks for the patch. You've got the most important things
> right -- patch sent as an email, and with a Signed-off-by:
> line from you. I've put this on my to-review queue, and I
> should be able to get to it within a week or so.
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-02-26 Thread Peter Maydell 
On Tue, 26 Feb 2019 at 11:38, bzt  wrote:
>
> Dear qemu developers,
>
> Honestly SubmitAPatch is a bit complicated for me, so I'm hoping I've
> done everything right. To be sure, you can find my patch here:
> https://github.com/bztsrc/qemu-local-timer and diff against the latest
> github repo here:
> https://github.com/bztsrc/qemu-local-timer/blob/patches/3.1.50.diff

Thanks for the patch. You've got the most important things
right -- patch sent as an email, and with a Signed-off-by:
line from you. I've put this on my to-review queue, and I
should be able to get to it within a week or so.

thanks
-- PMM

[Qemu-devel] [PATCH] Added periodic IRQ support for bcm2836_control local timer

2019-02-26 Thread bzt 
Dear qemu developers,

Honestly SubmitAPatch is a bit complicated for me, so I'm hoping I've
done everything right. To be sure, you can find my patch here:
https://github.com/bztsrc/qemu-local-timer and diff against the latest
github repo here:
https://github.com/bztsrc/qemu-local-timer/blob/patches/3.1.50.diff

Currently the IRQ_TIMER in bcm2836_control is defined, but not
implemented. This patch adds the basic functionality to qemu by
implementing 3 new registers in bcm2836_control. You can route the
interrupt to one of the cores' IRQ or FIQ line by writing 0x4024,
and you can set up a periodic interrupt with 38.4MHz frequency by
writing the divider into 0x4034. Prescaler are not taken into
account. When the IRQ fired, you'll see it in 0x4040+4*N bit 11
with the rest of the local IRQs, and you can acknowledge it by writing
1<<31 to 0x4038.

The patch is pretty simple, should be easy to review: it does not
create new classes, does not delete anything, does not change class
interface, and only two files modified, the bcm2836_control.c and it's
header.

Sign-off-by: Zoltán Baldaszti 
Subject: [PATCH] Added periodic IRQ support for bcm2836_control local timer

diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
index cfa5bc7365..fbd31de0f1 100644
--- a/hw/intc/bcm2836_control.c
+++ b/hw/intc/bcm2836_control.c
@@ -7,7 +7,11 @@
  * This code is licensed under the GNU GPLv2 and later.
  *
  * At present, only implements interrupt routing, and mailboxes (i.e.,
- * not local timer, PMU interrupt, or AXI counters).
+ * not PMU interrupt, or AXI counters).
+ *
+ * 64-bit ARM Local Timer Copyright (c) 2019. Zoltán Baldaszti
+ * The IRQ_TIMER support is still very basic, does not handle timer access,
+ * and such there's no point in enabling it without the interrupt flag set.
  *
  * Ref:
  * 
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
@@ -18,6 +22,9 @@
 #include "qemu/log.h"

 #define REG_GPU_ROUTE   0x0c
+#define REG_LOCALTIMERROUTING   0x24
+#define REG_LOCALTIMERCONTROL   0x34
+#define REG_LOCALTIMERACK   0x38
 #define REG_TIMERCONTROL0x40
 #define REG_MBOXCONTROL 0x50
 #define REG_IRQSRC  0x60
@@ -43,6 +50,13 @@
 #define IRQ_TIMER   11
 #define IRQ_MAX IRQ_TIMER

+#define LOCALTIMER_FREQ  3840
+#define LOCALTIMER_INTFLAG   (1 << 31)
+#define LOCALTIMER_RELOAD(1 << 30)
+#define LOCALTIMER_INTENABLE (1 << 29)
+#define LOCALTIMER_ENABLE(1 << 28)
+#define LOCALTIMER_VALUE(x)  ((x) & 0xfff)
+
 static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
   uint32_t controlreg, uint8_t controlidx)
 {
@@ -78,6 +92,15 @@ static void bcm2836_control_update(BCM2836ControlState *s)
 s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
 }

+/* handle THE local timer interrupt for one of the cores' IRQ/FIQ */
+if (s->triggered) {
+if (s->route_localtimer & 4) {
+s->fiqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+} else {
+s->irqsrc[(s->route_localtimer & 3)] |= (uint32_t)1 << IRQ_TIMER;
+}
+}
+
 for (i = 0; i < BCM2836_NCORES; i++) {
 /* handle local timer interrupts for this core */
 if (s->timerirqs[i]) {
@@ -162,6 +185,62 @@ static void bcm2836_control_set_gpu_fiq(void
*opaque, int irq, int level)
 bcm2836_control_update(s);
 }

+static void bcm2836_control_local_timer_set_next(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+uint64_t next_event;
+
+assert(s->period > 0);
+
+next_event = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+(NANOSECONDS_PER_SECOND * s->period / LOCALTIMER_FREQ);
+timer_mod(s->timer, next_event);
+}
+
+static void bcm2836_control_local_timer_tick(void *opaque)
+{
+BCM2836ControlState *s = opaque;
+
+bcm2836_control_local_timer_set_next(s);
+
+if (!s->triggered) {
+s->triggered = 1;
+bcm2836_control_update(s);
+}
+}
+
+static void bcm2836_control_local_timer_control(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+s->period = LOCALTIMER_VALUE(val);
+if (val & LOCALTIMER_INTENABLE) {
+if (!s->timer) {
+s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+bcm2836_control_local_timer_tick, s);
+}
+bcm2836_control_local_timer_set_next(s);
+} else {
+if (s->timer) {
+timer_del(s->timer);
+s->timer = NULL;
+}
+s->triggered = 0;
+}
+}
+
+static void bcm2836_control_local_timer_ack(void *opaque, uint32_t val)
+{
+BCM2836ControlState *s = opaque;
+
+if (val & LOCALTIMER_INTFLAG) {
+s->triggered = 0;
+}
+if (val & LOCALTIMER_RELOAD) {
+bcm2836_control_local_timer_set_next(s);
+}
+}
+
 static uint64_t bcm2836_control_read(void *opaque, hwaddr offset,
unsigned size)
 {
 BCM2836ControlState