This is an automated email from Gerrit.
"Antonio Borneo <borneo.anto...@gmail.com>" just uploaded a new patch set to
Gerrit, which you can find at https://review.openocd.org/c/openocd/+/7134
-- gerrit
commit d30abaf36c3b874485370865547f1f8dd33557bd
Author: Antonio Borneo <borneo.anto...@gmail.com>
Date: Thu Aug 18 01:29:33 2022 +0200
jtag: rewrite the state machine for power drop and srst deassert
The current state machine looks broken.
Rewrite it in a trivial way and document it through a diagram.
While there, use boolean type in place of integers.
Change-Id: I25b11e52a69d397e832724e01d18beb9770cc4b7
Signed-off-by: Antonio Borneo <borneo.anto...@gmail.com>
diff --git a/src/jtag/core.c b/src/jtag/core.c
index 806ee89265..e41b60d60d 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -1707,7 +1707,7 @@ bool jtag_will_verify_capture_ir(void)
return jtag_verify_capture_ir;
}
-int jtag_power_dropout(int *dropout)
+int jtag_power_dropout(bool *dropout)
{
if (!is_adapter_initialized()) {
/* TODO: as the jtag interface is not valid all
@@ -1718,16 +1718,16 @@ int jtag_power_dropout(int *dropout)
if (adapter_driver->power_dropout)
return adapter_driver->power_dropout(dropout);
- *dropout = 0; /* by default we can't detect power dropout */
+ *dropout = false; /* by default we can't detect power dropout */
return ERROR_OK;
}
-int jtag_srst_asserted(int *srst_asserted)
+int jtag_srst_asserted(bool *srst_asserted)
{
if (adapter_driver->srst_asserted)
return adapter_driver->srst_asserted(srst_asserted);
- *srst_asserted = 0; /* by default we can't detect srst asserted */
+ *srst_asserted = false; /* by default we can't detect srst asserted */
return ERROR_OK;
}
diff --git a/src/jtag/interface.h b/src/jtag/interface.h
index a8d9ee47e9..1a64e093ff 100644
--- a/src/jtag/interface.h
+++ b/src/jtag/interface.h
@@ -293,7 +293,7 @@ struct adapter_driver {
*
* @returns ERROR_OK on success, or an error code on failure.
*/
- int (*power_dropout)(int *power_dropout);
+ int (*power_dropout)(bool *power_dropout);
/**
* Read and clear the srst asserted detection flag.
@@ -307,7 +307,7 @@ struct adapter_driver {
* been asserted.
* @returns ERROR_OK on success, or an error code on failure.
*/
- int (*srst_asserted)(int *srst_asserted);
+ int (*srst_asserted)(bool *srst_asserted);
/**
* Configure trace parameters for the adapter
diff --git a/src/jtag/jtag.h b/src/jtag/jtag.h
index 4f94e99135..d5757d7a53 100644
--- a/src/jtag/jtag.h
+++ b/src/jtag/jtag.h
@@ -525,8 +525,8 @@ int jtag_get_flush_queue_count(void);
void jtag_notify_event(enum jtag_event);
/* can be implemented by hw + sw */
-int jtag_power_dropout(int *dropout);
-int jtag_srst_asserted(int *srst_asserted);
+int jtag_power_dropout(bool *dropout);
+int jtag_srst_asserted(bool *srst_asserted);
/* JTAG support functions */
diff --git a/src/jtag/tcl.c b/src/jtag/tcl.c
index 37a0285ce1..d7721492b6 100644
--- a/src/jtag/tcl.c
+++ b/src/jtag/tcl.c
@@ -1239,7 +1239,7 @@ COMMAND_HANDLER(handle_wait_srst_deassert)
}
LOG_USER("Waiting for srst assert + deassert for at most %dms",
timeout_ms);
- int asserted_yet;
+ bool asserted_yet;
int64_t then = timeval_ms();
while (jtag_srst_asserted(&asserted_yet) == ERROR_OK) {
if ((timeval_ms() - then) > timeout_ms) {
diff --git a/src/target/target.c b/src/target/target.c
index 10a25efde6..83769cd087 100644
--- a/src/target/target.c
+++ b/src/target/target.c
@@ -2908,63 +2908,118 @@ COMMAND_HANDLER(handle_targets_command)
return retval;
}
-/* every 300ms we check for reset & powerdropout and issue a "reset halt" if
so. */
+/*
+ * Periodic check for reset & power dropout.
+ *
+ * State diagram for power dropout detection.
+ * Similar diagram applies to srst detection.
+ *
+ * (START)
+ * v ------------ -----------
+ * | | | | |
+ * v v | v |
+ * ----------- no-drop | ----------- drop |
+ * | |>--------- | |>--------
+ * | POWER | | POWER |
+ * | ON | drop (run-dropout) | OFF | no-drop
+ * | |>------------------->| |>--------
+ * ----------- ----------- |
+ * ^ ^ |
+ * | | |
+ * | | v
+ * | yes (run-restore) | drop -----------
+ * _/^\_ --------<| |
+ * _/ \_ | POWER |
+ * / \ no-drop | WAIT ON |
+ * | TIMEOUT |<-------------------------------<| |
+ * \_ ? _/ -----------
+ * \_ _/ ^
+ * \v/ |
+ * | no |
+ * --------------------------------------------
+ */
-static int power_dropout;
-static int srst_asserted;
+#define SENSE_TIMEOUT_MS 2000
-static int run_power_restore;
-static int run_power_dropout;
-static int run_srst_asserted;
-static int run_srst_deasserted;
+enum sense_state {
+ POWER_STATE_ON = 0,
+ POWER_STATE_OFF,
+ POWER_STATE_WAIT_ON,
+
+ SRST_STATE_DEASSERTED = 0,
+ SRST_STATE_ASSERTED,
+ SRST_STATE_WAIT_DEASSERTED,
+};
+
+static bool run_power_restore, run_power_dropout;
+static bool run_srst_asserted, run_srst_deasserted;
+
+static enum sense_state power_state = POWER_STATE_ON;
+static enum sense_state srst_state = SRST_STATE_DEASSERTED;
static int sense_handler(void)
{
- static int prev_srst_asserted;
- static int prev_power_dropout;
+ static int64_t power_on_deadline, srst_deasserted_deadline;
+ bool power_dropout, srst_asserted;
int retval = jtag_power_dropout(&power_dropout);
if (retval != ERROR_OK)
return retval;
- int power_restored;
- power_restored = prev_power_dropout && !power_dropout;
- if (power_restored)
- run_power_restore = 1;
-
- int64_t current = timeval_ms();
- static int64_t last_power;
- bool wait_more = last_power + 2000 > current;
- if (power_dropout && !wait_more) {
- run_power_dropout = 1;
- last_power = current;
+ switch (power_state) {
+ case POWER_STATE_OFF:
+ if (!power_dropout) {
+ power_state = POWER_STATE_WAIT_ON;
+ power_on_deadline = timeval_ms() + SENSE_TIMEOUT_MS;
+ }
+ break;
+ case POWER_STATE_WAIT_ON:
+ if (power_dropout) {
+ power_state = POWER_STATE_OFF;
+ break;
+ }
+ if (timeval_ms() > power_on_deadline) {
+ power_state = POWER_STATE_ON;
+ run_power_restore = true;
+ }
+ break;
+ case POWER_STATE_ON:
+ default:
+ if (power_dropout) {
+ power_state = POWER_STATE_OFF;
+ run_power_dropout = true;
+ }
+ break;
}
retval = jtag_srst_asserted(&srst_asserted);
if (retval != ERROR_OK)
return retval;
- int srst_deasserted;
- srst_deasserted = prev_srst_asserted && !srst_asserted;
-
- static int64_t last_srst;
- wait_more = last_srst + 2000 > current;
- if (srst_deasserted && !wait_more) {
- run_srst_deasserted = 1;
- last_srst = current;
- }
-
- if (!prev_srst_asserted && srst_asserted)
- run_srst_asserted = 1;
-
- prev_srst_asserted = srst_asserted;
- prev_power_dropout = power_dropout;
-
- if (srst_deasserted || power_restored) {
- /* Other than logging the event we can't do anything here.
- * Issuing a reset is a particularly bad idea as we might
- * be inside a reset already.
- */
+ switch (srst_state) {
+ case SRST_STATE_ASSERTED:
+ if (!srst_asserted) {
+ srst_state = SRST_STATE_WAIT_DEASSERTED;
+ srst_deasserted_deadline = timeval_ms() +
SENSE_TIMEOUT_MS;
+ }
+ break;
+ case SRST_STATE_WAIT_DEASSERTED:
+ if (srst_asserted) {
+ srst_state = SRST_STATE_ASSERTED;
+ break;
+ }
+ if (timeval_ms() > srst_deasserted_deadline) {
+ srst_state = SRST_STATE_DEASSERTED;
+ run_srst_deasserted = true;
+ }
+ break;
+ case SRST_STATE_DEASSERTED:
+ default:
+ if (srst_asserted) {
+ srst_state = SRST_STATE_ASSERTED;
+ run_srst_asserted = true;
+ }
+ break;
}
return ERROR_OK;
@@ -2982,32 +3037,32 @@ static int handle_target(void *priv)
}
/* we do not want to recurse here... */
- static int recursive;
+ static bool recursive;
if (!recursive) {
- recursive = 1;
+ recursive = true;
sense_handler();
/* danger! running these procedures can trigger srst assertions
and power dropouts.
* We need to avoid an infinite loop/recursion here and we do
that by
* clearing the flags after running these events.
*/
- int did_something = 0;
+ bool did_something = false;
if (run_srst_asserted) {
LOG_INFO("srst asserted detected, running srst_asserted
proc.");
Jim_Eval(interp, "srst_asserted");
- did_something = 1;
+ did_something = true;
}
if (run_srst_deasserted) {
Jim_Eval(interp, "srst_deasserted");
- did_something = 1;
+ did_something = true;
}
if (run_power_dropout) {
LOG_INFO("Power dropout detected, running power_dropout
proc.");
Jim_Eval(interp, "power_dropout");
- did_something = 1;
+ did_something = true;
}
if (run_power_restore) {
Jim_Eval(interp, "power_restore");
- did_something = 1;
+ did_something = true;
}
if (did_something) {
@@ -3016,13 +3071,12 @@ static int handle_target(void *priv)
}
/* clear action flags */
+ run_srst_asserted = false;
+ run_srst_deasserted = false;
+ run_power_restore = false;
+ run_power_dropout = false;
- run_srst_asserted = 0;
- run_srst_deasserted = 0;
- run_power_restore = 0;
- run_power_dropout = 0;
-
- recursive = 0;
+ recursive = false;
}
/* Poll targets for state changes unless that's globally disabled.
@@ -3046,7 +3100,7 @@ static int handle_target(void *priv)
target->backoff.count = 0;
/* only poll target if we've got power and srst isn't asserted
*/
- if (!power_dropout && !srst_asserted) {
+ if (power_state == POWER_STATE_ON && srst_state ==
SRST_STATE_DEASSERTED) {
/* polling may fail silently until the target has been
examined */
retval = target_poll(target);
if (retval != ERROR_OK) {
--
