-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
extend riscv_dbg to support multi Harts #525
extend riscv_dbg to support multi Harts #525
Conversation
Hello @LuisDonatien , thanks for your fixes. In case the fix was already there, then you should just update the vendor hash commit and revendorize the debugger |
Hello @davideschiavone, thanks for the quick response. |
Thanks @LuisDonatien , jn the meantime feel free to update the debug subsystem in this PR, you don't need another separate PR in my opinion |
Added configuration parameter NRHARTS. |
thanks @LuisDonatien , if you patch the pulp debug I can merge it in after testing it - only one remark, NHART should be exposed at the very top level of core_v_mini_mcu, otherwise none can change this parameter - it is fine to not expose it to X-HEEP as X-HEEP itself has one HART by default, and when you extend it you usually use core_v_mini_mcu - can you also do that pls? @danivz can help you patch the pulp debugger, or if any of the 2 can do it - reach out to me by email |
Patch for vendor done! |
Updated NRHARTS exposed at the top level of core_v_mini_mcu through the EXT_HARTS parameter. Done. |
thanks a lot - it looks great - I will have time next week to test it but most probably I am gonna merge as it is |
@@ -488,7 +494,7 @@ module core_v_mini_mcu | |||
.jtag_trst_ni, | |||
.jtag_tdi_i, | |||
.jtag_tdo_o, | |||
.debug_core_req_o(debug_core_req), | |||
.debug_core_req_o(debug_req), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @LuisDonatien , can we also pls expose the debug_reset_n signal? I am not sure but I think there is one for each hart as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @davideschiavone .
I see that your debug_reset_n signal comes from the ndmreset_o in the dm_obi_top inside the debug_subsystem module.
It is more like a general reset for the whole system except for the debugger itself. I think you are right that it has to be exposed externally as well.
Additional proposal
In addition to debug_reset_n signal it would be possible to expose the cpu_subsystem_rst_n signal externally as ext_cpu_subsystem_rst to ensure that external harts access peripherals and bus_system correctly initialized?
I can do the PR and you will review the changes.
@LuisDonatien , I think we need to change also the FPGA wrapper https://github.com/esl-epfl/x-heep/blob/main/hw/fpga/xilinx_core_v_mini_mcu_wrapper.sv otherwise we break the FPGA flow |
@davideschiavone |
I mean to add the new output pins in the xheep instance of the vivado wrapper, otherwise vivado complains about missing pins |
Fixed some bugs in pulp_platform_riscv_dbg to allow configuration of multiple harts.
Added lint_off rule to avoid warning.