Feature/NVMulator #369

sajjadtamimi · 2023-07-17T13:12:40Z

No description provided.

cahz

Thank you for the contribution. I have a couple of change requests related to the toolflow part. Someone else needs to look at the runtime changes.

cahz · 2023-07-18T14:07:22Z

toolflow/vivado/platform/common/plugins/NVMulator.tcl

+		return false
+	}
+
+	proc add_nvmulator {} {


This needs to accept arguments and pass it through

Thanks for your review.
Done.

cahz · 2023-07-18T14:09:42Z

toolflow/vivado/platform/common/plugins/NVMulator.tcl

+		connect_bd_net [get_bd_pins /memory/mig/c0_ddr4_ui_clk] [get_bd_pins /memory/NVEmulator_0/CLK]
+		connect_bd_net [get_bd_pins /memory/mem_peripheral_aresetn] [get_bd_pins /memory/NVEmulator_0/RST_N]
+
+
+		puts "Adding NVMulator 0 programmer: memory-side"		
+		delete_bd_objs [get_bd_intf_nets /memory/mig_ic_M00_AXI]
+		connect_bd_intf_net [get_bd_intf_pins /memory/NVEmulator_0/M_AXI_MIG] [get_bd_intf_pins /memory/mig/C0_DDR4_S_AXI]
+		connect_bd_intf_net [get_bd_intf_pins /memory/mig_ic/M00_AXI] [get_bd_intf_pins /memory/NVEmulator_0/S_AXI_NV]
+
+		puts "Connecting NVMulator programmer: memory-side"
+		create_bd_cell -type ip -vlnv xilinx.com:ip:smartconnect:1.0 /memory/smartconnect_0
+		set_property -dict [list CONFIG.NUM_CLKS {2} CONFIG.NUM_MI {1} CONFIG.NUM_SI {1}] [get_bd_cells /memory/smartconnect_0]
+		connect_bd_net [get_bd_pins /memory/smartconnect_0/aclk] [get_bd_pins /memory/mig/c0_ddr4_ui_clk]
+		connect_bd_net [get_bd_pins /memory/smartconnect_0/aclk1] [get_bd_pins /memory/design_clk] 
+		connect_bd_net [get_bd_pins  /memory/mem_peripheral_aresetn] [get_bd_pins /memory/smartconnect_0/aresetn]
+		current_bd_instance "/memory"
+		set s_nvm [create_bd_intf_pin -mode Slave -vlnv xilinx.com:interface:aximm_rtl:1.0 "S_NVM"]
+		connect_bd_intf_net [get_bd_intf_pins /memory/S_NVM] [get_bd_intf_pins /memory/smartconnect_0/S00_AXI]
+		connect_bd_intf_net [get_bd_intf_pins /memory/smartconnect_0/M00_AXI] [get_bd_intf_pins /memory/NVEmulator_0/S_AXI]


This is not portable and has too many assumptions on port names. It will only work on the Alveo U280. In the current form, the plugin should not be under platform/common but instead restricted to U280.

I made it general. I tried it on AU250 and the plugin was able to integrate NVMulator as expected.

cahz · 2023-07-18T14:11:41Z

toolflow/vivado/platform/common/plugins/NVMulator.tcl

+			exit 1
+		}
+
+		save_bd_design


Please remove debugging commands such as save_bd_design

cahz · 2023-07-18T14:12:12Z

toolflow/vivado/platform/common/plugins/NVMulator.tcl

+		puts "Connecting NVMulator programmer: host to memory"
+		connect_bd_intf_net [get_bd_intf_pins /host/M_NVM] [get_bd_intf_pins /memory/S_NVM]
+
+		current_bd_instance


does this command have any function when used without parameter?

Yes, it uses default value (i.e., 0 for read_latency and write_latency).

I was refering to the current_bd_instance

I see. I also updated input for current_bd_instance

cahz · 2023-07-18T14:12:34Z

toolflow/vivado/platform/common/plugins/NVMulator.tcl

+
+		current_bd_instance
+	}
+}


Indentation is off

cahz · 2023-07-18T14:13:24Z

toolflow/vivado/platform/AU280/plugins/NVMulator.tcl

+namespace eval nvmulator {
+
+  proc is_nvmulator_supported {} {
+	  puts "AU280"


This output is not meaningful. Either leave it out or add more context.

I removed it.

wirthjohannes · 2023-07-19T09:18:34Z

I had a short look at the runtime changes.
In my opinion these changes should not be in the "standard" runtime, as they are very specific to this NVMulator (which probably won't be used by most people). This is not specific to this change, but a general problem with the runtime, especially as we don't have an equivalent to e.g. the features in the toolflow.
But I still think, that we shouldn't just continue always adding such specific changes to the general runtime. Especially when this probably can be achieved with less specific changes to the runtime, or maybe none at all.

If I understand it correctly, the changes to the runtime basically just configure the NVMulator IP (via an AXI-Lite slave), correct?

I want to propose some alternatives, on how to achieve this:

Add the NVMulator IP to the PEs configured for your composition with a special PE_ID. Then you could just configure it via the existing launch() calls. While this is somewhat hacky, but it would require no changes to the runtime at all.
Extend the runtime with an option to "write" to platform components (or at least some of them). Something like configure_platform_component(string component_name, uint64_t offset, uint64_t data) (and the other direction query_platform_component). This would look at the components specified in the status core and find the correct one via the name. While this of course requires changes to the runtime, they would not be specific to the NVMulator, but also useful for other stuff (e.g. querying ECC status information).

…_nvmulator proc, and Indentation

cahz · 2024-03-26T10:21:17Z

Is there any progress on this PR? I agree with the suggestions of @wirthjohannes, we need a more general approach to get this PR merged.

Add support for NVMulator

d281da2

sajjadtamimi requested a review from cahz July 17, 2023 13:12

cahz reviewed Jul 18, 2023

View reviewed changes

Remove debugging commands, making plugin general, adding input to add…

c478459

…_nvmulator proc, and Indentation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/NVMulator #369

Feature/NVMulator #369

sajjadtamimi commented Jul 17, 2023

cahz left a comment

cahz Jul 18, 2023

sajjadtamimi Jul 21, 2023

cahz Jul 18, 2023

sajjadtamimi Jul 21, 2023

cahz Jul 18, 2023

sajjadtamimi Jul 21, 2023

cahz Jul 18, 2023

sajjadtamimi Jul 21, 2023

cahz Jul 21, 2023

sajjadtamimi Jul 21, 2023

cahz Jul 18, 2023

sajjadtamimi Jul 21, 2023

cahz Jul 18, 2023

sajjadtamimi Jul 21, 2023

wirthjohannes commented Jul 19, 2023 •

edited

cahz commented Mar 26, 2024

Feature/NVMulator #369

Are you sure you want to change the base?

Feature/NVMulator #369

Conversation

sajjadtamimi commented Jul 17, 2023

cahz 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

wirthjohannes commented Jul 19, 2023 • edited

cahz commented Mar 26, 2024

wirthjohannes commented Jul 19, 2023 •

edited