QEMU's NVIC device provides an outbound qemu_irq "SYSRESETREQ" which
it signals when the guest sets the SYSRESETREQ bit in the AIRCR
register.  This matches the hardware design (where the CPU has a
signal of this name and it is up to the SoC to connect that up to an
actual reset mechanism), but in QEMU it mostly results in duplicated
code in SoC objects and bugs where SoC model implementors forget to
wire up the SYSRESETREQ line.

This patchseries provides a default behaviour for the case where
SYSRESETREQ is not actually connected to anything: use
qemu_system_reset_request() to perform a system reset. This is a
much more plausible default than "do nothing". It allows us to
allow us to remove the implementations of SYSRESETREQ handling from
the boards which currently hook up a reset function that just
does that (stellaris, emcraft-sf2), and also fixes the bugs
in these board models which forgot to wire up the signal:

 * microbit
 * mps2-an385
 * mps2-an505
 * mps2-an511
 * mps2-an521
 * musca-a
 * musca-b1
 * netduino
 * netduinoplus2

[I admit that I have not specifically checked for datasheets
and errata notes for all these boards to confirm that AIRCR.SYSRESETREQ
really does reset the system or to look for more complex
reset-control logic... As an example, though, the SSE-200 used in
the mps2-an521 and the musca boards has a RESET_MASK register in the
system control block that allows a guest to suppress reset requests from
one or both CPUs. Since we don't model that register, "always reset"
is still closer to reasonable behaviour than "do nothing".]

We still allow the board to wire up the signal if it needs to, in case
we need to model more complicated reset controller logic (eg if we
wanted to get that SSE-200 corner case right in future) or to model
buggy SoC hardware which forgot to wire up the line itself. But
defaulting to "reset the system" is going to be correct much more
often than "do nothing".

Patch 1 introduces a new API for "check whether my qemu_irq is
actually connected to anything" (the test is trivial but the
encapsulation seems worthwhile); patch 2 provides the new default
and patch 3 removes the now-unnecessary SYSRESETREQ handlers in
board/SoC code.

thanks
-- PMM

Peter Maydell (3):
  include/hw/irq.h: New function qemu_irq_is_connected()
  hw/intc/armv7m_nvic: Provide default "reset the system" behaviour for
    SYSRESETREQ
  msf2-soc, stellaris: Don't wire up SYSRESETREQ

 include/hw/arm/armv7m.h |  4 +++-
 include/hw/irq.h        | 18 ++++++++++++++++++
 hw/arm/msf2-soc.c       | 11 -----------
 hw/arm/stellaris.c      | 12 ------------
 hw/intc/armv7m_nvic.c   | 17 ++++++++++++++++-
 5 files changed, 37 insertions(+), 25 deletions(-)

-- 
2.20.1

On 7/28/20 12:37 PM, Peter Maydell wrote:
> QEMU's NVIC device provides an outbound qemu_irq "SYSRESETREQ" which
> it signals when the guest sets the SYSRESETREQ bit in the AIRCR
> register.  This matches the hardware design (where the CPU has a
> signal of this name and it is up to the SoC to connect that up to an
> actual reset mechanism), but in QEMU it mostly results in duplicated
> code in SoC objects and bugs where SoC model implementors forget to
> wire up the SYSRESETREQ line.
> 
> This patchseries provides a default behaviour for the case where
> SYSRESETREQ is not actually connected to anything: use
> qemu_system_reset_request() to perform a system reset. This is a
> much more plausible default than "do nothing". It allows us to
> allow us to remove the implementations of SYSRESETREQ handling from
> the boards which currently hook up a reset function that just
> does that (stellaris, emcraft-sf2), and also fixes the bugs
> in these board models which forgot to wire up the signal:
> 
>  * microbit
>  * mps2-an385
>  * mps2-an505
>  * mps2-an511
>  * mps2-an521
>  * musca-a
>  * musca-b1
>  * netduino
>  * netduinoplus2
> 
> [I admit that I have not specifically checked for datasheets
> and errata notes for all these boards to confirm that AIRCR.SYSRESETREQ
> really does reset the system or to look for more complex
> reset-control logic... As an example, though, the SSE-200 used in
> the mps2-an521 and the musca boards has a RESET_MASK register in the
> system control block that allows a guest to suppress reset requests from
> one or both CPUs. Since we don't model that register, "always reset"
> is still closer to reasonable behaviour than "do nothing".]
> 
> We still allow the board to wire up the signal if it needs to, in case
> we need to model more complicated reset controller logic (eg if we
> wanted to get that SSE-200 corner case right in future) or to model
> buggy SoC hardware which forgot to wire up the line itself. But
> defaulting to "reset the system" is going to be correct much more
> often than "do nothing".
> 
> Patch 1 introduces a new API for "check whether my qemu_irq is
> actually connected to anything" (the test is trivial but the
> encapsulation seems worthwhile); patch 2 provides the new default
> and patch 3 removes the now-unnecessary SYSRESETREQ handlers in
> board/SoC code.

Good cleanup!

Series:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

On Tue, Jul 28, 2020 at 3:38 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> QEMU's NVIC device provides an outbound qemu_irq "SYSRESETREQ" which
> it signals when the guest sets the SYSRESETREQ bit in the AIRCR
> register.  This matches the hardware design (where the CPU has a
> signal of this name and it is up to the SoC to connect that up to an
> actual reset mechanism), but in QEMU it mostly results in duplicated
> code in SoC objects and bugs where SoC model implementors forget to
> wire up the SYSRESETREQ line.
>
> This patchseries provides a default behaviour for the case where
> SYSRESETREQ is not actually connected to anything: use
> qemu_system_reset_request() to perform a system reset. This is a
> much more plausible default than "do nothing". It allows us to
> allow us to remove the implementations of SYSRESETREQ handling from
> the boards which currently hook up a reset function that just
> does that (stellaris, emcraft-sf2), and also fixes the bugs
> in these board models which forgot to wire up the signal:
>
>  * microbit
>  * mps2-an385
>  * mps2-an505
>  * mps2-an511
>  * mps2-an521
>  * musca-a
>  * musca-b1
>  * netduino
>  * netduinoplus2
>
> [I admit that I have not specifically checked for datasheets
> and errata notes for all these boards to confirm that AIRCR.SYSRESETREQ
> really does reset the system or to look for more complex
> reset-control logic... As an example, though, the SSE-200 used in
> the mps2-an521 and the musca boards has a RESET_MASK register in the
> system control block that allows a guest to suppress reset requests from
> one or both CPUs. Since we don't model that register, "always reset"
> is still closer to reasonable behaviour than "do nothing".]
>
> We still allow the board to wire up the signal if it needs to, in case
> we need to model more complicated reset controller logic (eg if we
> wanted to get that SSE-200 corner case right in future) or to model
> buggy SoC hardware which forgot to wire up the line itself. But
> defaulting to "reset the system" is going to be correct much more
> often than "do nothing".
>
> Patch 1 introduces a new API for "check whether my qemu_irq is
> actually connected to anything" (the test is trivial but the
> encapsulation seems worthwhile); patch 2 provides the new default
> and patch 3 removes the now-unnecessary SYSRESETREQ handlers in
> board/SoC code.

I checked the STM Reference Manual and this matches what they expect.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> thanks
> -- PMM
>
> Peter Maydell (3):
>   include/hw/irq.h: New function qemu_irq_is_connected()
>   hw/intc/armv7m_nvic: Provide default "reset the system" behaviour for
>     SYSRESETREQ
>   msf2-soc, stellaris: Don't wire up SYSRESETREQ
>
>  include/hw/arm/armv7m.h |  4 +++-
>  include/hw/irq.h        | 18 ++++++++++++++++++
>  hw/arm/msf2-soc.c       | 11 -----------
>  hw/arm/stellaris.c      | 12 ------------
>  hw/intc/armv7m_nvic.c   | 17 ++++++++++++++++-
>  5 files changed, 37 insertions(+), 25 deletions(-)
>
> --
> 2.20.1
>
>