armv7-m: exit on external reset request

Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
---
 hw/intc/armv7m_nvic.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On 8 October 2015 at 16:40, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com>
> ---
>  hw/intc/armv7m_nvic.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 3ec8408..a671d84 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -15,6 +15,7 @@
>  #include "hw/arm/arm.h"
>  #include "exec/address-spaces.h"
>  #include "gic_internal.h"
> +#include "sysemu/sysemu.h"
>
>  typedef struct {
>      GICState gic;
> @@ -348,10 +349,13 @@ static void nvic_writel(nvic_state *s, uint32_t offset, uint32_t value)
>          break;
>      case 0xd0c: /* Application Interrupt/Reset Control.  */
>          if ((value >> 16) == 0x05fa) {
> +            if (value & 4) {
> +                qemu_system_reset_request();
> +            }
>              if (value & 2) {
>                  qemu_log_mask(LOG_UNIMP, "VECTCLRACTIVE unimplemented\n");
>              }
> -            if (value & 5) {
> +            if (value & 1) {
>                  qemu_log_mask(LOG_UNIMP, "AIRCR system reset unimplemented\n");
>              }
>              if (value & 0x700) {

Strictly speaking what this bit does is assert a signal out of
the CPU to some external power management or similar device
in the SoC, which then is responsible for doing the reset.
So just calling qemu_system_reset_request() here is a bit of
a shortcut. But maybe it's a pragmatic shortcut?

thanks
-- PMM

On 10/09/2015 12:59 PM, Peter Maydell wrote:
> On 8 October 2015 at 16:40, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>> ...
>>      case 0xd0c: /* Application Interrupt/Reset Control.  */
>>          if ((value >> 16) == 0x05fa) {
>> +            if (value & 4) {
>> +                qemu_system_reset_request();
>> +            }
...
> 
> Strictly speaking what this bit does is assert a signal out of
> the CPU to some external power management or similar device
> in the SoC, which then is responsible for doing the reset.
> So just calling qemu_system_reset_request() here is a bit of
> a shortcut. But maybe it's a pragmatic shortcut?

I'm not sure what you mean by shortcut?  Most targets have some way to trigger qemu to exit.  I actually compiled a list recently (see below).  I couldn't find one for the lm3s6965evb machine, thus this patch.

FYI, one of my interests in QEMU is to automate testing of low level code.  Being able to cause the qemu process to exit when the tests complete is quite helpful (simplifies the test runner)


qemu_system_reset_request()
  highbank.c -> register write
  integratorcp.c -> register write
  musicpal.c -> register write  (musicpal board)
  omap1.c -> register write
  omap2.c -> register write
  pc.c -> register write (port 92)
  pckbd.c -> register write (two different registers)
  lpc_ich9.c -> register write
  arm_sysctl.c -> register write (vexpress only or board_ids 0x100, 0x178, 0x182 only, )
  cuda.c -> register write
  slavio_misc.c -> register write
  zynq_slcr.c -> register write  (xilinx-zynq-a9 board)
  apb.c -> register write
  bonito.c -> register write
  piix.c -> register write
  mpc8544_guts.c -> register write
  ppc.c -> e500 only ppce500_irq_init() w/ PPCE500_INPUT_MCK
  ppc405_uc.c -> store_40x_dbcr0()
  spapr_hcall.c -> KVM only
  spapr_rtas.c -> register RTAS_SYSTEM_REBOOT
  etraxfs_timer.c -> watchdog timer expire
  m48t59.c -> watchdog timer expire
  milkymist-sysctl.c -> register write
  pxa2xx_timer.c -> watchdog timer expire (bsp option)
  watchdog.c -> generic watchdog timer expire
  xtfpga.c -> register write

cpu_exit()
  pc.c -> DMA_init
  prep.c -> DMA_init (broken)
  spapr_rtas.c -> register RTAS_STOP_SELF

On Fri, Oct 9, 2015 at 10:25 AM, Michael Davidsaver
<mdavidsaver@gmail.com> wrote:
>
>
> On 10/09/2015 12:59 PM, Peter Maydell wrote:
>> On 8 October 2015 at 16:40, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>> ...
>>>      case 0xd0c: /* Application Interrupt/Reset Control.  */
>>>          if ((value >> 16) == 0x05fa) {
>>> +            if (value & 4) {
>>> +                qemu_system_reset_request();
>>> +            }
> ...
>>
>> Strictly speaking what this bit does is assert a signal out of
>> the CPU to some external power management or similar device
>> in the SoC, which then is responsible for doing the reset.
>> So just calling qemu_system_reset_request() here is a bit of
>> a shortcut. But maybe it's a pragmatic shortcut?
>
> I'm not sure what you mean by shortcut?  Most targets have some way to trigger qemu to exit.  I actually compiled a list recently (see below).  I couldn't find one for the lm3s6965evb machine, thus this patch.
>

We should check the lm3s docs to see what the implementation of this
signal is. I think it would be better for SoC (or board) level to
install the GPIO handler to do the reset. As far as target-arm is
concerned this is then just a GPIO.

> FYI, one of my interests in QEMU is to automate testing of low level code.  Being able to cause the qemu process to exit when the tests complete is quite helpful (simplifies the test runner)
>

Nice!

Regards,
Peter

>
> qemu_system_reset_request()
>   highbank.c -> register write
>   integratorcp.c -> register write
>   musicpal.c -> register write  (musicpal board)
>   omap1.c -> register write
>   omap2.c -> register write
>   pc.c -> register write (port 92)
>   pckbd.c -> register write (two different registers)
>   lpc_ich9.c -> register write
>   arm_sysctl.c -> register write (vexpress only or board_ids 0x100, 0x178, 0x182 only, )
>   cuda.c -> register write
>   slavio_misc.c -> register write
>   zynq_slcr.c -> register write  (xilinx-zynq-a9 board)
>   apb.c -> register write
>   bonito.c -> register write
>   piix.c -> register write
>   mpc8544_guts.c -> register write
>   ppc.c -> e500 only ppce500_irq_init() w/ PPCE500_INPUT_MCK
>   ppc405_uc.c -> store_40x_dbcr0()
>   spapr_hcall.c -> KVM only
>   spapr_rtas.c -> register RTAS_SYSTEM_REBOOT
>   etraxfs_timer.c -> watchdog timer expire
>   m48t59.c -> watchdog timer expire
>   milkymist-sysctl.c -> register write
>   pxa2xx_timer.c -> watchdog timer expire (bsp option)
>   watchdog.c -> generic watchdog timer expire
>   xtfpga.c -> register write
>
> cpu_exit()
>   pc.c -> DMA_init
>   prep.c -> DMA_init (broken)
>   spapr_rtas.c -> register RTAS_STOP_SELF
>

On 10/09/2015 02:18 PM, Peter Crosthwaite wrote:
> On Fri, Oct 9, 2015 at 10:25 AM, Michael Davidsaver
> <mdavidsaver@gmail.com> wrote:
>>
>>
>> On 10/09/2015 12:59 PM, Peter Maydell wrote:
>>> On 8 October 2015 at 16:40, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>>> ...
>>>>      case 0xd0c: /* Application Interrupt/Reset Control.  */
>>>>          if ((value >> 16) == 0x05fa) {
>>>> +            if (value & 4) {
>>>> +                qemu_system_reset_request();
>>>> +            }
>> ...
>>>
>>> Strictly speaking what this bit does is assert a signal out of
>>> the CPU to some external power management or similar device
>>> in the SoC, which then is responsible for doing the reset.
>>> So just calling qemu_system_reset_request() here is a bit of
>>> a shortcut. But maybe it's a pragmatic shortcut?
>>
>> I'm not sure what you mean by shortcut?  Most targets have some way to trigger qemu to exit.  I actually compiled a list recently (see below).  I couldn't find one for the lm3s6965evb machine, thus this patch.
>>
> 
...
> I think it would be better for SoC (or board) level to
> install the GPIO handler to do the reset. As far as target-arm is
> concerned this is then just a GPIO.

I don't think I'm well versed enough in qemu lingo to parse this sentence :)
Are you concerned about adding this function to AIRCR (which would effect every armv7-m board),
or about directly calling qemu_system_reset_request() in armv7m_nvic.c?


> We should check the lm3s docs to see what the implementation of this
> signal is.

To quote page 129 of http://www.ti.com/lit/ds/symlink/lm3s6965.pdf

> System Reset Request
> Value Description
> 0
>  No effect.
> 1
>  Resets the core and all on-chip peripherals except the Debug
> interface.
> This bit is automatically cleared during the reset of the core and reads
> as 0.

There is also mention of a board specific watchdog timer.

I also looked at the TI MSP432P4xx (I have one), which give a similar definition for this bit (along with a dozen board specific ways to do different reset levels...)

On 10/09/2015 02:51 PM, Michael Davidsaver wrote:
> On 10/09/2015 02:18 PM, Peter Crosthwaite wrote:
>> On Fri, Oct 9, 2015 at 10:25 AM, Michael Davidsaver
>> <mdavidsaver@gmail.com> wrote:
>>>
>>>
>>> On 10/09/2015 12:59 PM, Peter Maydell wrote:
>>>> On 8 October 2015 at 16:40, Michael Davidsaver <mdavidsaver@gmail.com> wrote:
>>>>> ...
>>>>>      case 0xd0c: /* Application Interrupt/Reset Control.  */
>>>>>          if ((value >> 16) == 0x05fa) {
>>>>> +            if (value & 4) {
>>>>> +                qemu_system_reset_request();
>>>>> +            }
>>> ...
>>>>
>>>> Strictly speaking what this bit does is assert a signal out of
>>>> the CPU to some external power management or similar device
>>>> in the SoC, which then is responsible for doing the reset.
>>>> So just calling qemu_system_reset_request() here is a bit of
>>>> a shortcut. But maybe it's a pragmatic shortcut?
>>>
>>> I'm not sure what you mean by shortcut?  Most targets have some way to trigger qemu to exit.  I actually compiled a list recently (see below).  I couldn't find one for the lm3s6965evb machine, thus this patch.
>>>
>>
> ...
>> I think it would be better for SoC (or board) level to
>> install the GPIO handler to do the reset. As far as target-arm is
>> concerned this is then just a GPIO.
> 
> I don't think I'm well versed enough in qemu lingo to parse this sentence :)
> Are you concerned about adding this function to AIRCR (which would effect every armv7-m board),
> or about directly calling qemu_system_reset_request() in armv7m_nvic.c?

I think I've answered my own question.  You mean to replace the call to qemu_system_reset_request() with something like qemu_irq_pulse()?

I think I see how to do this.  The simplest (adding another named GPIO to the NVIC devices) is complicated by the fact that the armv7m_init() doesn't return the nvic object, but rather an array of qemu_irq.  Is it reasonable to change armv7m_init() to return DeviceState*?