search

SpinalHDL / VexRiscv

A FPGA friendly 32 bit RISC-V CPU implementation
MIT License
2.52k stars 420 forks source link

EmbeddedRiscvJtag synthesis issue #381

Closed gregdavill closed 6 months ago

gregdavill commented 1 year ago

Summary

After adding EmbeddedRiscvJtag to a VexRiscv design as described in the README, I was not able to connect to it. I'm using yosys/nextpnr to target an ECP5, I have an FTDI 2232H connected to the ECP5 (channel A) and I/O pins (channel B).

I've started with the default LietX cpu and swapped in the EmbeddedRiscvJtag plugin.

Details

conf.cfg

adapter driver ftdi
ftdi vid_pid 0x0403 0x6010
ftdi channel 1
ftdi layout_init 0x0038 0x003b

transport select jtag
adapter speed 3000

set _CHIPNAME riscv
set _TARGETNAME $_CHIPNAME.cpu

jtag newtap $_CHIPNAME cpu -irlen 5 -expected-id 0x10002FFF
target create $_TARGETNAME.0 riscv -chain-position $_TARGETNAME

init
halt

Output from openocd

openocd -f ./openocd/conf.cfg 
Open On-Chip Debugger 0.12.0
Licensed under GNU GPL v2
For bug reports, read
        http://openocd.org/doc/doxygen/bugs.html
Info : clock speed 3000 kHz
Info : TAP riscv.cpu does not have valid IDCODE (idcode=0xfffffffe)
Error: riscv.cpu: IR capture error; saw 0x1e not 0x01
Warn : Bypassing JTAG setup events due to errors
Error: dtmcontrol is 0. Check JTAG connectivity/board power.
Warn : target riscv.cpu.0 examination failed
Info : starting gdb server for riscv.cpu.0 on 3333
Info : Listening on port 3333 for gdb connections
Error: Target not examined yet

Workaround

From the start I was getting correct results in sim. So focused debugging on the FPGA flow, and it appears that during synthesis something funky was happening leaving me with tdo kept in bypass mode. the yosys logs did not indicate logic removal of the debug core, etc.

I noticed that tap_fsm_state is not reset, and I suppose in theroy is doesn't have to be, since the JTAG tap can reset it's logic with TMS=1 + TCKs

If I alter the rtl and add an initial value to tap_fsm_state things work..

...
  reg        [1:0]    logic_jtagLogic_dmiStat_value_aheadValue;
  wire       [3:0]    tap_fsm_stateNext;
  reg        [3:0]    tap_fsm_state = 0;  <-------
  wire       [3:0]    _zz_tap_fsm_stateNext;
  wire       [3:0]    _zz_tap_fsm_stateNext_1;
...
$ openocd -f ./openocd/conf.cfg 
Open On-Chip Debugger 0.12.0
Licensed under GNU GPL v2
For bug reports, read
        http://openocd.org/doc/doxygen/bugs.html
Info : clock speed 3000 kHz
Info : JTAG tap: riscv.cpu tap/device found: 0x10002fff (mfg: 0x7ff (<invalid>), part: 0x0002, ver: 0x1)
Info : datacount=1 progbufsize=2
Info : Disabling abstract command reads from CSRs.
Info : Examined RISC-V core; found 1 harts
Info :  hart 0: XLEN=32, misa=0x0
Info : starting gdb server for riscv.cpu.0 on 3333
Info : Listening on port 3333 for gdb connections
Info : Listening on port 6666 for tcl connections
Info : Listening on port 4444 for telnet connections

My workaround doesn't really feel like a solution. I'm not sure if this is actually a vexriscv bug? Or a bug in yosys.

Dolu1990 commented 1 year ago

Hi,

Thanks for the issue ^^ First time i hear of something like it + the jtag stuff didn't changed since a quite long time, so it may be a yosys bug ? Would have to check the netlist i guess.

Are tap_fsm_state still in the netlist after yosys ? (without the added = 0) ?

gregdavill commented 6 months ago

It seems like when an initial value is provided the register isn't considered for one-hot translation by yosys step FSM_EXTRACT

Not marking orbtrace_mini.VexRiscv.debugTransportModuleJtagTap_1.tap_fsm_state as FSM state register:
    Register has an initialization value.

This can also be achieved with

  (* fsm_encoding = "none" *) reg        [3:0]    tap_fsm_state;

Curiously if I add the reset state and don't just rely on the default clause for it in the tap_state case statement:

      JtagState_RESET : begin
        _zz_tap_fsm_stateNext_16 = _zz_tap_fsm_stateNext;
      end

Yosys report this in the log. The resulting bitstream works on the FPGA.

Not marking orbtrace_mini.VexRiscv.debugTransportModuleJtagTap_1.tap_fsm_state as FSM state register:
    Circuit seems to be self-resetting.

Given that the addition of the JtagState_RESET state to the case shouldn't actually change anything logic wise I'm pretty sure the absence of it might be causing this issue. This is probably also tied into the fact that all 16 states are legal, and the TAP doesn't require a reset signal, which might make it un-suitable for this FSM extraction in yosys.

Dolu1990 commented 6 months ago

gg for the debug ^^

So, changing : 

  val stateNext = JtagState()
  val state = RegNext(stateNext) randBoot()
  stateNext := state.mux(
    default    -> (jtag.tms ? RESET     | IDLE),           //RESET
    IDLE       -> (jtag.tms ? DR_SELECT | IDLE),
    IR_SELECT  -> (jtag.tms ? RESET     | IR_CAPTURE),
    IR_CAPTURE -> (jtag.tms ? IR_EXIT1  | IR_SHIFT),
    IR_SHIFT   -> (jtag.tms ? IR_EXIT1  | IR_SHIFT),
    IR_EXIT1   -> (jtag.tms ? IR_UPDATE | IR_PAUSE),
    IR_PAUSE   -> (jtag.tms ? IR_EXIT2  | IR_PAUSE),
    IR_EXIT2   -> (jtag.tms ? IR_UPDATE | IR_SHIFT),
    IR_UPDATE  -> (jtag.tms ? DR_SELECT | IDLE),
    DR_SELECT  -> (jtag.tms ? IR_SELECT | DR_CAPTURE),
    DR_CAPTURE -> (jtag.tms ? DR_EXIT1  | DR_SHIFT),
    DR_SHIFT   -> (jtag.tms ? DR_EXIT1  | DR_SHIFT),
    DR_EXIT1   -> (jtag.tms ? DR_UPDATE | DR_PAUSE),
    DR_PAUSE   -> (jtag.tms ? DR_EXIT2  | DR_PAUSE),
    DR_EXIT2   -> (jtag.tms ? DR_UPDATE | DR_SHIFT),
    DR_UPDATE  -> (jtag.tms ? DR_SELECT | IDLE)
  )

into : 

  val stateNext = JtagState()
  val state = RegNext(stateNext) randBoot()
  stateNext := state.mux(
    default    -> (jtag.tms ? RESET     | IDLE),           //RESET
    RESET      -> (jtag.tms ? RESET     | IDLE),   // <---------------------------- adding this line 
    IDLE       -> (jtag.tms ? DR_SELECT | IDLE),
    IR_SELECT  -> (jtag.tms ? RESET     | IR_CAPTURE),
    IR_CAPTURE -> (jtag.tms ? IR_EXIT1  | IR_SHIFT),
    IR_SHIFT   -> (jtag.tms ? IR_EXIT1  | IR_SHIFT),
    IR_EXIT1   -> (jtag.tms ? IR_UPDATE | IR_PAUSE),
    IR_PAUSE   -> (jtag.tms ? IR_EXIT2  | IR_PAUSE),
    IR_EXIT2   -> (jtag.tms ? IR_UPDATE | IR_SHIFT),
    IR_UPDATE  -> (jtag.tms ? DR_SELECT | IDLE),
    DR_SELECT  -> (jtag.tms ? IR_SELECT | DR_CAPTURE),
    DR_CAPTURE -> (jtag.tms ? DR_EXIT1  | DR_SHIFT),
    DR_SHIFT   -> (jtag.tms ? DR_EXIT1  | DR_SHIFT),
    DR_EXIT1   -> (jtag.tms ? DR_UPDATE | DR_PAUSE),
    DR_PAUSE   -> (jtag.tms ? DR_EXIT2  | DR_PAUSE),
    DR_EXIT2   -> (jtag.tms ? DR_UPDATE | DR_SHIFT),
    DR_UPDATE  -> (jtag.tms ? DR_SELECT | IDLE)
  )

would fix it ?

Dolu1990 commented 6 months ago

(conceptualy, because SpinalHDL will complain that :

UNREACHABLE DEFAULT STATEMENT on spinal.lib.com.jtag.JtagFsm.(JtagTap.scala:67)

Dolu1990 commented 6 months ago 
  val stateNext = JtagState()
  val state = RegNext(stateNext) randBoot()
  switch(state, coverUnreachable = true){
    default(stateNext := jtag.tms ? RESET | IDLE)
    def add(k : JtagState.E, t : JtagState.E, f : JtagState.E) = is(k)(stateNext := jtag.tms.mux(t, f))
    add(RESET     , RESET    , IDLE)
    add(IDLE      , DR_SELECT, IDLE)
    add(IR_SELECT , RESET    , IR_CAPTURE)
    add(IR_CAPTURE, IR_EXIT1 , IR_SHIFT)
    add(IR_SHIFT  , IR_EXIT1 , IR_SHIFT)
    add(IR_EXIT1  , IR_UPDATE, IR_PAUSE)
    add(IR_PAUSE  , IR_EXIT2 , IR_PAUSE)
    add(IR_EXIT2  , IR_UPDATE, IR_SHIFT)
    add(IR_UPDATE , DR_SELECT, IDLE)
    add(DR_SELECT , IR_SELECT, DR_CAPTURE)
    add(DR_CAPTURE, DR_EXIT1 , DR_SHIFT)
    add(DR_SHIFT  , DR_EXIT1 , DR_SHIFT)
    add(DR_EXIT1  , DR_UPDATE, DR_PAUSE)
    add(DR_PAUSE  , DR_EXIT2 , DR_PAUSE)
    add(DR_EXIT2  , DR_UPDATE, DR_SHIFT)
    add(DR_UPDATE , DR_SELECT, IDLE)
  }

Would fix it.

So, that would be good for you ?

that would only change the generated verilog by adding the : 

  JtagState_RESET : begin
    _zz_tap_fsm_stateNext_16 = _zz_tap_fsm_stateNext;
  end
gregdavill commented 6 months ago

Thanks for the fix on the Scala side. Yes, atleast from my editing of the verilog with exactly that line seems to fix the issue encountered by Yosys.

I'd be inclined to remove the default case if it's guaranteed that state/StateNext are 4 bits in size, since the default will never be reached as the case statements cover all possible states. But I'm not too familiar with SpinalHDL, so I'll defer that to you.

Dolu1990 commented 6 months ago

Fixed with https://github.com/SpinalHDL/SpinalHDL/commit/2c5e8266c106cb2f6a7fc7b6ac1b96f7615ce138

It keep the default case empty and add the reset case. Thanks for the issue ^^