[chip_tb] Integrate usbdpi into chip tb #18063

alees24 · 2023-04-19T13:02:22Z

Integration of USBDPI module into DV chip sim test bench as a smoke test of usbdev. Test chip_sw_usbdev_dpi runs to successful completion in ten minutes:

chip sw sets up usbdev and connects to the bus
usbdpi model detect device presence
usbdpi sets device address, reads descriptors and sets configuration
chip sw presents "Hi!Hi" test message for usbdpi to fetch as IN traffic
usbdpi sends "Hi!Hi" test message in response
chip sw checks the received message and concludes the test

The existing usbdpi model is instantiated inside usb20_usbdpi which constructs the additional signals that usbdpi requires, by monitoring just the three physical USB wires (VBUS/SENSE, USB_P/D+, USB_N/D-). Historically the DPI model has been connected directly to the usbdev and has both responded to and - alas - been steered in its own behavior by the additional signals present. The 'pull up enables' permit the usbdpi to detect device presence and to ascertain the bus configuration, and the 'output enables' from usbdev are used to determine when the device is transmitting.

usb20_usbdpi monitors the bus signals directly and detects the device presence and activity from changes in the USB_P/USB_N signals. This logic is sufficient to permit the usbdpi model to be used and to run usbdev_test to completion but it is not itself compliant with the USB 2.0 specification.

It should be adequate for other top-level tests such as usbdev_stream_test which performs much more extensive traffic, to/form multiple endpoints concurrently, but may require some refinement for suspend/resume/reset testing, for example.

Points requiring attention

Pull ups at differential receiver

See Issue #17986

This draft PR instantiates pull up resistors at the inputs of the differential receiver (making those ports 'inout') and the drive strengths are chosen to permit usb20_usbdpi to apply a weak pull down to zero on both USB_P and USB_N, such. It can then detect device presence as per the USB 2.0 specification by testing for either line being pulled high.

As far as I can see we also have pull up functionality available on the USB_P/N pads via the pinmux/padring functionality, but this is under software control and may not readily support the required sleep behavior in which control of the pull ups is passed to the usbdev_aon_wake module in pinmux.

Caveat: It is not entirely clear to me what the intent here is with regard to pull up functionality, since the current differential receiver is a placeholder.

pinmux_testutils

Also note that I have had to introduce a - draft - change to pinmux_testutils to increase the drive strength such that the drivers will overcome the applied pullup.

dif_usbdev

In order to avoid reading 'X' bytes across the TL-UL connection to usbdev (these trigger assertions in DV sim) I have introduced temporary code to zero initialize the packet buffer memory.

That sidesteps this issue when receiving packets that are not a multiple of 32 bits in length. The problem also occurs with mmio_memcpy_to_mmio32 writing to the packet buffer because it will try to perform a final 32-bit read when asked to perform a partial write of the final 32 bits of the packet data.

HTH, all feedback welcomed, please.

EDIT: @msfschaffner I somehow missed your commit, so it hasn't (yet) been considered in the above/this draft

msfschaffner

Thanks @alees24 - I looked at the driving strength combinations, and I think this is fine for simulation purposes (I would just update the comment in the pinmux driver accordingly).

msfschaffner · 2023-04-19T19:32:54Z

hw/ip/prim_generic/rtl/prim_generic_usb_diff_rx.sv

-  input wire         input_pi,      // differential input
-  input wire         input_ni,      // differential input
+  inout              input_pi,      // differential input
+  inout              input_ni,      // differential input


This makes sense, since the pullups should affect the state on the pad as well as far as I understand.
Let's go with this for now - I can double check whether the ASIC version of prim_generic_usb_diff_rx behaves the same way.

Yes, the pull ups are essential in signaling to the USB host that the device is present, and the speed of the device. I think the ports should be renamed but I was trying to make minimal changes at this point, and I'm not at all familiar with the auto-generation of top level RTL.

it's ok to leave it as-is for now.

msfschaffner · 2023-04-19T19:40:10Z

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

+  // Weak pull downs so that we can detect the presence of the device, and we
+  // also prevent Z triggering 'X assertions' in usbdev
+  assign (weak0, weak1) usb_p = 1'b0;
+  assign (weak0, weak1) usb_n = 1'b0;


do I understand correctly that you need to set the diff_rx pullups to pull strength because of this?

msfschaffner · 2023-04-19T19:43:47Z

sw/device/lib/testing/pinmux_testutils.c

@@ -62,6 +62,18 @@ void pinmux_testutils_init(dif_pinmux_t *pinmux) {
  // Configure UART1 TX output to connect to MIO pad IOB5
  CHECK_DIF_OK(dif_pinmux_output_select(pinmux, kTopEarlgreyPinmuxMioOutIob5,
                                        kTopEarlgreyPinmuxOutselUart1Tx));
+
+  // TODO: We have a pull up resistor on the USB_P line, and we need extra drive
+  // strength in dvsim


I think this is fine for simulation purposes.

We only have 4 driving strength levels available in SV to model pad driving strengths and pullups, and we intentionally did not use supply strength, which leaves us with 3 levels.

So I think it is ok to remove the TODO keyword here and explain the use of weak, pull and strong in the simulation model for clarity.

msfschaffner · 2023-04-19T19:44:25Z

hw/ip/prim_generic/rtl/prim_generic_usb_diff_rx.sv

-  assign (weak0, weak1) input_n = pullup_n_en_i ? 1'b1 : 1'bz;
+  // pullup termination
+  assign (weak0, pull1) input_pi = pullup_p_en_i ? 1'b1 : 1'bz;
+  assign (weak0, pull1) input_ni = pullup_n_en_i ? 1'b1 : 1'bz;


a-will

Sorry I'm slow and still working my way through it! But it's looking pretty good so far. Some questions and notes of possible future work follow.

hw/dv/sv/usb20_agent/usb20_usbdpi.core

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

a-will · 2023-04-21T01:10:10Z

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

+  logic usb_pullupdn_d2p;
+
+  // Has either pull up been asserted by the device?
+  wire usb_pullup_detect = usb_pullupdp_d2p | usb_pullupdn_d2p;


Hm... we probably still need some way to detect a disconnection event at some point. Something like... some number of cycles of continuous usb_p == usb_n == usb_dp_en_p2d == usb_dn_en_p2d == 0? Just enough to distinguish between a disconnection and EOP. Does that sound right?

Or... I guess we could count out the full 2 us from the spec, if we wanted to model a proper host.

a-will · 2023-04-21T01:13:32Z

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

+      // Is the FS device pulling DN high, implying that it has been configured
+      // to perform pin-flipping?


We don't actually support pin swapping in this module yet, right? There are some hard-coded checks against a particular symbol below, in the activity detection.

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

a-will · 2023-04-21T01:33:31Z

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

+  ///////////////////////////////////////////////////////////////////////////
+  // Basic activity detection; DPI model indicates whether it is driving,
+  // but for the device we must detect a departure from the bus Idle state
+  ///////////////////////////////////////////////////////////////////////////


Maybe later, we might want to add the opposite: babble / loss of activity detection. An SOP followed by a long string of unchanging K or J (without EOP) will probably leave our agent in a bad state, until we hit a timeout.

sw/device/lib/dif/dif_usbdev.c

a-will

Looks good to me, though it looks like there are a few things to fix up (an 'x' prop assertion failing on usb_ref_pulse_o + the DIF unit tests no matching the new usbdev_configure() behavior).

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

a-will · 2023-04-26T15:48:13Z

hw/dv/sv/usb20_agent/usb20_usbdpi.sv

+  input   rst_ni,
+
+  // VBUS/SENSE is driven high to activate/power the device.
+  output  usb_sense_o,


Ahh, looks like we missed this. Statically connecting this to the MIOs interferes with other tests, resulting in the chip_sw_sleep_pin_mio_dio_val failure we're seeing.

I think we'll still want to add some minimal cfg that allows us to keep all outputs disconnected until they're used. Then the vseq can enable it.

If we wanted this to be like the other agents, we'd stuff that logic in usb20_if, instantiate the agent in chip_env (instead of the testbench), and connect the two in chip_if via uvm_config_db::set(). If you would prefer to do something simpler for now, though, that's fine with me.

Once we add the ability to disconnect the agent, I think we'll be ready to go!

Thanks. I've reinstated the weak DIO pull up in the test bench and introduced a minimal usb20_if between the usb20_usbdpi module and the ASIC pins, allowing us to disconnect entirely and preserve the existing behavior for non-DPI tests. The vseq now removes the default pull up as well as connecting the interface.

I've tried to mimic the way that the other interfaces work, but since the usbdpi implementation is a module rather than a UVM agent, it remains necessary to instantiate it in the test bench, unless I've missed something?

Both chip_sw_usbdev_dpi_vseq and chip_sw_sleep_pin_mio_dio_val now pass for me, so hopefully we've finally arrived.

github-actions · 2023-04-27T17:36:18Z

hw/dv/sv/usb20_agent/usb20_if.sv

+  bit connected = 0;
+
+  // Enable/disable the output drivers
+  function enable_driver(bit enabled);


⚠️ [verible-verilog-lint] _{reported by reviewdog 🐶}
Explicitly define static or automatic lifetime for non-class functions [Style: function-task-explicit-lifetime] [explicit-function-lifetime]

I've bumped into some linting issues, and also found it necessary to introduce an enable to prevent the DPI model from running in non-DPI tests, because it would still be running without traffic/connection.

Since I'm unfamiliar with the linting waivers, could someone please look over the waiver changes I've made to match the modified pull up behavior? Even if linting passes, I'd like to be sure that it's not doing something inappropriate and hiding a potential issue. Thanks -A

I looked at the lint waivers and they LGTM.

Initial integration of usbdpi model into chip dvsim/tb; usbdev_test is up and running Simple chip_sw_usbdev_dpi_vseq created but incomplete. Ensure usbdev buffer memory is zero-initialized in test bench (workaround for mmio32 routines reading X from memory). Add pull up resistors in diff receiver; driven by usbdev_aon_wake module in pinmux. Minimal usb20_if to connect usb20_usbdpi module for now; UVM agent may follow later. Introduce usb20_if to prevent conflicts with other use of MIO SENSE pin and conflict with weak pull up on USB_P DIO. Introduce enable signal for DPI model. Signed-off-by: Adrian Lees <[email protected]>

msfschaffner · 2023-04-27T22:34:40Z

hw/ip/prim_generic/lint/prim_generic_usb_diff_rx.waiver

      -comment "The simulation model has multiple drivers to emulate different IO terminations."
-waive -rules DRIVE_STRENGTH -regexp {Drive strength '\(weak0,weak1\)' encountered on assignment to '(input_p|input_n)'} -location {prim_generic_usb_diff_rx.sv} \
+waive -rules DRIVE_STRENGTH -regexp {Drive strength '\(weak0,pull1\)' encountered on assignment to '(input_pi|input_ni)'} -location {prim_generic_usb_diff_rx.sv} \


These changes LGTM

a-will

Awesome work, LGTM

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

a-will · 2023-04-28T15:38:15Z

The airgapped Bazel build test failed, but it looks like it was due to a simple mismatch between the actual run time histogram and the timeout spec (ostensibly needed just a minute or two, judging by the progress).

I'll go ahead and merge this.

a-will · 2023-04-30T16:58:03Z

@sha-ron FYI, this PR added usbdev's memory to the mem_bkdr_util set tracked in chip_mem_e.

@msfschaffner @matutem Since it's only accessed from the vseq classes, though, did we go a little too far there? It doesn't use the file-based load... Should we have instantiated the access object from the vseq instead of the testbench (and avoided the new entry in chip_mem_e)?

msfschaffner · 2023-05-02T02:20:42Z

Since this is an actual macro memory instance, I think it's ok to add it to chip_mem_e - although you are right that we have not really added FIFO-like buffers to the mem_bkdr_util set.

Since this is already merged, it's probably simpler to just align the closed source environment - or are there any unforeseen issues?

a-will · 2023-05-02T03:37:55Z

I don't think so. I was merely noticing there is room for improvement in our processes in retargeting prims. After thinking about it, the vseq doesn't sound like a great place to put this, else we'd potentially proliferate hard-coded hierarchical paths across more files.

alees24 requested review from msfschaffner and a-will April 19, 2023 13:02

alees24 force-pushed the chip-usbdpi branch from 5aa6c79 to 2fa0de8 Compare April 19, 2023 13:41

msfschaffner reviewed Apr 19, 2023

View reviewed changes

msfschaffner mentioned this pull request Apr 19, 2023

[usb_diff_rx] Model pull-up behavior #18043

Merged

alees24 force-pushed the chip-usbdpi branch from 2fa0de8 to 8364a5d Compare April 20, 2023 13:26

a-will reviewed Apr 21, 2023

View reviewed changes

hw/top_earlgrey/dv/env/seq_lib/chip_sw_usbdev_dpi_vseq.sv Show resolved Hide resolved

alees24 force-pushed the chip-usbdpi branch from 8364a5d to b8a834e Compare April 25, 2023 12:11

alees24 marked this pull request as ready for review April 25, 2023 12:17

alees24 requested review from a team as code owners April 25, 2023 12:17

alees24 requested review from matutem and alphan and removed request for a team April 25, 2023 12:17

alees24 force-pushed the chip-usbdpi branch 4 times, most recently from 76be363 to 84dc7ca Compare April 26, 2023 09:19

a-will reviewed Apr 26, 2023

View reviewed changes

alees24 force-pushed the chip-usbdpi branch from 84dc7ca to 58f64ed Compare April 27, 2023 17:33

github-actions bot reviewed Apr 27, 2023

View reviewed changes

alees24 force-pushed the chip-usbdpi branch 2 times, most recently from e05b4fa to b55a182 Compare April 27, 2023 21:01

alees24 force-pushed the chip-usbdpi branch from b55a182 to ed89784 Compare April 27, 2023 21:23

msfschaffner approved these changes Apr 27, 2023

View reviewed changes

a-will approved these changes Apr 27, 2023

View reviewed changes

hw/top_earlgrey/dv/env/seq_lib/chip_sw_usbdev_dpi_vseq.sv Show resolved Hide resolved

a-will merged commit 8359737 into lowRISC:master Apr 28, 2023

mundaym added the Submilestone:S1 label May 3, 2023

johngt mentioned this pull request May 3, 2023

[usbdev] Verification sign-off for V3 #17892

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[chip_tb] Integrate usbdpi into chip tb #18063

[chip_tb] Integrate usbdpi into chip tb #18063

alees24 commented Apr 19, 2023 •

edited

Loading

msfschaffner left a comment

msfschaffner Apr 19, 2023

alees24 Apr 20, 2023

msfschaffner Apr 20, 2023

msfschaffner Apr 19, 2023

alees24 Apr 20, 2023

msfschaffner Apr 19, 2023

msfschaffner Apr 19, 2023

a-will left a comment

a-will Apr 21, 2023

a-will Apr 21, 2023

a-will Apr 21, 2023

a-will left a comment

a-will Apr 26, 2023

alees24 Apr 27, 2023

github-actions bot Apr 27, 2023

alees24 Apr 27, 2023 •

edited

Loading

msfschaffner Apr 27, 2023

msfschaffner Apr 27, 2023

alees24 Apr 28, 2023

a-will left a comment

a-will commented Apr 28, 2023

a-will commented Apr 30, 2023

msfschaffner commented May 2, 2023

a-will commented May 2, 2023

		// Is the FS device pulling DN high, implying that it has been configured
		// to perform pin-flipping?

[chip_tb] Integrate usbdpi into chip tb #18063

[chip_tb] Integrate usbdpi into chip tb #18063

Conversation

alees24 commented Apr 19, 2023 • edited Loading

msfschaffner 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

a-will 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

a-will left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 27, 2023

Choose a reason for hiding this comment

alees24 Apr 27, 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

a-will left a comment

Choose a reason for hiding this comment

a-will commented Apr 28, 2023

a-will commented Apr 30, 2023

msfschaffner commented May 2, 2023

a-will commented May 2, 2023

alees24 commented Apr 19, 2023 •

edited

Loading

alees24 Apr 27, 2023 •

edited

Loading