Re: [Openocd-development] [PATCH] remove cmd_queue_cur_state
I need to look into details, but I believe that this breaks synchronous execution of JTAG calls (for hardware implementations of JTAG FIFOs). I believe this is what Dick was trying to point out, but I need more time to look into this. -- Øyvind Harboe Embedded software and hardware consulting services http://consulting.zylin.com ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] remove cmd_queue_cur_state
Magnus Lundin wrote:
Dick Hollenbeck skrev:
Zach,
I think this patch is fundamentally wrong and is a disaster the
moment it is applied.
The queue sits between the jtag api and the drivers. The api calls
track future state according to the most recent api call submitted
(and put onto the back end of the queue) via the cmd_queue_cur_state
variable.
The drivers consume commands in batch mode off the back end of the
queue. They have their own notion of state, and that is what the
cable helper API is for. That API has that name deliberately so
that you know these functions are for cable drivers, not for the jtag
api layer. Rather than worrying about a future state, they are
worrying about current state of the taps as the driver actually
manipulates TMS and the clock.
Right, but the situation where this came up is when the tap state, or
actually the target jtag controller changes state and the drivers must
be informed about this.
We cannot use cmd_queue_cur_state for this since it only records the
state for sanity checking.
Nonsense.
cmd_queue_cur_state is a record of the future effect of the most
recently queued command, i.e. what was last put into the queue. The
queue can be thought of as a pipe with a delay, and the contents of the
queue should eventually find its way to the driver then to the tap
controllers. Anything in the queue cannot be undone, but is not in
effect yet. Image a piece of PVC pipe and you put billiard balls into
it of different colors. There is a delay, and then the balls come out
in the same sequence some time later.
Two observers, one at the entrance to the pipe and one at the tail of
the pipe will see the same balls, but not at the same time. The balls
are analogous to the commands.
tap_set_state() is a reporting mechanism by the last observer. The
driver is reporting what it did to the taps and what state the driver
believes the actual hardware taps to be in because of actions that the
driver took in response to commands coming in via the queue. Only the
driver executes commands and therefore it is the only code which knows
what it did, and how this effects the actual taps. Therefore they
should be the only ones to call tap_set_state().
If you allow anybody but a driver to call tap_set_state() you have
perverted the purpose of tap_set_state() and you have brought on
insanity, so checking for it, as you state, is not needed, because it is
sure to be present.
You can create an accessor/variable pair for the front of the queue, but
it must be separate from the cable helper api.
I would suggest
jtag_set_future_state(),
jtag_record_future_state(), or
jtag_set_requested_state()
as possible function names for this encapsulation of the global variable
that you would like to hide.
jtag_set_requested_state( aState )
{
cmd_queue_cur_state = aState
}
jtag_get_requested_state()
{
return cmd_queue_cur_state;
}
All suggestions use a prefix different from the tap_ prefix which seems
to be common among functions in the cable helper api. Seems to be,
because that is how I wrote them.
Do not confuse what you are seeing at the entrance to the pipe and the
expected consequences of commands issued there at the jtag level, with
what is actually in the tap controllers beyond the end of the pipe and
accomplished by the drivers.
Dick
We can add a jtag_add_forced_set_state or someting similar.
Or we can do a jtag_execute_queue to flush the command queue and then
a tap_set_state().
These two concepts are separate, and cannot be merged because of the
queue. tap_set_state() is for current state of the taps. But
realize there is can be some delay here also because sometimes their
are low level commands buffered before being sent out on USB. So
tap_set_state() only tracks to the best of its ability the current
state.
Dick
Best regards
Magnus
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] remove cmd_queue_cur_state
Zach,
I think this patch is fundamentally wrong and is a disaster the moment
it is applied.
The queue sits between the jtag api and the drivers. The api calls
track future state according to the most recent api call submitted
(and put onto the back end of the queue) via the cmd_queue_cur_state
variable.
The drivers consume commands in batch mode off the back end of the
queue. They have their own notion of state, and that is what the cable
helper API is for. That API has that name deliberately so that you
know these functions are for cable drivers, not for the jtag api layer.
Rather than worrying about a future state, they are worrying about
current state of the taps as the driver actually manipulates TMS and the
clock.
These two concepts are separate, and cannot be merged because of the
queue. tap_set_state() is for current state of the taps. But realize
there is can be some delay here also because sometimes their are low
level commands buffered before being sent out on USB. So
tap_set_state() only tracks to the best of its ability the current state.
Dick
Hi all,
The attached patch removes the cmd_queue_cur_state global variable,
replacing all uses with calls to tap_set_state() or tap_get_state().
While this seems completely trivial, I wanted to post it for review.
Any objections? All in favor?
Cheers,
Zach
---
jtag/jtag.c | 13 ++---
jtag/jtag.h |3 +--
jtag/zy1000.c |4 ++--
svf/svf.c |9 ++---
target/arm11_dbgtap.c |4 ++--
xsvf/xsvf.c |4 ++--
6 files changed, 19 insertions(+), 18 deletions(-)
Index: src/jtag/zy1000.c
===
--- src/jtag/zy1000.c (revision 1893)
+++ src/jtag/zy1000.c (working copy)
@@ -709,7 +709,7 @@
int interface_jtag_add_clocks(int num_cycles)
{
- return zy1000_jtag_add_clocks(num_cycles, cmd_queue_cur_state,
cmd_queue_end_state);
+ return zy1000_jtag_add_clocks(num_cycles, tap_get_state(),
cmd_queue_end_state);
}
int interface_jtag_add_sleep(u32 us)
@@ -728,7 +728,7 @@
state_count = 0;
- tap_state_t cur_state=cmd_queue_cur_state;
+ tap_state_t cur_state = tap_get_state();
while (num_states)
{
Index: src/jtag/jtag.c
===
--- src/jtag/jtag.c (revision 1893)
+++ src/jtag/jtag.c (working copy)
@@ -94,7 +94,6 @@
enum reset_types jtag_reset_config = RESET_NONE;
tap_state_t cmd_queue_end_state = TAP_RESET;
-tap_state_t cmd_queue_cur_state = TAP_RESET;
int jtag_verify_capture_ir = 1;
int jtag_verify = 1;
@@ -565,7 +564,7 @@
if (state != TAP_INVALID)
jtag_add_end_state(state);
- cmd_queue_cur_state = cmd_queue_end_state;
+ tap_set_state(cmd_queue_end_state);
}
void jtag_add_ir_scan_noverify(int in_num_fields, const scan_field_t
*in_fields, tap_state_t state)
@@ -1066,7 +1065,7 @@
void jtag_add_pathmove(int num_states, const tap_state_t *path)
{
- tap_state_t cur_state = cmd_queue_cur_state;
+ tap_state_t cur_state = tap_get_state();
int i;
int retval;
@@ -1097,7 +1096,7 @@
jtag_prelude1();
retval = interface_jtag_add_pathmove(num_states, path);
- cmd_queue_cur_state = path[num_states - 1];
+ tap_set_state(path[num_states - 1]);
if (retval!=ERROR_OK)
jtag_error=retval;
}
@@ -1168,11 +1167,11 @@
void jtag_add_clocks( int num_cycles )
{
int retval;
-
- if( !tap_is_state_stable(cmd_queue_cur_state) )
+ tap_state_t cur_state = tap_get_state();
+ if( !tap_is_state_stable(cur_state) )
{
LOG_ERROR( jtag_add_clocks() was called with TAP in
non-stable state \%s\,
- tap_state_name(cmd_queue_cur_state) );
+ tap_state_name(cur_state) );
jtag_error = ERROR_JTAG_NOT_STABLE_STATE;
return;
}
Index: src/jtag/jtag.h
===
--- src/jtag/jtag.h (revision 1893)
+++ src/jtag/jtag.h (working copy)
@@ -256,7 +256,6 @@
extern tap_state_t cmd_queue_end_state; /* finish DR scans in
dr_end_state */
-extern tap_state_t cmd_queue_cur_state; /* current TAP state */
typedef void* error_handler_t; /* Later on we can delete error_handler_t,
but keep it for now to make patches more readable */
@@ -891,7 +890,7 @@
{
if (end_state != TAP_INVALID)
cmd_queue_end_state = end_state;
- cmd_queue_cur_state = cmd_queue_end_state;
+ tap_set_state(cmd_queue_end_state);
interface_jtag_add_dr_out(tap, num_fields, num_bits, value,
cmd_queue_end_state);
}
Index: src/target/arm11_dbgtap.c
Re: [Openocd-development] [PATCH] remove cmd_queue_cur_state
Dick Hollenbeck skrev: Zach, I think this patch is fundamentally wrong and is a disaster the moment it is applied. The queue sits between the jtag api and the drivers. The api calls track future state according to the most recent api call submitted (and put onto the back end of the queue) via the cmd_queue_cur_state variable. The drivers consume commands in batch mode off the back end of the queue. They have their own notion of state, and that is what the cable helper API is for. That API has that name deliberately so that you know these functions are for cable drivers, not for the jtag api layer. Rather than worrying about a future state, they are worrying about current state of the taps as the driver actually manipulates TMS and the clock. Right, but the situation where this came up is when the tap state, or actually the target jtag controller changes state and the drivers must be informed about this. We cannot use cmd_queue_cur_state for this since it only records the state for sanity checking. We can add a jtag_add_forced_set_state or someting similar. Or we can do a jtag_execute_queue to flush the command queue and then a tap_set_state(). These two concepts are separate, and cannot be merged because of the queue. tap_set_state() is for current state of the taps. But realize there is can be some delay here also because sometimes their are low level commands buffered before being sent out on USB. So tap_set_state() only tracks to the best of its ability the current state. Dick Best regards Magnus ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] remove cmd_queue_cur_state
Sorry I take back my earlier comment. I didn't check tap_set_state() and only assumed that it wraps cmd_queue_cur_state. But it doesn't and I just checked the patch on arm11 and it breaks arm11. When arm11 queries cmd_queue_cur_state it wants to know in what state the previously queue command left the queue so that it can correctly queue up the next scan command. Michael On Sun, May 24, 2009 at 10:50 AM, Magnus Lundin lun...@mlu.mine.nu wrote: Dick Hollenbeck skrev: Zach, I think this patch is fundamentally wrong and is a disaster the moment it is applied. The queue sits between the jtag api and the drivers. The api calls track future state according to the most recent api call submitted (and put onto the back end of the queue) via the cmd_queue_cur_state variable. The drivers consume commands in batch mode off the back end of the queue. They have their own notion of state, and that is what the cable helper API is for. That API has that name deliberately so that you know these functions are for cable drivers, not for the jtag api layer. Rather than worrying about a future state, they are worrying about current state of the taps as the driver actually manipulates TMS and the clock. Right, but the situation where this came up is when the tap state, or actually the target jtag controller changes state and the drivers must be informed about this. We cannot use cmd_queue_cur_state for this since it only records the state for sanity checking. We can add a jtag_add_forced_set_state or someting similar. Or we can do a jtag_execute_queue to flush the command queue and then a tap_set_state(). These two concepts are separate, and cannot be merged because of the queue. tap_set_state() is for current state of the taps. But realize there is can be some delay here also because sometimes their are low level commands buffered before being sent out on USB. So tap_set_state() only tracks to the best of its ability the current state. Dick Best regards Magnus ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
[Openocd-development] [PATCH] remove cmd_queue_cur_state
Hi all,
The attached patch removes the cmd_queue_cur_state global variable,
replacing all uses with calls to tap_set_state() or tap_get_state().
While this seems completely trivial, I wanted to post it for review.
Any objections? All in favor?
Cheers,
Zach
---
jtag/jtag.c | 13 ++---
jtag/jtag.h |3 +--
jtag/zy1000.c |4 ++--
svf/svf.c |9 ++---
target/arm11_dbgtap.c |4 ++--
xsvf/xsvf.c |4 ++--
6 files changed, 19 insertions(+), 18 deletions(-)
Index: src/jtag/zy1000.c
===
--- src/jtag/zy1000.c (revision 1893)
+++ src/jtag/zy1000.c (working copy)
@@ -709,7 +709,7 @@
int interface_jtag_add_clocks(int num_cycles)
{
- return zy1000_jtag_add_clocks(num_cycles, cmd_queue_cur_state,
cmd_queue_end_state);
+ return zy1000_jtag_add_clocks(num_cycles, tap_get_state(),
cmd_queue_end_state);
}
int interface_jtag_add_sleep(u32 us)
@@ -728,7 +728,7 @@
state_count = 0;
- tap_state_t cur_state=cmd_queue_cur_state;
+ tap_state_t cur_state = tap_get_state();
while (num_states)
{
Index: src/jtag/jtag.c
===
--- src/jtag/jtag.c (revision 1893)
+++ src/jtag/jtag.c (working copy)
@@ -94,7 +94,6 @@
enum reset_types jtag_reset_config = RESET_NONE;
tap_state_t cmd_queue_end_state = TAP_RESET;
-tap_state_t cmd_queue_cur_state = TAP_RESET;
int jtag_verify_capture_ir = 1;
int jtag_verify = 1;
@@ -565,7 +564,7 @@
if (state != TAP_INVALID)
jtag_add_end_state(state);
- cmd_queue_cur_state = cmd_queue_end_state;
+ tap_set_state(cmd_queue_end_state);
}
void jtag_add_ir_scan_noverify(int in_num_fields, const scan_field_t
*in_fields, tap_state_t state)
@@ -1066,7 +1065,7 @@
void jtag_add_pathmove(int num_states, const tap_state_t *path)
{
- tap_state_t cur_state = cmd_queue_cur_state;
+ tap_state_t cur_state = tap_get_state();
int i;
int retval;
@@ -1097,7 +1096,7 @@
jtag_prelude1();
retval = interface_jtag_add_pathmove(num_states, path);
- cmd_queue_cur_state = path[num_states - 1];
+ tap_set_state(path[num_states - 1]);
if (retval!=ERROR_OK)
jtag_error=retval;
}
@@ -1168,11 +1167,11 @@
void jtag_add_clocks( int num_cycles )
{
int retval;
-
- if( !tap_is_state_stable(cmd_queue_cur_state) )
+ tap_state_t cur_state = tap_get_state();
+ if( !tap_is_state_stable(cur_state) )
{
LOG_ERROR( jtag_add_clocks() was called with TAP in
non-stable state \%s\,
-tap_state_name(cmd_queue_cur_state) );
+tap_state_name(cur_state) );
jtag_error = ERROR_JTAG_NOT_STABLE_STATE;
return;
}
Index: src/jtag/jtag.h
===
--- src/jtag/jtag.h (revision 1893)
+++ src/jtag/jtag.h (working copy)
@@ -256,7 +256,6 @@
extern tap_state_t cmd_queue_end_state; /* finish DR scans in
dr_end_state */
-extern tap_state_t cmd_queue_cur_state; /* current TAP state */
typedef void* error_handler_t; /* Later on we can delete error_handler_t, but
keep it for now to make patches more readable */
@@ -891,7 +890,7 @@
{
if (end_state != TAP_INVALID)
cmd_queue_end_state = end_state;
- cmd_queue_cur_state = cmd_queue_end_state;
+ tap_set_state(cmd_queue_end_state);
interface_jtag_add_dr_out(tap, num_fields, num_bits, value,
cmd_queue_end_state);
}
Index: src/target/arm11_dbgtap.c
===
--- src/target/arm11_dbgtap.c (revision 1893)
+++ src/target/arm11_dbgtap.c (working copy)
@@ -48,7 +48,7 @@
int arm11_add_ir_scan_vc(int num_fields, scan_field_t *fields, tap_state_t
state)
{
- if (cmd_queue_cur_state == TAP_IRPAUSE)
+ if (tap_get_state() == TAP_IRPAUSE)
jtag_add_pathmove(asizeof(arm11_move_pi_to_si_via_ci),
arm11_move_pi_to_si_via_ci);
jtag_add_ir_scan(num_fields, fields, state);
@@ -62,7 +62,7 @@
int arm11_add_dr_scan_vc(int num_fields, scan_field_t *fields, tap_state_t
state)
{
- if (cmd_queue_cur_state == TAP_DRPAUSE)
+ if (tap_get_state() == TAP_DRPAUSE)
jtag_add_pathmove(asizeof(arm11_move_pd_to_sd_via_cd),
arm11_move_pd_to_sd_via_cd);
jtag_add_dr_scan(num_fields, fields, state);
Index: src/xsvf/xsvf.c
===
--- src/xsvf/xsvf.c (revision 1893)
+++ src/xsvf/xsvf.c (working copy)
@@ -171,7 +171,7 @@
int retval = ERROR_OK;
tap_state_t moves[8];
- tap_state_t cur_state = cmd_queue_cur_state;
+ tap_state_t
Re: [Openocd-development] [PATCH] remove cmd_queue_cur_state
Looks good.
Michael
On Sat, May 23, 2009 at 11:08 PM, Zach Welch z...@superlucidity.net wrote:
Hi all,
The attached patch removes the cmd_queue_cur_state global variable,
replacing all uses with calls to tap_set_state() or tap_get_state().
While this seems completely trivial, I wanted to post it for review.
Any objections? All in favor?
Cheers,
Zach
---
jtag/jtag.c | 13 ++---
jtag/jtag.h | 3 +--
jtag/zy1000.c | 4 ++--
svf/svf.c | 9 ++---
target/arm11_dbgtap.c | 4 ++--
xsvf/xsvf.c | 4 ++--
6 files changed, 19 insertions(+), 18 deletions(-)
Index: src/jtag/zy1000.c
===
--- src/jtag/zy1000.c (revision 1893)
+++ src/jtag/zy1000.c (working copy)
@@ -709,7 +709,7 @@
int interface_jtag_add_clocks(int num_cycles)
{
- return zy1000_jtag_add_clocks(num_cycles, cmd_queue_cur_state,
cmd_queue_end_state);
+ return zy1000_jtag_add_clocks(num_cycles, tap_get_state(),
cmd_queue_end_state);
}
int interface_jtag_add_sleep(u32 us)
@@ -728,7 +728,7 @@
state_count = 0;
- tap_state_t cur_state=cmd_queue_cur_state;
+ tap_state_t cur_state = tap_get_state();
while (num_states)
{
Index: src/jtag/jtag.c
===
--- src/jtag/jtag.c (revision 1893)
+++ src/jtag/jtag.c (working copy)
@@ -94,7 +94,6 @@
enum reset_types jtag_reset_config = RESET_NONE;
tap_state_t cmd_queue_end_state = TAP_RESET;
-tap_state_t cmd_queue_cur_state = TAP_RESET;
int jtag_verify_capture_ir = 1;
int jtag_verify = 1;
@@ -565,7 +564,7 @@
if (state != TAP_INVALID)
jtag_add_end_state(state);
- cmd_queue_cur_state = cmd_queue_end_state;
+ tap_set_state(cmd_queue_end_state);
}
void jtag_add_ir_scan_noverify(int in_num_fields, const scan_field_t
*in_fields, tap_state_t state)
@@ -1066,7 +1065,7 @@
void jtag_add_pathmove(int num_states, const tap_state_t *path)
{
- tap_state_t cur_state = cmd_queue_cur_state;
+ tap_state_t cur_state = tap_get_state();
int i;
int retval;
@@ -1097,7 +1096,7 @@
jtag_prelude1();
retval = interface_jtag_add_pathmove(num_states, path);
- cmd_queue_cur_state = path[num_states - 1];
+ tap_set_state(path[num_states - 1]);
if (retval!=ERROR_OK)
jtag_error=retval;
}
@@ -1168,11 +1167,11 @@
void jtag_add_clocks( int num_cycles )
{
int retval;
-
- if( !tap_is_state_stable(cmd_queue_cur_state) )
+ tap_state_t cur_state = tap_get_state();
+ if( !tap_is_state_stable(cur_state) )
{
LOG_ERROR( jtag_add_clocks() was called with TAP in
non-stable state \%s\,
- tap_state_name(cmd_queue_cur_state) );
+ tap_state_name(cur_state) );
jtag_error = ERROR_JTAG_NOT_STABLE_STATE;
return;
}
Index: src/jtag/jtag.h
===
--- src/jtag/jtag.h (revision 1893)
+++ src/jtag/jtag.h (working copy)
@@ -256,7 +256,6 @@
extern tap_state_t cmd_queue_end_state; /* finish DR scans in
dr_end_state */
-extern tap_state_t cmd_queue_cur_state; /* current TAP state */
typedef void* error_handler_t; /* Later on we can delete error_handler_t,
but keep it for now to make patches more readable */
@@ -891,7 +890,7 @@
{
if (end_state != TAP_INVALID)
cmd_queue_end_state = end_state;
- cmd_queue_cur_state = cmd_queue_end_state;
+ tap_set_state(cmd_queue_end_state);
interface_jtag_add_dr_out(tap, num_fields, num_bits, value,
cmd_queue_end_state);
}
Index: src/target/arm11_dbgtap.c
===
--- src/target/arm11_dbgtap.c (revision 1893)
+++ src/target/arm11_dbgtap.c (working copy)
@@ -48,7 +48,7 @@
int arm11_add_ir_scan_vc(int num_fields, scan_field_t *fields, tap_state_t
state)
{
- if (cmd_queue_cur_state == TAP_IRPAUSE)
+ if (tap_get_state() == TAP_IRPAUSE)
jtag_add_pathmove(asizeof(arm11_move_pi_to_si_via_ci),
arm11_move_pi_to_si_via_ci);
jtag_add_ir_scan(num_fields, fields, state);
@@ -62,7 +62,7 @@
int arm11_add_dr_scan_vc(int num_fields, scan_field_t *fields, tap_state_t
state)
{
- if (cmd_queue_cur_state == TAP_DRPAUSE)
+ if (tap_get_state() == TAP_DRPAUSE)
jtag_add_pathmove(asizeof(arm11_move_pd_to_sd_via_cd),
arm11_move_pd_to_sd_via_cd);
jtag_add_dr_scan(num_fields, fields, state);
Index: src/xsvf/xsvf.c
===
--- src/xsvf/xsvf.c (revision 1893)
+++ src/xsvf/xsvf.c
