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.

Sign up for GitHub

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

Aligned with HERO and solved 64-bits atomics warnings. #12

Open
wants to merge 4 commits into
base: atomics_merge
Choose a base branch
from
Open

Aligned with HERO and solved 64-bits atomics warnings. #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9242841
Added condition for LIC topology within assertion.
Feb 14, 2022
ba37ace
Modified support for 64-bit datawidth to avoid datawidth warnings.
Feb 14, 2022
b265a44
Comments on modified code and bitwidth fix.
Feb 14, 2022
de2ef4f
Fixed amo_result assignment.
Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 27 additions & 22 deletions rtl/tcdm_interconnect/amo_shim.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,20 +51,32 @@ module amo_shim #(
logic [AddrMemWidth-1:0] addr_q;

logic [31:0] amo_operand_a;
logic [31:0] amo_operand_b_q;
logic [31:0] amo_operand_b_d,
amo_operand_b_q;
// requested amo should be performed on upper 32 bit
logic upper_word_q;
logic [31:0] swap_value_q;
logic [31:0] amo_result; // result of atomic memory operation

always_comb begin
if (DataWidth == 64 && upper_word_q) begin
amo_operand_a = out_rdata_i[63:32];
end else begin
amo_operand_a = out_rdata_i[31:0];
logic upper_word_d,
upper_word_q;
logic [31:0] swap_value_d,
swap_value_q;
logic [31:0] amo_res,
amo_result; // result of atomic memory operation

always_comb begin // Generate parametric code for 64-bit datawidth (only 32-bit and 64-bit supported)
if (DataWidth == 64) begin
amo_operand_a = (upper_word_q) ? out_rdata_i[DataWidth-1:DataWidth-32] : out_rdata_i[31:0];
amo_operand_b_d = (!in_be_i[0]) ? in_wdata_i[DataWidth-1:DataWidth-32] : in_wdata_i[31:0];
upper_word_d = in_be_i[4];
swap_value_d = in_wdata_i[DataWidth-1:DataWidth-32];
amo_res = (upper_word_q) ? out_rdata_i[DataWidth-1:DataWidth-32] : out_rdata_i[31:0];
end else begin // Standard 32-bit datawidth implementation
amo_operand_a = out_rdata_i[31:0];
amo_operand_b_d = in_wdata_i[31:0];
upper_word_d = '0;
swap_value_d = '0;
amo_res = out_rdata_i[31:0];
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid whitespace changes.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, this was undesired.

always_comb begin
// feed-through
out_req_o = in_req_i;
Expand Down Expand Up @@ -125,16 +137,9 @@ module amo_shim #(
if (load_amo) begin
amo_op_q <= amo_op_t'(in_amo_i);
addr_q <= in_add_i;
if (DataWidth == 64) begin
if (!in_be_i[0]) begin
amo_operand_b_q <= in_wdata_i[63:32];
end
upper_word_q <= in_be_i[4];
// swap value is located in the upper word
swap_value_q <= in_wdata_i[63:32];
end else begin
amo_operand_b_q <= in_wdata_i[31:0];
end
amo_operand_b_q <= amo_operand_b_d;
upper_word_q <= upper_word_d;
swap_value_q <= swap_value_d;
state_q <= DoAMO;
end else begin
amo_op_q <= AMONone;
Expand Down Expand Up @@ -191,7 +196,7 @@ module amo_shim #(
amo_result = swap_value_q;
// values are not euqal -> don't update
end else begin
amo_result = upper_word_q ? out_rdata_i[63:32] : out_rdata_i[31:0];
amo_result = amo_res;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do both implementations behave the same? In the one you changed, amo_result would get assigned to out_rdata_i[63:32] if both upper_word_q and DataWidth == 64. In your diff, it seems like you only take the DataWidth == 64 condition into account. Could you please check this?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, it should be fixed now!

end
`ifndef TARGET_SYNTHESIS
end else begin
Expand Down
2 changes: 1 addition & 1 deletion rtl/tcdm_interconnect/tcdm_interconnect.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ end
initial begin
assert(AddrMemWidth+NumOutLog2 <= AddrWidth) else
$fatal(1,"Address not wide enough to accomodate the requested TCDM configuration.");
assert(NumOut >= NumIn) else
assert( (NumOut >= NumIn) | (Topology == tcdm_interconnect_pkg::LIC) ) else
$fatal(1,"NumOut < NumIn is not supported.");
end
Comment on lines 315 to 320
Copy link
Contributor

@suehtamacv suehtamacv Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since those are static checks, could you move them out of the initial block? Then the synthesis tools cold also check the validity of those parameters.

if (AddrMemWidth+NumOutLog2 > AddrWidth)
    $fatal(1,"Address not wide enough to accomodate the requested TCDM configuration.");

if ((NumOut < NumIn) && (Topology != tcdm_interconnect_pkg::LIC))
    $fatal(1,"NumOut < NumIn is not supported.");

Copy link
Author

Choose a reason for hiding this comment

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

Ok! There are actually more assertions made the same way (such as this https://github.com/AlSaqr-platform/cluster_interconnect/blob/alsaqr_cluster/rtl/tcdm_interconnect/tcdm_interconnect.sv#L268 and also this https://github.com/AlSaqr-platform/cluster_interconnect/blob/alsaqr_cluster/rtl/tcdm_interconnect/tcdm_interconnect.sv#L302), should I apply the same modifications also there?

// pragma translate_on
Expand Down