dtmcs.idle hasn't been used to control idle/run_test which may result in get wrong values

[gxflying]

Hi, sir, I find that in the current openOCD， we have a variable "dtmcs_idle" for dtmcs.idle but do NOT use it at all except displaying info https://github.com/riscv/riscv-openocd/blob/f0151889f0f5a51e2555980d74e7a86d0b9aa849/src/target/riscv/riscv-013.c#L193 I think it should be used in some way like this, and this is why we have a "dtmcs.idle" in the spec : https://github.com/riscv/riscv-openocd/blob/f0151889f0f5a51e2555980d74e7a86d0b9aa849/src/target/riscv/riscv-013.c#L518

actually , in my implemention, we do need this "dtmcs.idle" to be added when do dmi_scan

[cinlyooi-intel]

+1

(1) I also find the same thing as @gxflying, i.e. riscv013_info_t instance info->dtmcs_idle set but not used.

My colleague who is doing playing with RISCV and I also thought that the value will be used and had set it. When there was no effect, we examine riscv-013.c to find out that it was read but not used.

(1.1) I understand that riscv-013 does its own delay computation and the delay computation is dynamic and is designed to vary according to the need of the particular board/debug instance. I like it.

(1.2) Noted that the debug specification says that This is a hint to the debugger of the minimum number of cycles a debugger should spend in RunTest/Idle after every DMI scan to avoid a 'busy' return code (dmistat of 3). meaning that it is your privilege, as the developer/maintainer of riscv-013, to decide whether to use it or not. But being the reference implementation for RISCV debugger, I would like to suggest that you consider honouring the hint. However, I feel that @gxflying suggestion to incorporate it into idle_count in dmi_scan() is too rigid in light of your effort to compute a delay based on circumstance, I suggest we have a configurable option that allow us to (a) ignore the value (default), (b) have the value respected.

Instead of adding it to idle_count like @gxflying suggests, add it to the initial value of info->dmi_busy_delay. It will have the same effect, remove a add operation everytime we compute idle_count and also retain the flexibility of ignoring it if your delay computation decided it is not needed, assuming a delay reduction is possible.

(2) Another question I have is with regards to the declaration of dtmcs_idle. [28724d996a9b68e490f6bd90b5537c71631c753d src/target/riscv/riscv-013.c] says

        /* Number of run-test/idle cycles the target requests we do after each dbus
	 * access. */
	unsigned int dtmcs_idle;

which for a novice like me, suggests the idle value, if you will be using it, will be used only if we choose System Bus Access over Program Buffer. Is this novice reading it wrongly?

[JanMatCodasip]

Hi Cinly,

thank you for your message. Your observations are correct, info->dtmcs_idle is currently read from the target but unused.

I believe the current logic in with the exponential back-off for the number of cycles is sufficient.

But being the reference implementation for RISCV debugger, I would like to suggest that you consider honouring the hint.

The current version of riscv-openocd is implemented on lazy/best-effort basis, meaning that it does not cover all the functionality in the debug spec. Instead, features are being added as needed.

Your suggestion to use info->dtmcs_idle as the initial value of info->dmi_busy_delay sounds fine. If you'd like, you can submit your patch :)

I suggest we have a configurable option that allow us to (a) ignore the value (default), (b) have the value respected.

This seems to me like bit of an overkill. Or do you have a concrete use case in mind, in which the user would need to manually configure such a low-level option, instead of using the automatic exponential back-off logic?

/* Number of run-test/idle cycles the target requests we do after each dbus access. */

Correct, the comment should perhaps say ... after each DMI scan, to make it clear to the reader.

Regards, Jan