From 88258042730260a763115d0718bd027a7708200d Mon Sep 17 00:00:00 2001 From: Tomas Vanek Date: Sun, 15 Mar 2015 19:09:15 +0100 Subject: [PATCH] target: improve robustness of reset command Before this change jim_target_reset() checked examined state of a target and failed without calling .assert_reset in particular target layer (and without comprehensible warning to user). Cortex-M target (which refuses access to DP under active SRST): If connection is lost then reset process fails before asserting SRST and connection with MCU is not restored. This resulted in: 1) A lot of Cortex-M MCUs required use of reset button or cycling power after firmware blocked SWD access somehow (sleep, misconfigured clock etc). If firmware blocks SWD access early during initialization, a MCU could become completely inaccessible by SWD. 2) If OpenOCD is (re)started and a MCU is in a broken state unresponsive to SWD, reset command does not work even if it could help to restore communication. Hopefully this scenario is not possible under full JTAG. jim_target_reset() in target.c now does not check examined state and delegates this task to a particular target. All targets have been checked and xx_assert_reset() (or xx_deassert_reset()) procedures were changed to check examined state if needed. Targets except arm11, cortex_a and cortex_m just fail if target is not examined although it may be possible to use at least hw reset. Left as TODO for developers familiar with these targets. cortex_m_assert_reset(): memory access errors are stored instead of immediate returning them to a higher level. Errors from less important reads/writes are ignored. Requested reset always leads to a configured action. arm11_assert_reset() just asserts hw reset in case of not examined target. cortex_a_assert_reset() works as usual in case of not examined target. Change-Id: I84fa869f4f58e2fa83b6ea75de84440d9dc3d929 Signed-off-by: Tomas Vanek Reviewed-on: http://openocd.zylin.com/2606 Tested-by: jenkins Reviewed-by: Matthias Welwarsky Reviewed-by: Paul Fertser --- src/target/arm11.c | 39 ++++++++++++++++---------- src/target/arm7_9_common.c | 7 +++++ src/target/cortex_a.c | 2 ++ src/target/cortex_m.c | 57 +++++++++++++++++--------------------- src/target/feroceon.c | 7 +++++ src/target/mips_m4k.c | 7 +++++ src/target/nds32.c | 7 +++++ src/target/target.c | 5 +--- src/target/xscale.c | 7 +++++ 9 files changed, 89 insertions(+), 49 deletions(-) diff --git a/src/target/arm11.c b/src/target/arm11.c index 0cb1d8c8..be21c523 100644 --- a/src/target/arm11.c +++ b/src/target/arm11.c @@ -694,21 +694,32 @@ static int arm11_assert_reset(struct target *target) { struct arm11_common *arm11 = target_to_arm11(target); - /* optionally catch reset vector */ - if (target->reset_halt && !(arm11->vcr & 1)) - CHECK_RETVAL(arm11_sc7_set_vcr(arm11, arm11->vcr | 1)); - - /* Issue some kind of warm reset. */ - if (target_has_event_action(target, TARGET_EVENT_RESET_ASSERT)) - target_handle_event(target, TARGET_EVENT_RESET_ASSERT); - else if (jtag_get_reset_config() & RESET_HAS_SRST) { - /* REVISIT handle "pulls" cases, if there's - * hardware that needs them to work. - */ - jtag_add_reset(0, 1); + if (!(target_was_examined(target))) { + if (jtag_get_reset_config() & RESET_HAS_SRST) + jtag_add_reset(0, 1); + else { + LOG_WARNING("Reset is not asserted because the target is not examined."); + LOG_WARNING("Use a reset button or power cycle the target."); + return ERROR_TARGET_NOT_EXAMINED; + } } else { - LOG_ERROR("%s: how to reset?", target_name(target)); - return ERROR_FAIL; + + /* optionally catch reset vector */ + if (target->reset_halt && !(arm11->vcr & 1)) + CHECK_RETVAL(arm11_sc7_set_vcr(arm11, arm11->vcr | 1)); + + /* Issue some kind of warm reset. */ + if (target_has_event_action(target, TARGET_EVENT_RESET_ASSERT)) + target_handle_event(target, TARGET_EVENT_RESET_ASSERT); + else if (jtag_get_reset_config() & RESET_HAS_SRST) { + /* REVISIT handle "pulls" cases, if there's + * hardware that needs them to work. + */ + jtag_add_reset(0, 1); + } else { + LOG_ERROR("%s: how to reset?", target_name(target)); + return ERROR_FAIL; + } } /* registers are now invalid */ diff --git a/src/target/arm7_9_common.c b/src/target/arm7_9_common.c index 7b40f505..73e0a85b 100644 --- a/src/target/arm7_9_common.c +++ b/src/target/arm7_9_common.c @@ -875,6 +875,13 @@ int arm7_9_assert_reset(struct target *target) enum reset_types jtag_reset_config = jtag_get_reset_config(); bool use_event = false; + /* TODO: apply hw reset signal in not examined state */ + if (!(target_was_examined(target))) { + LOG_WARNING("Reset is not asserted because the target is not examined."); + LOG_WARNING("Use a reset button or power cycle the target."); + return ERROR_TARGET_NOT_EXAMINED; + } + LOG_DEBUG("target->state: %s", target_state_name(target)); if (target_has_event_action(target, TARGET_EVENT_RESET_ASSERT)) diff --git a/src/target/cortex_a.c b/src/target/cortex_a.c index 27a8a9b9..4c0d163f 100644 --- a/src/target/cortex_a.c +++ b/src/target/cortex_a.c @@ -1903,6 +1903,8 @@ static int cortex_a_assert_reset(struct target *target) /* FIXME when halt is requested, make it work somehow... */ + /* This function can be called in "target not examined" state */ + /* Issue some kind of warm reset. */ if (target_has_event_action(target, TARGET_EVENT_RESET_ASSERT)) target_handle_event(target, TARGET_EVENT_RESET_ASSERT); diff --git a/src/target/cortex_m.c b/src/target/cortex_m.c index 6786c46c..831d01a7 100644 --- a/src/target/cortex_m.c +++ b/src/target/cortex_m.c @@ -992,34 +992,24 @@ static int cortex_m_assert_reset(struct target *target) /* Enable debug requests */ int retval; retval = mem_ap_read_atomic_u32(armv7m->debug_ap, DCB_DHCSR, &cortex_m->dcb_dhcsr); - if (retval != ERROR_OK) - return retval; - if (!(cortex_m->dcb_dhcsr & C_DEBUGEN)) { + /* Store important errors instead of failing and proceed to reset assert */ + + if (retval != ERROR_OK || !(cortex_m->dcb_dhcsr & C_DEBUGEN)) retval = mem_ap_write_u32(armv7m->debug_ap, DCB_DHCSR, DBGKEY | C_DEBUGEN); - if (retval != ERROR_OK) - return retval; - } /* If the processor is sleeping in a WFI or WFE instruction, the * C_HALT bit must be asserted to regain control */ - if (cortex_m->dcb_dhcsr & S_SLEEP) { + if (retval == ERROR_OK && (cortex_m->dcb_dhcsr & S_SLEEP)) retval = mem_ap_write_u32(armv7m->debug_ap, DCB_DHCSR, DBGKEY | C_HALT | C_DEBUGEN); - if (retval != ERROR_OK) - return retval; - } - retval = mem_ap_write_u32(armv7m->debug_ap, DCB_DCRDR, 0); - if (retval != ERROR_OK) - return retval; + mem_ap_write_u32(armv7m->debug_ap, DCB_DCRDR, 0); + /* Ignore less important errors */ if (!target->reset_halt) { /* Set/Clear C_MASKINTS in a separate operation */ - if (cortex_m->dcb_dhcsr & C_MASKINTS) { - retval = mem_ap_write_atomic_u32(armv7m->debug_ap, DCB_DHCSR, + if (cortex_m->dcb_dhcsr & C_MASKINTS) + mem_ap_write_atomic_u32(armv7m->debug_ap, DCB_DHCSR, DBGKEY | C_DEBUGEN | C_HALT); - if (retval != ERROR_OK) - return retval; - } /* clear any debug flags before resuming */ cortex_m_clear_halt(target); @@ -1033,16 +1023,20 @@ static int cortex_m_assert_reset(struct target *target) * bad vector table entries. Should this include MMERR or * other flags too? */ - retval = mem_ap_write_atomic_u32(armv7m->debug_ap, DCB_DEMCR, + int retval2; + retval2 = mem_ap_write_atomic_u32(armv7m->debug_ap, DCB_DEMCR, TRCENA | VC_HARDERR | VC_BUSERR | VC_CORERESET); - if (retval != ERROR_OK) - return retval; + if (retval != ERROR_OK || retval2 != ERROR_OK) + LOG_INFO("AP write error, reset will not halt"); } if (jtag_reset_config & RESET_HAS_SRST) { /* default to asserting srst */ if (!srst_asserted) adapter_assert_reset(); + + /* srst is asserted, ignore AP access errors */ + retval = ERROR_OK; } else { /* Use a standard Cortex-M3 software reset mechanism. * We default to using VECRESET as it is supported on all current cores. @@ -1057,27 +1051,24 @@ static int cortex_m_assert_reset(struct target *target) "handler to reset any peripherals or configure hardware srst support."); } - retval = mem_ap_write_atomic_u32(armv7m->debug_ap, NVIC_AIRCR, + int retval3; + retval3 = mem_ap_write_atomic_u32(armv7m->debug_ap, NVIC_AIRCR, AIRCR_VECTKEY | ((reset_config == CORTEX_M_RESET_SYSRESETREQ) ? AIRCR_SYSRESETREQ : AIRCR_VECTRESET)); - if (retval != ERROR_OK) + if (retval3 != ERROR_OK) LOG_DEBUG("Ignoring AP write error right after reset"); - retval = dap_dp_init(armv7m->debug_ap->dap); - if (retval != ERROR_OK) { + retval3 = dap_dp_init(armv7m->debug_ap->dap); + if (retval3 != ERROR_OK) LOG_ERROR("DP initialisation failed"); - return retval; - } - { + else { /* I do not know why this is necessary, but it * fixes strange effects (step/resume cause NMI * after reset) on LM3S6918 -- Michael Schwingen */ uint32_t tmp; - retval = mem_ap_read_atomic_u32(armv7m->debug_ap, NVIC_AIRCR, &tmp); - if (retval != ERROR_OK) - return retval; + mem_ap_read_atomic_u32(armv7m->debug_ap, NVIC_AIRCR, &tmp); } } @@ -1086,6 +1077,10 @@ static int cortex_m_assert_reset(struct target *target) register_cache_invalidate(cortex_m->armv7m.arm.core_cache); + /* now return stored error code if any */ + if (retval != ERROR_OK) + return retval; + if (target->reset_halt) { retval = target_halt(target); if (retval != ERROR_OK) diff --git a/src/target/feroceon.c b/src/target/feroceon.c index 9c204f04..1d3cc594 100644 --- a/src/target/feroceon.c +++ b/src/target/feroceon.c @@ -64,6 +64,13 @@ static int feroceon_assert_reset(struct target *target) struct arm7_9_common *arm7_9 = arm->arch_info; int ud = arm7_9->use_dbgrq; + /* TODO: apply hw reset signal in not examined state */ + if (!(target_was_examined(target))) { + LOG_WARNING("Reset is not asserted because the target is not examined."); + LOG_WARNING("Use a reset button or power cycle the target."); + return ERROR_TARGET_NOT_EXAMINED; + } + arm7_9->use_dbgrq = 0; if (target->reset_halt) arm7_9_halt(target); diff --git a/src/target/mips_m4k.c b/src/target/mips_m4k.c index 5b740cc4..650f7992 100644 --- a/src/target/mips_m4k.c +++ b/src/target/mips_m4k.c @@ -302,6 +302,13 @@ static int mips_m4k_assert_reset(struct target *target) struct mips_m4k_common *mips_m4k = target_to_m4k(target); struct mips_ejtag *ejtag_info = &mips_m4k->mips32.ejtag_info; + /* TODO: apply hw reset signal in not examined state */ + if (!(target_was_examined(target))) { + LOG_WARNING("Reset is not asserted because the target is not examined."); + LOG_WARNING("Use a reset button or power cycle the target."); + return ERROR_TARGET_NOT_EXAMINED; + } + LOG_DEBUG("target->state: %s", target_state_name(target)); diff --git a/src/target/nds32.c b/src/target/nds32.c index b6c5f612..afdafefe 100644 --- a/src/target/nds32.c +++ b/src/target/nds32.c @@ -2199,6 +2199,13 @@ int nds32_assert_reset(struct target *target) struct aice_port_s *aice = target_to_aice(target); struct nds32_cpu_version *cpu_version = &(nds32->cpu_version); + /* TODO: apply hw reset signal in not examined state */ + if (!(target_was_examined(target))) { + LOG_WARNING("Reset is not asserted because the target is not examined."); + LOG_WARNING("Use a reset button or power cycle the target."); + return ERROR_TARGET_NOT_EXAMINED; + } + if (target->reset_halt) { if ((nds32->soft_reset_halt) || (nds32->edm.version < 0x51) diff --git a/src/target/target.c b/src/target/target.c index d634a12e..e12760c8 100644 --- a/src/target/target.c +++ b/src/target/target.c @@ -4920,10 +4920,7 @@ static int jim_target_reset(Jim_Interp *interp, int argc, Jim_Obj *const *argv) struct target *target = Jim_CmdPrivData(goi.interp); if (!target->tap->enabled) return jim_target_tap_disabled(interp); - if (!(target_was_examined(target))) { - LOG_ERROR("Target not examined yet"); - return ERROR_TARGET_NOT_EXAMINED; - } + if (!target->type->assert_reset || !target->type->deassert_reset) { Jim_SetResultFormatted(interp, "No target-specific reset for %s", diff --git a/src/target/xscale.c b/src/target/xscale.c index 8799e5c4..713a31e1 100644 --- a/src/target/xscale.c +++ b/src/target/xscale.c @@ -1446,6 +1446,13 @@ static int xscale_assert_reset(struct target *target) { struct xscale_common *xscale = target_to_xscale(target); + /* TODO: apply hw reset signal in not examined state */ + if (!(target_was_examined(target))) { + LOG_WARNING("Reset is not asserted because the target is not examined."); + LOG_WARNING("Use a reset button or power cycle the target."); + return ERROR_TARGET_NOT_EXAMINED; + } + LOG_DEBUG("target->state: %s", target_state_name(target)); -- 2.39.5