Skip to content

[hw,dv,sw] Add support for HW_ID for SW#584

Open
martin-velay wants to merge 3 commits into
lowRISC:mainfrom
martin-velay:sw_dv_hw_id
Open

[hw,dv,sw] Add support for HW_ID for SW#584
martin-velay wants to merge 3 commits into
lowRISC:mainfrom
martin-velay:sw_dv_hw_id

Conversation

@martin-velay
Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Collaborator

@engdoreis engdoreis left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, can you read the HW_ID in the test_framework and print it as test?

Comment thread hw/top_chip/dv/env/top_chip_dv_env_pkg.sv
Copy link
Copy Markdown
Collaborator

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

We should also have an ID for the FPGA platform.

Comment thread hw/top_chip/dv/tb/tb.sv Outdated
localparam bit [31:0] VERILATOR_SW_DV_TEST_STATUS_ADDR = VERILATOR_SW_DV_START_ADDR + 'h00;
localparam bit [31:0] VERILATOR_SW_DV_HW_ID_ADDR = VERILATOR_SW_DV_START_ADDR + 'h08;

localparam bit [31:0] VERILATOR_HW_ID = 32'h0000_001A; // Verilator specific ID
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How did you choose this value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I followed @engdoreis instructions from this issue: #423

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@marnovandermaas, shall I keep it as it is or do you have another preference?

@martin-velay
Copy link
Copy Markdown
Contributor Author

@marnovandermaas, about:

We should also have an ID for the FPGA platform.

I am unsure how this can be implemented for the FPGA platform. Maybe I can add something similar as done for Verilator?

@martin-velay
Copy link
Copy Markdown
Contributor Author

@engdoreis

I have tested, and indeed there was an issue. Now I can see this in the trace_hart_0.log:

547633ns    27228 M 00000000100016e6 0 00056603 lwu            a2, 0(a0)             a2  :000000000000002a a0  :0000000020020008 VA: 0000000020020008 PA: 00000020020008

With this patch: diff_sw_dv_hw_id.patch

@martin-velay martin-velay force-pushed the sw_dv_hw_id branch 3 times, most recently from cd48d60 to af8e982 Compare June 1, 2026 15:14
@martin-velay
Copy link
Copy Markdown
Contributor Author

I have added similar thing for the FPGA via an "AXI sink". I have never done any FPGA, can I have guidance on how I can verify it works?

@martin-velay martin-velay marked this pull request as ready for review June 3, 2026 09:25
@engdoreis engdoreis closed this Jun 4, 2026
@engdoreis engdoreis reopened this Jun 4, 2026
@engdoreis
Copy link
Copy Markdown
Collaborator

I'm sorry for accidentally closing the PR,

Copy link
Copy Markdown
Contributor

@elliotb-lowrisc elliotb-lowrisc left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned that the current changes are affecting the Mocha RTL despite this being for DV purposes. In particular, I think the strange range of the 'rest of chip' entry could make the AXI crossbar harder for the tool to implement (possibly affecting timing/area/performance). I'd like to see a before/after pair of FPGA bitstream builds to check if the impact is negligible or not.

@martin-velay
Copy link
Copy Markdown
Contributor Author

@elliotb-lowrisc, your comment is fair (except that it's not DV purposes only 😄). Do you see another cleaner way of doing it?

@elliotb-lowrisc
Copy link
Copy Markdown
Contributor

@martin-velay Nevermind, it seems I was mistaken in thinking that the axi_xbar address mapping mattered from a synthesis perspective. I had imagined the mapping being synthesised into fixed logic gates like our TL-UL crossbars, but instead it seems the axi_xbar address map is dynamic (specified via a port rather than a parameter or template). If changing the address map doesn't change the synthesis inputs, then it shouldn't worsen timing/area/performance.

Might be worth updating the tag in the PR title if this is not a DV-only change though.

@martin-velay martin-velay changed the title [dv] Add support for HW_ID for SW [hw,dv,sw] Add support for HW_ID for SW Jun 4, 2026
@martin-velay
Copy link
Copy Markdown
Contributor Author

I have added a C check which should verify the FPGA logic will work too. I haven't verified on the board, but I rather rely on CI with this test.

Copy link
Copy Markdown
Contributor

@elliotb-lowrisc elliotb-lowrisc left a comment

Choose a reason for hiding this comment

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

Alright, I think this is ready to be merged

@martin-velay
Copy link
Copy Markdown
Contributor Author

@ziuziakowska or maybe @marnovandermaas, as Douglas is on holidays, could you confirm that you are OK with these SW changes?

- Related to issue lowRISC#423
- This commit adds support for a HW_ID register that can be used by
  SW DV tests to identify the hardware they are running on. This is
  useful for SW DV tests that need to run on multiple hardware platforms
  and need a way to differentiate between them.
- Add framework C test

Signed-off-by: martin-velay <mvelay@lowrisc.org>
- Extend RestOfChipBase/Length in top_pkg to encompass the SW-DV window
(0x2002_0000), routing it through the rest_of_chip port without touching
top_chip_system.
- Add a second device to the rest-of-chip crossbar in
chip_mocha_genesys2 backed by axi_to_mem; reads to offset +0x08 return
the Genesys2 platform identifier (0x0000_000A), enabling SW tests to
detect the platform at runtime consistently with the sim infrastructure.

Signed-off-by: martin-velay <mvelay@lowrisc.org>
Update the test framework smoketest to read the HW_ID register,
validate it against known values, and fail on an unrecognised ID.

Signed-off-by: martin-velay <mvelay@lowrisc.org>
@martin-velay
Copy link
Copy Markdown
Contributor Author

I've just added the test_framework_test for the UVM based regression

Comment on lines 48 to +49
parameter bit [31:0] SW_DV_LOG_ADDR = SW_DV_START_ADDR + 'h04;
parameter bit [31:0] SW_DV_HW_ID_ADDR = SW_DV_START_ADDR + 'h08;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On a slightly unrelated note, I wonder if at some point we might want to have the DV Log window be 8 bytes instead of 4 bytes long, so that we can pass through 64-bit pointers/integers to it. Just in case, I would prefer if the HW ID window was at offset 4 instead and the log window at offset 8, to give it room to potentially grow, but that might not be possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also noticed that you do this in #600 but for different reasons, would be best to do it here first.

Comment thread sw/device/lib/hal/mocha.h
Comment on lines +57 to +61
typedef enum : uint32_t {
HW_ID_FPGA_GENESYS2 = 0x0000000Au,
HW_ID_SIM_VERILATOR = 0x0000001Au,
HW_ID_SIM_UVM = 0x0000002Au,
} dv_hw_platform_t;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mocha code style is more like this:

Suggested change
typedef enum : uint32_t {
HW_ID_FPGA_GENESYS2 = 0x0000000Au,
HW_ID_SIM_VERILATOR = 0x0000001Au,
HW_ID_SIM_UVM = 0x0000002Au,
} dv_hw_platform_t;
enum : uint32_t {
hwid_fpga_genesys2 = 0xau,
hwid_sim_verilator = 0x1au,
hwid_sim_uvm = 0x2au,
};

Comment thread sw/device/lib/hal/mocha.c
Comment on lines +205 to +213

void *mocha_system_dv_hw_id(void)
{
#if defined(__riscv_zcherihybrid)
return create_mmio_capability(dv_hw_id_base, 0x4u);
#else /* !defined(__riscv_zcherihybrid) */
return (void *)dv_hw_id_base;
#endif /* defined(__riscv_zcherihybrid) */
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The above mocha_system_dv_test_status spans the entire DV window though its a bit of a misnomer at the moment, could you rename that and the associated base to mocha_system_dv_window and create a struct in a separate hal/dv.h like so (you can look in hal/autogen to see how others are done):

typedef volatile struct [[gnu::aligned(4)]] dv_window_memory_layout {
    uint32_t test_status;
    uint32_t hw_id;
} *dv_window_t;


switch (hw_id) {
case HW_ID_FPGA_GENESYS2:
uart_puts(console, "Platform: FpgaGenesys2\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would put "Genesys2 FPGA", "Verilator Simulation", and "UVM Simulation", "Unknown Hardware ID" to make them nicer to read as this will be visible to users.

Copy link
Copy Markdown
Collaborator

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

I think we shouldn't use the rest of chip crossbar for the DV window. Can we change this before merging this?

parameter int unsigned PeriClkFreq = 50_000_000;

// SW DV special write locations for test status and logging will always fit in 32-bits
// SW-DV special fake registers
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wouldn't call these fake registers. I would prefer to expand the previous comment instead.

`define DUT u_top_chip_system
`define SIM_SRAM_IF u_sim_sram.u_sim_sram_if

// Special adddresses for SW-DV communication
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: spelling mistake.

"rv_timer_irq",
"rv_timer_irq_cheri",
"test_framework_test",
"test_framework_test_cheri",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You're adding these before the tests are added.

hw_id_mem_req_d <= 1'b0;
hw_id_sel_d <= 1'b0;
end else begin
hw_id_mem_req_d <= hw_id_mem_req;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Latching should be called _q not _d.

localparam longint unsigned DebugMemLength = 64'h0000_1000;
localparam longint unsigned MailboxLength = 64'h0001_0000;
localparam longint unsigned RestOfChipLength = 64'h0000_8000;
localparam longint unsigned RestOfChipLength = 64'h0FFE_8000; // 0x2002_0000 to 0x3000_7FFF
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You shouldn't add the DV window to the rest of chip bus. It should be a separate AXI port.

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.

5 participants