From ea0b302160b96cf032bae69242800cc756d839ad Mon Sep 17 00:00:00 2001 From: James Wainwright Date: Thu, 18 Jan 2024 14:54:43 +0000 Subject: [PATCH] [test,uart] Rework uart_tx_rx_test to use UART console This merges the versions of this test for each UART into one C binary and configures the OTTF console to use UART1 when UART0 is under test. Signed-off-by: James Wainwright --- hw/top_earlgrey/dv/chip_sim_cfg.hjson | 20 ++-- .../dv/env/seq_lib/chip_sw_uart_tx_rx_vseq.sv | 2 +- sw/device/lib/testing/uart_testutils.c | 2 - sw/device/tests/BUILD | 104 +++++++++--------- sw/device/tests/sival/BUILD | 4 +- sw/device/tests/uart_tx_rx_test.c | 60 ++++++---- .../app/config/hyperdebug_chipwhisperer.json | 4 + .../src/app/config/opentitan_cw310.json | 4 + sw/host/tests/chip/uart/src/main.rs | 52 +++++++-- 9 files changed, 153 insertions(+), 99 deletions(-) diff --git a/hw/top_earlgrey/dv/chip_sim_cfg.hjson b/hw/top_earlgrey/dv/chip_sim_cfg.hjson index 93fba71cf042a..60f94529fffcd 100644 --- a/hw/top_earlgrey/dv/chip_sim_cfg.hjson +++ b/hw/top_earlgrey/dv/chip_sim_cfg.hjson @@ -327,9 +327,9 @@ // Format SW image names (which are Bazel labels concatenated with an index // and/or flags, see below) into output file names separated by commas to feed into // +sw_images plusarg. For example, if the input list of SW images is - // ["//sw/device/tests:uart0_tx_rx_test:1", + // ["//sw/device/tests:uart_tx_rx_test:1", // "//sw/device/lib/testing/test_rom:test_rom:0"], then the output of this eval_cmd - // will be: "uart0_tx_rx_test:1,test_rom:0". + // will be: "uart_tx_rx_test:1,test_rom:0". '''+sw_images={eval_cmd} \ reformatted_sw_images=; \ for image in {sw_images}; do \ @@ -525,7 +525,7 @@ { name: chip_sw_uart_tx_rx uvm_test_seq: chip_sw_uart_tx_rx_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+uart_idx=0", "+calibrate_usb_clk=1"] reseed: 5 @@ -533,7 +533,7 @@ { name: chip_sw_uart_tx_rx_idx1 uvm_test_seq: chip_sw_uart_tx_rx_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+uart_idx=1", "+calibrate_usb_clk=1"] reseed: 5 @@ -541,7 +541,7 @@ { name: chip_sw_uart_tx_rx_idx2 uvm_test_seq: chip_sw_uart_tx_rx_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+uart_idx=2", "+calibrate_usb_clk=1"] reseed: 5 @@ -549,7 +549,7 @@ { name: chip_sw_uart_tx_rx_idx3 uvm_test_seq: chip_sw_uart_tx_rx_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+uart_idx=3", "+calibrate_usb_clk=1"] reseed: 5 @@ -557,7 +557,7 @@ { name: chip_sw_uart_tx_rx_bootstrap uvm_test_seq: chip_sw_uart_tx_rx_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+use_spi_load_bootstrap=1", "+calibrate_usb_clk=1", "+test_timeout_ns=80_000_000"] @@ -633,7 +633,7 @@ { name: chip_sw_uart_rand_baudrate uvm_test_seq: chip_sw_uart_rand_baudrate_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+sw_test_timeout_ns=80_000_000", "+calibrate_usb_clk=1"] run_timeout_mins: 120 @@ -642,7 +642,7 @@ { name: chip_sw_uart_tx_rx_alt_clk_freq uvm_test_seq: chip_sw_uart_rand_baudrate_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+sw_test_timeout_ns=80_000_000", "+chip_clock_source=ChipClockSourceExternal96Mhz", "+calibrate_usb_clk=1"] @@ -652,7 +652,7 @@ { name: chip_sw_uart_tx_rx_alt_clk_freq_low_speed uvm_test_seq: chip_sw_uart_rand_baudrate_vseq - sw_images: ["//sw/device/tests:uart0_tx_rx_test:1:new_rules"] + sw_images: ["//sw/device/tests:uart_tx_rx_test:1:new_rules"] en_run_modes: ["sw_test_mode_test_rom"] run_opts: ["+sw_test_timeout_ns=80_000_000", "+calibrate_usb_clk=1", "+chip_clock_source=ChipClockSourceExternal48Mhz"] diff --git a/hw/top_earlgrey/dv/env/seq_lib/chip_sw_uart_tx_rx_vseq.sv b/hw/top_earlgrey/dv/env/seq_lib/chip_sw_uart_tx_rx_vseq.sv index 47cb778abcaa6..c49180d2fa55c 100644 --- a/hw/top_earlgrey/dv/env/seq_lib/chip_sw_uart_tx_rx_vseq.sv +++ b/hw/top_earlgrey/dv/env/seq_lib/chip_sw_uart_tx_rx_vseq.sv @@ -40,7 +40,7 @@ class chip_sw_uart_tx_rx_vseq extends chip_sw_uart_smoke_vseq; super.cpu_init(); sw_symbol_backdoor_overwrite("kUartTxData", exp_uart_tx_data); sw_symbol_backdoor_overwrite("kExpUartRxData", uart_rx_data); - sw_symbol_backdoor_overwrite("kUartIdx", uart_idx_data); + sw_symbol_backdoor_overwrite("kUartIdxDv", uart_idx_data); endtask virtual task body(); diff --git a/sw/device/lib/testing/uart_testutils.c b/sw/device/lib/testing/uart_testutils.c index e5661c7b7049e..7d9ed07212e49 100644 --- a/sw/device/lib/testing/uart_testutils.c +++ b/sw/device/lib/testing/uart_testutils.c @@ -61,7 +61,6 @@ static const pinmux_testutils_mio_pin_t .mio_out = kTopEarlgreyPinmuxMioOutIoc4, .insel = kTopEarlgreyPinmuxInselIoc3, }, - // FIXME: select the other available pins. [UartPinmuxChannelDut] = { .mio_out = kTopEarlgreyPinmuxMioOutIob5, @@ -74,7 +73,6 @@ static const pinmux_testutils_mio_pin_t .mio_out = kTopEarlgreyPinmuxMioOutIoc4, .insel = kTopEarlgreyPinmuxInselIoc3, }, - // FIXME: select the other available pins. [UartPinmuxChannelDut] = { .mio_out = kTopEarlgreyPinmuxMioOutIob5, .insel = kTopEarlgreyPinmuxInselIob4, diff --git a/sw/device/tests/BUILD b/sw/device/tests/BUILD index 9cb259bfca00f..4f1b0465df200 100644 --- a/sw/device/tests/BUILD +++ b/sw/device/tests/BUILD @@ -4453,60 +4453,56 @@ opentitan_binary( ], ) -[ - opentitan_test( - name = "uart{}_tx_rx_test".format(uart_idx), - srcs = ["uart_tx_rx_test.c"], - copts = ["-DUART_IDX={}".format(uart_idx)], - cw310 = new_cw310_params( - otp = "//hw/ip/otp_ctrl/data/earlgrey_a0_skus/sival:otp_img_prod_manuf_personalized", - test_cmd = " ".join([ - "--bitstream=\"{bitstream}\"", - "--bootstrap=\"{firmware}\"", - "--firmware-elf=\"{firmware:elf}\"", - ]), - test_harness = "//sw/host/tests/chip/uart:uart_tx_rx", - ), - exec_env = { - # TODO: this test currently fails on FPGA. - # "//hw/top_earlgrey:fpga_cw310_sival": None, - # "//hw/top_earlgrey:fpga_cw310_sival_rom_ext": None, - # "//hw/top_earlgrey:fpga_cw310_test_rom": None, - "//hw/top_earlgrey:silicon_creator": None, - "//hw/top_earlgrey:silicon_owner_sival_rom_ext": "silicon_owner", - "//hw/top_earlgrey:silicon_owner_prodc_rom_ext": "silicon_owner", - "//hw/top_earlgrey:sim_dv": None, - }, - silicon = silicon_params( - test_cmd = " ".join([ - "--bootstrap=\"{firmware}\"", - "--firmware-elf=\"{firmware:elf}\"", - ]), - test_harness = "//sw/host/tests/chip/uart:uart_tx_rx", - ), - silicon_owner = silicon_params( - tags = ["broken"], - ), - deps = [ - "//hw/ip/lc_ctrl/data:lc_ctrl_regs", - "//hw/top_earlgrey/ip/clkmgr/data/autogen:clkmgr_regs", - "//hw/top_earlgrey/ip/pinmux/data/autogen:pinmux_regs", - "//hw/top_earlgrey/sw/autogen:top_earlgrey", - "//sw/device/lib/base:mmio", - "//sw/device/lib/dif:clkmgr", - "//sw/device/lib/dif:lc_ctrl", - "//sw/device/lib/dif:rv_plic", - "//sw/device/lib/dif:uart", - "//sw/device/lib/runtime:hart", - "//sw/device/lib/runtime:irq", - "//sw/device/lib/runtime:log", - "//sw/device/lib/testing:clkmgr_testutils", - "//sw/device/lib/testing:uart_testutils", - "//sw/device/lib/testing/test_framework:ottf_main", - ], - ) - for uart_idx in range(0, 4, 1) -] +opentitan_test( + name = "uart_tx_rx_test", + srcs = ["uart_tx_rx_test.c"], + cw310 = new_cw310_params( + otp = "//hw/ip/otp_ctrl/data/earlgrey_a0_skus/sival:otp_img_prod_manuf_personalized", + test_cmd = " ".join([ + "--bitstream=\"{bitstream}\"", + "--bootstrap=\"{firmware}\"", + "--firmware-elf=\"{firmware:elf}\"", + ]), + test_harness = "//sw/host/tests/chip/uart:uart_tx_rx", + ), + exec_env = { + "//hw/top_earlgrey:fpga_cw310_sival": None, + "//hw/top_earlgrey:fpga_cw310_sival_rom_ext": None, + "//hw/top_earlgrey:silicon_creator": None, + "//hw/top_earlgrey:silicon_owner_sival_rom_ext": "silicon_owner", + "//hw/top_earlgrey:silicon_owner_prodc_rom_ext": "silicon_owner", + "//hw/top_earlgrey:sim_dv": None, + }, + silicon = silicon_params( + test_cmd = " ".join([ + "--bootstrap=\"{firmware}\"", + "--firmware-elf=\"{firmware:elf}\"", + ]), + test_harness = "//sw/host/tests/chip/uart:uart_tx_rx", + ), + silicon_owner = silicon_params( + tags = ["broken"], + ), + deps = [ + "//hw/ip/lc_ctrl/data:lc_ctrl_regs", + "//hw/top_earlgrey/ip/clkmgr/data/autogen:clkmgr_regs", + "//hw/top_earlgrey/ip/pinmux/data/autogen:pinmux_regs", + "//hw/top_earlgrey/sw/autogen:top_earlgrey", + "//sw/device/lib/base:mmio", + "//sw/device/lib/dif:clkmgr", + "//sw/device/lib/dif:lc_ctrl", + "//sw/device/lib/dif:rv_plic", + "//sw/device/lib/dif:uart", + "//sw/device/lib/runtime:hart", + "//sw/device/lib/runtime:irq", + "//sw/device/lib/runtime:log", + "//sw/device/lib/testing:clkmgr_testutils", + "//sw/device/lib/testing:uart_testutils", + "//sw/device/lib/testing/test_framework:ottf_console", + "//sw/device/lib/testing/test_framework:ottf_main", + "//sw/device/lib/testing/test_framework:ottf_utils", + ], +) opentitan_test( name = "rv_core_ibex_mem_test_functest", diff --git a/sw/device/tests/sival/BUILD b/sw/device/tests/sival/BUILD index bd0d9872fa1c0..082ea8098e53c 100644 --- a/sw/device/tests/sival/BUILD +++ b/sw/device/tests/sival/BUILD @@ -57,10 +57,8 @@ test_suite( "//sw/device/tests:sram_ctrl_sleep_sram_ret_contents_no_scramble_test", "//sw/device/tests:sram_ctrl_sleep_sram_ret_contents_scramble_test", "//sw/device/tests:sram_ctrl_smoketest", - "//sw/device/tests:uart0_tx_rx_test", - "//sw/device/tests:uart1_tx_rx_test", - "//sw/device/tests:uart2_tx_rx_test", "//sw/device/tests:uart_smoketest", + "//sw/device/tests:uart_tx_rx_test", "//sw/device/tests/pmod:i2c_host_eeprom_test", ], ) diff --git a/sw/device/tests/uart_tx_rx_test.c b/sw/device/tests/uart_tx_rx_test.c index 4374f27a0b7e0..9b4fbe54df17d 100644 --- a/sw/device/tests/uart_tx_rx_test.c +++ b/sw/device/tests/uart_tx_rx_test.c @@ -15,7 +15,9 @@ #include "sw/device/lib/runtime/log.h" #include "sw/device/lib/testing/clkmgr_testutils.h" #include "sw/device/lib/testing/test_framework/check.h" +#include "sw/device/lib/testing/test_framework/ottf_console.h" #include "sw/device/lib/testing/test_framework/ottf_main.h" +#include "sw/device/lib/testing/test_framework/ottf_utils.h" #include "sw/device/lib/testing/test_framework/status.h" #include "sw/device/lib/testing/uart_testutils.h" @@ -58,18 +60,23 @@ typedef enum uart_direction { /** * Indicates the UART instance under test. * - * From the software / compiler's perspective, this is a constant (hence the - * `const` qualifier). However, the external DV testbench finds this symbol's + * When running in `dv_sim`, the external DV testbench finds this symbol's * address and modifies it via backdoor, to test a different UART instance with * the same test SW image. Hence, we add the `volatile` keyword to prevent the * compiler from optimizing it out. + * * The `const` is needed to put it in the .rodata section, otherwise it gets * placed in .data section in the main SRAM. We cannot backdoor write anything * in SRAM at the start of the test because the CRT init code wipes it to 0s. - * This constant remains unchanged for non-simulation environments (FPGAs and - * silicon) where this constant must be changed at compile-time to each UART. */ -static volatile const uint8_t kUartIdx = UART_IDX; +static volatile const uint8_t kUartIdxDv = 0xff; +/** + * Outside of DV simulation environments, the `kUartIdx` symbol needs to be + * _non_ `const` so that we can modify it via OTTF commands. `kUartIdx` is used + * as the source of truth in the test but we copy the value from `kUartIdxDv` + * to here if it has been set. + */ +static volatile uint8_t kUartIdx = 0xff; /** * Indicates if ext_clk is used and what speed. @@ -139,6 +146,10 @@ static volatile bool uart_irq_tx_empty_fired; static volatile bool exp_uart_irq_rx_overflow; static volatile bool uart_irq_rx_overflow_fired; +enum { + kCommandTimeout = 5000000, // microseconds +}; + void update_uart_base_addr_and_irq_id(void) { switch (kUartIdx) { case 0: @@ -496,11 +507,20 @@ void config_external_clock(const dif_clkmgr_t *clkmgr) { clkmgr_testutils_enable_external_clock_blocking(clkmgr, kUseLowSpeedSel)); } -OTTF_DEFINE_TEST_CONFIG(.console.type = kOttfConsoleSpiDevice, - .console.base_addr = TOP_EARLGREY_SPI_DEVICE_BASE_ADDR, - .console.test_may_clobber = false); +OTTF_DEFINE_TEST_CONFIG(.enable_uart_flow_control = true); bool test_main(void) { + uart_pinmux_platform_id_t platform_id = UartPinmuxPlatformIdCount; + if (kDeviceType == kDeviceFpgaCw310) { + platform_id = UartPinmuxPlatformIdHyper310; + } else if (kDeviceType == kDeviceSimDV) { + platform_id = UartPinmuxPlatformIdDvsim; + } else if (kDeviceType == kDeviceSilicon) { + platform_id = UartPinmuxPlatformIdSilicon; + } else { + CHECK(false, "Unsupported platform %d", kDeviceType); + } + mmio_region_t base_addr; base_addr = mmio_region_from_addr(TOP_EARLGREY_CLKMGR_AON_BASE_ADDR); @@ -509,21 +529,23 @@ bool test_main(void) { base_addr = mmio_region_from_addr(TOP_EARLGREY_PINMUX_AON_BASE_ADDR); CHECK_DIF_OK(dif_pinmux_init(base_addr, &pinmux)); + if (kUartIdxDv != 0xff) { + kUartIdx = kUartIdxDv; + } else { + OTTF_WAIT_FOR(kUartIdx != 0xff, kCommandTimeout); + } + + // If we're testing UART0 we need to move the console to UART1. + if (kUartIdx == 0 && kDeviceType != kDeviceSimDV) { + CHECK_STATUS_OK(uart_testutils_select_pinmux(&pinmux, 1, platform_id, + UartPinmuxChannelConsole)); + ottf_console_configure_uart(TOP_EARLGREY_UART1_BASE_ADDR); + } + update_uart_base_addr_and_irq_id(); LOG_INFO("Test UART%d with base_addr: %08x", kUartIdx, uart_base_addr); - uart_pinmux_platform_id_t platform_id = UartPinmuxPlatformIdCount; - if (kDeviceType == kDeviceFpgaCw310) { - platform_id = UartPinmuxPlatformIdHyper310; - } else if (kDeviceType == kDeviceSimDV) { - platform_id = UartPinmuxPlatformIdDvsim; - } else if (kDeviceType == kDeviceSilicon) { - platform_id = UartPinmuxPlatformIdSilicon; - } else { - CHECK(false, "Unsupported platform %d", kDeviceType); - } - // Attach the UART under test. CHECK_STATUS_OK(uart_testutils_select_pinmux(&pinmux, kUartIdx, platform_id, UartPinmuxChannelDut)); diff --git a/sw/host/opentitanlib/src/app/config/hyperdebug_chipwhisperer.json b/sw/host/opentitanlib/src/app/config/hyperdebug_chipwhisperer.json index e2a4a0f803538..31db1ccac667d 100644 --- a/sw/host/opentitanlib/src/app/config/hyperdebug_chipwhisperer.json +++ b/sw/host/opentitanlib/src/app/config/hyperdebug_chipwhisperer.json @@ -246,6 +246,10 @@ { "name": "console", "alias_of": "UART2" + }, + { + "name": "dut", + "alias_of": "UART3" } ], "strappings": [ diff --git a/sw/host/opentitanlib/src/app/config/opentitan_cw310.json b/sw/host/opentitanlib/src/app/config/opentitan_cw310.json index c880a989478ac..b44697a43be39 100644 --- a/sw/host/opentitanlib/src/app/config/opentitan_cw310.json +++ b/sw/host/opentitanlib/src/app/config/opentitan_cw310.json @@ -51,6 +51,10 @@ { "name": "console", "alias_of": "0" + }, + { + "name": "dut", + "alias_of": "1" } ] } diff --git a/sw/host/tests/chip/uart/src/main.rs b/sw/host/tests/chip/uart/src/main.rs index f762be5de10c6..9800c03b464ac 100644 --- a/sw/host/tests/chip/uart/src/main.rs +++ b/sw/host/tests/chip/uart/src/main.rs @@ -9,12 +9,14 @@ use std::time::Duration; use anyhow::{Context, Result}; use clap::Parser; +use object::{Object, ObjectSymbol}; use opentitanlib::app::TransportWrapper; -use opentitanlib::console::spi::SpiConsoleDevice; use opentitanlib::execute_test; +use opentitanlib::io::uart::Uart; use opentitanlib::test_utils; use opentitanlib::test_utils::init::InitializeTest; +use opentitanlib::test_utils::mem::MemWriteReq; use opentitanlib::uart::console::UartConsole; #[derive(Debug, Parser)] @@ -36,15 +38,16 @@ struct TxRxData { tx_data: Vec, } +struct TestData<'a> { + tx_rx_data: &'a TxRxData, + uart_id: u8, + uart_id_addr: u64, +} + fn main() -> Result<()> { let opts = Opts::parse(); opts.init.init_logging(); - let transport = opts.init.init_target()?; - - let spi = transport.spi("BOOTSTRAP")?; - let console = SpiConsoleDevice::new(&*spi); - let elf_file = fs::read(&opts.firmware_elf).context("failed to read ELF")?; let object = object::File::parse(elf_file.as_ref()).context("failed to parse ELF")?; @@ -53,7 +56,26 @@ fn main() -> Result<()> { tx_data: test_utils::object::symbol_data(&object, "kUartTxData")?, }; - execute_test!(uart_tx_rx, &opts, &transport, &console, &tx_rx_data); + let uart_id_addr = object + .symbols() + .find(|symbol| symbol.name() == Ok("kUartIdx")) + .context("failed to find kUartIdx symbol")? + .address(); + + let transport = opts.init.init_target()?; + let uart_console = transport.uart("console")?; + + for uart_id in 0..4 { + transport.reset_target(Duration::from_millis(500), true)?; + + let test_data = TestData { + tx_rx_data: &tx_rx_data, + uart_id, + uart_id_addr, + }; + + execute_test!(uart_tx_rx, &opts, &transport, &*uart_console, &test_data); + } Ok(()) } @@ -62,12 +84,22 @@ fn main() -> Result<()> { fn uart_tx_rx( opts: &Opts, transport: &TransportWrapper, - console: &SpiConsoleDevice, - tx_rx_data: &TxRxData, + console: &dyn Uart, + test_data: &TestData, ) -> Result<()> { - let uart = transport.uart("console")?; + let TestData { + tx_rx_data, + uart_id, + uart_id_addr, + } = test_data; + + UartConsole::wait_for(console, r"waiting for commands", opts.timeout)?; + MemWriteReq::execute(console, *uart_id_addr as u32, &[*uart_id])?; + UartConsole::wait_for(console, r"Executing the test", opts.timeout)?; + let uart = transport.uart("dut")?; + log::info!("Sending data..."); uart.write(&tx_rx_data.rx_data) .context("failed to send data")?;