Bus Pirate driver: Small assorted fixes. 37/1437/2
authorR. Diez <rdiezmail-openocd@yahoo.de>
Sat, 8 Jun 2013 14:07:28 +0000 (16:07 +0200)
committerSpencer Oliver <spen@spen-soft.co.uk>
Fri, 21 Jun 2013 11:25:26 +0000 (11:25 +0000)
Fixes are:
- Discard any stale data from the previous connection.
- Disable CR/LF translation on the (virtual USB) serial port.
- Increase the average USB packet size. The 1 KiB buffer was underutilised.
- Option "buspirate_speed fast" now works out of the box.
- Some extra comments, error checking, etc.

Change-Id: I72c49d943a8ce9c5e5d1644ef90cb1482f93c618
Signed-off-by: R. Diez <rdiezmail-openocd@yahoo.de>
Reviewed-on: http://openocd.zylin.com/1437
Tested-by: jenkins
Reviewed-by: Spencer Oliver <spen@spen-soft.co.uk>
src/jtag/drivers/buspirate.c

index 34dcd2b..12d34b9 100644 (file)
@@ -1,6 +1,7 @@
 /***************************************************************************
  *   Copyright (C) 2010 by Michal Demin                                    *
  *   based on usbprog.c and arm-jtag-ew.c                                  *
+ *   Several fixes by R. Diez in 2013.                                     *
  *                                                                         *
  *   This program is free software; you can redistribute it and/or modify  *
  *   it under the terms of the GNU General Public License as published by  *
@@ -81,6 +82,9 @@ enum {
        SERIAL_FAST = 1
 };
 
+static const cc_t SHORT_TIMEOUT  = 1; /* Must be at least 1. */
+static const cc_t NORMAL_TIMEOUT = 10;
+
 static int buspirate_fd = -1;
 static int buspirate_pinmode = MODE_JTAG_OD;
 static int buspirate_baudrate = SERIAL_NORMAL;
@@ -88,6 +92,9 @@ static int buspirate_vreg;
 static int buspirate_pullup;
 static char *buspirate_port;
 
+static enum tap_state last_tap_state = TAP_RESET;
+
+
 /* TAP interface */
 static void buspirate_tap_init(void);
 static int buspirate_tap_execute(void);
@@ -109,7 +116,7 @@ static void buspirate_jtag_get_adcs(int);
 
 /* low level HW communication interface */
 static int buspirate_serial_open(char *port);
-static int buspirate_serial_setspeed(int fd, char speed);
+static int buspirate_serial_setspeed(int fd, char speed, cc_t timeout);
 static int buspirate_serial_write(int fd, char *buf, int size);
 static int buspirate_serial_read(int fd, char *buf, int size);
 static void buspirate_serial_close(int fd);
@@ -196,6 +203,54 @@ static int buspirate_execute_queue(void)
        return buspirate_tap_execute();
 }
 
+
+/* Returns true if successful, false if error. */
+
+static bool read_and_discard_all_data(const int fd)
+{
+       /* LOG_INFO("Discarding any stale data from a previous connection..."); */
+
+       bool was_msg_already_printed = false;
+
+       for ( ; ; ) {
+               char buffer[1024];  /* Any size will do, it's a trade-off between stack size and performance. */
+
+               const ssize_t read_count = read(fd, buffer, sizeof(buffer));
+
+               if (read_count == 0) {
+                       /* This is the "end of file" or "connection closed at the other end" condition. */
+                       return true;
+               }
+
+               if (read_count > 0) {
+                       if (!was_msg_already_printed)   {
+                               LOG_INFO("Some stale data from a previous connection was discarded.");
+                               was_msg_already_printed = true;
+                       }
+
+                       continue;
+               }
+
+               assert(read_count == -1);  /* According to the specification. */
+
+               const int errno_code = errno;
+
+               if (errno_code == EINTR)
+                       continue;
+
+               if (errno_code == EAGAIN ||
+                       errno_code == EWOULDBLOCK) {
+                       /* We know that the file descriptor has been opened with O_NONBLOCK or O_NDELAY,
+                          and these codes mean that there is no data to read at present. */
+                       return true;
+               }
+
+               /* Some other error has occurred. */
+               return false;
+       }
+}
+
+
 static int buspirate_init(void)
 {
        if (buspirate_port == NULL) {
@@ -209,7 +264,33 @@ static int buspirate_init(void)
                return ERROR_JTAG_INIT_FAILED;
        }
 
-       buspirate_serial_setspeed(buspirate_fd, SERIAL_NORMAL);
+       /* The Operating System or the device itself may deliver stale data from the last connection,
+          so discard all available bytes right after the new connection has been established.
+          After all, we are implementing here a master/slave protocol, so the slave should have nothing
+          to say until the master sends the first command.
+
+          In the past, there was a tcflush() call in buspirate_serial_setspeed(), but that
+          was not enough. I guess you must actively read from the serial port to trigger any
+          data collection from the device and/or lower USB layers. If you disable the serial port
+          read timeout (if you set SHORT_TIMEOUT to 0), then the discarding does not work any more.
+
+          Note that we are lowering the serial port timeout for this first read operation,
+          otherwise the normal initialisation would be delayed for too long. */
+
+       if (-1 == buspirate_serial_setspeed(buspirate_fd, SERIAL_NORMAL, SHORT_TIMEOUT)) {
+               LOG_ERROR("Error configuring the serial port.");
+               return ERROR_JTAG_INIT_FAILED;
+       }
+
+       if (!read_and_discard_all_data(buspirate_fd)) {
+               LOG_ERROR("Error while attempting to discard any stale data right after establishing the connection.");
+               return ERROR_JTAG_INIT_FAILED;
+       }
+
+       if (-1 == buspirate_serial_setspeed(buspirate_fd, SERIAL_NORMAL, NORMAL_TIMEOUT)) {
+               LOG_ERROR("Error configuring the serial port.");
+               return ERROR_JTAG_INIT_FAILED;
+       }
 
        buspirate_jtag_enable(buspirate_fd);
 
@@ -524,8 +605,20 @@ static void buspirate_scan(bool ir_scan, enum scan_type type,
 
 /************************* TAP related stuff **********/
 
+/* This buffer size matches the maximum CMD_TAP_SHIFT bit length in the Bus Pirate firmware,
+   look for constant 0x2000 in OpenOCD.c . */
 #define BUSPIRATE_BUFFER_SIZE 1024
-#define BUSPIRATE_MAX_PENDING_SCANS 32
+
+/* The old value of 32 scans was not enough to achieve near 100% utilisation ratio
+   for the current BUSPIRATE_BUFFER_SIZE value of 1024.
+   With 128 scans I am getting full USB 2.0 high speed packets (512 bytes long) when
+   using the JtagDue firmware on the Arduino Due instead of the Bus Pirate, which
+   amounts approximately to a 10% overall speed gain. Bigger packets should also
+   benefit the Bus Pirate, but the speed difference is much smaller.
+   Unfortunately, each 512-byte packet is followed by a 329-byte one, which is not ideal.
+   However, increasing BUSPIRATE_BUFFER_SIZE for the benefit of the JtagDue would
+   make it incompatible with the Bus Pirate firmware. */
+#define BUSPIRATE_MAX_PENDING_SCANS 128
 
 static char tms_chain[BUSPIRATE_BUFFER_SIZE]; /* send */
 static char tdi_chain[BUSPIRATE_BUFFER_SIZE]; /* send */
@@ -551,6 +644,8 @@ static void buspirate_tap_init(void)
 
 static int buspirate_tap_execute(void)
 {
+       static const int CMD_TAP_SHIFT_HEADER_LEN = 3;
+
        char tmp[4096];
        uint8_t *in_buf;
        int i;
@@ -564,13 +659,13 @@ static int buspirate_tap_execute(void)
        LOG_DEBUG("executing tap num bits = %i scans = %i",
                        tap_chain_index, tap_pending_scans_num);
 
-       bytes_to_send = (tap_chain_index+7) / 8;
+       bytes_to_send = DIV_ROUND_UP(tap_chain_index, 8);
 
        tmp[0] = CMD_TAP_SHIFT; /* this command expects number of bits */
        tmp[1] = (char)(tap_chain_index >> 8);  /* high */
        tmp[2] = (char)(tap_chain_index);  /* low */
 
-       fill_index = 3;
+       fill_index = CMD_TAP_SHIFT_HEADER_LEN;
        for (i = 0; i < bytes_to_send; i++) {
                tmp[fill_index] = tdi_chain[i];
                fill_index++;
@@ -578,18 +673,26 @@ static int buspirate_tap_execute(void)
                fill_index++;
        }
 
-       ret = buspirate_serial_write(buspirate_fd, tmp, 3 + bytes_to_send*2);
-       if (ret != bytes_to_send*2+3) {
+       /* jlink.c calls the routine below, which may be useful for debugging purposes.
+          For example, enabling this allows you to compare the log outputs from jlink.c
+          and from this module for JTAG development or troubleshooting purposes. */
+       if (false) {
+               last_tap_state = jtag_debug_state_machine(tms_chain, tdi_chain,
+                                                                                                 tap_chain_index, last_tap_state);
+       }
+
+       ret = buspirate_serial_write(buspirate_fd, tmp, CMD_TAP_SHIFT_HEADER_LEN + bytes_to_send*2);
+       if (ret != bytes_to_send*2+CMD_TAP_SHIFT_HEADER_LEN) {
                LOG_ERROR("error writing :(");
                return ERROR_JTAG_DEVICE_ERROR;
        }
 
-       ret = buspirate_serial_read(buspirate_fd, tmp, bytes_to_send + 3);
-       if (ret != bytes_to_send + 3) {
+       ret = buspirate_serial_read(buspirate_fd, tmp, bytes_to_send + CMD_TAP_SHIFT_HEADER_LEN);
+       if (ret != bytes_to_send + CMD_TAP_SHIFT_HEADER_LEN) {
                LOG_ERROR("error reading");
                return ERROR_FAIL;
        }
-       in_buf = (uint8_t *)(&tmp[3]);
+       in_buf = (uint8_t *)(&tmp[CMD_TAP_SHIFT_HEADER_LEN]);
 
        /* parse the scans */
        for (i = 0; i < tap_pending_scans_num; i++) {
@@ -609,8 +712,7 @@ static int buspirate_tap_execute(void)
 
                free(buffer);
        }
-       tap_pending_scans_num = 0;
-       tap_chain_index = 0;
+       buspirate_tap_init();
        return ERROR_OK;
 }
 
@@ -634,6 +736,19 @@ static void buspirate_tap_append(int tms, int tdi)
                int bit_index = tap_chain_index % 8;
                uint8_t bit = 1 << bit_index;
 
+               if (0 == bit_index) {
+                       /* Let's say that the TAP shift operation wants to shift 9 bits,
+                          so we will be sending to the Bus Pirate a bit count of 9 but still
+                          full 16 bits (2 bytes) of shift data.
+                          If we don't clear all bits at this point, the last 7 bits will contain
+                          random data from the last buffer contents, which is not pleasant to the eye.
+                          Besides, the Bus Pirate (or some clone) may want to assert in debug builds
+                          that, after consuming all significant data bits, the rest of them are zero.
+                          Therefore, for aesthetic and for assert purposes, we clear all bits below. */
+                       tms_chain[chain_index] = 0;
+                       tdi_chain[chain_index] = 0;
+               }
+
                if (tms)
                        tms_chain[chain_index] |= bit;
                else
@@ -645,9 +760,13 @@ static void buspirate_tap_append(int tms, int tdi)
                        tdi_chain[chain_index] &= ~bit;
 
                tap_chain_index++;
-       } else
+       } else {
                LOG_ERROR("tap_chain overflow, bad things will happen");
-
+               /* Exit abruptly, like jlink.c does. After a buffer overflow we don't want
+                  to carry on, as data will be corrupt. Another option would be to return
+                  some error code at this point. */
+               exit(-1);
+       }
 }
 
 static void buspirate_tap_append_scan(int length, uint8_t *buffer,
@@ -770,7 +889,10 @@ static void buspirate_jtag_set_speed(int fd, char speed)
        buspirate_jtag_command(fd, tmp, 2);
 
        /* here the adapter changes speed, we need follow */
-       buspirate_serial_setspeed(fd, speed);
+       if (-1 == buspirate_serial_setspeed(fd, speed, NORMAL_TIMEOUT)) {
+               LOG_ERROR("Error configuring the serial port.");
+               exit(-1);
+       }
 
        buspirate_serial_write(fd, ack, 2);
        ret = buspirate_serial_read(fd, tmp, 2);
@@ -779,7 +901,7 @@ static void buspirate_jtag_set_speed(int fd, char speed)
                exit(-1);
        }
        if ((tmp[0] != CMD_UART_SPEED) || (tmp[1] != speed)) {
-               LOG_ERROR("Buspirate did not reply as expected");
+               LOG_ERROR("Buspirate did not reply as expected to the speed change command");
                exit(-1);
        }
        LOG_INFO("Buspirate switched to %s mode",
@@ -865,28 +987,50 @@ static int buspirate_serial_open(char *port)
        return fd;
 }
 
-static int buspirate_serial_setspeed(int fd, char speed)
+
+/* Returns -1 on error. */
+
+static int buspirate_serial_setspeed(int fd, char speed, cc_t timeout)
 {
        struct termios t_opt;
        speed_t baud = (speed == SERIAL_FAST) ? B1000000 : B115200;
 
        /* set the serial port parameters */
        fcntl(fd, F_SETFL, 0);
-       tcgetattr(fd, &t_opt);
-       cfsetispeed(&t_opt, baud);
-       cfsetospeed(&t_opt, baud);
+       if (0 != tcgetattr(fd, &t_opt))
+               return -1;
+
+       if (0 != cfsetispeed(&t_opt, baud))
+               return -1;
+
+       if (0 != cfsetospeed(&t_opt, baud))
+               return -1;
+
        t_opt.c_cflag |= (CLOCAL | CREAD);
        t_opt.c_cflag &= ~PARENB;
        t_opt.c_cflag &= ~CSTOPB;
        t_opt.c_cflag &= ~CSIZE;
        t_opt.c_cflag |= CS8;
        t_opt.c_lflag &= ~(ICANON | ECHO | ECHOE | ISIG);
-       t_opt.c_iflag &= ~(IXON | IXOFF | IXANY);
+
+       /* The serial port may have been configured for human interaction with
+          the Bus Pirate console, but OpenOCD is going to use a binary protocol,
+          so make sure to turn off any CR/LF translation and the like. */
+       t_opt.c_iflag &= ~(IXON | IXOFF | IXANY | INLCR | ICRNL);
+
        t_opt.c_oflag &= ~OPOST;
        t_opt.c_cc[VMIN] = 0;
-       t_opt.c_cc[VTIME] = 10;
-       tcflush(fd, TCIFLUSH);
-       tcsetattr(fd, TCSANOW, &t_opt);
+       t_opt.c_cc[VTIME] = timeout;
+
+       /* Note that, in the past, TCSANOW was used below instead of TCSADRAIN,
+          and CMD_UART_SPEED did not work properly then, at least with
+          the Bus Pirate v3.5 (USB). */
+       if (0 != tcsetattr(fd, TCSADRAIN, &t_opt)) {
+               /* According to the Linux documentation, this is actually not enough
+                  to detect errors, you need to call tcgetattr() and check that
+                  all changes have been performed successfully. */
+               return -1;
+       }
 
        return 0;
 }