Handle DMI busy in sba write. (#437)

* Handle DMI busy in sba write.

If we encounter DMI busy on the NOP after a read, we'll never get the
value out because DMI busy is sticky. The read must be retried, but we
don't know whether it was ever issued. Since the read has side effects
(incrementing of the address) this retry must be handled at a higher
layer. So now dmi_op_timeout can be told to retry or not, and if retry
is disabled it'll return an error when busy.

Also actually properly do the retry in dmi_op_timeout(). Previously the
code would not reissue the command and end up returning a garbage value.

Change-Id: I3b52ebd51ebbbedd6e425676ac861b57fbe711b1

* Fix whitespace.

Change-Id: Icb76d964e681b22346368d224d1930c9342343f3

* Handle a few more DMI busy cases.

Change-Id: I8503a44e4bf935c0ebfff0d598fe4c322fda702a

* Explain when to use dmi_op_timeout(retry).

Change-Id: I1a5c6d76ac41a84472a8f79faecb2f48105191ff

* dmi_reset does not affect the current transaction.

That means the retry scheme we had been using works fine. This does
contain some minor tweaks, and now we pass my tests which hammer the DMI
busy case harder.

Change-Id: I13eee384dbba82bc5a5b1d387c75c547afe557b5

* Remove unnecessary changes to make the PR readable

Change-Id: I87079876e6965563cf590e3936b3595aeab8715d

* Move idle to end of line...

... because we go through run-test/idle after the scan.

Change-Id: I21a8cff22471f0b895d8cd8d25373dced9bf1ca9

* Remove unused code.

Change-Id: I07a7cdd2d64ca40a4fe181111a34cf55ff1928d1

2 files changed, 54 insertions, 55 deletions

diff --git a/src/target/riscv/batch.c b/src/target/riscv/batch.c
index 0d84c42..3087283 100644
--- a/src/target/riscv/batch.c
+++ b/src/target/riscv/batch.c
@@ -162,14 +162,13 @@ void dump_field(int idle, const struct scan_field *field)
 
 		log_printf_lf(LOG_LVL_DEBUG,
 				__FILE__, __LINE__, __PRETTY_FUNCTION__,
-				"%db %di %s %08x @%02x -> %s %08x @%02x",
-				field->num_bits, idle,
-				op_string[out_op], out_data, out_address,
-				status_string[in_op], in_data, in_address);
+				"%db %s %08x @%02x -> %s %08x @%02x; %di",
+				field->num_bits, op_string[out_op], out_data, out_address,
+				status_string[in_op], in_data, in_address, idle);
 	} else {
 		log_printf_lf(LOG_LVL_DEBUG,
-				__FILE__, __LINE__, __PRETTY_FUNCTION__, "%db %di %s %08x @%02x -> ?",
-				field->num_bits, idle, op_string[out_op], out_data, out_address);
+				__FILE__, __LINE__, __PRETTY_FUNCTION__, "%db %s %08x @%02x -> ?; %di",
+				field->num_bits, op_string[out_op], out_data, out_address, idle);
 	}
 }
 
diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c
index bc7035f..a206b4b 100644
--- a/src/target/riscv/riscv-013.c
+++ b/src/target/riscv/riscv-013.c
@@ -395,10 +395,9 @@ static void dump_field(int idle, const struct scan_field *field)
 
 	log_printf_lf(LOG_LVL_DEBUG,
 			__FILE__, __LINE__, "scan",
-			"%db %di %s %08x @%02x -> %s %08x @%02x",
-			field->num_bits, idle,
-			op_string[out_op], out_data, out_address,
-			status_string[in_op], in_data, in_address);
+			"%db %s %08x @%02x -> %s %08x @%02x; %di",
+			field->num_bits, op_string[out_op], out_data, out_address,
+			status_string[in_op], in_data, in_address, idle);
 
 	char out_text[500];
 	char in_text[500];
@@ -542,8 +541,20 @@ static dmi_status_t dmi_scan(struct target *target, uint32_t *address_in,
 	return buf_get_u32(in, DTM_DMI_OP_OFFSET, DTM_DMI_OP_LENGTH);
 }
 
-/* If dmi_busy_encountered is non-NULL, this function will use it to tell the
- * caller whether DMI was ever busy during this call. */
+/**
+ * @param data_in  The data we received from the target.
+ * @param dmi_op   The operation to perform (read/write/nop).
+ * @param dmi_busy_encountered
+ *                 If non-NULL, will be updated to reflect whether DMI busy was
+ *                 encountered while executing this operation or not.
+ * @param address  The address argument to that operation.
+ * @param data_out The data to send to the target.
+ * @param exec     When true, this scan will execute something, so extra RTI
+ *                 cycles may be added.
+ * @param ensure_success
+ *                 Scan a nop after the requested operation, ensuring the
+ *                 DMI operation succeeded.
+ */
 static int dmi_op_timeout(struct target *target, uint32_t *data_in,
 		bool *dmi_busy_encountered, int dmi_op, uint32_t address,
 		uint32_t data_out, int timeout_sec, bool exec, bool ensure_success)
@@ -606,27 +617,23 @@ static int dmi_op_timeout(struct target *target, uint32_t *data_in,
 					false);
 			if (status == DMI_STATUS_BUSY) {
 				increase_dmi_busy_delay(target);
+				if (dmi_busy_encountered)
+					*dmi_busy_encountered = true;
 			} else if (status == DMI_STATUS_SUCCESS) {
 				break;
 			} else {
-				LOG_ERROR("failed %s (NOP) at 0x%x, status=%d", op_name, address,
-						status);
+				if (data_in) {
+					LOG_ERROR("Failed %s (NOP) at 0x%x; value=0x%x, status=%d",
+							op_name, address, *data_in, status);
+				} else {
+					LOG_ERROR("Failed %s (NOP) at 0x%x; status=%d", op_name, address,
+							status);
+				}
 				return ERROR_FAIL;
 			}
 			if (time(NULL) - start > timeout_sec)
 				return ERROR_TIMEOUT_REACHED;
 		}
-
-		if (status != DMI_STATUS_SUCCESS) {
-			if (status == DMI_STATUS_FAILED || !data_in) {
-				LOG_ERROR("Failed %s (NOP) at 0x%x; status=%d", op_name, address,
-						status);
-			} else {
-				LOG_ERROR("Failed %s (NOP) at 0x%x; value=0x%x, status=%d",
-						op_name, address, *data_in, status);
-			}
-			return ERROR_FAIL;
-		}
 	}
 
 	return ERROR_OK;
@@ -2063,28 +2070,16 @@ static int read_memory_bus_word(struct target *target, target_addr_t address,
 		uint32_t size, uint8_t *buffer)
 {
 	uint32_t value;
-	if (size > 12) {
-		if (dmi_read(target, &value, DMI_SBDATA3) != ERROR_OK)
-			return ERROR_FAIL;
-		write_to_buf(buffer + 12, value, 4);
-		log_memory_access(address + 12, value, 4, true);
-	}
-	if (size > 8) {
-		if (dmi_read(target, &value, DMI_SBDATA2) != ERROR_OK)
-			return ERROR_FAIL;
-		write_to_buf(buffer + 8, value, 4);
-		log_memory_access(address + 8, value, 4, true);
-	}
-	if (size > 4) {
-		if (dmi_read(target, &value, DMI_SBDATA1) != ERROR_OK)
-			return ERROR_FAIL;
-		write_to_buf(buffer + 4, value, 4);
-		log_memory_access(address + 4, value, 4, true);
+	int result;
+	static int sbdata[4] = { DMI_SBDATA0, DMI_SBDATA1, DMI_SBDATA2, DMI_SBDATA3 };
+	assert(size <= 16);
+	for (int i = (size-1) / 4; i >= 0; i--) {
+		result = dmi_op(target, &value, NULL, DMI_OP_READ, sbdata[i], 0, false, true);
+		if (result != ERROR_OK)
+			return result;
+		write_to_buf(buffer + i * 4, value, MIN(size, 4));
+		log_memory_access(address + i * 4, value, MIN(size, 4), true);
 	}
-	if (dmi_read(target, &value, DMI_SBDATA0) != ERROR_OK)
-		return ERROR_FAIL;
-	write_to_buf(buffer, value, MIN(size, 4));
-	log_memory_access(address, value, MIN(size, 4), true);
 	return ERROR_OK;
 }
 
@@ -2318,6 +2313,7 @@ static int read_memory_bus_v1(struct target *target, target_addr_t address,
 				return ERROR_FAIL;
 		}
 
+		/* Read the last word, after we disabled sbreadondata if necessary. */
 		if (!get_field(sbcs_read, DMI_SBCS_SBERROR) &&
 				!get_field(sbcs_read, DMI_SBCS_SBBUSYERROR)) {
 			if (read_memory_bus_word(target, address + (count - 1) * size, size,
@@ -2499,10 +2495,10 @@ static int read_memory_progbuf_inner(struct target *target, target_addr_t addres
 
 	int result = ERROR_OK;
 
-	/* Write address to S0, and execute buffer. */
+	/* Write address to S0. */
 	result = register_write_direct(target, GDB_REGNO_S0, address);
 	if (result != ERROR_OK)
-		goto error;
+		return result;
 	uint32_t command = access_register_command(target, GDB_REGNO_S1,
 			riscv_xlen(target),
 			AC_ACCESS_REGISTER_TRANSFER | AC_ACCESS_REGISTER_POSTEXEC);
@@ -2510,7 +2506,6 @@ static int read_memory_progbuf_inner(struct target *target, target_addr_t addres
 		return ERROR_FAIL;
 
 	/* First read has just triggered. Result is in s1. */
-
 	if (count == 1) {
 		uint64_t value;
 		if (register_read_direct(target, &value, GDB_REGNO_S1) != ERROR_OK)
@@ -3087,11 +3082,12 @@ static int write_memory_bus_v1(struct target *target, target_addr_t address,
 
 		bool dmi_busy_encountered;
 		if (dmi_op(target, &sbcs, &dmi_busy_encountered, DMI_OP_READ,
-			   DMI_SBCS, 0, false, false) != ERROR_OK)
+				DMI_SBCS, 0, false, false) != ERROR_OK)
 			return ERROR_FAIL;
 
 		time_t start = time(NULL);
-		while (get_field(sbcs, DMI_SBCS_SBBUSY)) {
+		bool dmi_busy = dmi_busy_encountered;
+		while (get_field(sbcs, DMI_SBCS_SBBUSY) || dmi_busy) {
 			if (time(NULL) - start > riscv_command_timeout_sec) {
 				LOG_ERROR("Timed out after %ds waiting for sbbusy to go low (sbcs=0x%x). "
 					  "Increase the timeout with riscv set_command_timeout_sec.",
@@ -3099,15 +3095,18 @@ static int write_memory_bus_v1(struct target *target, target_addr_t address,
 				return ERROR_FAIL;
 			}
 
-			if (dmi_read(target, &sbcs, DMI_SBCS) != ERROR_OK)
+			if (dmi_op(target, &sbcs, &dmi_busy, DMI_OP_READ,
+						DMI_SBCS, 0, false, true) != ERROR_OK)
 				return ERROR_FAIL;
 		}
 
-		if (get_field(sbcs, DMI_SBCS_SBBUSYERROR) || dmi_busy_encountered) {
+		if (get_field(sbcs, DMI_SBCS_SBBUSYERROR)) {
 			/* We wrote while the target was busy. Slow down and try again. */
 			dmi_write(target, DMI_SBCS, DMI_SBCS_SBBUSYERROR);
 			info->bus_master_write_delay += info->bus_master_write_delay / 10 + 1;
+		}
 
+		if (get_field(sbcs, DMI_SBCS_SBBUSYERROR) || dmi_busy_encountered) {
 			next_address = sb_read_address(target);
 			if (next_address < address) {
 				/* This should never happen, probably buggy hardware. */
@@ -3271,8 +3270,9 @@ static int write_memory_progbuf(struct target *target, target_addr_t address,
 
 		uint32_t abstractcs;
 		bool dmi_busy_encountered;
-		if (dmi_op(target, &abstractcs, &dmi_busy_encountered, DMI_OP_READ,
-					DMI_ABSTRACTCS, 0, false, true) != ERROR_OK)
+		result = dmi_op(target, &abstractcs, &dmi_busy_encountered,
+				DMI_OP_READ, DMI_ABSTRACTCS, 0, false, true);
+		if (result != ERROR_OK)
 			goto error;
 		while (get_field(abstractcs, DMI_ABSTRACTCS_BUSY))
 			if (dmi_read(target, &abstractcs, DMI_ABSTRACTCS) != ERROR_OK)