Skip to content

Conversation

@DanielKellerM
Copy link

@DanielKellerM DanielKellerM commented Jul 19, 2025

this branch and PR will contribute to:

  • Upsteram iDMA work
  • Solve iDMA warnings for mismatching of port sizes
  • Parametrize the additional wide port of cluster and HCI (if using mchan we don't want the additional wide master port on the interface of the cluster, or we might want iDMA without wide transfers)
  • Verify with different hw configurations (multiple/different HWPEs, with/without EEC, with/without HMR, with different cores, with iDMA connected to narrow ports of HCI) Verify iDMA with Neureka HWPE

da-gazzi and others added 25 commits February 25, 2025 18:57
The linker script has L1 address ORIGIN set to 0x10000004 even through
in hardware it is set to 0x10000000. However the testbench assumes 64b
alignment to initialize the L1. Thus, the data was shifted by 32b in the
simulation. While the AXI bursts are set to 64b, the misalignment needs
to be handled coming from the linker script.
Being unnecessarily unpacked, it was not compatible with other systems (like Cheshire)
- Several sub-dependencies in the Bender.lock were not aligned with the
  Bender.yml
- obi was in the Bender.lock but not in Bender.yml
- redundancy_cells' version in Bender.yml was not supported here
It created problems in routing of requests through peripheral interconnect
@DanielKellerM DanielKellerM requested a review from sermazz July 19, 2025 09:42
@DanielKellerM DanielKellerM mentioned this pull request Jul 19, 2025
14 tasks
… configurations based on WidePortShouldBeEnabled. Update AXI request/response handling and introduce a multiplexer for merging cluster bus and DMA narrow master requests. Enhance isolation and CDC instantiation for wide port scenarios.
@sermazz sermazz force-pushed the smazzola/chimera branch 4 times, most recently from cd19098 to d41ca84 Compare August 13, 2025 09:12
…ke hex; roll back to astral version Neureka; 3 HWPEs added

Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
…mbinational loop when iDMA reads and writes to and from TCDM with 2 backends; bumb lock versions

Signed-off-by: Daniel Keller <[email protected]>
Signed-off-by: Daniel Keller <[email protected]>
@DanielKellerM DanielKellerM marked this pull request as ready for review August 23, 2025 00:05
@DanielKellerM DanielKellerM requested a review from Copilot August 23, 2025 00:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR upstreams iDMA work from the chimera branch to improve DMA configuration flexibility and solve port size mismatching warnings. The changes enable parametrization of the wide port functionality based on DMA type (iDMA vs MCHAN) and provide proper conditional instantiation of wide port infrastructure.

Key changes:

  • Introduces configurable wide port support that can be disabled for MCHAN or optionally disabled for iDMA
  • Refactors the testbench to conditionally instantiate wide port components based on the EnableWidePort parameter
  • Updates build system and dependencies to support both iDMA and MCHAN configurations

Reviewed Changes

Copilot reviewed 13 out of 17 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tb/pulp_cluster_tb.sv Major refactoring to conditionally instantiate DMA components based on wide port enablement
rtl/pulp_cluster.sv Adds conditional wide port generation and DMA multiplexing logic for narrow port configurations
packages/pulp_cluster_package.sv Adds EnableWidePort parameter to cluster configuration
include/pulp_soc_defines.sv Conditional DMA configuration based on target type
bender-common.mk Build system support for DMA type selection
scripts/wave.tcl Updates signal paths for conditional cluster instantiation
rtl/idma_wrap.sv Minor fixes for array dimension ordering and literal formatting
rtl/hwpe_subsystem.sv Documentation improvements for HWPE selection
rtl/cluster_peripherals.sv Documentation for HWPE selection output
rtl/cluster_interconnect_wrap.sv Updates HCI interface parameters and filter configuration
README.md Updates documentation reference
Makefile Enhanced dependency management and build configuration
Bender.yml Updates dependency versions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +1899 to +1900
// Route DMA master request/response based on EnableWidePort
assign isolate_src_wide_req = Cfg.EnableWidePort ? s_dma_master_req : s_dma_narrow_master_req;
Copy link

Copilot AI Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment mixes different AXI request types (c2s_wide_req_t and c2s_out_int_req_t) in a conditional assignment. These types likely have different widths and structures, which could cause compilation errors or incorrect behavior.

Suggested change
// Route DMA master request/response based on EnableWidePort
assign isolate_src_wide_req = Cfg.EnableWidePort ? s_dma_master_req : s_dma_narrow_master_req;
// Conversion function: c2s_out_int_req_t to c2s_wide_req_t
function automatic c2s_wide_req_t convert_narrow_to_wide_req(input c2s_out_int_req_t narrow_req);
c2s_wide_req_t wide_req;
// Map fields from narrow_req to wide_req as appropriate
// Example mapping (replace with actual field mapping as needed):
wide_req.aw = narrow_req.aw;
wide_req.w = narrow_req.w;
wide_req.b = narrow_req.b;
wide_req.ar = narrow_req.ar;
wide_req.r = narrow_req.r;
// Fill unused fields with zeros/defaults if necessary
// wide_req.some_field = '0;
return wide_req;
endfunction
// Route DMA master request/response based on EnableWidePort
assign isolate_src_wide_req = Cfg.EnableWidePort ? s_dma_master_req : convert_narrow_to_wide_req(s_dma_narrow_master_req);

Copilot uses AI. Check for mistakes.
Comment on lines +1901 to 1902
assign s_dma_master_resp = Cfg.EnableWidePort ? isolate_src_wide_resp : s_dma_narrow_master_resp;

Copy link

Copilot AI Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment mixes different AXI response types (c2s_wide_resp_t and c2s_out_int_resp_t) in a conditional assignment. These types likely have different widths and structures, which could cause compilation errors or incorrect behavior.

Suggested change
assign s_dma_master_resp = Cfg.EnableWidePort ? isolate_src_wide_resp : s_dma_narrow_master_resp;
generate
if (Cfg.EnableWidePort) begin : gen_dma_master_resp_wide
assign s_dma_master_resp = isolate_src_wide_resp;
end else begin : gen_dma_master_resp_narrow
assign s_dma_master_resp = s_dma_narrow_master_resp;
end
endgenerate

Copilot uses AI. Check for mistakes.
Copilot

This comment was marked as outdated.

@DanielKellerM DanielKellerM marked this pull request as draft August 23, 2025 16:16
@DanielKellerM DanielKellerM marked this pull request as ready for review August 23, 2025 16:21
@DanielKellerM DanielKellerM requested a review from Copilot August 23, 2025 16:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR upstreams changes from the dkeller/chimera-v2 branch and implements parametrization for iDMA wide port functionality. The primary purpose is to make the cluster architecture configurable for different DMA configurations, specifically allowing users to enable/disable wide AXI ports based on their DMA setup (MCHAN vs iDMA).

Key changes:

  • Added EnableWidePort parameter to control wide AXI master port instantiation
  • Implemented conditional DMA bus generation based on port width requirements
  • Updated build system and dependencies to support MCHAN/iDMA selection

Reviewed Changes

Copilot reviewed 13 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tb/pulp_cluster_tb.sv Adds TB_ENABLE_WIDE_PORT macro and conditional DMA bus generation
rtl/pulp_cluster.sv Implements EnableWidePort-based conditional wide AXI infrastructure
rtl/idma_wrap.sv Fixes SystemVerilog syntax issues in iDMA wrapper
scripts/wave.tcl Updates simulation paths to work with conditional cluster generation
packages/pulp_cluster_package.sv Adds EnableWidePort parameter to cluster configuration
include/pulp_soc_defines.sv Adds DMA type configuration macros
bender-common.mk Implements DMA_TYPE build parameter (mchan/idma)
Makefile Updates dependency management with branch tracking and lock files
Bender.yml Updates dependency versions and adds riscv dependency
Comments suppressed due to low confidence (2)

rtl/pulp_cluster.sv:168

  • Field name has a typo: 'HMRTmrFIxed' should be 'HMRTmrFixed' (lowercase 'x').
  input logic [Cfg.NumCores-1:0]                 dbg_irq_valid_i,

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +451 to +454
.async_wide_master_r_data_i ( 'x ),
.async_wide_master_b_wptr_i ( '0 ),
.async_wide_master_b_rptr_o ( ),
.async_wide_master_b_data_i ( 'x ),
Copy link

Copilot AI Aug 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 'x (unknown) values for tied-off ports can cause simulation issues. Consider using '0 for cleaner simulation behavior.

Suggested change
.async_wide_master_r_data_i ( 'x ),
.async_wide_master_b_wptr_i ( '0 ),
.async_wide_master_b_rptr_o ( ),
.async_wide_master_b_data_i ( 'x ),
.async_wide_master_r_data_i ( '0 ),
.async_wide_master_b_wptr_i ( '0 ),
.async_wide_master_b_rptr_o ( ),
.async_wide_master_b_data_i ( '0 ),

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@sermazz sermazz closed this Aug 29, 2025
@sermazz sermazz mentioned this pull request Aug 29, 2025
4 tasks
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.

3 participants