Upstream `carfield`/`astral` features to `main` #87

belanoa · 2024-11-13T17:01:38Z

No description provided.

…e the address width.

* Fix syntax error

…; other general fixes.

alex96295 · 2024-12-04T13:38:23Z

General comment for the CI:

It seems that the main common issue across the tests that you have is:

fatal: could not get a repository handle for submodule 'pulp-runtime'

There is only one test for which you can get past the error above, i.e. ParMatrixMul32. So maybe this hints at some conflicts between the parallel runs.
Even if the test ParMatrixMul32 starts correctly, note that it is functionally broken as it raises illegal instructions, so this is a real bug that needs to be fixed locally first -> https://iis-git.ee.ethz.ch/github-mirror/pulp_cluster/-/jobs/1499281

alex96295 · 2024-12-19T15:22:39Z

Bender.yml

+  riscv:                  { git: "https://github.com/AlSaqr-platform/riscv_nn.git", rev: astral-v1.0 }
+  cv32e40p:               { git: "https://github.com/pulp-platform/cv32e40p.git", rev: e863f576699815b38cc9d80dbdede8ed5efd5991 } # `michaero/safety-island-clic` branch
+  ibex:                   { git: "https://github.com/pulp-platform/ibex.git", rev: "pulpissimo-v6.1.2" }
+  scm:                    { git: "https://github.com/pulp-platform/scm.git", rev: 74426dee36f28ae1c02f7635cf844a0156145320 } # branch: yt/bump-clkgating
+  hci:                    { git: "https://github.com/pulp-platform/hci.git", rev: 38fc2a7 } # branch: lg/ecc_rebase_v2.1.1
+  register_interface:     { git: "https://github.com/pulp-platform/register_interface.git", version: 0.4.4  }
+  redundancy_cells:       { git: "https://github.com/pulp-platform/redundancy_cells.git", rev: 49e714b97a19a7aaddf064ae2757c8f02d1f62dc } # branch: astral-v0
+  redmule:                { git: "https://github.com/pulp-platform/redmule.git", rev: 9223ccc } # branch: astral-hci-v2.1


It is fine to point to branches, just keep in mind that at some point these dependencies have to become releases (vx.y.x). No hurry on that side, as long as:

branches are preserved and nobody force-pushes

we keep this in mind and slowly align the various IPs

I agree. But to merge in the master, I would actually enforce pointing to version releases.

alex96295 · 2024-12-19T16:43:22Z

I gave a pass and left some comments

I suggest we:

Write a detailed section in the CHANGELOG to highlight what was added and fixed; for a release, we should ideally wait for all the dependencies to point to a release (stable version) instead of a branch, but we can release an alpha version
Improve the README with a Dependencies section. In the future, we could use online docs like in other repos (e.g. snitch cluster etc)

FrancescoConti

Hi all, I left a few comments. Some of these are cleanup and could be postponed to another PR, but given the already pretty massive size of this one I think it makes more sense to do the job cleanly already here.

FrancescoConti · 2024-12-19T16:46:25Z

Bender.local

do we really need this override?

It really depends on which core version we want to keep. As long as we stick to RISCY-NN it is not required, but I think at a certain point it would be better to merge all the features we developed into our main of CV32E40P (and I speak about features for mixed precision, redundancy, and fast IRQs, not sure if there are even others).

FrancescoConti · 2024-12-19T16:47:22Z

.gitignore

why .bender is removed from the .gitignore?

I agree, @belanoa please add it back.

FrancescoConti · 2024-12-19T16:48:06Z

Bender.yml

+  riscv:                  { git: "https://github.com/AlSaqr-platform/riscv_nn.git", rev: astral-v1.0 }
+  cv32e40p:               { git: "https://github.com/pulp-platform/cv32e40p.git", rev: e863f576699815b38cc9d80dbdede8ed5efd5991 } # `michaero/safety-island-clic` branch
+  ibex:                   { git: "https://github.com/pulp-platform/ibex.git", rev: "pulpissimo-v6.1.2" }
+  scm:                    { git: "https://github.com/pulp-platform/scm.git", rev: 74426dee36f28ae1c02f7635cf844a0156145320 } # branch: yt/bump-clkgating
+  hci:                    { git: "https://github.com/pulp-platform/hci.git", rev: 38fc2a7 } # branch: lg/ecc_rebase_v2.1.1
+  register_interface:     { git: "https://github.com/pulp-platform/register_interface.git", version: 0.4.4  }
+  redundancy_cells:       { git: "https://github.com/pulp-platform/redundancy_cells.git", rev: 49e714b97a19a7aaddf064ae2757c8f02d1f62dc } # branch: astral-v0
+  redmule:                { git: "https://github.com/pulp-platform/redmule.git", rev: 9223ccc } # branch: astral-hci-v2.1


I agree. But to merge in the master, I would actually enforce pointing to version releases.

FrancescoConti · 2024-12-19T16:52:17Z

env/astral-env.sh

This script does not export correctly the environment if not at IIS, for example on UNIBO machines. Please fix it so that it initializes to a reasonable default in this cases, e.g.,

export VLOG="$QUESTA vlog"

It would be best to also check that other variables (e.g., PULP_RUNTIME_GCC_TOOLCHAIN) are also properly set.

BTW: where are QUESTA_HOME and QUESTASIM_HOME used?

@belanoa you can copy this from here.

FrancescoConti · 2024-12-19T16:52:39Z

env/carfield-env.sh

same as astral-env.sh

FrancescoConti · 2024-12-19T17:44:46Z

rtl/pulp_cluster.sv

+logic [1:0]                                 s_TCDM_arb_policy;
+logic                                       tcdm_sleep;
+
+// FIXME: iDMA


remove dead code...

FrancescoConti · 2024-12-19T17:45:14Z

rtl/pulp_cluster.sv

+logic [Cfg.NumCores-1:0] hmr_dmr_sw_resynch_req, hmr_tmr_sw_resynch_req;
+logic [Cfg.NumCores-1:0] hmr_dmr_sw_synch_req, hmr_tmr_sw_synch_req;
+
+// FIXME: iDMA


remove dead code... unless you know what this FIXME actually means

FrancescoConti · 2024-12-19T17:47:23Z

rtl/pulp_cluster.sv

+//***************************************************
+//*********************DMAC WRAP*********************
+//***************************************************
+`ifdef TARGET_MCHAN


can we make this a parameter instead of this define?

See one of the comments above for the icache define. I think it is worth trying to use a parameter, but if I don't go wrong some of the code within the parameter scope was not properly handled by either the simulator or the synthesizer. We can try using a parameter of the cache selection first, and if it works, we use it also for the DMA. This said, in my opinion we should really choose for one DMA and discard the other.

FrancescoConti · 2024-12-19T17:48:21Z

rtl/pulp_cluster.sv

+  end
+endgenerate
+
+`ifdef SNITCH_ICACHE


can we make this a parameter?

also: what is the default? are we sure that the Snitch I$ is faster/more efficient/... than the shared I$?

I have left a comment above, unfortunately after taping out Astral I discovered the snitch cache is a bit buggy in handling the compressed instructions. The default should really be the hierarchical cache, but see other comments about parameters vs. defines I left above. I would push for trying using a parameter for the icache and, if it does not brake the simulator or the synthesizer, we can use it also for the DMAs. But I am quite sure the parameters in this case caused some issues, otherwise I cannot find any other reason for using the defines (I prefer parameters as well).

FrancescoConti · 2024-12-19T17:50:43Z

rtl/tcdm_banks_wrap.sv

+      assign tcdm_slave_dec.r_id = resp_id_q;
+
+      hci_ecc_dec #(
+        .`HCI_SIZE_PARAM(tcdm_target) ( HCI_MEM_SIZE ),


same commeent as above on hci_helpers.svh.

FrancescoConti · 2024-12-19T17:56:43Z

Also, @DanielKellerM @arpansur @sermazz as we discussed already please keep track of this PR

yvantor

I have requested a few changes, but overall great job! Sorry for getting here so late :)

yvantor · 2025-01-04T07:44:04Z

.gitignore

I agree, @belanoa please add it back.

yvantor · 2025-01-04T07:47:22Z

Bender.local

It really depends on which core version we want to keep. As long as we stick to RISCY-NN it is not required, but I think at a certain point it would be better to merge all the features we developed into our main of CV32E40P (and I speak about features for mixed precision, redundancy, and fast IRQs, not sure if there are even others).

yvantor · 2025-01-04T07:50:17Z

Bender.lock

+  cluster_icache:
+    revision: dd0e8f3497903a9ca99fc9f349d5a4f688ceb3ae
+    version: null
+    source:
+      Git: https://github.com/pulp-platform/cluster_icache.git
+    dependencies:
+    - axi
+    - common_cells
+    - register_interface
+    - scm
+    - tech_cells_generic


Small note now that I see this. Unfortunately, only after the Astral tapeout I figured out there is a bug with this icache handling compressed instructions. As long as someone can debug it properly I would avoit its usage (I should have added a define to discriminate between the snitch icache and the old hierarchical one).

yvantor · 2025-01-04T08:02:42Z

Bender.yml

-  - target: rtl
+  - rtl/core_region.sv


What's this for? I think we externalized the core demux from the old core region so this should no longer be required.

yvantor · 2025-01-04T08:04:25Z

Makefile

+bender_defs += -D PRIVATE_ICACHE
+bender_defs += -D HIERARCHY_ICACHE_32BIT
+bender_defs += -D ICAHE_USE_FF
+bender_defs += -D NO_FPU


Check this define, I remember the FPU selection was in the end made with a parameter, so if this define is no longer needed we can delete it.

yvantor · 2025-01-04T08:58:07Z

rtl/pulp_cluster.sv

+  end
+endgenerate
+
+`ifdef SNITCH_ICACHE


I have left a comment above, unfortunately after taping out Astral I discovered the snitch cache is a bit buggy in handling the compressed instructions. The default should really be the hierarchical cache, but see other comments about parameters vs. defines I left above. I would push for trying using a parameter for the icache and, if it does not brake the simulator or the synthesizer, we can use it also for the DMAs. But I am quite sure the parameters in this case caused some issues, otherwise I cannot find any other reason for using the defines (I prefer parameters as well).

yvantor · 2025-01-04T09:00:04Z

rtl/pulp_cluster.sv

+  .test_mode_i           ( test_mode_i              ),
+  // Scrubber
+  .scrub_trigger_i       ( scrubber_trigger         ),
+                                                       // in the cluster control unit.


Put this in line.

yvantor · 2025-01-04T09:03:08Z

rtl/xbar_pe_wrap.sv

+  parameter NB_CORES           = 12,
+  parameter NB_MPERIPHS        = 1,
+  parameter NB_SPERIPHS        = 10,   
+  parameter ADDR_WIDTH         = 32,
+  parameter DATA_WIDTH         = 32,
+  parameter BE_WIDTH           = 0,
+  parameter PE_ROUTING_LSB     = 10,
+  parameter PE_ROUTING_MSB     = 13,
+  parameter bit HWPE_PRESENT   = 1'b1,
+  parameter CLUSTER_ALIAS      = 1,
+  parameter CLUSTER_ALIAS_BASE =  12'h000,
+  parameter ADDREXT         = 1'b0,


Suggested change

parameter NB_CORES = 12,

parameter NB_MPERIPHS = 1,

parameter NB_SPERIPHS = 10,

parameter ADDR_WIDTH = 32,

parameter DATA_WIDTH = 32,

parameter BE_WIDTH = 0,

parameter PE_ROUTING_LSB = 10,

parameter PE_ROUTING_MSB = 13,

parameter bit HWPE_PRESENT = 1'b1,

parameter CLUSTER_ALIAS = 1,

parameter CLUSTER_ALIAS_BASE = 12'h000,

parameter ADDREXT = 1'b0,

parameter int unsigned NB_CORES = 12,

parameter int unsigned NB_MPERIPHS = 1,

parameter int unsigned NB_SPERIPHS = 10,

parameter int unsigned ADDR_WIDTH = 32,

parameter int unsigned DATA_WIDTH = 32,

parameter int unsigned BE_WIDTH = 0,

parameter int unsigned PE_ROUTING_LSB = 10,

parameter int unsigned PE_ROUTING_MSB = 13,

parameter bit HWPE_PRESENT = 1'b1,

parameter bit CLUSTER_ALIAS = 1,

parameter logic [11:0] CLUSTER_ALIAS_BASE = 'h000,

parameter bit ADDREXT = 1'b0,

yvantor · 2025-01-04T09:04:30Z

rtl/xbar_pe_wrap.sv

+  if (CLUSTER_ALIAS == 1)
+    assign cluster_alias = 1'b1;
+  else 
+    assign cluster_alias = 1'b0;


Suggested change

if (CLUSTER_ALIAS == 1)

assign cluster_alias = 1'b1;

else

assign cluster_alias = 1'b0;

assign cluster_alias = CLUSTER_ALIAS;

yvantor · 2025-01-04T09:09:51Z

wave.do

Do we need this file? I think having a script for automatically loading the interesting waves makes sense but this is uselessly large. Have a look at how I did it in RedMulE, it should simplify the waves script also for future changes.

Yvan Tortorella and others added 30 commits May 11, 2023 22:42

Removed all references to MP and SP instruction caches.

5b85a5a

Bumped core, FPU interco, and icache.

701b4d9

Bumped fpu_interco.

035b12c

Removed internal reset synchronizer and clock gating cell.

09d9dd1

Moved XBAR address map rule out of pulp_cluster_package to parametriz…

2072843

…e the address width.

Unconnected signals.

11c1e53

Added AXI isolate.

9376b77

rtl: Fix parameter name propagation

65936b7

Fixed typos in AXI buses.

a7ced88

Removed slave axi_isolate.

f8a5697

Bump FPU dependency to pulp-v0.1.1

6c0b6b6

Bump fpu_interco

9445f35

Bump fpu_interco to FPU pulp-v0.1.1

3f42045

Bump fpu_interco

f18f07d

* Fix syntax error

Partial iDMA integration; bump mchan to fixed parametrization version…

507ef16

…; other general fixes.

Fix CI flow and clearify standalone simulation steps in README.

91ec9e6

Bumped core and added setback signal.

d50efde

Bumped FPU interconnect.

2fe9296

Updating FPU interconnect to align it with updated FPU.

56d6463

First draft with power-rest and AXI isolate synchronization.

0435086

Synchronized control signals and added mailbox interrupt.

43bb73f

Connected pwr_on_rst_ni and mbox_irq_i in testbench.

bd69804

Making parallel matrix multiplication work

01c2d00

Externalized core demultiplexers.

1cc9961

Added RI5CY selectable via parameter.

adc8068

HMR integration.

4c5e675

Fixed unconnected signals.

48cf448

Integrated RedMulE.

9404fdb

Updated dependencies.

ba8f3cc

Turning core_id from parameter to input.

62ee862

alex96295 changed the title ~~Port astral features to master branch~~ Upstream carfield/astral features to main Dec 4, 2024

alex96295 self-requested a review December 4, 2024 12:33

alex96295 assigned belanoa Dec 4, 2024

Update nonfree commit

a46fd2b

alex96295 marked this pull request as draft December 6, 2024 15:37

alex96295 and others added 15 commits December 6, 2024 16:48

Makefile: Update nonfree commit

78c6a02

Makefile: update nonfree commit

866cfbc

Update .gitignore

bbbe426

Remove submodules

296ad3b

Makefile: update nonfree commit

7f1fddf

README.md: Align to flow without submodules

879d0e0

treewide: Remove submodules for direct clone

6071fc5

Makefile: Cleanup and remove deadcode

c3b674d

Makefile: Update nonfree commit

80ae936

Makefile: update pulp-runtime commit

8c75699

Makefile: update pulp-runtime commit

825dc01

Makefile: update regression-tests commit

709ea78

Makefile: update nonfree commit and regression_tests commit

a4701e1

Improve HMR Unit parametrization

e024ebd

Improve ECC parametrization

c1f176f

belanoa marked this pull request as ready for review December 13, 2024 09:46

This was referenced Dec 19, 2024

Bump NEureka to use FF-based RegFile. #84

Closed

Update icache with bugfixes from injection testing #83

Open

alex96295 reviewed Dec 19, 2024

View reviewed changes

Update instruction cache with injection testing

1eb04b0

FrancescoConti requested changes Dec 19, 2024

View reviewed changes

yvantor reviewed Jan 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream `carfield`/`astral` features to `main` #87

Upstream `carfield`/`astral` features to `main` #87

belanoa commented Nov 13, 2024

alex96295 commented Dec 4, 2024 •

edited

Loading

alex96295 Dec 19, 2024 •

edited

Loading

FrancescoConti Dec 19, 2024

alex96295 commented Dec 19, 2024 •

edited

Loading

FrancescoConti left a comment

FrancescoConti Dec 19, 2024

yvantor Jan 4, 2025

FrancescoConti Dec 19, 2024

yvantor Jan 4, 2025

FrancescoConti Dec 19, 2024

FrancescoConti Dec 19, 2024

yvantor Jan 4, 2025

FrancescoConti Dec 19, 2024

yvantor Jan 4, 2025

FrancescoConti Dec 19, 2024

FrancescoConti Dec 19, 2024

FrancescoConti Dec 19, 2024

yvantor Jan 4, 2025

FrancescoConti Dec 19, 2024

FrancescoConti Dec 19, 2024

yvantor Jan 4, 2025

FrancescoConti Dec 19, 2024

FrancescoConti commented Dec 19, 2024

yvantor left a comment

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

yvantor Jan 4, 2025

Upstream carfield/astral features to main #87

Are you sure you want to change the base?

Upstream carfield/astral features to main #87

Conversation

belanoa commented Nov 13, 2024

alex96295 commented Dec 4, 2024 • edited Loading

alex96295 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex96295 commented Dec 19, 2024 • edited Loading

FrancescoConti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrancescoConti commented Dec 19, 2024

yvantor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Upstream `carfield`/`astral` features to `main` #87

Upstream `carfield`/`astral` features to `main` #87

alex96295 commented Dec 4, 2024 •

edited

Loading

alex96295 Dec 19, 2024 •

edited

Loading

alex96295 commented Dec 19, 2024 •

edited

Loading