Fix: JTAG correctness

src/platforms/common/jtagtap.c

@@ -48,10 +48,9 @@ void jtagtap_init(void)
 	jtag_proc.tap_idle_cycles = 1;
 
 	/* Ensure we're in JTAG mode */
-	for (size_t i = 0; i <= 50U; ++i)
+	for (size_t i = 0; i < 50U; ++i)
 		jtagtap_next(true, false); /* 50 idle cylces for SWD reset */
 	jtagtap_tms_seq(0xe73cU, 16U); /* SWD to JTAG sequence */
-	jtagtap_soft_reset();
 }
 
 static void jtagtap_reset(void)
@@ -169,33 +168,42 @@ static void jtagtap_tdi_tdo_seq_swd_delay(
 		data_out[byte] = value;
 }
 
+static inline void jtag_next_tms_tdi(const bool tms, const bool tdi)
+{
+	/* Initiate the falling edge on the bus */
+	gpio_clear(TCK_PORT, TCK_PIN);
+	/* It is now safe to change TMS and TDI */
+	gpio_set_val(TMS_PORT, TMS_PIN, tms);
+	gpio_set_val(TDI_PORT, TDI_PIN, tdi);
+}
+
 static void jtagtap_tdi_tdo_seq_no_delay(
 	const uint8_t *const data_in, uint8_t *const data_out, const bool final_tms, const size_t clock_cycles)
 {
 	uint8_t value = 0;
-	for (size_t cycle = 0; cycle < clock_cycles; ++cycle) {
-		const size_t bit = cycle & 7U;
+	for (size_t cycle = 0; cycle < clock_cycles;) {
+		/* Calculate the next bit and byte to consume data from */
+		const uint8_t bit = cycle & 7U;
 		const size_t byte = cycle >> 3U;
-		/* On the last tick, assert final_tms to TMS_PIN */
-		gpio_set_val(TMS_PORT, TMS_PIN, cycle + 1U >= clock_cycles && final_tms);
-		/* Set up the TDI pin and start the clock cycle */
-		gpio_set_val(TDI_PORT, TDI_PIN, data_in[byte] & (1U << bit));
+		/* Configure the bus for the next cycle */
+		jtag_next_tms_tdi(cycle + 1U >= clock_cycles && final_tms, data_in[byte] & (1U << bit));
+		/* Increment the cycle counter */
+		++cycle;
+		/* Block the compiler from re-ordering the calculations to preserve timings */
+		__asm__ volatile("" ::: "memory");
+		/* Start the clock cycle */
 		gpio_set(TCK_PORT, TCK_PIN);
 		/* If TDO is high, store a 1 in the appropriate position in the value being accumulated */
 		if (gpio_get(TDO_PORT, TDO_PIN))
-			value |= (1 << bit);
-		/* If we've got to the next whole byte, store the accumulated value and reset state */
+			value |= 1U << bit;
+		/* If we've got the next whole byte, store the accumulated value and reset state */
 		if (bit == 7U) {
 			data_out[byte] = value;
 			value = 0;
 		}
 		/* Finish the clock cycle */
-		gpio_clear(TCK_PORT, TCK_PIN);
 	}
-	const size_t bit = (clock_cycles - 1U) & 7U;
-	const size_t byte = (clock_cycles - 1U) >> 3U;
-	if (bit)
-		data_out[byte] = value;
+	gpio_clear(TCK_PORT, TCK_PIN);
 }
 
 static void jtagtap_tdi_tdo_seq(
@@ -231,6 +239,7 @@ static void jtagtap_tdi_seq_swd_delay(const uint8_t *const data_in, const bool f
 static void jtagtap_tdi_seq_no_delay(const uint8_t *const data_in, const bool final_tms, size_t clock_cycles)
 {
 	for (size_t cycle = 0; cycle < clock_cycles; ++cycle) {
+		gpio_clear(TCK_PORT, TCK_PIN);
 		const size_t bit = cycle & 7U;
 		const size_t byte = cycle >> 3U;
 		/* On the last tick, assert final_tms to TMS_PIN */
@@ -239,8 +248,10 @@ static void jtagtap_tdi_seq_no_delay(const uint8_t *const data_in, const bool fi
 		gpio_set_val(TDI_PORT, TDI_PIN, data_in[byte] & (1U << bit));
 		gpio_set(TCK_PORT, TCK_PIN);
 		/* Finish the clock cycle */
-		gpio_clear(TCK_PORT, TCK_PIN);
 	}
+	__asm__("nop");
+	__asm__("nop");
+	gpio_clear(TCK_PORT, TCK_PIN);
 }
 
 static void jtagtap_tdi_seq(const bool final_tms, const uint8_t *const data_in, const size_t clock_cycles)

src/platforms/hosted/remote_jtagtap.c

@@ -64,6 +64,7 @@ bool remote_jtagtap_init(void)
 	jtag_proc.jtagtap_tms_seq = jtagtap_tms_seq;
 	jtag_proc.jtagtap_tdi_tdo_seq = jtagtap_tdi_tdo_seq;
 	jtag_proc.jtagtap_tdi_seq = jtagtap_tdi_seq;
+	jtag_proc.tap_idle_cycles = 1;
 
 	platform_buffer_write((uint8_t *)REMOTE_HL_CHECK_STR, sizeof(REMOTE_HL_CHECK_STR));
 	length = platform_buffer_read((uint8_t *)buffer, REMOTE_MAX_MSG_SIZE);
@@ -116,25 +117,36 @@ static void jtagtap_tdi_tdo_seq(
 	char buffer[REMOTE_MAX_MSG_SIZE];
 	size_t in_offset = 0;
 	size_t out_offset = 0;
+	char tms_state = REMOTE_TDITDO_NOTMS;
+	/* Loop through the data to send/receive and handle it in chunks of up to 64 bits */
 	for (size_t cycle = 0; cycle < clock_cycles;) {
-		size_t chunk;
+		/* Calculate how many bits need to be in this chunk, capped at 64 */
+		size_t chunk = 64;
 		if (clock_cycles - cycle <= 64)
 			chunk = clock_cycles - cycle;
-		else
-			chunk = 64;
 		cycle += chunk;
+		/* If the result would complete the transaction, check if TMS needs to be high at the end */
+		if (cycle == clock_cycles && final_tms)
+			/* If it does, we then need to tell the firmware this */
+			tms_state = REMOTE_TDITDO_TMS;
 
+		/* Build a representation of the data to send safely */
 		uint64_t data = 0;
 		const size_t bytes = (chunk + 7U) >> 3U;
 		if (data_in) {
-			for (size_t i = 0; i < bytes; ++i)
-				data |= data_in[in_offset++] << (i * 8U);
+			for (size_t idx = 0; idx < bytes; ++idx, ++in_offset)
+				data |= data_in[in_offset] << (idx * 8U);
 		}
-		/* PRIx64 differs with system. Use it explicit in the format string*/
-		int length = snprintf(buffer, REMOTE_MAX_MSG_SIZE, "!J%c%02zx%" PRIx64 "%c",
-			!clock_cycles && final_tms ? REMOTE_TDITDO_TMS : REMOTE_TDITDO_NOTMS, chunk, data, REMOTE_EOM);
+		/*
+		 * Build the remote protocol message to send, and send it.
+		 * This uses its own copy of the REMOTE_JTAG_TDIDO_STR to correct for how
+		 * formatting a uint64_t is platform-specific.
+		 */
+		int length =
+			snprintf(buffer, REMOTE_MAX_MSG_SIZE, "!J%c%02zx%" PRIx64 "%c", tms_state, chunk, data, REMOTE_EOM);
 		platform_buffer_write((uint8_t *)buffer, length);
 
+		/* Receive the response and check if it's an error response */
 		length = platform_buffer_read((uint8_t *)buffer, REMOTE_MAX_MSG_SIZE);
 		if (!length || buffer[0] == REMOTE_RESP_ERR) {
 			DEBUG_WARN("jtagtap_tdi_tdo_seq failed, error %s\n", length ? buffer + 1 : "unknown");

src/target/jtag_scan.c

@@ -35,7 +35,7 @@ jtag_dev_s jtag_devs[JTAG_MAX_DEVS + 1U];
 uint32_t jtag_dev_count = 0;
 
 /* bucket of ones for don't care TDI */
-static const uint8_t ones[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
+const uint8_t ones[8] = {0xffU, 0xffU, 0xffU, 0xffU, 0xffU, 0xffU, 0xffU, 0xffU};
 
 #if PC_HOSTED == 0
 void jtag_add_device(const uint32_t dev_index, const jtag_dev_s *jtag_dev)
@@ -151,7 +151,7 @@ uint32_t jtag_scan(const uint8_t *irlens)
 	}
 
 	DEBUG_INFO("Return to Run-Test/Idle\n");
-	jtag_proc.jtagtap_next(1, 1);
+	jtag_proc.jtagtap_next(true, true);
 	jtagtap_return_idle(1);
 
 	/* All devices should be in BYPASS now */
@@ -194,7 +194,7 @@ uint32_t jtag_scan(const uint8_t *irlens)
 	}
 	DEBUG_INFO("Return to Run-Test/Idle\n");
 	jtag_proc.jtagtap_next(true, true);
-	jtagtap_return_idle(jtag_proc.tap_idle_cycles);
+	jtagtap_return_idle(1);
 
 #if PC_HOSTED == 1
 	/*Transfer needed device information to firmware jtag_devs */
@@ -234,13 +234,16 @@ uint32_t jtag_scan(const uint8_t *irlens)
 void jtag_dev_write_ir(const uint8_t dev_index, const uint32_t ir)
 {
 	jtag_dev_s *device = &jtag_devs[dev_index];
+	/* If the request would duplicate work already done, do nothing */
 	if (ir == device->current_ir)
 		return;
 
+	/* Set all the other devices IR's to being in bypass */
 	for (size_t device = 0; device < jtag_dev_count; device++)
-		jtag_devs[device].current_ir = -1;
+		jtag_devs[device].current_ir = UINT32_MAX;
 	device->current_ir = ir;
 
+	/* Do the work to make the scanchain match the jtag_devs state */
 	jtagtap_shift_ir();
 	jtag_proc.jtagtap_tdi_seq(false, ones, device->ir_prescan);
 	jtag_proc.jtagtap_tdi_seq(!device->ir_postscan, (const uint8_t *)&ir, device->ir_len);

src/target/jtag_scan.h

@@ -46,6 +46,7 @@ typedef struct jtag_dev {
 
 extern jtag_dev_s jtag_devs[JTAG_MAX_DEVS + 1U];
 extern uint32_t jtag_dev_count;
+extern const uint8_t ones[8];
 
 void jtag_dev_write_ir(uint8_t jd_index, uint32_t ir);
 void jtag_dev_shift_dr(uint8_t jd_index, uint8_t *dout, const uint8_t *din, size_t ticks);