pulp-platform / riscv-dbg

RISC-V Debug Support for our PULP RISC-V Cores
Other
197 stars 70 forks source link

ndmreset acknowledgement and resetting halted state in dm_mem #118

Open GregAC opened 2 years ago

GregAC commented 2 years ago

Currently the debug module seems to assume any assertion of ndmreset_o will instantly cause a reset. The dmstatus.allhavereset/dmstatus.anyhavereset bits are set purely on ndmreset_o being asserted rather than any acknowledgement a reset has occurred.

There's also an issue with trigger a non-debug reset (by writing to dmcontrol via the DMI interface) when a hart is in halted state (so dmstatus.anyhalted/dmstatus.allhalted is asserted). When the hart resets the debug module still considers the hart to be halted when it is not.

The issue is the handling of halted_q in dm_mem which produces the halted_o output that feeds into dmstatus. Currently this is solely under the control of the debug ROM (which continuously sets it in a loop until it is requested to do something else). When the hart is reset whilst in the middle of the debug ROM it never gets unset so after reset dmstatus.anyhalted/dmstatus.allhalted remains asserted. So if you run a reset run command in OpenOCD it times out stating the hart remains halted.

I think the ndmreset needs to be factored into the logic setting halted_q and potentially resuming_q within dm_mem (maybe other state there too, I'm not familiar enough with it to be sure). Directly feeding the ndmreset signal into it and clearing halted_q when it is asserted can work but if you have a system where it takes a while for the ndmreset signal to be acted on perhaps the ROM will successfully set halted_q again before the hart reset takes effect.

Perhaps a top-level ndmreset_ack_i should be introduced which can feed into dmstatus.anyhalted/dmstatus.allhalted as well as reset the relevent state in dm_mem?

bluewww commented 2 years ago

Indeed the debug module assumes ndmreset_o to take effect immediately, which doesn't work for systems taking a while to reset.

Introducing ndmreset_ack_i and thereby making the reset a ready/valid handshake seems good to me.