hla: move memory read/write functionality to driver

Due to issues reported when using the jtag mode of the stlink (see Trac #61),
the functionality/checking has been moved to the driver.

This change also fixes unaligned 32bit memory read/write for the stlink.

From testing this change also brings a 3KiB/s speed increase, this is due
to the larger read/write packets.

Change-Id: I8234110e7e49a683f4dadd54c442ecdc3c47b320
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
Reviewed-on: http://openocd.zylin.com/1632
Tested-by: jenkins
Reviewed-by: Andreas Fritiofson <andreas.fritiofson@gmail.com>

diff --git a/src/jtag/drivers/stlink_usb.c b/src/jtag/drivers/stlink_usb.c
index 4154b93c0afac56f4c9946f4fcde5ee75b254eac..31f08cbb8eb2687573ad620d4817c73fbea05af4 100644 (file)
--- a/src/jtag/drivers/stlink_usb.c
+++ b/src/jtag/drivers/stlink_usb.c
@@ -50,10 +50,14 @@
 #define STLINK_TX_EP       (2|ENDPOINT_OUT)
 #define STLINK_TRACE_EP    (3|ENDPOINT_IN)
 #define STLINK_SG_SIZE     (31)
 #define STLINK_TX_EP       (2|ENDPOINT_OUT)
 #define STLINK_TRACE_EP    (3|ENDPOINT_IN)
 #define STLINK_SG_SIZE     (31)

-#define STLINK_DATA_SIZE   (4*128)
+#define STLINK_DATA_SIZE   (4096)

 #define STLINK_CMD_SIZE_V2 (16)
 #define STLINK_CMD_SIZE_V1 (10)
 
 #define STLINK_CMD_SIZE_V2 (16)
 #define STLINK_CMD_SIZE_V1 (10)
 

+/* the current implementation of the stlink limits
+ * 8bit read/writes to max 64 bytes. */
+#define STLINK_MAX_RW8         (64)
+

 enum stlink_jtag_api_version {
        STLINK_JTAG_API_V1 = 1,
        STLINK_JTAG_API_V2,
 enum stlink_jtag_api_version {
        STLINK_JTAG_API_V1 = 1,
        STLINK_JTAG_API_V2,


@@ -86,6 +90,8 @@ struct stlink_usb_handle_s {
        /** */
        uint8_t databuf[STLINK_DATA_SIZE];
        /** */
        /** */
        uint8_t databuf[STLINK_DATA_SIZE];
        /** */

+       uint32_t max_mem_packet;
+       /** */

        enum hl_transports transport;
        /** */
        struct stlink_usb_version version;
        enum hl_transports transport;
        /** */
        struct stlink_usb_version version;


@@ -1317,6 +1323,12 @@ static int stlink_usb_read_mem8(void *handle, uint32_t addr, uint16_t len,
 
        assert(handle != NULL);
 
 
        assert(handle != NULL);
 

+       /* max 8bit read/write is 64bytes */
+       if (len > STLINK_MAX_RW8) {
+               LOG_DEBUG("max buffer length exceeded");
+               return ERROR_FAIL;
+       }
+

        h = (struct stlink_usb_handle_s *)handle;
 
        stlink_usb_init_buffer(handle, STLINK_RX_EP, read_len);
        h = (struct stlink_usb_handle_s *)handle;
 
        stlink_usb_init_buffer(handle, STLINK_RX_EP, read_len);


@@ -1351,6 +1363,12 @@ static int stlink_usb_write_mem8(void *handle, uint32_t addr, uint16_t len,
 
        assert(handle != NULL);
 
 
        assert(handle != NULL);
 

+       /* max 8bit read/write is 64bytes */
+       if (len > STLINK_MAX_RW8) {
+               LOG_DEBUG("max buffer length exceeded");
+               return ERROR_FAIL;
+       }
+

        h = (struct stlink_usb_handle_s *)handle;
 
        stlink_usb_init_buffer(handle, STLINK_TX_EP, len);
        h = (struct stlink_usb_handle_s *)handle;
 
        stlink_usb_init_buffer(handle, STLINK_TX_EP, len);


@@ -1379,9 +1397,13 @@ static int stlink_usb_read_mem32(void *handle, uint32_t addr, uint16_t len,
 
        assert(handle != NULL);
 
 
        assert(handle != NULL);
 

-       h = (struct stlink_usb_handle_s *)handle;
+       /* data must be a multiple of 4 and word aligned */
+       if (len % 4 || addr % 4) {
+               LOG_DEBUG("Invalid data alignment");
+               return ERROR_TARGET_UNALIGNED_ACCESS;
+       }

 
 

-       len *= 4;
+       h = (struct stlink_usb_handle_s *)handle;

 
        stlink_usb_init_buffer(handle, STLINK_RX_EP, len);
 
 
        stlink_usb_init_buffer(handle, STLINK_RX_EP, len);
 


@@ -1411,9 +1433,13 @@ static int stlink_usb_write_mem32(void *handle, uint32_t addr, uint16_t len,
 
        assert(handle != NULL);
 
 
        assert(handle != NULL);
 

-       h = (struct stlink_usb_handle_s *)handle;
+       /* data must be a multiple of 4 and word aligned */
+       if (len % 4 || addr % 4) {
+               LOG_DEBUG("Invalid data alignment");
+               return ERROR_TARGET_UNALIGNED_ACCESS;
+       }

 
 

-       len *= 4;
+       h = (struct stlink_usb_handle_s *)handle;

 
        stlink_usb_init_buffer(handle, STLINK_TX_EP, len);
 
 
        stlink_usb_init_buffer(handle, STLINK_TX_EP, len);
 


@@ -1432,22 +1458,134 @@ static int stlink_usb_write_mem32(void *handle, uint32_t addr, uint16_t len,
        return stlink_usb_get_rw_status(handle);
 }
 
        return stlink_usb_get_rw_status(handle);
 }
 

+static uint32_t stlink_max_block_size(uint32_t tar_autoincr_block, uint32_t address)
+{
+       uint32_t max_tar_block = (tar_autoincr_block - ((tar_autoincr_block - 1) & address));
+       if (max_tar_block == 0)
+               max_tar_block = 4;
+       return max_tar_block;
+}
+

 static int stlink_usb_read_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, uint8_t *buffer)
 {
 static int stlink_usb_read_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, uint8_t *buffer)
 {

-       if (size == 4)
-               return stlink_usb_read_mem32(handle, addr, count, buffer);
-       else
-               return stlink_usb_read_mem8(handle, addr, count, buffer);
+       int retval = ERROR_OK;
+       uint32_t bytes_remaining;
+       struct stlink_usb_handle_s *h = (struct stlink_usb_handle_s *)handle;
+
+       /* calculate byte count */
+       count *= size;
+
+       while (count) {
+
+               bytes_remaining = (size == 4) ? \
+                               stlink_max_block_size(h->max_mem_packet, addr) : STLINK_MAX_RW8;
+
+               if (count < bytes_remaining)
+                       bytes_remaining = count;
+
+               /* the stlink only supports 8/32bit memory read/writes
+                * honour 32bit, all others will be handled as 8bit access */
+               if (size == 4) {
+
+                       /* When in jtag mode the stlink uses the auto-increment functinality.
+                        * However it expects us to pass the data correctly, this includes
+                        * alignment and any page boundaries. We already do this as part of the
+                        * adi_v5 implementation, but the stlink is a hla adapter and so this
+                        * needs implementiong manually.
+                        * currently this only affects jtag mode, according to ST they do single
+                        * access in SWD mode - but this may change and so we do it for both modes */
+
+                       /* we first need to check for any unaligned bytes */
+                       if (addr % 4) {
+
+                               uint32_t head_bytes = 4 - (addr % 4);
+                               retval = stlink_usb_read_mem8(handle, addr, head_bytes, buffer);
+                               if (retval != ERROR_OK)
+                                       return retval;
+                               buffer += head_bytes;
+                               addr += head_bytes;
+                               count -= head_bytes;
+                               bytes_remaining -= head_bytes;
+                       }
+
+                       if (bytes_remaining % 4)
+                               retval = stlink_usb_read_mem(handle, addr, 1, bytes_remaining, buffer);
+                       else
+                               retval = stlink_usb_read_mem32(handle, addr, bytes_remaining, buffer);
+               } else
+                       retval = stlink_usb_read_mem8(handle, addr, bytes_remaining, buffer);
+
+               if (retval != ERROR_OK)
+                       return retval;
+
+               buffer += bytes_remaining;
+               addr += bytes_remaining;
+               count -= bytes_remaining;
+       }
+
+       return retval;

 }
 
 static int stlink_usb_write_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, const uint8_t *buffer)
 {
 }
 
 static int stlink_usb_write_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, const uint8_t *buffer)
 {

-       if (size == 4)
-               return stlink_usb_write_mem32(handle, addr, count, buffer);
-       else
-               return stlink_usb_write_mem8(handle, addr, count, buffer);
+       int retval = ERROR_OK;
+       uint32_t bytes_remaining;
+       struct stlink_usb_handle_s *h = (struct stlink_usb_handle_s *)handle;
+
+       /* calculate byte count */
+       count *= size;
+
+       while (count) {
+
+               bytes_remaining = (size == 4) ? \
+                               stlink_max_block_size(h->max_mem_packet, addr) : STLINK_MAX_RW8;
+
+               if (count < bytes_remaining)
+                       bytes_remaining = count;
+
+               /* the stlink only supports 8/32bit memory read/writes
+                * honour 32bit, all others will be handled as 8bit access */
+               if (size == 4) {
+
+                       /* When in jtag mode the stlink uses the auto-increment functinality.
+                        * However it expects us to pass the data correctly, this includes
+                        * alignment and any page boundaries. We already do this as part of the
+                        * adi_v5 implementation, but the stlink is a hla adapter and so this
+                        * needs implementiong manually.
+                        * currently this only affects jtag mode, according to ST they do single
+                        * access in SWD mode - but this may change and so we do it for both modes */
+
+                       /* we first need to check for any unaligned bytes */
+                       if (addr % 4) {
+
+                               uint32_t head_bytes = 4 - (addr % 4);
+                               retval = stlink_usb_write_mem8(handle, addr, head_bytes, buffer);
+                               if (retval != ERROR_OK)
+                                       return retval;
+                               buffer += head_bytes;
+                               addr += head_bytes;
+                               count -= head_bytes;
+                               bytes_remaining -= head_bytes;
+                       }
+
+                       if (bytes_remaining % 4)
+                               retval = stlink_usb_write_mem(handle, addr, 1, bytes_remaining, buffer);
+                       else
+                               retval = stlink_usb_write_mem32(handle, addr, bytes_remaining, buffer);
+
+               } else
+                       retval = stlink_usb_write_mem8(handle, addr, bytes_remaining, buffer);
+               if (retval != ERROR_OK)
+                       return retval;
+
+               buffer += bytes_remaining;
+               addr += bytes_remaining;
+               count -= bytes_remaining;
+       }
+
+       return retval;

 }
 
 /** */
 }
 
 /** */


@@ -1483,9 +1621,6 @@ static int stlink_usb_open(struct hl_interface_param_s *param, void **fd)
 
        h->transport = param->transport;
 
 
        h->transport = param->transport;
 

-       /* set max read/write buffer size in bytes */
-       param->max_buffer = 512;
-

        const uint16_t vids[] = { param->vid, 0 };
        const uint16_t pids[] = { param->pid, 0 };
 
        const uint16_t vids[] = { param->vid, 0 };
        const uint16_t pids[] = { param->pid, 0 };
 


@@ -1616,6 +1751,23 @@ static int stlink_usb_open(struct hl_interface_param_s *param, void **fd)
                goto error_open;
        }
 
                goto error_open;
        }
 

+       /* get cpuid, so we can determine the max page size
+        * start with a safe default */
+       h->max_mem_packet = (1 << 10);
+
+       uint8_t buffer[4];
+       err = stlink_usb_read_mem32(h, CPUID, 4, buffer);
+       if (err == ERROR_OK) {
+               uint32_t cpuid = le_to_h_u32(buffer);
+               int i = (cpuid >> 4) & 0xf;
+               if (i == 4 || i == 3) {
+                       /* Cortex-M3/M4 has 4096 bytes autoincrement range */
+                       h->max_mem_packet = (1 << 12);
+               }
+       }
+
+       LOG_DEBUG("Using TAR autoincrement: %" PRIu32, h->max_mem_packet);
+

        *fd = h;
 
        return ERROR_OK;
        *fd = h;
 
        return ERROR_OK;

diff --git a/src/jtag/drivers/ti_icdi_usb.c b/src/jtag/drivers/ti_icdi_usb.c
index ae2f240fc58e9053d687c7efbae839b841439dc8..0d7d943bede0ed04734930860b4c3bb152d34b68 100644 (file)
--- a/src/jtag/drivers/ti_icdi_usb.c
+++ b/src/jtag/drivers/ti_icdi_usb.c
@@ -53,6 +53,7 @@ struct icdi_usb_handle_s {
        char *write_buffer;
        int max_packet;
        int read_count;
        char *write_buffer;
        int max_packet;
        int read_count;

+       uint32_t max_rw_packet; /* max X packet (read/write memory) transfers */

 };
 
 static int icdi_usb_read_mem(void *handle, uint32_t addr, uint32_t size,
 };
 
 static int icdi_usb_read_mem(void *handle, uint32_t addr, uint32_t size,


@@ -592,17 +593,57 @@ static int icdi_usb_write_mem_int(void *handle, uint32_t addr, uint32_t len, con
 static int icdi_usb_read_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, uint8_t *buffer)
 {
 static int icdi_usb_read_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, uint8_t *buffer)
 {

-       if (size == 4)
-               count *= size;
-       return icdi_usb_read_mem_int(handle, addr, count, buffer);
+       int retval = ERROR_OK;
+       struct icdi_usb_handle_s *h = (struct icdi_usb_handle_s *)handle;
+       uint32_t bytes_remaining;
+
+       /* calculate byte count */
+       count *= size;
+
+       while (count) {
+
+               bytes_remaining = h->max_rw_packet;
+               if (count < bytes_remaining)
+                       bytes_remaining = count;
+
+               retval = icdi_usb_read_mem_int(handle, addr, bytes_remaining, buffer);
+               if (retval != ERROR_OK)
+                       return retval;
+
+               buffer += bytes_remaining;
+               addr += bytes_remaining;
+               count -= bytes_remaining;
+       }
+
+       return retval;

 }
 
 static int icdi_usb_write_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, const uint8_t *buffer)
 {
 }
 
 static int icdi_usb_write_mem(void *handle, uint32_t addr, uint32_t size,
                uint32_t count, const uint8_t *buffer)
 {

-       if (size == 4)
-               count *= size;
-       return icdi_usb_write_mem_int(handle, addr, count, buffer);
+       int retval = ERROR_OK;
+       struct icdi_usb_handle_s *h = (struct icdi_usb_handle_s *)handle;
+       uint32_t bytes_remaining;
+
+       /* calculate byte count */
+       count *= size;
+
+       while (count) {
+
+               bytes_remaining = h->max_rw_packet;
+               if (count < bytes_remaining)
+                       bytes_remaining = count;
+
+               retval = icdi_usb_write_mem_int(handle, addr, bytes_remaining, buffer);
+               if (retval != ERROR_OK)
+                       return retval;
+
+               buffer += bytes_remaining;
+               addr += bytes_remaining;
+               count -= bytes_remaining;
+       }
+
+       return retval;

 }
 
 static int icdi_usb_close(void *handle)
 }
 
 static int icdi_usb_close(void *handle)


@@ -707,7 +748,7 @@ static int icdi_usb_open(struct hl_interface_param_s *param, void **fd)
         * as we are using gdb binary packets to transfer memory we have to
         * reserve half the buffer for any possible escape chars plus
         * at least 64 bytes for the gdb packet header */
         * as we are using gdb binary packets to transfer memory we have to
         * reserve half the buffer for any possible escape chars plus
         * at least 64 bytes for the gdb packet header */

-       param->max_buffer = (((h->max_packet - 64) / 4) * 4) / 2;
+       h->max_rw_packet = (((h->max_packet - 64) / 4) * 4) / 2;

 
        return ERROR_OK;
 
 
        return ERROR_OK;
 

diff --git a/src/jtag/hla/hla_interface.c b/src/jtag/hla/hla_interface.c
index f8b5c61b4fe0d65b0e1e76ffffa8fb140f2d18f3..0176a4823820229be5bf75d1c1945094f4c43176 100644 (file)
--- a/src/jtag/hla/hla_interface.c
+++ b/src/jtag/hla/hla_interface.c
@@ -37,7 +37,7 @@
 
 #include <target/target.h>
 
 
 #include <target/target.h>
 

-static struct hl_interface_s hl_if = { {0, 0, 0, 0, 0, HL_TRANSPORT_UNKNOWN, 0, false, NULL, 0}, 0, 0 };
+static struct hl_interface_s hl_if = { {0, 0, 0, 0, 0, HL_TRANSPORT_UNKNOWN, false, NULL, 0}, 0, 0 };

 
 int hl_interface_open(enum hl_transports tr)
 {
 
 int hl_interface_open(enum hl_transports tr)
 {

diff --git a/src/jtag/hla/hla_interface.h b/src/jtag/hla/hla_interface.h
index 398aa806234eeaefb58994be2938cb980eedbf03..fcf1d9e228683cd7c99005453bbcad6cc8d2f91c 100644 (file)
--- a/src/jtag/hla/hla_interface.h
+++ b/src/jtag/hla/hla_interface.h
@@ -45,8 +45,6 @@ struct hl_interface_param_s {
        /** */
        enum hl_transports transport;
        /** */
        /** */
        enum hl_transports transport;
        /** */

-       int max_buffer;
-       /** */

        bool connect_under_reset;
        /** Output file for trace data (if any) */
        FILE *trace_f;
        bool connect_under_reset;
        /** Output file for trace data (if any) */
        FILE *trace_f;

diff --git a/src/jtag/hla/hla_layout.c b/src/jtag/hla/hla_layout.c
index ef24fa3c3c3ee8b684df0f8a84e16c3982ea22cd..1d22fe13e1d18ec5dd0d1b3cb357628ae15e7d63 100644 (file)
--- a/src/jtag/hla/hla_layout.c
+++ b/src/jtag/hla/hla_layout.c
@@ -50,12 +50,6 @@ static int hl_layout_open(struct hl_interface_s *adapter)
                return res;
        }
 
                return res;
        }
 

-       /* make sure adapter has set the buffer size */
-       if (!adapter->param.max_buffer) {
-               LOG_ERROR("buffer size not set");
-               return ERROR_FAIL;
-       }
-

        return ERROR_OK;
 }
 
        return ERROR_OK;
 }
 

diff --git a/src/target/hla_target.c b/src/target/hla_target.c
index dc81ee89a6d39955808867fd3318188fe853e93d..a65ba805e4ff991874a618c8aeb720b0c074b263 100644 (file)
--- a/src/target/hla_target.c
+++ b/src/target/hla_target.c
@@ -758,41 +758,13 @@ static int adapter_read_memory(struct target *target, uint32_t address,
                uint8_t *buffer)
 {
        struct hl_interface_s *adapter = target_to_adapter(target);
                uint8_t *buffer)
 {
        struct hl_interface_s *adapter = target_to_adapter(target);

-       int res;
-       uint32_t buffer_threshold = (adapter->param.max_buffer / 4);
-       uint32_t addr_increment = 4;
-       uint32_t c;

 
        if (!count || !buffer)
                return ERROR_COMMAND_SYNTAX_ERROR;
 
        LOG_DEBUG("%s 0x%08x %d %d", __func__, address, size, count);
 
 
        if (!count || !buffer)
                return ERROR_COMMAND_SYNTAX_ERROR;
 
        LOG_DEBUG("%s 0x%08x %d %d", __func__, address, size, count);
 

-       /* prepare byte count, buffer threshold
-        * and address increment for none 32bit access
-        */
-       if (size != 4) {
-               count *= size;
-               buffer_threshold = (adapter->param.max_buffer / 4) / 2;
-               addr_increment = 1;
-       }
-
-       while (count) {
-               if (count > buffer_threshold)
-                       c = buffer_threshold;
-               else
-                       c = count;
-
-               res = adapter->layout->api->read_mem(adapter->fd, address, size, c, buffer);
-               if (res != ERROR_OK)
-                       return res;
-
-               address += (c * addr_increment);
-               buffer += (c * addr_increment);
-               count -= c;
-       }
-
-       return ERROR_OK;
+       return adapter->layout->api->read_mem(adapter->fd, address, size, count, buffer);

 }
 
 static int adapter_write_memory(struct target *target, uint32_t address,
 }
 
 static int adapter_write_memory(struct target *target, uint32_t address,


@@ -800,41 +772,13 @@ static int adapter_write_memory(struct target *target, uint32_t address,
                const uint8_t *buffer)
 {
        struct hl_interface_s *adapter = target_to_adapter(target);
                const uint8_t *buffer)
 {
        struct hl_interface_s *adapter = target_to_adapter(target);

-       int res;
-       uint32_t buffer_threshold = (adapter->param.max_buffer / 4);
-       uint32_t addr_increment = 4;
-       uint32_t c;

 
        if (!count || !buffer)
                return ERROR_COMMAND_SYNTAX_ERROR;
 
        LOG_DEBUG("%s 0x%08x %d %d", __func__, address, size, count);
 
 
        if (!count || !buffer)
                return ERROR_COMMAND_SYNTAX_ERROR;
 
        LOG_DEBUG("%s 0x%08x %d %d", __func__, address, size, count);
 

-       /* prepare byte count, buffer threshold
-        * and address increment for none 32bit access
-        */
-       if (size != 4) {
-               count *= size;
-               buffer_threshold = (adapter->param.max_buffer / 4) / 2;
-               addr_increment = 1;
-       }
-
-       while (count) {
-               if (count > buffer_threshold)
-                       c = buffer_threshold;
-               else
-                       c = count;
-
-               res = adapter->layout->api->write_mem(adapter->fd, address, size, c, buffer);
-               if (res != ERROR_OK)
-                       return res;
-
-               address += (c * addr_increment);
-               buffer += (c * addr_increment);
-               count -= c;
-       }
-
-       return ERROR_OK;
+       return adapter->layout->api->write_mem(adapter->fd, address, size, count, buffer);

 }
 
 static const struct command_registration adapter_command_handlers[] = {
 }
 
 static const struct command_registration adapter_command_handlers[] = {