Multiple HWPE support + ECC by FrancescoConti · Pull Request #53 · pulp-platform/hci

FrancescoConti · 2025-03-07T21:43:25Z

This PR adds:

support for multiple HWPEs on the shallow branch. Supersedes multiple hwpe support in parallel #45 (prasadar/multi-hwpe-support) @arpansur
support for ECC on the shallow branch. Supersedes Extend HCI with ECC #25, Extend HCI with ECC #39, Extend HCI with ECC #43 (lg/ecc_rebase_v2.1.1) @LuigiGhionda

… interconnect-side

…/decoding is needed

* Introduce additional encoders/decoders along the interconnect to properly handle ECC bits across the different modules * Add router and arbiter modules able to properly handle ECC bits

…nd by the slave port type

…dify the original modules accordingly Initially, dedicated modules were introduced for simplicity. However, to avoid adding unnecessary files, they have now been removed and merged with the original modules

UW default value is now 1 instead of 0

* Errors on data are evaluated on read valid * Errors on metadata are evaluated on successful handshake

…in parallel

Some HWPE might depend on the r_valid for the read response whereas don't care for the write and they can support multiple outstanding instructions. So a r_valid generated for a write coming after multiple cycles could behave like a r_valid for a read response(read request follows a write request). Even though the TCDM responds the data after a single cycle latency, any FIFO in the request path can cause the r_valid to arrive a few cycles later even though the handshake is done(writing data to the FIFO). For selected HWPEs we can filter the r_valid for the write requests using FILTER_WRITE_R_VALID parameter exposed to the top.

fix arbiter tree to handle any number of wide ports

2D interface arrays are not synthesizable at the moment. Thus, 2D interfaces are flattened to 1D arrays. Additionally the alignements are fixed

FrancescoConti

Cannot be an "official" review because I opened the PR, but I do have a few comments, mostly cosmetical. Despite the name of the PR, which mostly refers to @arpansur's work, the bulk of the comments refer to the ECC work from @LuigiGhionda.
@sermazz in an ideal world it would be a bit better to split this in two PRs (one for ECC, one for multi-HWPE) but since it's already here, and tested, I suggest we complete it a bit and then merge as-is, without splitting.

@LuigiGhionda on the latter, I think that there is a need for a bit of cleanup in documentation, very minor things such as parameter names using the Lowrisc style instead of the style used in the rest of hci, and my always-hated parametric types (I know, I'm boring!). Moreover, it would be good since you have a register generator to include the hjson source in a subfolder (e.g., ecc/gen?) and, importantly, also include the script used for generation. All of this is non-urgent and non-blocking and if for any reason we decide we need this PR merged sooner, we can go on - perhaps opening an issue to not forget to fix these things.

FrancescoConti · 2025-03-09T21:59:52Z

Bender.yml

  hwpe-stream:            { git: "https://github.com/pulp-platform/hwpe-stream.git", version: 1.8.0 }
  cluster_interconnect:   { git: "https://github.com/pulp-platform/cluster_interconnect.git", rev: 1284def6c0b7f7e9355eb093d00883ad9dead1b7 }
  L2_tcdm_hybrid_interco: { git: "https://github.com/pulp-platform/L2_tcdm_hybrid_interco.git", version: 1.0.0 }
+  redundancy_cells:       { git: "https://github.com/pulp-platform/redundancy_cells.git", rev: c37bdb47339bf70e8323de8df14ea8bbeafb6583 } # branch: astral_rebase


can we make this a version @LuigiGhionda ?

FrancescoConti · 2025-03-09T22:03:58Z

rtl/ecc/hci_ecc_dec.sv

+ */
+
+ /**
+ * ADD DESCRIPTION


Necessary to add (minimal) description before merging!

FrancescoConti · 2025-03-09T22:04:25Z

rtl/ecc/hci_ecc_enc.sv

+ */
+
+/**
+ * ADD DESCRIPTION


Add description!

FrancescoConti · 2025-03-09T22:05:11Z

rtl/ecc/hci_ecc_interconnect.sv

+ *   | *EXPFIFO*           | 0                           | Depth of HCI router FIFO.                                                        |
+ *   +---------------------+-----------------------------+----------------------------------------------------------------------------------+
+ *   | *SEL_LIC*           | 0                           | Kind of LIC to instantiate (0=regular L1, 1=L2).                                 |
+ *   +---------------------+-----------------------------+----------------------------------------------------------------------------------+


missing some parameters here!

FrancescoConti · 2025-03-09T22:06:56Z

rtl/ecc/hci_ecc_sink.sv

+ * this License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+ * CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */


Add doc or refer to core_sink

FrancescoConti · 2025-03-09T22:16:17Z

rtl/ecc/hci_ecc_manager.sv

+  import hci_ecc_manager_reg_pkg::*;
+#(
+  parameter int unsigned N_CHUNK = 1,
+  parameter int unsigned ParData = 1,


In hci we typically use a snake-case all-caps style for parameters (from before Lowrisc). Can you align ParData to PAR_DATA, ParMeta to PAR_META?

FrancescoConti · 2025-03-09T22:16:58Z

rtl/ecc/hci_ecc_manager_reg_pkg.sv

+// Licensed under the Apache License, Version 2.0, see LICENSE for details.
+// SPDX-License-Identifier: Apache-2.0
+//
+// Register Package auto-generated by `reggen` containing data structure


can you include the actual script used to generate from hjson? I am a reggen newbie ;)

FrancescoConti · 2025-03-09T22:18:54Z

rtl/interco/hci_router.sv

 #(
  parameter int unsigned FIFO_DEPTH  = 0,
  parameter int unsigned NB_OUT_CHAN = 8,
+  parameter bit          UseECC      = 0,


In hci we typically use a snake-case all-caps style for parameters (from before Lowrisc). Can you align UseECC to USE_ECC?

FrancescoConti · 2025-03-09T22:19:12Z

rtl/interco/hci_router_reorder.sv

  parameter int unsigned NB_OUT_CHAN = 2,
-  parameter int unsigned FILTER_WRITE_R_VALID = 0
+  parameter int unsigned FILTER_WRITE_R_VALID = 0,
+  parameter bit          UseECC = 0


In hci we typically use a snake-case all-caps style for parameters (from before Lowrisc). Can you align UseECC to USE_ECC?

FrancescoConti · 2025-03-09T22:19:27Z

rtl/interco/hci_router_reorder.sv


 );

+  localparam int unsigned RespDataWidth = (UseECC) ? (32+7) : 32;


In hci we typically use a snake-case all-caps style for parameters (from before Lowrisc). Can you align RespDataWidth to RESP_DATA_WIDTH?

FrancescoConti · 2025-03-09T22:40:51Z

rtl/ecc/hci_ecc_manager_reg_top.sv

+  // Register instances
+  // R[data_correctable_errors]: V(False)
+
+  prim_subreg #(


I fear these autogenerated registers are not soft-clearable. Can we fix that by using some reggen option?

sermazz

@FrancescoConti I don't mind not splitting, we can complete this PR and merge everything together. I took the freedom to extend the description of the PR for documentation purposes :)

I have just one more comment: when using the top hci_interconnect we can have multiple HWPEs on the shallow branch (line). However, the top for ECC, hci_ecc_interconnect is still hardcoded to one HWPE only (line).
I stumbled upon this when integrating Chimera's PULP Cluster on Astral's, where you can configure whether you want ECC or not. This difference in the interface creates issues when instantiating the HCI, so currently in the PULP Cluster I have an assertion that errors out if ECC is enabled when HCI has >1 HWPE ports (see here).
I started implementing support for this as I thought it was a matter of only hci_ecc_enc and _dec but then I got blocked when I realized also hci_ecc_manager requires modifications, obviously.
Is there a use-case for multiple HWPEs using ECC? Do we want to add support for that? Is it complicated? @LuigiGhionda

FrancescoConti · 2025-03-10T20:34:33Z

I would say it is for sure interesting (for all the post-Astral line of research, which is very active and alive) but not necessarily something to fix in this PR which is already big. my2c, @LuigiGhionda what's your view on that (also @yvantor might have opinions)

yvantor · 2025-03-10T22:08:03Z

I would say it is for sure interesting (for all the post-Astral line of research, which is very active and alive) but not necessarily something to fix in this PR which is already big. my2c, @LuigiGhionda what's your view on that (also @yvantor might have opinions)

I think allowing for multiple HWPEs that support ECC is in general very interesting since now we have two accelerators that have fault tolerance features (NEureka and RedMulE). Might be useful to open an issue to keep it in the back of our minds and tackle it in a different dedicated PR if the HCI requires significant changes.

sermazz · 2025-03-10T22:48:46Z

I would say it is for sure interesting (for all the post-Astral line of research, which is very active and alive) but not necessarily something to fix in this PR which is already big. my2c, @LuigiGhionda what's your view on that (also @yvantor might have opinions)

I think allowing for multiple HWPEs that support ECC is in general very interesting since now we have two accelerators that have fault tolerance features (NEureka and RedMulE). Might be useful to open an issue to keep it in the back of our minds and tackle it in a different dedicated PR if the HCI requires significant changes.

Ok then, sounds good. Do you think we can put an assert somewhere in the HCI so the top-level instantiating it doesn't need to be aware of this?
I think it makes sense to align the interface of hci and hci_ecc (so they both have an array of HWPE interfaces) and error out in the hci_ecc when N_HWPE != 1.

ECC HCI top-level now has the same interface of the normal HCI (i.e., with multiple HWPE ports) to avoid problems with higher-level parametric instantiations of the HCI. The ECC HCI top-level currently does not support multiple HWPEs and errors out when N_HWPE > 1

sermazz · 2025-03-11T11:39:25Z

Ok then, sounds good. Do you think we can put an assert somewhere in the HCI so the top-level instantiating it doesn't need to be aware of this? I think it makes sense to align the interface of hci and hci_ecc (so they both have an array of HWPE interfaces) and error out in the hci_ecc when N_HWPE != 1.

Done, and opened issue #54

Moved to issue #54

FrancescoConti · 2025-04-24T16:29:56Z

Ping @LuigiGhionda @arpansur !

arpansur

multi-hwpe integration looks fine. Some enhancements are needed is tracked using
#56 (comment)

rtl/interco/hci_router.sv

rtl/interco/hci_router_reorder.sv

rtl/hci_interconnect.sv

Luigi Ghionda and others added 30 commits August 14, 2024 15:07

Add HCI ECC encoder and decoder modules

83d46c5

Add HCI ECC source and sink modules

84df642

Add HCI ECC interconnect module

b5d70c3

Fix ecc signals propagation

5e91ea1

Add memory-mapped registers to store the number of detected errors on…

dc89363

… interconnect-side

Add option to bypass data encoding/decoding if only metadata encoding…

624b12f

…/decoding is needed

Update interconnect to support ECC extension up to the memory side

dcae39b

* Introduce additional encoders/decoders along the interconnect to properly handle ECC bits across the different modules * Add router and arbiter modules able to properly handle ECC bits

Add error signaling to ECC encoder

7a6094c

Redesign memory-mapped error-counting registers

41b62a1

Add error signaling to ECC source module

72dd758

Remove periph_to_reg module from inside the ecc_manager to not be bou…

3696b5f

…nd by the slave port type

Update authors

d2f505b

Remove the dedicated router and router_reorder modules and instead mo…

7f73e9b

…dify the original modules accordingly Initially, dedicated modules were introduced for simplicity. However, to avoid adding unnecessary files, they have now been removed and merged with the original modules

Small cleanup

a3a784a

Remove r_opc signal from the encoding since it's not used at the moment

85f5675

Change the minimum value of UW to encode also the user/r_user signal

f903d0a

UW default value is now 1 instead of 0

Fix errors exposure

75bcc61

* Errors on data are evaluated on read valid * Errors on metadata are evaluated on successful handshake

Parameters cleanup

38fc2a7

[WIP] add hci_core_assign_expand module, enable multiple HWPEs

1cf84f8

hci_interconnect.sv: fix ID widths

f889112

modify hci_core_id_filter so that it supports r_valid delay

11892e7

[cosmetic] modified the code with more meaningful name and comments

26aa20b

[update] added arbiter tree to enable multiple hwpe accessing memory …

cd89c25

…in parallel

[update] added 2D and 3D array definitions to the hci interface

79e447f

[update] instantiated arbiter_tree to arbitrate between multiple hwpes

a5ad0c5

[fix] bug fix on handling the requests through arbiter tree

fa1f930

[fix] fix arbiter tree

beb5f09

fix arbiter tree to handle any number of wide ports

[update] removed reliance on 2D interface arrays

14915aa

2D interface arrays are not synthesizable at the moment. Thus, 2D interfaces are flattened to 1D arrays. Additionally the alignements are fixed

[cosmetic] white space

fb1f401

Fix merge issues

2d5ae78

FrancescoConti mentioned this pull request Mar 7, 2025

multiple hwpe support in parallel #45

Closed

FrancescoConti requested review from arpansur and sermazz March 7, 2025 21:44

FrancescoConti self-assigned this Mar 7, 2025

FrancescoConti commented Mar 9, 2025

View reviewed changes

FrancescoConti requested a review from LuigiGhionda March 9, 2025 22:28

FrancescoConti commented Mar 9, 2025

View reviewed changes

sermazz mentioned this pull request Mar 10, 2025

Upstream features from chimera pulp-platform/pulp_cluster#89

Closed

14 tasks

sermazz previously requested changes Mar 10, 2025

View reviewed changes

sermazz changed the title ~~Multiple HWPE support (new)~~ Multiple HWPE support + ECC Mar 11, 2025

sermazz force-pushed the smazzola/chimera branch from 50a977a to 047d0a0 Compare March 11, 2025 11:28

sermazz force-pushed the smazzola/chimera branch from 047d0a0 to 4257a73 Compare March 11, 2025 11:35

sermazz mentioned this pull request Mar 11, 2025

Add support for multiple HWPEs in ECC interconnect #54

Closed

arpansur approved these changes Apr 25, 2025

View reviewed changes

Whitespace + Cosmetic changes

4dcc216

FrancescoConti merged commit 47aa527 into master Apr 28, 2025

LuigiGhionda mentioned this pull request Apr 29, 2025

Minor Fixes for ECC-HCI (Follow-up on #53) #57

Merged


		);

		localparam int unsigned RespDataWidth = (UseECC) ? (32+7) : 32;

Conversation

FrancescoConti commented Mar 7, 2025 • edited by sermazz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrancescoConti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sermazz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoConti commented Mar 10, 2025

Uh oh!

yvantor commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sermazz commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sermazz commented Mar 11, 2025

Uh oh!

FrancescoConti commented Apr 24, 2025

Uh oh!

arpansur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

FrancescoConti commented Mar 7, 2025 •

edited by sermazz

Loading

sermazz left a comment •

edited

Loading

yvantor commented Mar 10, 2025 •

edited

Loading

sermazz commented Mar 10, 2025 •

edited

Loading