diff mbox

hw/arm/bcm2836: Wire up CPU timer interrupts correctly

Message ID 1458210790-6621-1-git-send-email-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Maydell March 17, 2016, 10:33 a.m. UTC
Wire up the CPU timer interrupts in the right order, with the
nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,
and the secure physical timer on cntpsirq. (We did get the
virt timer right, at least.)

Reported-by: Antonio Huete Jiménez <tuxillo@quantumachine.net>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/bcm2836.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Thomas Hanson March 17, 2016, 1:37 p.m. UTC | #1
On Mar 17, 2016 4:33 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> Wire up the CPU timer interrupts in the right order, with the
> nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,
> and the secure physical timer on cntpsirq. (We did get the
> virt timer right, at least.)
>
> Reported-by: Antonio Huete Jiménez <tuxillo@quantumachine.net>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/bcm2836.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 89a6b35..1cefe41 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -136,9 +136,13 @@ static void bcm2836_realize(DeviceState *dev, Error
**errp)
>
>          /* Connect timers from the CPU to the interrupt controller */
>          qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_PHYS,
> -                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq",
n));
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpnsirq",
n));
>          qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_VIRT,
>                  qdev_get_gpio_in_named(DEVICE(&s->control), "cntvirq",
n));
> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_HYP,
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cnthpirq",
n));
> +        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_SEC,
> +                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq",
n));
>      }
>  }
>
> --
> 1.9.1
>
>

What drives the order?
Peter Maydell March 17, 2016, 1:46 p.m. UTC | #2
On 17 March 2016 at 13:37, Thomas Hanson <thomas.hanson@linaro.org> wrote:
>
> On Mar 17, 2016 4:33 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>>
>> Wire up the CPU timer interrupts in the right order, with the
>> nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,
>> and the secure physical timer on cntpsirq. (We did get the
>> virt timer right, at least.)

> What drives the order?

This is modelling the hardware:
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/README.md

The interrupt controller has 4 lines named CNTPSIRQ, CNTPNSIRQ, etc,
and we need to hook the lines from the CPU up to the interrupt
controller appropriately. The names we ended up with in QEMU's
CPU model (GTIMER_PHYS, etc) don't match the signal names that
a hardware A7 uses, which is slightly irritating, but not a big deal.

The A7 TRM lists which timer is which signal:
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0464f/BABBABDJ.html
(though you can pretty much guess from the signal names).

thanks
-- PMM
Thomas Hanson March 17, 2016, 1:54 p.m. UTC | #3
On 17 March 2016 at 07:46, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 March 2016 at 13:37, Thomas Hanson <thomas.hanson@linaro.org> wrote:
>>
>> On Mar 17, 2016 4:33 AM, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>>>
>>> Wire up the CPU timer interrupts in the right order, with the
>>> nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,
>>> and the secure physical timer on cntpsirq. (We did get the
>>> virt timer right, at least.)
>
>> What drives the order?
>
> This is modelling the hardware:
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/README.md
>
> The interrupt controller has 4 lines named CNTPSIRQ, CNTPNSIRQ, etc,
> and we need to hook the lines from the CPU up to the interrupt
> controller appropriately. The names we ended up with in QEMU's
> CPU model (GTIMER_PHYS, etc) don't match the signal names that
> a hardware A7 uses, which is slightly irritating, but not a big deal.
>
> The A7 TRM lists which timer is which signal:
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0464f/BABBABDJ.html
> (though you can pretty much guess from the signal names).
>
> thanks
> -- PMM

Ah, thanks.  I was thinking time-order. It's still early...
Andrew Baumann March 17, 2016, 4:43 p.m. UTC | #4
> From: Peter Maydell [mailto:peter.maydell@linaro.org]

> Sent: Thursday, 17 March 2016 3:33 AM

> 

> Wire up the CPU timer interrupts in the right order, with the

> nonsecure physical timer on cntpnsirq, the hyp timer on cnthpirq,

> and the secure physical timer on cntpsirq. (We did get the

> virt timer right, at least.)

> 

> Reported-by: Antonio Huete Jiménez <tuxillo@quantumachine.net>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>


Thanks Peter!
Andrew
diff mbox

Patch

diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 89a6b35..1cefe41 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -136,9 +136,13 @@  static void bcm2836_realize(DeviceState *dev, Error **errp)
 
         /* Connect timers from the CPU to the interrupt controller */
         qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_PHYS,
-                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq", n));
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpnsirq", n));
         qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_VIRT,
                 qdev_get_gpio_in_named(DEVICE(&s->control), "cntvirq", n));
+        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_HYP,
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cnthpirq", n));
+        qdev_connect_gpio_out(DEVICE(&s->cpus[n]), GTIMER_SEC,
+                qdev_get_gpio_in_named(DEVICE(&s->control), "cntpsirq", n));
     }
 }