chipsalliance / rocket-chip

Rocket Chip Generator
Other
3.27k stars 1.13k forks source link

RoCC Core Clock Gating Bug #3695

Open moniriki opened 2 weeks ago

moniriki commented 2 weeks ago

Type of issue: bug report | feature request | other enhancement Bug report

Impact: no functional change | API addition (no impact on existing code) | API modification | unknown Functional change

Development Phase: request | proposal Request Other information

The current clock gating logic inside the Rocket Cores do not take into account the RoCC busy signal. As a result, there are times when the RoCC request is being responded to but the core clock gating kicks in and doesn't allow the response to be registered inside the core. This results in the core hanging.

My suggestion here is to change clock_en_reg to also include the io.rocc.busy signal inside RocketCore.scala. I can put up a pull request for this bug fix if required.

What is the current behavior? Clock gate enable gets deasserted while RoCC is busy.

What is the expected behavior? Clock gate logic needs to account for RoCC custom instructions before gating the core clock.

Waveform below shows the current bug.

Screenshot 2024-11-08 at 2 30 16 PM
moniriki commented 2 weeks ago

@jerryz123 I'm attempting to put up a pull request for this fix, but unfortunately am running into permission issues trying to push my branch to the remote repository. The fix is simple, I've placed it below for your reference.

diff --git a/src/main/scala/rocket/RocketCore.scala b/src/main/scala/rocket/RocketCore.scala
index 1491a4143..dffffb4fa 100644
--- a/src/main/scala/rocket/RocketCore.scala
+++ b/src/main/scala/rocket/RocketCore.scala
@@ -1174,6 +1174,7 @@ class Rocket(tile: RocketTile)(implicit p: Parameters) extends CoreModule()(p)
       !div.io.req.ready || // mul/div in flight
       usingFPU.B && !io.fpu.fcsr_rdy || // long-latency FPU in flight
       io.dmem.replay_next || // long-latency load replaying
+      id_rocc_busy || // RoCC command in flight
       (!long_latency_stall && (ibuf.io.inst(0).valid || io.imem.resp.valid)) // instruction pending

     assert(!(ex_pc_valid || mem_pc_valid || wb_pc_valid) || clock_en)
jerryz123 commented 2 weeks ago

@moniriki you need to fork the repo through github, then push your branch to your fork, then open a PR from your fork into the base repo.

moniriki commented 2 weeks ago

PR posted here: https://github.com/chipsalliance/rocket-chip/pull/3696. Thanks Jerry