This is an automated email from Gerrit.
"Jan Matyas <jan.mat...@codasip.com>" just uploaded a new patch set to Gerrit,
which you can find at https://review.openocd.org/c/openocd/+/8339
-- gerrit
commit 98004abce0b98abd3a64e03332d8d96225b76f22
Author: Jan Matyas <jan.mat...@codasip.com>
Date: Fri Jun 14 16:52:19 2024 +0200
drivers: jtag_vpi: separate "host" and "wire" data structures + cleanup
In jtag_vpi, use different data structures for the actual
(logical) exchanged information and their physical encoding
while being transmitted in binary packets.
Clearly document the meaning of the packet fields.
While there, clean up the overal code of the jtag_vpi driver
(data types, prints, assertions etc.).
Change-Id: I577966d671a0238524d1ee9cfeecc2489355ae76
Signed-off-by: Jan Matyas <jan.mat...@codasip.com>
diff --git a/src/jtag/drivers/jtag_vpi.c b/src/jtag/drivers/jtag_vpi.c
index 9dec0d19df..52cf1bdf9f 100644
--- a/src/jtag/drivers/jtag_vpi.c
+++ b/src/jtag/drivers/jtag_vpi.c
@@ -31,12 +31,14 @@
#define DEFAULT_SERVER_PORT 5555
#define XFERT_MAX_SIZE 512
+#define XFERT_MAX_BITS (XFERT_MAX_SIZE * 8)
+#define VPI_CMD_BUF_SIZE (4 + XFERT_MAX_SIZE + XFERT_MAX_SIZE + 4 + 4)
-#define CMD_RESET 0
-#define CMD_TMS_SEQ 1
-#define CMD_SCAN_CHAIN 2
-#define CMD_SCAN_CHAIN_FLIP_TMS 3
-#define CMD_STOP_SIMU 4
+#define CMD_RESET 0u
+#define CMD_TMS_SEQ 1u
+#define CMD_SCAN_CHAIN 2u
+#define CMD_SCAN_CHAIN_FLIP_TMS 3u
+#define CMD_STOP_SIMU 4u
/* jtag_vpi server port and address to connect to */
static int server_port = DEFAULT_SERVER_PORT;
@@ -48,25 +50,78 @@ static bool stop_sim_on_exit;
static int sockfd;
static struct sockaddr_in serv_addr;
-/* One jtag_vpi "packet" as sent over a TCP channel. */
-struct vpi_cmd {
- union {
- uint32_t cmd;
- unsigned char cmd_buf[4];
- };
+/*
+ * One jtag_vpi "packet" in its "wire" format - as sent over a TCP channel.
+ *
+ * Jtag_vpi protocol uses fixed-length and fixed-structure packets
+ * for all command types, and for both commands (CLI -> SRV)
+ * and responses (SRV -> CLI).
+ *
+ * For that reason, some packet fields are "don't care" depending
+ * on the command type and whether it is request or response.
+ */
+struct jtag_vpi_packet {
+ /* CLI -> SRV: Command type (CMD_*). Transmitted in little endian.
+ *
+ * SRV -> CLI: Don't care.
+ */
+ uint8_t cmd_type[4];
+
+ /* CLI -> SRV: CMD_TMS_SEQ: TMS bit stream.
+ * CMD_SCAN_CHAIN
+ * or CMD_SCAN_CHAIN_FLIP_TMS: TDO bit stream.
+ * Other commands: Don't care.
+ *
+ * SRV -> CLI: Don't care.
+ */
+ uint8_t buffer_out[XFERT_MAX_SIZE];
+
+ /* CLI -> SRV: Don't care.
+ *
+ * SRV -> CLI: TDI bit stream in responses to CMD_SCAN_CHAIN
+ * or CMD_SCAN_CHAIN_FLIP_TMS.
+ */
+ uint8_t buffer_in[XFERT_MAX_SIZE];
+
+ /* CLI -> SRV: CMD_TMS_SEQ or CMD_SCAN_CHAIN or CMD_SCAN_CHAIN_FLIP_TMS:
+ * DIV_ROUND_UP(nb_bits, 8)
+ * Note this is redundant information w.r.t. nb_bits.
+ * Transmitted in little endian.
+ *
+ * SRV -> CLI: Don't care.
+ */
+ uint8_t length[4];
+
+ /* CLI -> SRV: CMD_TMS_SEQ or CMD_SCAN_CHAIN or CMD_SCAN_CHAIN_FLIP_TMS:
+ * Number of bits stored in buffer_out.
+ * Transmitted in little endian.
+ *
+ * SRV -> CLI: Don't care. Reason: The effective nb_bits is always equal
+ * to the nb_bits transmitted in the payload.
+ */
+ uint8_t nb_bits[4];
+};
+
+/**
+ * Data for one jtag_vpi command to be sent
+ */
+struct jtag_vpi_cmd {
+ uint32_t cmd_type;
unsigned char buffer_out[XFERT_MAX_SIZE];
+ uint32_t nb_bits;
+};
+
+/**
+ * Data from one received jtag_vpi response
+ */
+struct jtag_vpi_resp {
+ /* In the response, only the data payload is relevant:
+ * Nb_bits is always equal to the command payload and
+ * the rest is irrelevant. */
unsigned char buffer_in[XFERT_MAX_SIZE];
- union {
- uint32_t length;
- unsigned char length_buf[4];
- };
- union {
- uint32_t nb_bits;
- unsigned char nb_bits_buf[4];
- };
};
-static char *jtag_vpi_cmd_to_str(int cmd_num)
+static char *jtag_vpi_cmd_to_str(unsigned int cmd_num)
{
switch (cmd_num) {
case CMD_RESET:
@@ -84,49 +139,71 @@ static char *jtag_vpi_cmd_to_str(int cmd_num)
}
}
-static int jtag_vpi_send_cmd(struct vpi_cmd *vpi)
+/* Only certain commands can carry the payload (TMS/TDO bitstream). */
+static bool can_carry_payload(struct jtag_vpi_cmd *cmd)
{
+ assert(cmd);
+ return cmd->cmd_type == CMD_SCAN_CHAIN
+ || cmd->cmd_type == CMD_SCAN_CHAIN_FLIP_TMS
+ || cmd->cmd_type == CMD_TMS_SEQ;
+}
+
+static int jtag_vpi_send_cmd(struct jtag_vpi_cmd *cmd)
+{
+ assert(cmd);
+
+ /* TODO: allocate all instances of jtag_vpi_packet, jtag_vpi_cmd and
jtag_vpi_resp
+ in order to go easy on the stack? */
+ struct jtag_vpi_packet packet;
int retval;
+ /* Safety: */
+ assert(can_carry_payload(cmd) || cmd->nb_bits == 0);
+ assert(cmd->nb_bits <= XFERT_MAX_BITS);
+
/* Optional low-level JTAG debug */
if (LOG_LEVEL_IS(LOG_LVL_DEBUG_IO)) {
- if (vpi->nb_bits > 0) {
+ if (cmd->nb_bits > 0) {
/* command with a non-empty data payload */
- char *char_buf = buf_to_hex_str(vpi->buffer_out,
- (vpi->nb_bits > DEBUG_JTAG_IOZ)
+ char *char_buf = buf_to_hex_str(cmd->buffer_out,
+ (cmd->nb_bits > DEBUG_JTAG_IOZ)
? DEBUG_JTAG_IOZ
- : vpi->nb_bits);
+ : cmd->nb_bits);
LOG_DEBUG_IO("sending JTAG VPI cmd: cmd=%s, "
- "length=%" PRIu32 ", "
"nb_bits=%" PRIu32 ", "
"buf_out=0x%s%s",
- jtag_vpi_cmd_to_str(vpi->cmd),
- vpi->length,
- vpi->nb_bits,
+ jtag_vpi_cmd_to_str(cmd->cmd_type),
+ cmd->nb_bits,
char_buf,
- (vpi->nb_bits > DEBUG_JTAG_IOZ) ?
"(...)" : "");
+ (cmd->nb_bits > DEBUG_JTAG_IOZ) ?
"(...)" : "");
free(char_buf);
} else {
/* command without data payload */
LOG_DEBUG_IO("sending JTAG VPI cmd: cmd=%s, "
- "length=%" PRIu32 ", "
"nb_bits=%" PRIu32,
- jtag_vpi_cmd_to_str(vpi->cmd),
- vpi->length,
- vpi->nb_bits);
+ jtag_vpi_cmd_to_str(cmd->cmd_type),
+ cmd->nb_bits);
}
}
+ /* Clear the packet */
+ memset((void *)&packet, 0, sizeof(packet));
+
/* Use little endian when transmitting/receiving jtag_vpi cmds.
- The choice of little endian goes against usual networking conventions
- but is intentional to remain compatible with most older OpenOCD
builds
- (i.e. builds on little-endian platforms). */
- h_u32_to_le(vpi->cmd_buf, vpi->cmd);
- h_u32_to_le(vpi->length_buf, vpi->length);
- h_u32_to_le(vpi->nb_bits_buf, vpi->nb_bits);
+ The choice of little endian goes against the usual networking
+ conventions but is intentional to remain compatible with old
+ OpenOCD versions in which jtag_vpi packet endianness was not
+ taken care of (it is assumed that little endian platforms
+ are prevalent). */
+ h_u32_to_le(packet.cmd_type, cmd->cmd_type);
+ buf_cpy(cmd->buffer_out, packet.buffer_out, cmd->nb_bits);
+ /* "length" is redundant but fill it in anyway for compatibility
+ (in case any server expects it). */
+ h_u32_to_le(packet.length, DIV_ROUND_UP(cmd->nb_bits, 8));
+ h_u32_to_le(packet.nb_bits, cmd->nb_bits);
retry_write:
- retval = write_socket(sockfd, vpi, sizeof(struct vpi_cmd));
+ retval = write_socket(sockfd, (void *)&packet, sizeof(packet));
if (retval < 0) {
/* Account for the case when socket write is interrupted. */
@@ -144,7 +221,7 @@ retry_write:
/* TODO: Clean way how adapter drivers can report fatal errors
to upper layers of OpenOCD and let it perform an orderly
shutdown? */
exit(-1);
- } else if (retval < (int)sizeof(struct vpi_cmd)) {
+ } else if (retval < (int)sizeof(packet)) {
/* This means we could not send all data, which is most likely
fatal
for the jtag_vpi connection (the underlying TCP connection
likely not
usable anymore) */
@@ -156,12 +233,16 @@ retry_write:
return ERROR_OK;
}
-static int jtag_vpi_receive_cmd(struct vpi_cmd *vpi)
+static int jtag_vpi_receive_resp(struct jtag_vpi_resp *resp)
{
+ struct jtag_vpi_packet packet;
unsigned bytes_buffered = 0;
- while (bytes_buffered < sizeof(struct vpi_cmd)) {
- int bytes_to_receive = sizeof(struct vpi_cmd) - bytes_buffered;
- int retval = read_socket(sockfd, ((char *)vpi) +
bytes_buffered, bytes_to_receive);
+
+ assert(resp);
+
+ while (bytes_buffered < sizeof(packet)) {
+ int bytes_to_receive = sizeof(packet) - bytes_buffered;
+ int retval = read_socket(sockfd, ((char *)&packet) +
bytes_buffered, bytes_to_receive);
if (retval < 0) {
#ifdef _WIN32
int wsa_err = WSAGetLastError();
@@ -187,10 +268,8 @@ static int jtag_vpi_receive_cmd(struct vpi_cmd *vpi)
bytes_buffered += retval;
}
- /* Use little endian when transmitting/receiving jtag_vpi cmds. */
- vpi->cmd = le_to_h_u32(vpi->cmd_buf);
- vpi->length = le_to_h_u32(vpi->length_buf);
- vpi->nb_bits = le_to_h_u32(vpi->nb_bits_buf);
+ /* Use little endian when transmitting/receiving jtag_vpi packets. */
+ memcpy(packet.buffer_in, resp->buffer_in, XFERT_MAX_SIZE);
return ERROR_OK;
}
@@ -202,12 +281,12 @@ static int jtag_vpi_receive_cmd(struct vpi_cmd *vpi)
*/
static int jtag_vpi_reset(int trst, int srst)
{
- struct vpi_cmd vpi;
- memset(&vpi, 0, sizeof(struct vpi_cmd));
+ struct jtag_vpi_cmd cmd;
+ memset(&cmd, 0, sizeof(struct jtag_vpi_cmd));
- vpi.cmd = CMD_RESET;
- vpi.length = 0;
- return jtag_vpi_send_cmd(&vpi);
+ cmd.cmd_type = CMD_RESET;
+ cmd.nb_bits = 0;
+ return jtag_vpi_send_cmd(&cmd);
}
/**
@@ -223,18 +302,18 @@ static int jtag_vpi_reset(int trst, int srst)
*/
static int jtag_vpi_tms_seq(const uint8_t *bits, int nb_bits)
{
- struct vpi_cmd vpi;
- int nb_bytes;
+ /* TODO: clean up the type of nb_bits - prefer unsigned in the whole
module */
+ assert(nb_bits >= 0);
+ assert(nb_bits <= XFERT_MAX_BITS);
- memset(&vpi, 0, sizeof(struct vpi_cmd));
- nb_bytes = DIV_ROUND_UP(nb_bits, 8);
+ struct jtag_vpi_cmd cmd;
+ memset(&cmd, 0, sizeof(struct jtag_vpi_cmd));
- vpi.cmd = CMD_TMS_SEQ;
- memcpy(vpi.buffer_out, bits, nb_bytes);
- vpi.length = nb_bytes;
- vpi.nb_bits = nb_bits;
+ cmd.cmd_type = CMD_TMS_SEQ;
+ buf_cpy(bits, cmd.buffer_out, nb_bits);
+ cmd.nb_bits = (unsigned int)nb_bits;
- return jtag_vpi_send_cmd(&vpi);
+ return jtag_vpi_send_cmd(&cmd);
}
/**
@@ -291,32 +370,37 @@ static int jtag_vpi_state_move(tap_state_t state)
static int jtag_vpi_queue_tdi_xfer(uint8_t *bits, int nb_bits, int tap_shift)
{
- struct vpi_cmd vpi;
+ /* TODO: tap_shift - prefer a bool or an enum instead of int & macros */
+ assert(nb_bits >= 0);
+
+ struct jtag_vpi_cmd cmd;
int nb_bytes = DIV_ROUND_UP(nb_bits, 8);
- memset(&vpi, 0, sizeof(struct vpi_cmd));
+ memset(&cmd, 0, sizeof(struct jtag_vpi_cmd));
- vpi.cmd = tap_shift ? CMD_SCAN_CHAIN_FLIP_TMS : CMD_SCAN_CHAIN;
+ cmd.cmd_type = tap_shift ? CMD_SCAN_CHAIN_FLIP_TMS : CMD_SCAN_CHAIN;
if (bits)
- memcpy(vpi.buffer_out, bits, nb_bytes);
+ memcpy(cmd.buffer_out, bits, nb_bytes);
else
- memset(vpi.buffer_out, 0xff, nb_bytes);
+ /* used when looping in Run-Test/Idle state */
+ memset(cmd.buffer_out, 0xff, nb_bytes);
- vpi.length = nb_bytes;
- vpi.nb_bits = nb_bits;
+ cmd.nb_bits = nb_bits;
- int retval = jtag_vpi_send_cmd(&vpi);
+ int retval = jtag_vpi_send_cmd(&cmd);
if (retval != ERROR_OK)
return retval;
- retval = jtag_vpi_receive_cmd(&vpi);
+ struct jtag_vpi_resp resp;
+
+ retval = jtag_vpi_receive_resp(&resp);
if (retval != ERROR_OK)
return retval;
/* Optional low-level JTAG debug */
if (LOG_LEVEL_IS(LOG_LVL_DEBUG_IO)) {
- char *char_buf = buf_to_hex_str(vpi.buffer_in,
+ char *char_buf = buf_to_hex_str(resp.buffer_in,
(nb_bits > DEBUG_JTAG_IOZ) ? DEBUG_JTAG_IOZ :
nb_bits);
LOG_DEBUG_IO("recvd JTAG VPI data: nb_bits=%d, buf_in=0x%s%s",
nb_bits, char_buf, (nb_bits > DEBUG_JTAG_IOZ) ? "(...)"
: "");
@@ -324,29 +408,30 @@ static int jtag_vpi_queue_tdi_xfer(uint8_t *bits, int
nb_bits, int tap_shift)
}
if (bits)
- memcpy(bits, vpi.buffer_in, nb_bytes);
+ buf_cpy(resp.buffer_in, bits, nb_bits);
return ERROR_OK;
}
/**
* jtag_vpi_queue_tdi - short description
- * @param bits bits to be queued on TDI (or NULL if 0 are to be queued)
+ * @param bits bits to be queued on TDI (or NULL if all FFs are to be queued)
* @param nb_bits number of bits
* @param tap_shift
*/
static int jtag_vpi_queue_tdi(uint8_t *bits, int nb_bits, int tap_shift)
{
+ assert(nb_bits >= 0);
+
int nb_xfer = DIV_ROUND_UP(nb_bits, XFERT_MAX_SIZE * 8);
- int retval;
while (nb_xfer) {
if (nb_xfer == 1) {
- retval = jtag_vpi_queue_tdi_xfer(bits, nb_bits,
tap_shift);
+ int retval = jtag_vpi_queue_tdi_xfer(bits, nb_bits,
tap_shift);
if (retval != ERROR_OK)
return retval;
} else {
- retval = jtag_vpi_queue_tdi_xfer(bits, XFERT_MAX_SIZE *
8, NO_TAP_SHIFT);
+ int retval = jtag_vpi_queue_tdi_xfer(bits,
XFERT_MAX_SIZE * 8, NO_TAP_SHIFT);
if (retval != ERROR_OK)
return retval;
nb_bits -= XFERT_MAX_SIZE * 8;
@@ -569,11 +654,10 @@ static int jtag_vpi_init(void)
static int jtag_vpi_stop_simulation(void)
{
- struct vpi_cmd cmd;
- memset(&cmd, 0, sizeof(struct vpi_cmd));
- cmd.length = 0;
+ struct jtag_vpi_cmd cmd;
+ memset(&cmd, 0, sizeof(struct jtag_vpi_cmd));
cmd.nb_bits = 0;
- cmd.cmd = CMD_STOP_SIMU;
+ cmd.cmd_type = CMD_STOP_SIMU;
return jtag_vpi_send_cmd(&cmd);
}
--
