esl-epfl / x-heep

eXtendable Heterogeneous Energy-Efficient Platform based on RISC-V
Other
141 stars 75 forks source link

extend riscv_dbg to support multi Harts #525

Closed LuisDonatien closed 3 months ago

LuisDonatien commented 3 months ago

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

Added lint_off rule to avoid warning.

davideschiavone commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

davideschiavone commented 3 months ago

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 commented 3 months ago

Patch for vendor done!

LuisDonatien commented 3 months ago

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

Done.

davideschiavone commented 3 months ago

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

davideschiavone commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

@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