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 <vanekt@fbl.cz>
Reviewed-on: http://openocd.zylin.com/2606
Tested-by: jenkins
Reviewed-by: Matthias Welwarsky <matthias@welwarsky.de>
Reviewed-by: Paul Fertser <fercerpav@gmail.com>

diff --git a/src/target/arm11.c b/src/target/arm11.c
index 0cb1d8c83fbd982f61270036330f31472af618b9..be21c52354f171d9d83338cfb85c77c344e19e9a 100644 (file)
--- 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 7b40f5057e6bd6c9de0a83985c81b3c4bd56f30e..73e0a85b61d6445c2eec6c0d1bcdf404ba4e47e7 100644 (file)
--- 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 27a8a9b9c10e66947bdb7b84ea4fbe171ea90c86..4c0d163fbc9f603c5543972b2be7f8690774ddad 100644 (file)
--- 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 6786c46c97e9d6a90dced8112174a9d19d0c2ec7..831d01a79f35df9949e207ba2c21bed3635cb513 100644 (file)
--- 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 9c204f04f615f5bb8eb6a4dbf3156b5e8d12f17f..1d3cc5948194e1bb668e63ffb8eed60c39deb114 100644 (file)
--- 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 5b740cc45242b614800d4e66a11fc4cc73467cb8..650f7992554a0ed80d7d740702cb9397281e8799 100644 (file)
--- 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 b6c5f6122ba07dc3002652e752bd4b39b3a49485..afdafefe23487b9eb25e5d02c2b94c241a394f3d 100644 (file)
--- 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 d634a12ed04815d1d557ff257c056697bcd34509..e12760c8f13c6a80f1d9b479c3812606a445a65c 100644 (file)
--- 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 8799e5c499786d25522b767dbfbb38c6325b94ac..713a31e1e729318081077b4c18b237f93d5427bf 100644 (file)
--- 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));