From 848062d0d11679de25be573981df45e2c4880db8 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 12 Mar 2018 17:26:29 -0700 Subject: [PATCH 1/9] Propagate errors in more places Change-Id: I5a7594d4b44c524537827f403348d0c10814546f --- src/target/riscv/riscv-013.c | 113 +++++++++++++++-------------------- src/target/riscv/riscv.c | 76 ++++++++++++++--------- src/target/riscv/riscv.h | 7 ++- 3 files changed, 99 insertions(+), 97 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 2af2221d5..990379e84 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -39,7 +39,7 @@ static void riscv013_clear_abstract_error(struct target *target); static int riscv013_get_register(struct target *target, riscv_reg_t *value, int hid, int rid); static int riscv013_set_register(struct target *target, int hartid, int regid, uint64_t value); -static void riscv013_select_current_hart(struct target *target); +static int riscv013_select_current_hart(struct target *target); static int riscv013_halt_current_hart(struct target *target); static int riscv013_resume_current_hart(struct target *target); static int riscv013_step_current_hart(struct target *target); @@ -479,7 +479,8 @@ 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); } -static int dmi_read(struct target *target, uint32_t *value, uint32_t address) +static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, + uint32_t address, uint32_t data_out) { select_dmi(target); @@ -488,49 +489,66 @@ static int dmi_read(struct target *target, uint32_t *value, uint32_t address) unsigned i = 0; - /* This first loop ensures that the read request was actually sent - * to the target. Note that if for some reason this stays busy, - * it is actually due to the previous dmi_read or dmi_write. */ + const char *op_name; + switch (dmi_op) { + case DMI_OP_NOP: + op_name = "nop"; + break; + case DMI_OP_READ: + op_name = "read"; + break; + case DMI_OP_WRITE: + op_name = "write"; + break; + default: + LOG_ERROR("Invalid DMI operation: %d", dmi_op); + return ERROR_FAIL; + } + + /* This first loop performs the request. Note that if for some reason this + * stays busy, it is actually due to the previous access. */ for (i = 0; i < 256; i++) { - status = dmi_scan(target, NULL, NULL, DMI_OP_READ, address, 0, + status = dmi_scan(target, NULL, NULL, dmi_op, address, data_out, false); if (status == DMI_STATUS_BUSY) { increase_dmi_busy_delay(target); } else if (status == DMI_STATUS_SUCCESS) { break; } else { - LOG_ERROR("failed read from 0x%x, status=%d", address, status); + LOG_ERROR("failed %s at 0x%x, status=%d", op_name, address, status); return ERROR_FAIL; } } if (status != DMI_STATUS_SUCCESS) { - LOG_ERROR("Failed read from 0x%x; status=%d", address, status); + LOG_ERROR("Failed %s at 0x%x; status=%d", op_name, address, status); return ERROR_FAIL; } - /* This second loop ensures that we got the read - * data back. Note that NOP can result in a 'busy' result as well, but - * that would be noticed on the next DMI access we do. */ + /* This second loop ensures the request succeeded, and gets back data. + * Note that NOP can result in a 'busy' result as well, but that would be + * noticed on the next DMI access we do. */ for (i = 0; i < 256; i++) { - status = dmi_scan(target, &address_in, value, DMI_OP_NOP, address, 0, + status = dmi_scan(target, &address_in, data_in, DMI_OP_NOP, address, 0, false); if (status == DMI_STATUS_BUSY) { increase_dmi_busy_delay(target); } else if (status == DMI_STATUS_SUCCESS) { break; } else { - LOG_ERROR("failed read (NOP) at 0x%x, status=%d", address, status); + LOG_ERROR("failed %s (NOP) at 0x%x, status=%d", op_name, address, + status); return ERROR_FAIL; } } if (status != DMI_STATUS_SUCCESS) { - if (status == DMI_STATUS_FAILED) { - LOG_ERROR("Failed read (NOP) from 0x%x; status=%d", address, status); + 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 read (NOP) from 0x%x; value=0x%x, status=%d", - address, *value, status); + LOG_ERROR("Failed %s (NOP) at 0x%x; value=0x%x, status=%d", + op_name, address, *data_in, status); } return ERROR_FAIL; } @@ -538,53 +556,14 @@ static int dmi_read(struct target *target, uint32_t *value, uint32_t address) return ERROR_OK; } +static int dmi_read(struct target *target, uint32_t *value, uint32_t address) +{ + return dmi_op(target, value, DMI_OP_READ, address, 0); +} + static int dmi_write(struct target *target, uint32_t address, uint32_t value) { - select_dmi(target); - dmi_status_t status = DMI_STATUS_BUSY; - unsigned i = 0; - - /* The first loop ensures that we successfully sent the write request. */ - for (i = 0; i < 256; i++) { - status = dmi_scan(target, NULL, NULL, DMI_OP_WRITE, address, value, - address == DMI_COMMAND); - if (status == DMI_STATUS_BUSY) { - increase_dmi_busy_delay(target); - } else if (status == DMI_STATUS_SUCCESS) { - break; - } else { - LOG_ERROR("failed write to 0x%x, status=%d", address, status); - break; - } - } - - if (status != DMI_STATUS_SUCCESS) { - LOG_ERROR("Failed write to 0x%x;, status=%d", - address, status); - return ERROR_FAIL; - } - - /* The second loop isn't strictly necessary, but would ensure that the - * write is complete/ has no non-busy errors before returning from this - * function. */ - for (i = 0; i < 256; i++) { - status = dmi_scan(target, NULL, NULL, DMI_OP_NOP, address, 0, - false); - if (status == DMI_STATUS_BUSY) { - increase_dmi_busy_delay(target); - } else if (status == DMI_STATUS_SUCCESS) { - break; - } else { - LOG_ERROR("failed write (NOP) at 0x%x, status=%d", address, status); - break; - } - } - if (status != DMI_STATUS_SUCCESS) { - LOG_ERROR("failed to write (NOP) 0x%x to 0x%x; status=%d", value, address, status); - return ERROR_FAIL; - } - - return ERROR_OK; + return dmi_op(target, NULL, DMI_OP_WRITE, address, value); } int dmstatus_read(struct target *target, uint32_t *dmstatus, @@ -1352,7 +1331,8 @@ static int examine(struct target *target) continue; r->current_hartid = i; - riscv013_select_current_hart(target); + if (riscv013_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; uint32_t s; if (dmstatus_read(target, &s, true) != ERROR_OK) @@ -2480,14 +2460,15 @@ static int riscv013_set_register(struct target *target, int hid, int rid, uint64 return ERROR_OK; } -static void riscv013_select_current_hart(struct target *target) +static int riscv013_select_current_hart(struct target *target) { RISCV_INFO(r); uint32_t dmcontrol; - dmi_read(target, &dmcontrol, DMI_DMCONTROL); + if (dmi_read(target, &dmcontrol, DMI_DMCONTROL) != ERROR_OK) + return ERROR_FAIL; dmcontrol = set_field(dmcontrol, hartsel_mask(target), r->current_hartid); - dmi_write(target, DMI_DMCONTROL, dmcontrol); + return dmi_write(target, DMI_DMCONTROL, dmcontrol); } static int riscv013_halt_current_hart(struct target *target) diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 9f16a400a..15ac8318c 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -737,19 +737,20 @@ static int old_or_new_riscv_resume( return riscv_openocd_resume(target, current, address, handle_breakpoints, debug_execution); } -static void riscv_select_current_hart(struct target *target) +static int riscv_select_current_hart(struct target *target) { RISCV_INFO(r); if (r->rtos_hartid != -1 && riscv_rtos_enabled(target)) - riscv_set_current_hartid(target, r->rtos_hartid); + return riscv_set_current_hartid(target, r->rtos_hartid); else - riscv_set_current_hartid(target, target->coreid); + return riscv_set_current_hartid(target, target->coreid); } static int riscv_read_memory(struct target *target, target_addr_t address, uint32_t size, uint32_t count, uint8_t *buffer) { - riscv_select_current_hart(target); + if (riscv_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; struct target_type *tt = get_target_type(target); return tt->read_memory(target, address, size, count, buffer); } @@ -757,7 +758,8 @@ static int riscv_read_memory(struct target *target, target_addr_t address, static int riscv_write_memory(struct target *target, target_addr_t address, uint32_t size, uint32_t count, const uint8_t *buffer) { - riscv_select_current_hart(target); + if (riscv_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; struct target_type *tt = get_target_type(target); return tt->write_memory(target, address, size, count, buffer); } @@ -775,7 +777,8 @@ static int riscv_get_gdb_reg_list(struct target *target, return ERROR_FAIL; } - riscv_select_current_hart(target); + if (riscv_select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; switch (reg_class) { case REG_CLASS_GENERAL: @@ -964,24 +967,29 @@ int riscv_blank_check_memory(struct target *target, /*** OpenOCD Helper Functions ***/ -/* 0 means nothing happened, 1 means the hart's state changed (and thus the - * poll should terminate), and -1 means there was an error. */ -static int riscv_poll_hart(struct target *target, int hartid) +enum riscv_poll_hart { + RPH_NO_CHANGE, + RPH_CHANGE, + RPH_ERROR +}; +static enum riscv_poll_hart riscv_poll_hart(struct target *target, int hartid) { RISCV_INFO(r); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return RPH_ERROR; - LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid, target->state, TARGET_HALTED); + LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid, + target->state, TARGET_HALTED); - /* If OpenOCD this we're running but this hart is halted then it's time + /* If OpenOCD thinks we're running but this hart is halted then it's time * to raise an event. */ if (target->state != TARGET_HALTED && riscv_is_halted(target)) { LOG_DEBUG(" triggered a halt"); r->on_halt(target); - return 1; + return RPH_CHANGE; } - return 0; + return RPH_NO_CHANGE; } /*** OpenOCD Interface ***/ @@ -992,14 +1000,14 @@ int riscv_openocd_poll(struct target *target) if (riscv_rtos_enabled(target)) { /* Check every hart for an event. */ for (int i = 0; i < riscv_count_harts(target); ++i) { - int out = riscv_poll_hart(target, i); + enum riscv_poll_hart out = riscv_poll_hart(target, i); switch (out) { - case 0: + case RPH_NO_CHANGE: continue; - case 1: + case RPH_CHANGE: triggered_hart = i; break; - case -1: + case RPH_ERROR: return ERROR_FAIL; } } @@ -1018,8 +1026,12 @@ int riscv_openocd_poll(struct target *target) for (int i = 0; i < riscv_count_harts(target); ++i) riscv_halt_one_hart(target, i); } else { - if (riscv_poll_hart(target, riscv_current_hartid(target)) == 0) + enum riscv_poll_hart out = riscv_poll_hart(target, + riscv_current_hartid(target)); + if (out == RPH_NO_CHANGE) return ERROR_OK; + else if (out == RPH_ERROR) + return ERROR_FAIL; triggered_hart = riscv_current_hartid(target); LOG_DEBUG(" hart %d halted", triggered_hart); @@ -1042,6 +1054,8 @@ int riscv_openocd_poll(struct target *target) case RISCV_HALT_UNKNOWN: target->debug_reason = DBG_REASON_UNDEFINED; break; + case RISCV_HALT_ERROR: + return ERROR_FAIL; } if (riscv_rtos_enabled(target)) { @@ -1562,7 +1576,8 @@ int riscv_halt_one_hart(struct target *target, int hartid) { RISCV_INFO(r); LOG_DEBUG("halting hart %d", hartid); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return ERROR_FAIL; if (riscv_is_halted(target)) { LOG_DEBUG(" hart %d requested halt, but was already halted", hartid); return ERROR_OK; @@ -1588,7 +1603,8 @@ int riscv_resume_one_hart(struct target *target, int hartid) { RISCV_INFO(r); LOG_DEBUG("resuming hart %d", hartid); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return ERROR_FAIL; if (!riscv_is_halted(target)) { LOG_DEBUG(" hart %d requested resume, but was already resumed", hartid); return ERROR_OK; @@ -1609,7 +1625,8 @@ int riscv_step_rtos_hart(struct target *target) hartid = 0; } } - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return ERROR_FAIL; LOG_DEBUG("stepping hart %d", hartid); if (!riscv_is_halted(target)) { @@ -1659,33 +1676,35 @@ bool riscv_rtos_enabled(const struct target *target) return target->rtos != NULL; } -void riscv_set_current_hartid(struct target *target, int hartid) +int riscv_set_current_hartid(struct target *target, int hartid) { RISCV_INFO(r); if (!r->select_current_hart) - return; + return ERROR_FAIL; int previous_hartid = riscv_current_hartid(target); r->current_hartid = hartid; assert(riscv_hart_enabled(target, hartid)); LOG_DEBUG("setting hartid to %d, was %d", hartid, previous_hartid); - r->select_current_hart(target); + if (r->select_current_hart(target) != ERROR_OK) + return ERROR_FAIL; /* This might get called during init, in which case we shouldn't be * setting up the register cache. */ if (!target_was_examined(target)) - return; + return ERROR_OK; /* Avoid invalidating the register cache all the time. */ if (r->registers_initialized && (!riscv_rtos_enabled(target) || (previous_hartid == hartid)) && target->reg_cache->reg_list[GDB_REGNO_ZERO].size == (unsigned)riscv_xlen(target) && (!riscv_rtos_enabled(target) || (r->rtos_hartid != -1))) { - return; + return ERROR_OK; } else LOG_DEBUG("Initializing registers: xlen=%d", riscv_xlen(target)); riscv_invalidate_register_cache(target); + return ERROR_OK; } void riscv_invalidate_register_cache(struct target *target) @@ -1775,7 +1794,8 @@ bool riscv_is_halted(struct target *target) enum riscv_halt_reason riscv_halt_reason(struct target *target, int hartid) { RISCV_INFO(r); - riscv_set_current_hartid(target, hartid); + if (riscv_set_current_hartid(target, hartid) != ERROR_OK) + return RISCV_HALT_ERROR; if (!riscv_is_halted(target)) { LOG_ERROR("Hart is not halted!"); return RISCV_HALT_UNKNOWN; diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 626b71f83..1e6ffb94b 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -31,7 +31,8 @@ enum riscv_halt_reason { RISCV_HALT_BREAKPOINT, RISCV_HALT_SINGLESTEP, RISCV_HALT_TRIGGER, - RISCV_HALT_UNKNOWN + RISCV_HALT_UNKNOWN, + RISCV_HALT_ERROR }; typedef struct { @@ -93,7 +94,7 @@ typedef struct { riscv_reg_t *value, int hid, int rid); int (*set_register)(struct target *, int hartid, int regid, uint64_t value); - void (*select_current_hart)(struct target *); + int (*select_current_hart)(struct target *); bool (*is_halted)(struct target *target); int (*halt_current_hart)(struct target *); int (*resume_current_hart)(struct target *target); @@ -191,7 +192,7 @@ bool riscv_rtos_enabled(const struct target *target); /* Sets the current hart, which is the hart that will actually be used when * issuing debug commands. */ -void riscv_set_current_hartid(struct target *target, int hartid); +int riscv_set_current_hartid(struct target *target, int hartid); int riscv_current_hartid(const struct target *target); /*** Support functions for the RISC-V 'RTOS', which provides multihart support From fd2759a63ddf5e9939a6844844e66f2f5d91469e Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Fri, 16 Mar 2018 12:25:28 -0700 Subject: [PATCH 2/9] Clear havereset in examine() and deassert_reset(). Change-Id: I89f32a44ebd6f3df0d0e2f6b54b111daa6ab06f7 --- src/target/riscv/riscv-013.c | 38 ++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 990379e84..fc1b8b5da 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -280,8 +280,11 @@ static void decode_dmi(char *text, unsigned address, unsigned data) { DMI_DMCONTROL, ((1L<<10)-1) << DMI_DMCONTROL_HARTSEL_OFFSET, "hartsel" }, { DMI_DMCONTROL, DMI_DMCONTROL_NDMRESET, "ndmreset" }, { DMI_DMCONTROL, DMI_DMCONTROL_DMACTIVE, "dmactive" }, + { DMI_DMCONTROL, DMI_DMCONTROL_ACKHAVERESET, "ackhavereset" }, { DMI_DMSTATUS, DMI_DMSTATUS_IMPEBREAK, "impebreak" }, + { DMI_DMSTATUS, DMI_DMSTATUS_ALLHAVERESET, "allhavereset" }, + { DMI_DMSTATUS, DMI_DMSTATUS_ANYHAVERESET, "anyhavereset" }, { DMI_DMSTATUS, DMI_DMSTATUS_ALLRESUMEACK, "allresumeack" }, { DMI_DMSTATUS, DMI_DMSTATUS_ANYRESUMEACK, "anyresumeack" }, { DMI_DMSTATUS, DMI_DMSTATUS_ALLNONEXISTENT, "allnonexistent" }, @@ -1348,6 +1351,14 @@ static int examine(struct target *target) } } + /* TODO: We read dmstatus above, then in is_halted, then several times + * in halt_current_hart, and then again here. */ + if (dmstatus_read(target, &s, true) != ERROR_OK) + return ERROR_FAIL; + if (get_field(s, DMI_DMSTATUS_ANYHAVERESET)) + dmi_write(target, DMI_DMCONTROL, + DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET); + /* Without knowing anything else we can at least mess with the * program buffer. */ r->debug_buffer_size[i] = info->progbufsize; @@ -1572,9 +1583,24 @@ static int deassert_reset(struct target *target) int dmi_busy_delay = info->dmi_busy_delay; time_t start = time(NULL); + LOG_DEBUG("Waiting for hart to be reset."); + do { + if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) + return ERROR_FAIL; + + if (time(NULL) - start > riscv_reset_timeout_sec) { + LOG_ERROR("Hart didn't reset in %ds; dmstatus=0x%x; " + "Increase the timeout with riscv set_reset_timeout_sec.", + riscv_reset_timeout_sec, dmstatus); + return ERROR_FAIL; + } + } while (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET) == 0); + /* Ack reset. */ + dmi_write(target, DMI_DMCONTROL, control | DMI_DMCONTROL_ACKHAVERESET); + if (target->reset_halt) { LOG_DEBUG("Waiting for hart to be halted."); - do { + while (get_field(dmstatus, DMI_DMSTATUS_ALLHALTED) == 0) { if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return ERROR_FAIL; if (time(NULL) - start > riscv_reset_timeout_sec) { @@ -1584,15 +1610,15 @@ static int deassert_reset(struct target *target) riscv_reset_timeout_sec, dmstatus); return ERROR_FAIL; } - target->state = TARGET_HALTED; - } while (get_field(dmstatus, DMI_DMSTATUS_ALLHALTED) == 0); + } + target->state = TARGET_HALTED; control = set_field(control, DMI_DMCONTROL_HALTREQ, 0); dmi_write(target, DMI_DMCONTROL, control); } else { LOG_DEBUG("Waiting for hart to be running."); - do { + while (get_field(dmstatus, DMI_DMSTATUS_ALLRUNNING) == 0) { if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return ERROR_FAIL; if (get_field(dmstatus, DMI_DMSTATUS_ANYHALTED) || @@ -1608,7 +1634,7 @@ static int deassert_reset(struct target *target) riscv_reset_timeout_sec, dmstatus); return ERROR_FAIL; } - } while (get_field(dmstatus, DMI_DMSTATUS_ALLRUNNING) == 0); + } target->state = TARGET_RUNNING; } info->dmi_busy_delay = dmi_busy_delay; @@ -2538,7 +2564,7 @@ static bool riscv013_is_halted(struct target *target) if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return false; if (get_field(dmstatus, DMI_DMSTATUS_ANYUNAVAIL)) - LOG_ERROR("hart %d is unavailiable", riscv_current_hartid(target)); + LOG_ERROR("hart %d is unavailable", riscv_current_hartid(target)); if (get_field(dmstatus, DMI_DMSTATUS_ANYNONEXISTENT)) LOG_ERROR("hart %d doesn't exist", riscv_current_hartid(target)); return get_field(dmstatus, DMI_DMSTATUS_ALLHALTED); From 4d2d1f73245c762ddcefed144cbf9fdf7149e469 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Fri, 16 Mar 2018 15:02:47 -0700 Subject: [PATCH 3/9] Notice when a hart has reset. Attempt to notify the user. Deal correctly with a halted target that is suddenly running. Change-Id: Ib0e0aa843d1da22df673713687ec884f6af14949 --- src/target/riscv/riscv-013.c | 15 ++++++++++---- src/target/riscv/riscv.c | 38 +++++++++++++++++++++--------------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index fc1b8b5da..19493a9ed 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1107,10 +1107,9 @@ static int register_read_direct(struct target *target, uint64_t *value, uint32_t int result = register_read_abstract(target, value, number, register_size(target, number)); - if (result != ERROR_OK && info->progbufsize + r->impebreak >= 2 && - riscv_is_halted(target)) { - assert(number != GDB_REGNO_S0); - + if (result != ERROR_OK && + info->progbufsize + r->impebreak >= 2 && + number > GDB_REGNO_XPR31) { struct riscv_program program; riscv_program_init(&program, target); @@ -2567,6 +2566,14 @@ static bool riscv013_is_halted(struct target *target) LOG_ERROR("hart %d is unavailable", riscv_current_hartid(target)); if (get_field(dmstatus, DMI_DMSTATUS_ANYNONEXISTENT)) LOG_ERROR("hart %d doesn't exist", riscv_current_hartid(target)); + if (get_field(dmstatus, DMI_DMSTATUS_ANYHAVERESET)) { + int hartid = riscv_current_hartid(target); + LOG_INFO("hart %d unexpectedly reset!", hartid); + /* TODO: Can we make this more obvious to eg. a gdb user? */ + dmi_write(target, DMI_DMCONTROL, + set_field(DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET, + hartsel_mask(target), hartid)); + } return get_field(dmstatus, DMI_DMSTATUS_ALLHALTED); } diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 15ac8318c..6b2658353 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -969,7 +969,8 @@ int riscv_blank_check_memory(struct target *target, enum riscv_poll_hart { RPH_NO_CHANGE, - RPH_CHANGE, + RPH_DISCOVERED_HALTED, + RPH_DISCOVERED_RUNNING, RPH_ERROR }; static enum riscv_poll_hart riscv_poll_hart(struct target *target, int hartid) @@ -978,15 +979,19 @@ static enum riscv_poll_hart riscv_poll_hart(struct target *target, int hartid) if (riscv_set_current_hartid(target, hartid) != ERROR_OK) return RPH_ERROR; - LOG_DEBUG("polling hart %d, target->state=%d (TARGET_HALTED=%d)", hartid, - target->state, TARGET_HALTED); + LOG_DEBUG("polling hart %d, target->state=%d", hartid, target->state); /* If OpenOCD thinks we're running but this hart is halted then it's time * to raise an event. */ - if (target->state != TARGET_HALTED && riscv_is_halted(target)) { + bool halted = riscv_is_halted(target); + if (target->state != TARGET_HALTED && halted) { LOG_DEBUG(" triggered a halt"); r->on_halt(target); - return RPH_CHANGE; + return RPH_DISCOVERED_HALTED; + } else if (target->state != TARGET_RUNNING && !halted) { + LOG_DEBUG(" triggered running"); + target->state = TARGET_RUNNING; + return RPH_DISCOVERED_RUNNING; } return RPH_NO_CHANGE; @@ -996,26 +1001,27 @@ static enum riscv_poll_hart riscv_poll_hart(struct target *target, int hartid) int riscv_openocd_poll(struct target *target) { LOG_DEBUG("polling all harts"); - int triggered_hart = -1; + int halted_hart = -1; if (riscv_rtos_enabled(target)) { /* Check every hart for an event. */ for (int i = 0; i < riscv_count_harts(target); ++i) { enum riscv_poll_hart out = riscv_poll_hart(target, i); switch (out) { case RPH_NO_CHANGE: + case RPH_DISCOVERED_RUNNING: continue; - case RPH_CHANGE: - triggered_hart = i; + case RPH_DISCOVERED_HALTED: + halted_hart = i; break; case RPH_ERROR: return ERROR_FAIL; } } - if (triggered_hart == -1) { + if (halted_hart == -1) { LOG_DEBUG(" no harts just halted, target->state=%d", target->state); return ERROR_OK; } - LOG_DEBUG(" hart %d halted", triggered_hart); + LOG_DEBUG(" hart %d halted", halted_hart); /* If we're here then at least one hart triggered. That means * we want to go and halt _every_ hart in the system, as that's @@ -1028,17 +1034,17 @@ int riscv_openocd_poll(struct target *target) } else { enum riscv_poll_hart out = riscv_poll_hart(target, riscv_current_hartid(target)); - if (out == RPH_NO_CHANGE) + if (out == RPH_NO_CHANGE || out == RPH_DISCOVERED_RUNNING) return ERROR_OK; else if (out == RPH_ERROR) return ERROR_FAIL; - triggered_hart = riscv_current_hartid(target); - LOG_DEBUG(" hart %d halted", triggered_hart); + halted_hart = riscv_current_hartid(target); + LOG_DEBUG(" hart %d halted", halted_hart); } target->state = TARGET_HALTED; - switch (riscv_halt_reason(target, triggered_hart)) { + switch (riscv_halt_reason(target, halted_hart)) { case RISCV_HALT_BREAKPOINT: target->debug_reason = DBG_REASON_BREAKPOINT; break; @@ -1059,8 +1065,8 @@ int riscv_openocd_poll(struct target *target) } if (riscv_rtos_enabled(target)) { - target->rtos->current_threadid = triggered_hart + 1; - target->rtos->current_thread = triggered_hart + 1; + target->rtos->current_threadid = halted_hart + 1; + target->rtos->current_thread = halted_hart + 1; } target->state = TARGET_HALTED; From 40e0c5b9760c2bd5540620835ba34cff66dd660a Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Mon, 19 Mar 2018 12:46:10 -0700 Subject: [PATCH 4/9] Format error messages. Change-Id: I50c21319765e1ead279223466ed02a06ecf6a522 --- src/target/riscv/riscv-013.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 19493a9ed..aed485042 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -2563,12 +2563,12 @@ static bool riscv013_is_halted(struct target *target) if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return false; if (get_field(dmstatus, DMI_DMSTATUS_ANYUNAVAIL)) - LOG_ERROR("hart %d is unavailable", riscv_current_hartid(target)); + LOG_ERROR("Hart %d is unavailable.", riscv_current_hartid(target)); if (get_field(dmstatus, DMI_DMSTATUS_ANYNONEXISTENT)) - LOG_ERROR("hart %d doesn't exist", riscv_current_hartid(target)); + LOG_ERROR("Hart %d doesn't exist.", riscv_current_hartid(target)); if (get_field(dmstatus, DMI_DMSTATUS_ANYHAVERESET)) { int hartid = riscv_current_hartid(target); - LOG_INFO("hart %d unexpectedly reset!", hartid); + LOG_INFO("Hart %d unexpectedly reset!", hartid); /* TODO: Can we make this more obvious to eg. a gdb user? */ dmi_write(target, DMI_DMCONTROL, set_field(DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET, From e5591c258412d421d66b9efd285ff3c3c14dcac6 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Tue, 20 Mar 2018 12:34:17 -0700 Subject: [PATCH 5/9] Halt the target again if it was reset while halted Change-Id: I59707e7b2e1646c312d4eb8e96e9d7dfd1e128c2 --- src/target/riscv/riscv-013.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index aed485042..142b5f705 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -2570,9 +2570,17 @@ static bool riscv013_is_halted(struct target *target) int hartid = riscv_current_hartid(target); LOG_INFO("Hart %d unexpectedly reset!", hartid); /* TODO: Can we make this more obvious to eg. a gdb user? */ - dmi_write(target, DMI_DMCONTROL, - set_field(DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET, - hartsel_mask(target), hartid)); + uint32_t dmcontrol = DMI_DMCONTROL_DMACTIVE | + DMI_DMCONTROL_ACKHAVERESET; + dmcontrol = set_field(dmcontrol, hartsel_mask(target), hartid); + /* If we had been halted when we reset, request another halt. If we + * ended up running out of reset, then the user will (hopefully) get a + * message that a reset happened, that the target is running, and then + * that it is halted again once the request goes through. + */ + if (target->state == TARGET_HALTED) + dmcontrol |= DMI_DMCONTROL_HALTREQ; + dmi_write(target, DMI_DMCONTROL, dmcontrol); } return get_field(dmstatus, DMI_DMSTATUS_ALLHALTED); } From d7282d0bfe35ef06471f28c486f2de02d6917bd8 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Thu, 22 Mar 2018 12:44:15 -0700 Subject: [PATCH 6/9] Add set_supports_havereset This lets reset work on targets that don't implement havereset. Change-Id: I09eb20970fac740eb6465541db6e739ae3e6b0d5 --- src/target/riscv/riscv-013.c | 61 +++++++++++++++++------------------- src/target/riscv/riscv.c | 22 +++++++++++++ src/target/riscv/riscv.h | 3 ++ 3 files changed, 54 insertions(+), 32 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 142b5f705..e5edfef24 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1343,6 +1343,11 @@ static int examine(struct target *target) break; r->hart_count = i + 1; + if (!riscv_havereset_not_supported && + get_field(s, DMI_DMSTATUS_ANYHAVERESET)) + dmi_write(target, DMI_DMCONTROL, + DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET); + if (!riscv_is_halted(target)) { if (riscv013_halt_current_hart(target) != ERROR_OK) { LOG_ERROR("Fatal: Hart %d failed to halt during examine()", i); @@ -1350,14 +1355,6 @@ static int examine(struct target *target) } } - /* TODO: We read dmstatus above, then in is_halted, then several times - * in halt_current_hart, and then again here. */ - if (dmstatus_read(target, &s, true) != ERROR_OK) - return ERROR_FAIL; - if (get_field(s, DMI_DMSTATUS_ANYHAVERESET)) - dmi_write(target, DMI_DMCONTROL, - DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET); - /* Without knowing anything else we can at least mess with the * program buffer. */ r->debug_buffer_size[i] = info->progbufsize; @@ -1569,8 +1566,6 @@ static int deassert_reset(struct target *target) RISCV013_INFO(info); select_dmi(target); - LOG_DEBUG("%d", r->current_hartid); - /* Clear the reset, but make sure haltreq is still set */ uint32_t control = 0; control = set_field(control, DMI_DMCONTROL_HALTREQ, target->reset_halt ? 1 : 0); @@ -1582,31 +1577,33 @@ static int deassert_reset(struct target *target) int dmi_busy_delay = info->dmi_busy_delay; time_t start = time(NULL); - LOG_DEBUG("Waiting for hart to be reset."); - do { - if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) - return ERROR_FAIL; + if (!riscv_havereset_not_supported) { + LOG_DEBUG("Waiting for hart %d to be reset.", r->current_hartid); + do { + if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) + return ERROR_FAIL; - if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart didn't reset in %ds; dmstatus=0x%x; " - "Increase the timeout with riscv set_reset_timeout_sec.", - riscv_reset_timeout_sec, dmstatus); - return ERROR_FAIL; - } - } while (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET) == 0); - /* Ack reset. */ - dmi_write(target, DMI_DMCONTROL, control | DMI_DMCONTROL_ACKHAVERESET); + if (time(NULL) - start > riscv_reset_timeout_sec) { + LOG_ERROR("Hart %d didn't reset in %ds; dmstatus=0x%x; " + "Increase the timeout with riscv set_reset_timeout_sec.", + r->current_hartid, riscv_reset_timeout_sec, dmstatus); + return ERROR_FAIL; + } + } while (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET) == 0); + /* Ack reset. */ + dmi_write(target, DMI_DMCONTROL, control | DMI_DMCONTROL_ACKHAVERESET); + } if (target->reset_halt) { - LOG_DEBUG("Waiting for hart to be halted."); + LOG_DEBUG("Waiting for hart %d to be halted.", r->current_hartid); while (get_field(dmstatus, DMI_DMSTATUS_ALLHALTED) == 0) { if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return ERROR_FAIL; if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart didn't halt coming out of reset in %ds; " + LOG_ERROR("Hart %d didn't halt coming out of reset in %ds; " "dmstatus=0x%x; " "Increase the timeout with riscv set_reset_timeout_sec.", - riscv_reset_timeout_sec, dmstatus); + r->current_hartid, riscv_reset_timeout_sec, dmstatus); return ERROR_FAIL; } } @@ -1616,21 +1613,21 @@ static int deassert_reset(struct target *target) dmi_write(target, DMI_DMCONTROL, control); } else { - LOG_DEBUG("Waiting for hart to be running."); + LOG_DEBUG("Waiting for hart %d to be running.", r->current_hartid); while (get_field(dmstatus, DMI_DMSTATUS_ALLRUNNING) == 0) { if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return ERROR_FAIL; if (get_field(dmstatus, DMI_DMSTATUS_ANYHALTED) || get_field(dmstatus, DMI_DMSTATUS_ANYUNAVAIL)) { - LOG_ERROR("Unexpected hart status during reset. dmstatus=0x%x", - dmstatus); + LOG_ERROR("Unexpected hart %d status during reset. dmstatus=0x%x", + r->current_hartid, dmstatus); return ERROR_FAIL; } if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart didn't run coming out of reset in %ds; " + LOG_ERROR("Hart %d didn't run coming out of reset in %ds; " "dmstatus=0x%x; " "Increase the timeout with riscv set_reset_timeout_sec.", - riscv_reset_timeout_sec, dmstatus); + r->current_hartid, riscv_reset_timeout_sec, dmstatus); return ERROR_FAIL; } } @@ -2566,7 +2563,7 @@ static bool riscv013_is_halted(struct target *target) LOG_ERROR("Hart %d is unavailable.", riscv_current_hartid(target)); if (get_field(dmstatus, DMI_DMSTATUS_ANYNONEXISTENT)) LOG_ERROR("Hart %d doesn't exist.", riscv_current_hartid(target)); - if (get_field(dmstatus, DMI_DMSTATUS_ANYHAVERESET)) { + if (!riscv_havereset_not_supported && get_field(dmstatus, DMI_DMSTATUS_ANYHAVERESET)) { int hartid = riscv_current_hartid(target); LOG_INFO("Hart %d unexpectedly reset!", hartid); /* TODO: Can we make this more obvious to eg. a gdb user? */ diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 6b2658353..72afb855a 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -188,6 +188,8 @@ int riscv_reset_timeout_sec = DEFAULT_RESET_TIMEOUT_SEC; bool riscv_use_scratch_ram; uint64_t riscv_scratch_ram_address; +bool riscv_havereset_not_supported; + /* In addition to the ones in the standard spec, we'll also expose additional * CSRs in this list. * The list is either NULL, or a series of ranges (inclusive), terminated with @@ -1247,6 +1249,17 @@ COMMAND_HANDLER(riscv_set_scratch_ram) return ERROR_OK; } +COMMAND_HANDLER(riscv_set_supports_havereset) +{ + if (CMD_ARGC != 1) { + LOG_ERROR("Command takes exactly 1 parameter"); + return ERROR_COMMAND_SYNTAX_ERROR; + } + COMMAND_PARSE_ON_OFF(CMD_ARGV[0], riscv_havereset_not_supported); + riscv_havereset_not_supported = !riscv_havereset_not_supported; + return ERROR_OK; +} + void parse_error(const char *string, char c, unsigned position) { char buf[position+2]; @@ -1458,6 +1471,15 @@ static const struct command_registration riscv_exec_command_handlers[] = { .usage = "riscv set_scratch_ram none|[address]", .help = "Set address of 16 bytes of scratch RAM the debugger can use, or 'none'." }, + { + .name = "set_supports_havereset", + .handler = riscv_set_supports_havereset, + .mode = COMMAND_ANY, + .usage = "riscv set_supports_havereset on|off", + .help = "Set this off if one of the targets doesn't implement " + "allhavereset/anyhavereset in dmcontrol. When on, OpenOCD will wait " + "for these bits to go high during a reset." + }, { .name = "expose_csrs", .handler = riscv_set_expose_csrs, diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 1e6ffb94b..9bcf7f2c9 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -128,6 +128,9 @@ extern int riscv_reset_timeout_sec; extern bool riscv_use_scratch_ram; extern uint64_t riscv_scratch_ram_address; +/* Negative so that the default value of 0 is what we want. */ +extern bool riscv_havereset_not_supported; + /* Everything needs the RISC-V specific info structure, so here's a nice macro * that provides that. */ static inline riscv_info_t *riscv_info(const struct target *target) __attribute__((unused)); From c534a37fc35ee43964766bc8ac39e91d86d23f02 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Fri, 23 Mar 2018 12:53:24 -0700 Subject: [PATCH 7/9] Make reset work again for multicore Both regular multicore and RTOS hack methods. Change-Id: I9a0998de0f33ef8a4d163f36ddf01c7675893b3d --- src/target/riscv/batch.c | 5 +- src/target/riscv/riscv-013.c | 154 +++++++++++++++++------------------ 2 files changed, 79 insertions(+), 80 deletions(-) diff --git a/src/target/riscv/batch.c b/src/target/riscv/batch.c index 117119dd6..9327cb38b 100644 --- a/src/target/riscv/batch.c +++ b/src/target/riscv/batch.c @@ -160,11 +160,10 @@ void dump_field(const struct scan_field *field) log_printf_lf(LOG_LVL_DEBUG, __FILE__, __LINE__, __PRETTY_FUNCTION__, - "%db %s %08x @%02x -> %s %08x @%02x [0x%p -> 0x%p]", + "%db %s %08x @%02x -> %s %08x @%02x", field->num_bits, op_string[out_op], out_data, out_address, - status_string[in_op], in_data, in_address, - field->out_value, field->in_value); + status_string[in_op], in_data, in_address); } else { log_printf_lf(LOG_LVL_DEBUG, __FILE__, __LINE__, __PRETTY_FUNCTION__, "%db %s %08x @%02x -> ?", diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index e5edfef24..31edae6a4 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1346,7 +1346,8 @@ static int examine(struct target *target) if (!riscv_havereset_not_supported && get_field(s, DMI_DMSTATUS_ANYHAVERESET)) dmi_write(target, DMI_DMCONTROL, - DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET); + set_field(DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET, + hartsel_mask(target), i)); if (!riscv_is_halted(target)) { if (riscv013_halt_current_hart(target) != ERROR_OK) { @@ -1518,7 +1519,7 @@ static int assert_reset(struct target *target) /* TODO: Try to use hasel in dmcontrol */ - /* Set haltreq/resumereq for each hart. */ + /* Set haltreq for each hart. */ uint32_t control = control_base; for (int i = 0; i < riscv_count_harts(target); ++i) { if (!riscv_hart_enabled(target, i)) @@ -1539,20 +1540,8 @@ static int assert_reset(struct target *target) r->current_hartid); control = set_field(control, DMI_DMCONTROL_HALTREQ, target->reset_halt ? 1 : 0); - control = set_field(control, DMI_DMCONTROL_HARTRESET, 1); + control = set_field(control, DMI_DMCONTROL_NDMRESET, 1); dmi_write(target, DMI_DMCONTROL, control); - - /* Read back to check if hartreset is supported. */ - uint32_t rb; - if (dmi_read(target, &rb, DMI_DMCONTROL) != ERROR_OK) - return ERROR_FAIL; - if (!get_field(rb, DMI_DMCONTROL_HARTRESET)) { - /* Use ndmreset instead. That will reset the entire device, but - * that's probably what OpenOCD wants anyway. */ - control = set_field(control, DMI_DMCONTROL_HARTRESET, 0); - control = set_field(control, DMI_DMCONTROL_NDMRESET, 1); - dmi_write(target, DMI_DMCONTROL, control); - } } target->state = TARGET_RESET; @@ -1569,69 +1558,83 @@ static int deassert_reset(struct target *target) /* Clear the reset, but make sure haltreq is still set */ uint32_t control = 0; control = set_field(control, DMI_DMCONTROL_HALTREQ, target->reset_halt ? 1 : 0); - control = set_field(control, hartsel_mask(target), r->current_hartid); control = set_field(control, DMI_DMCONTROL_DMACTIVE, 1); - dmi_write(target, DMI_DMCONTROL, control); + dmi_write(target, DMI_DMCONTROL, + set_field(control, hartsel_mask(target), r->current_hartid)); uint32_t dmstatus; int dmi_busy_delay = info->dmi_busy_delay; time_t start = time(NULL); - if (!riscv_havereset_not_supported) { - LOG_DEBUG("Waiting for hart %d to be reset.", r->current_hartid); - do { - if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) - return ERROR_FAIL; - - if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart %d didn't reset in %ds; dmstatus=0x%x; " - "Increase the timeout with riscv set_reset_timeout_sec.", - r->current_hartid, riscv_reset_timeout_sec, dmstatus); - return ERROR_FAIL; - } - } while (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET) == 0); - /* Ack reset. */ - dmi_write(target, DMI_DMCONTROL, control | DMI_DMCONTROL_ACKHAVERESET); - } - - if (target->reset_halt) { - LOG_DEBUG("Waiting for hart %d to be halted.", r->current_hartid); - while (get_field(dmstatus, DMI_DMSTATUS_ALLHALTED) == 0) { - if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) - return ERROR_FAIL; - if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart %d didn't halt coming out of reset in %ds; " - "dmstatus=0x%x; " - "Increase the timeout with riscv set_reset_timeout_sec.", - r->current_hartid, riscv_reset_timeout_sec, dmstatus); - return ERROR_FAIL; - } + for (int i = 0; i < riscv_count_harts(target); ++i) { + int index = i; + if (target->rtos) { + if (!riscv_hart_enabled(target, index)) + continue; + dmi_write(target, DMI_DMCONTROL, + set_field(control, hartsel_mask(target), index)); + } else { + index = r->current_hartid; } - target->state = TARGET_HALTED; - control = set_field(control, DMI_DMCONTROL_HALTREQ, 0); - dmi_write(target, DMI_DMCONTROL, control); + if (!riscv_havereset_not_supported) { + LOG_DEBUG("Waiting for hart %d to be reset.", index); + do { + if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) + return ERROR_FAIL; - } else { - LOG_DEBUG("Waiting for hart %d to be running.", r->current_hartid); - while (get_field(dmstatus, DMI_DMSTATUS_ALLRUNNING) == 0) { - if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) - return ERROR_FAIL; - if (get_field(dmstatus, DMI_DMSTATUS_ANYHALTED) || - get_field(dmstatus, DMI_DMSTATUS_ANYUNAVAIL)) { - LOG_ERROR("Unexpected hart %d status during reset. dmstatus=0x%x", - r->current_hartid, dmstatus); - return ERROR_FAIL; - } - if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart %d didn't run coming out of reset in %ds; " - "dmstatus=0x%x; " - "Increase the timeout with riscv set_reset_timeout_sec.", - r->current_hartid, riscv_reset_timeout_sec, dmstatus); - return ERROR_FAIL; - } + if (time(NULL) - start > riscv_reset_timeout_sec) { + LOG_ERROR("Hart %d didn't reset in %ds; dmstatus=0x%x; " + "Increase the timeout with riscv set_reset_timeout_sec.", + r->current_hartid, riscv_reset_timeout_sec, dmstatus); + return ERROR_FAIL; + } + } while (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET) == 0); + /* Ack reset. */ + dmi_write(target, DMI_DMCONTROL, + set_field(control, hartsel_mask(target), index) | + DMI_DMCONTROL_ACKHAVERESET); } - target->state = TARGET_RUNNING; + + if (target->reset_halt) { + LOG_DEBUG("Waiting for hart %d to be halted.", index); + do { + if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) + return ERROR_FAIL; + if (time(NULL) - start > riscv_reset_timeout_sec) { + LOG_ERROR("Hart %d didn't halt coming out of reset in %ds; " + "dmstatus=0x%x; " + "Increase the timeout with riscv set_reset_timeout_sec.", + index, riscv_reset_timeout_sec, dmstatus); + return ERROR_FAIL; + } + } while (get_field(dmstatus, DMI_DMSTATUS_ALLHALTED) == 0); + target->state = TARGET_HALTED; + + } else { + LOG_DEBUG("Waiting for hart %d to be running.", index); + while (get_field(dmstatus, DMI_DMSTATUS_ALLRUNNING) == 0) { + if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) + return ERROR_FAIL; + if (get_field(dmstatus, DMI_DMSTATUS_ANYHALTED) || + get_field(dmstatus, DMI_DMSTATUS_ANYUNAVAIL)) { + LOG_ERROR("Unexpected hart %d status during reset. dmstatus=0x%x", + index, dmstatus); + return ERROR_FAIL; + } + if (time(NULL) - start > riscv_reset_timeout_sec) { + LOG_ERROR("Hart %d didn't run coming out of reset in %ds; " + "dmstatus=0x%x; " + "Increase the timeout with riscv set_reset_timeout_sec.", + index, riscv_reset_timeout_sec, dmstatus); + return ERROR_FAIL; + } + } + target->state = TARGET_RUNNING; + } + + if (!target->rtos) + break; } info->dmi_busy_delay = dmi_busy_delay; return ERROR_OK; @@ -2707,11 +2710,9 @@ static int riscv013_step_or_resume_current_hart(struct target *target, bool step return ERROR_FAIL; /* Issue the resume command, and then wait for the current hart to resume. */ - uint32_t dmcontrol; - if (dmi_read(target, &dmcontrol, DMI_DMCONTROL) != ERROR_OK) - return ERROR_FAIL; - dmcontrol = set_field(dmcontrol, DMI_DMCONTROL_RESUMEREQ, 1); - dmi_write(target, DMI_DMCONTROL, dmcontrol); + uint32_t dmcontrol = DMI_DMCONTROL_DMACTIVE; + dmcontrol = set_field(dmcontrol, hartsel_mask(target), r->current_hartid); + dmi_write(target, DMI_DMCONTROL, dmcontrol | DMI_DMCONTROL_RESUMEREQ); uint32_t dmstatus; for (size_t i = 0; i < 256; ++i) { @@ -2723,17 +2724,16 @@ static int riscv013_step_or_resume_current_hart(struct target *target, bool step if (step && get_field(dmstatus, DMI_DMSTATUS_ALLHALTED) == 0) continue; - dmcontrol = set_field(dmcontrol, DMI_DMCONTROL_RESUMEREQ, 0); dmi_write(target, DMI_DMCONTROL, dmcontrol); return ERROR_OK; } - if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) - return ERROR_FAIL; + LOG_ERROR("unable to resume hart %d", r->current_hartid); if (dmi_read(target, &dmcontrol, DMI_DMCONTROL) != ERROR_OK) return ERROR_FAIL; - LOG_ERROR("unable to resume hart %d", r->current_hartid); LOG_ERROR(" dmcontrol=0x%08x", dmcontrol); + if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) + return ERROR_FAIL; LOG_ERROR(" dmstatus =0x%08x", dmstatus); if (step) { From 55e427b72b2772f85510cf9e27d43175dd538e9c Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Tue, 27 Mar 2018 11:31:39 -0700 Subject: [PATCH 8/9] Don't rely on havereset when deasserting reset. This removes the need for the supports_havereset config option as well. Change-Id: Ic4391ce8c15d15e2ef662d170d483f336e8e8a5e --- src/target/riscv/riscv-013.c | 35 +++++++++++------------------------ src/target/riscv/riscv.c | 22 ---------------------- src/target/riscv/riscv.h | 3 --- 3 files changed, 11 insertions(+), 49 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 31edae6a4..337625ad3 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -1343,8 +1343,7 @@ static int examine(struct target *target) break; r->hart_count = i + 1; - if (!riscv_havereset_not_supported && - get_field(s, DMI_DMSTATUS_ANYHAVERESET)) + if (get_field(s, DMI_DMSTATUS_ANYHAVERESET)) dmi_write(target, DMI_DMCONTROL, set_field(DMI_DMCONTROL_DMACTIVE | DMI_DMCONTROL_ACKHAVERESET, hartsel_mask(target), i)); @@ -1577,27 +1576,8 @@ static int deassert_reset(struct target *target) index = r->current_hartid; } - if (!riscv_havereset_not_supported) { - LOG_DEBUG("Waiting for hart %d to be reset.", index); - do { - if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) - return ERROR_FAIL; - - if (time(NULL) - start > riscv_reset_timeout_sec) { - LOG_ERROR("Hart %d didn't reset in %ds; dmstatus=0x%x; " - "Increase the timeout with riscv set_reset_timeout_sec.", - r->current_hartid, riscv_reset_timeout_sec, dmstatus); - return ERROR_FAIL; - } - } while (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET) == 0); - /* Ack reset. */ - dmi_write(target, DMI_DMCONTROL, - set_field(control, hartsel_mask(target), index) | - DMI_DMCONTROL_ACKHAVERESET); - } - if (target->reset_halt) { - LOG_DEBUG("Waiting for hart %d to be halted.", index); + LOG_DEBUG("Waiting for hart %d to halt out of reset.", index); do { if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return ERROR_FAIL; @@ -1612,7 +1592,7 @@ static int deassert_reset(struct target *target) target->state = TARGET_HALTED; } else { - LOG_DEBUG("Waiting for hart %d to be running.", index); + LOG_DEBUG("Waiting for hart %d to run out of reset.", index); while (get_field(dmstatus, DMI_DMSTATUS_ALLRUNNING) == 0) { if (dmstatus_read(target, &dmstatus, true) != ERROR_OK) return ERROR_FAIL; @@ -1633,6 +1613,13 @@ static int deassert_reset(struct target *target) target->state = TARGET_RUNNING; } + if (get_field(dmstatus, DMI_DMSTATUS_ALLHAVERESET)) { + /* Ack reset. */ + dmi_write(target, DMI_DMCONTROL, + set_field(control, hartsel_mask(target), index) | + DMI_DMCONTROL_ACKHAVERESET); + } + if (!target->rtos) break; } @@ -2566,7 +2553,7 @@ static bool riscv013_is_halted(struct target *target) LOG_ERROR("Hart %d is unavailable.", riscv_current_hartid(target)); if (get_field(dmstatus, DMI_DMSTATUS_ANYNONEXISTENT)) LOG_ERROR("Hart %d doesn't exist.", riscv_current_hartid(target)); - if (!riscv_havereset_not_supported && get_field(dmstatus, DMI_DMSTATUS_ANYHAVERESET)) { + if (get_field(dmstatus, DMI_DMSTATUS_ANYHAVERESET)) { int hartid = riscv_current_hartid(target); LOG_INFO("Hart %d unexpectedly reset!", hartid); /* TODO: Can we make this more obvious to eg. a gdb user? */ diff --git a/src/target/riscv/riscv.c b/src/target/riscv/riscv.c index 72afb855a..6b2658353 100644 --- a/src/target/riscv/riscv.c +++ b/src/target/riscv/riscv.c @@ -188,8 +188,6 @@ int riscv_reset_timeout_sec = DEFAULT_RESET_TIMEOUT_SEC; bool riscv_use_scratch_ram; uint64_t riscv_scratch_ram_address; -bool riscv_havereset_not_supported; - /* In addition to the ones in the standard spec, we'll also expose additional * CSRs in this list. * The list is either NULL, or a series of ranges (inclusive), terminated with @@ -1249,17 +1247,6 @@ COMMAND_HANDLER(riscv_set_scratch_ram) return ERROR_OK; } -COMMAND_HANDLER(riscv_set_supports_havereset) -{ - if (CMD_ARGC != 1) { - LOG_ERROR("Command takes exactly 1 parameter"); - return ERROR_COMMAND_SYNTAX_ERROR; - } - COMMAND_PARSE_ON_OFF(CMD_ARGV[0], riscv_havereset_not_supported); - riscv_havereset_not_supported = !riscv_havereset_not_supported; - return ERROR_OK; -} - void parse_error(const char *string, char c, unsigned position) { char buf[position+2]; @@ -1471,15 +1458,6 @@ static const struct command_registration riscv_exec_command_handlers[] = { .usage = "riscv set_scratch_ram none|[address]", .help = "Set address of 16 bytes of scratch RAM the debugger can use, or 'none'." }, - { - .name = "set_supports_havereset", - .handler = riscv_set_supports_havereset, - .mode = COMMAND_ANY, - .usage = "riscv set_supports_havereset on|off", - .help = "Set this off if one of the targets doesn't implement " - "allhavereset/anyhavereset in dmcontrol. When on, OpenOCD will wait " - "for these bits to go high during a reset." - }, { .name = "expose_csrs", .handler = riscv_set_expose_csrs, diff --git a/src/target/riscv/riscv.h b/src/target/riscv/riscv.h index 9bcf7f2c9..1e6ffb94b 100644 --- a/src/target/riscv/riscv.h +++ b/src/target/riscv/riscv.h @@ -128,9 +128,6 @@ extern int riscv_reset_timeout_sec; extern bool riscv_use_scratch_ram; extern uint64_t riscv_scratch_ram_address; -/* Negative so that the default value of 0 is what we want. */ -extern bool riscv_havereset_not_supported; - /* Everything needs the RISC-V specific info structure, so here's a nice macro * that provides that. */ static inline riscv_info_t *riscv_info(const struct target *target) __attribute__((unused)); From 755c6a4caa8fb6f7aef54d9568533c60e889d6e0 Mon Sep 17 00:00:00 2001 From: Tim Newsome Date: Fri, 30 Mar 2018 15:24:16 -0700 Subject: [PATCH 9/9] Add wall clock timeout to dmi_op() If the target is held in reset we'd keep adding more delays, and since those grow exponentially they'd get so huge it would take forever to exit out of the loop. Change-Id: Ieaab8b124c101fd1b12f81f905a6de22192ac662 --- src/target/riscv/riscv-013.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/target/riscv/riscv-013.c b/src/target/riscv/riscv-013.c index 337625ad3..467056940 100644 --- a/src/target/riscv/riscv-013.c +++ b/src/target/riscv/riscv-013.c @@ -490,8 +490,6 @@ static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, dmi_status_t status; uint32_t address_in; - unsigned i = 0; - const char *op_name; switch (dmi_op) { case DMI_OP_NOP: @@ -508,9 +506,10 @@ static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, return ERROR_FAIL; } + time_t start = time(NULL); /* This first loop performs the request. Note that if for some reason this * stays busy, it is actually due to the previous access. */ - for (i = 0; i < 256; i++) { + while (1) { status = dmi_scan(target, NULL, NULL, dmi_op, address, data_out, false); if (status == DMI_STATUS_BUSY) { @@ -521,6 +520,13 @@ static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, LOG_ERROR("failed %s at 0x%x, status=%d", op_name, address, status); return ERROR_FAIL; } + if (time(NULL) - start > riscv_command_timeout_sec) { + LOG_ERROR("dmi.op is still busy after %d seconds. The target is " + "either really slow or broken. You could increase the " + "timeout with riscv set_command_timeout_sec.", + riscv_command_timeout_sec); + return ERROR_FAIL; + } } if (status != DMI_STATUS_SUCCESS) { @@ -531,7 +537,7 @@ static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, /* This second loop ensures the request succeeded, and gets back data. * Note that NOP can result in a 'busy' result as well, but that would be * noticed on the next DMI access we do. */ - for (i = 0; i < 256; i++) { + while (1) { status = dmi_scan(target, &address_in, data_in, DMI_OP_NOP, address, 0, false); if (status == DMI_STATUS_BUSY) { @@ -543,6 +549,13 @@ static int dmi_op(struct target *target, uint32_t *data_in, int dmi_op, status); return ERROR_FAIL; } + if (time(NULL) - start > riscv_command_timeout_sec) { + LOG_ERROR("dmi.op is still busy after %d seconds. The target is " + "either really slow or broken. You could increase the " + "timeout with riscv set_command_timeout_sec.", + riscv_command_timeout_sec); + return ERROR_FAIL; + } } if (status != DMI_STATUS_SUCCESS) { @@ -2745,7 +2758,7 @@ void riscv013_clear_abstract_error(struct target *target) LOG_ERROR("abstractcs.busy is not going low after %d seconds " "(abstractcs=0x%x). The target is either really slow or " "broken. You could increase the timeout with riscv " - "set_reset_timeout_sec.", + "set_command_timeout_sec.", riscv_command_timeout_sec, abstractcs); break; }