Skip to content
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

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

LuisDonatien
Copy link
Contributor

Fixed some bugs in pulp_platform_riscv_dbg to allow configuration of multiple harts.

Added lint_off rule to avoid warning.

@davideschiavone
Copy link
Member

Hello @LuisDonatien , thanks for your fixes.
It seems that you are fixing the pulp debugger, so you should send your PR to this repository https://github.com/pulp-platform/riscv-dbg maintained by ETH and University of Bologna.

In case the fix was already there, then you should just update the vendor hash commit and revendorize the debugger

@LuisDonatien
Copy link
Contributor Author

Hello @LuisDonatien , thanks for your fixes. It seems that you are fixing the pulp debugger, so you should send your PR to this repository https://github.com/pulp-platform/riscv-dbg maintained by ETH and University of Bologna.

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.
I'm going to do the PR to the pulp repo and update the hash when it will be accepted.
I will also update this PR with another commit involving the debug_subsystem.sv to allow multiple harts as well.

@davideschiavone
Copy link
Member

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

@LuisDonatien
Copy link
Contributor Author

LuisDonatien commented Jun 11, 2024

Added configuration parameter NRHARTS.
Implemented hartinfo_t initialization vector for multiple hart configuration.

@davideschiavone
Copy link
Member

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

@danivz
Copy link
Contributor

danivz commented Jun 12, 2024

Patch for vendor done!

@LuisDonatien
Copy link
Contributor Author

Updated NRHARTS exposed at the top level of core_v_mini_mcu through the EXT_HARTS parameter.

Done.

@davideschiavone
Copy link
Member

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),
Copy link
Member

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

Copy link
Contributor Author

@LuisDonatien LuisDonatien Jun 19, 2024

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.

@davideschiavone
Copy link
Member

@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

@LuisDonatien
Copy link
Contributor Author

@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
Do you mean to expose ext_harts parameters through xheep_system to fpga wrapper parameters?

@davideschiavone
Copy link
Member

@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 Do you mean to expose ext_harts parameters through xheep_system to fpga wrapper parameters?

I mean to add the new output pins in the xheep instance of the vivado wrapper, otherwise vivado complains about missing pins

@davideschiavone davideschiavone changed the title fix riscv_dbg to allow n Harts config extend riscv_dbg to support multi Harts Jun 27, 2024
@davideschiavone davideschiavone merged commit 7149eeb into esl-epfl:main Jun 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants