[usbdev] Added frequency corners testing #19093

alees24 · 2023-06-30T14:39:19Z

Introduced an independent, asynchronous clock for USB DPI model. ~~The clock is implemented directly at present, but I think it should be possible to replace it with a clk_rst_if~~.

In order to configure usbdev_stream_test to generate max-length packets - a necessary prerequisite of testing the ability of the receiver to track the transmitter after synchronizing - chip_sw_usbdev_stream_vseq overwrites configuration switches, and then two further regression tests are added:

chip_sw_usbdev_hiclk_stream operates usbdev at a clock frequency higher than that of the usbdpi model
chip_sw_usbdev_loclk_stream operates usbdev at a lower clock frequency

The tests are found to pass at a frequency offset of 18.5kHz (of 48MHz), but they are unreliable at 30kHz. The failure frequency depends upon the exact data being sent (because of bit stuffing) and the phase relationship of the two clocks at the start of packet transmission.

Update: initial implementation of clk/rst for usbdpi has been replaced by the common clk_rst_if.

Introduce independent clock for usbdpi host model. Modify usbdpi to synchronize to device traffic. Support parameterization of usbdev streaming test. Add regression tests for hi and low device clk. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>

a-will · 2024-04-12T02:14:39Z

hw/dv/dpi/usbdpi/usbdpi.c

+  // If the bus is idle but we've detected a non-idle state, adjust the sampling
+  // phase; this synchronization mimics that of the usbdev logic, although we
+  // could potentially do something more sophisticated over the 6 or 8 bit
+  // intervals of the known SYNC (KJKJKJKK) sequence that starts each packet.
+  if (ctx->state == ST_IDLE) {


This isn't quite as sophisticated as usbdev, which adapts the sampling point on every transition. I'm not sure we can get away without it, if we're going to test the limits.

At the 48 MHz sampling clock and for 64 raw bytes without bitstuffing (64 bytes * 8 bits / byte * 4 clocks / bit), we end up sampling 1 clock early for the next bit with only a 24 kHz offset (0.049%, 488 ppm).

rswarbrick · 2024-04-12T13:23:27Z

hw/dv/dpi/usbdpi/usbdpi.c

+  }
+
+  // Return state of DP/DN signals
+  if (pdp)


I think that pdp and pdn are non-null at the only call site. I'd suggest moving this check into an assertion at the top instead.

rswarbrick · 2024-04-12T13:24:11Z

hw/dv/dpi/usbdpi/usbdpi.c

+    switch ((dp << 1) | dn) {
+      // Both signals transitioned simultaneously.
+      case 1U:
+        // Aim for the middle of the Eye


Nit: Does "eye" normally get capitalised here? (Genuine question!)

rswarbrick · 2024-04-12T13:34:54Z

hw/dv/dpi/usbdpi/usbdpi.c

+// Sample the bus signals
+static bool bus_sample(usbdpi_ctx_t *ctx, unsigned *pdp, unsigned *pdn,
+                       uint32_t d2p) {


Would you mind expanding this comment slightly? In particular, it took me quite a while to figure out what the return value meant.

rswarbrick · 2024-04-12T13:38:31Z

hw/dv/dpi/usbdpi/usbdpi.c

+    }
+  }
+
+  return ((ctx->tick & 3U) == ctx->tick_sample);


This check appears in a couple of places. Maybe factor it out to a function like this?

static bool should_sample(const usbdpi_ctx_t *ctx) { return ((ctx->tick & 3U) == ctx->tick_sample); }

That could also come out of this function entirely, which might make it a bit more obvious what's going on at the call site, which would end up like this:

bus_sample(ctx, &dp, &dn, d2p); if (!should_sample(ctx)) { // Not sampling on this clock phase. return; }

(Looking at this code, I definitely picked a stupid name for the function!)

The reason I thought about this was that I noticed equivalent code in usbdpi_host_to_device, which might become something like

if (!should_sample(ctx)) { return ctx->driving; }

rswarbrick · 2024-04-12T13:43:58Z

hw/dv/dpi/usbdpi/usbdpi.c

@@ -1120,14 +1179,15 @@ uint8_t usbdpi_host_to_device(void *ctx_void, const svBitVecVal *usb_d2p) {
  int force_stat = 0;
  int dat;

+  // The point at which we raise the VBUS/SENSE signal


It might be worth adding a note to the comment about the fact that this is independent of the clock phase stuff that goes into the tick_sample check below.

rswarbrick · 2024-04-12T14:01:17Z

hw/top_earlgrey/dv/env/seq_lib/chip_sw_usbdev_dpi_vseq.sv

+  // Perform the typical start up of the USB DPI automatically.
+  bit do_usbdpi_start = 1;


Do we currently have a use case for do_usbdpi_start = 0? If not, I'd be tempted to drop the flag entirely.

There will be, down the line. A number of changes have had to be applied to the block level DV code because it assumed and performed too much at startup, making the development of other sequences impossible. Let's not make the same mistake here.

rswarbrick · 2024-04-12T14:04:09Z

hw/top_earlgrey/dv/env/seq_lib/chip_sw_usbdev_dpi_vseq.sv

+
+  // Start the USB DPI model.
+  virtual task usbdpi_start();
+    int freq_khz = (usbdpi_freq + 500) / 1000;


Is there any meaning to a usbdpi_freq of 1001? If not, maybe it would be cleaner to either specify the frequency in kHz in the first place or to stick constraints and assertions in to make sure things come out with round numbers.

(My preference is to specify in kHz in the first place)

This is just standard round-to-nearest behavior because the code presently uses kHz to set the host frequency. In another pending PR (frequency tracking) even kHz is inadequate, so we'll ultimately end up using 'set_period.'

rswarbrick · 2024-04-12T14:06:58Z

hw/top_earlgrey/dv/env/seq_lib/chip_sw_usbdev_dpi_vseq.sv

+  // Reset the USB DPI model; leave the reset asserted for around 100us because the CPU
+  // has a body of code to run before it will connect to the USB.


I'm not sure I completely understand this comment. Is it saying that Ibex has a bunch of code that it must run before it will connect to USB? If so, how does this relate to the reset width on the usbdpi clk/rst interface?

rswarbrick · 2024-04-12T14:11:42Z

hw/top_earlgrey/dv/env/seq_lib/chip_sw_usbdev_stream_vseq.sv

+    if (!$value$plusargs("sw_usbdev_stream_count=%0d", stream_count[0])) begin
+      stream_count[0] = 8'h0B;
+    end


I wonder whether this could be a bit easier. I'm imagining something like this:

stream_count[0] = 8'h0B; void'($value$plusargs("sw_usbdev_stream_count=%0d", stream_count[0]));

I don't think we can get rid of the arrays that we're building to pass to sw_symbol_backdoor_overwrite because I don't think I know a "singleton array constructor" in SystemVerilog.

That does rely upon the argument being unmodified by $value$plusargs in the event that zero is returned, because no matching argument was found. Reading the code rather relies upon knowing and trusting that behaviour - which I have just checked in the spec - but I prefer it to be clearer/explicit.

rswarbrick · 2024-04-12T14:13:25Z

sw/device/tests/usbdev_stream_test.c

@@ -135,43 +135,49 @@ static usb_testutils_streams_ctx_t stream_test;
 *   (Note that this substantially alters the timing of interactions with the
 * DPI model and will increase the simulation time)
 */
-static bool verbose = false;
+static bool kVerbose = false;


It might not be worth refactoring now, but I'd be tempted to pull the "trivial rename to match style guide" changes into separate commits :-)

This PR was created a long time ago and has been lingering for many months, awaiting only the change to use our conventional 'clk_rst_if'

alees24 mentioned this pull request Jun 30, 2023

[usbdev] Test operation at frequency corners #18944

Open

[usbdev] Added frequency corners testing

3a3ea33

Introduce independent clock for usbdpi host model. Modify usbdpi to synchronize to device traffic. Support parameterization of usbdev streaming test. Add regression tests for hi and low device clk. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>

alees24 force-pushed the usbdev-freq-corners branch from ae00a12 to 3a3ea33 Compare April 11, 2024 16:20

alees24 requested a review from a-will April 11, 2024 16:23

alees24 marked this pull request as ready for review April 11, 2024 16:24

alees24 requested review from a team as code owners April 11, 2024 16:24

alees24 requested review from matutem, timothytrippel and rswarbrick and removed request for a team and matutem April 11, 2024 16:24

a-will reviewed Apr 12, 2024

View reviewed changes

rswarbrick reviewed Apr 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[usbdev] Added frequency corners testing #19093

[usbdev] Added frequency corners testing #19093

alees24 commented Jun 30, 2023 •

edited

Loading

a-will Apr 12, 2024

rswarbrick Apr 12, 2024

rswarbrick Apr 12, 2024

rswarbrick Apr 12, 2024

rswarbrick Apr 12, 2024

rswarbrick Apr 12, 2024

rswarbrick Apr 12, 2024

alees24 Apr 26, 2024

rswarbrick Apr 12, 2024

alees24 Apr 26, 2024

rswarbrick Apr 12, 2024

rswarbrick Apr 12, 2024

alees24 Apr 26, 2024

rswarbrick Apr 12, 2024

alees24 Apr 26, 2024 •

edited

Loading

		// Perform the typical start up of the USB DPI automatically.
		bit do_usbdpi_start = 1;

		// Reset the USB DPI model; leave the reset asserted for around 100us because the CPU
		// has a body of code to run before it will connect to the USB.

[usbdev] Added frequency corners testing #19093

Are you sure you want to change the base?

[usbdev] Added frequency corners testing #19093

Conversation

alees24 commented Jun 30, 2023 • edited Loading

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

alees24 Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

alees24 commented Jun 30, 2023 •

edited

Loading

alees24 Apr 26, 2024 •

edited

Loading