[hw,lc_ctrl,rtl] Unified LC CTRL for JTAG TAP and TLUL-based DMI #24507

Razer6 · 2024-09-04T12:36:10Z

This PR cherry picks the changes from Darjeeling and adds them under a dedicated parameter to select between the JTAG Tap and the TLUL-based DMI interface.

When merging this, we might bump the version number of the IP.

rswarbrick

This looks sensible to me in general. The DV changes are small, but I think they are the right first steps. I'll let a designer review the design changes as well as me: they look right to me, but it probably makes sense for at least one other person to take a look.

hw/top_earlgrey/sw/autogen/top_earlgrey.h

rswarbrick

As a sort of "meta note": I see there are some opens listed in the commit message for the first commit, at least. Are these still open, and do you intend to work on them as part of this PR?

rswarbrick · 2024-10-22T11:52:04Z

hw/ip/lc_ctrl/rtl/lc_ctrl.sv


- lc_ctrl_reg_top u_reg_tap (
+ // Statically MUX DMI TLUL port and the one coming from the JTAG TAP


Really minor nit, but I think we're using mux as a verb here (rather than e.g. a name), so it doesn't really need to be capitalised.

rswarbrick · 2024-10-22T12:00:12Z

hw/ip/lc_ctrl/dv/tb.sv

@@ -43,6 +43,10 @@ module tb;
 assign lc_ctrl_if.otp_vendor_test_ctrl_o = otp_vendor_test_ctrl;
 assign otp_vendor_test_status = lc_ctrl_if.otp_vendor_test_status_i;

+ // Used for JTAG DTM connections via TL-UL.


I think that maybe this commit ("Update DV environment") should be squashed with the first one. Otherwise, you end up with an intermediate state where the tests won't actually build.

I was going to suggest putting some stub version in the first commit, but I've looked at this commit more carefully and I'm now convinced it's easiest just to squash it in.

rswarbrick · 2024-10-22T12:01:44Z

hw/ip/lc_ctrl/data/lc_ctrl.hjson

+ }
+ }
+ ]
+ }
 }


Nit: I'm not sure the indentation is right here: the "registers: " line on line 692 hasn't moved horizontally but now the final closing brace is 2 characters to the left.

I'm puzzled. It looks right to me. A new level of indentation was added though.

rswarbrick · 2024-10-22T12:08:16Z

hw/ip/lc_ctrl/data/lc_ctrl.hjson

@@ -25,7 +25,7 @@
 {clock: "clk_kmac_i", reset: "rst_kmac_ni"}
 ]
 bus_interfaces: [
- { protocol: "tlul", direction: "device" }
+ { protocol: "tlul", direction: "device", name: "regs" }


Crikey, this commit was big! If the rest of this PR gets held up at all, it might be worth splitting out this commit and getting it merged. I think it looks right, but it was long and I'd rather not read through it again :-)

I think the others are not that big?

rswarbrick · 2024-10-22T12:19:22Z

hw/dv/sv/jtag_agent/jtag_dtm_reg_block.sv

@@ -184,7 +184,11 @@ class jtag_dtm_reg_dtmcs extends jtag_dtm_base_reg;
 .access("RO"),
 .mubi_access("NONE"),
 .volatile(0),
+`ifdef USE_DMI_INTERFACE


I wonder whether it would make sense to give the agent something like a "configure_use_dmi" function? Then we wouldn't need all the ifdefs: we could just call it with the correct value after construction, avoiding tangling the config so much with the construction.

Working through the items:

For the abits field, I think we'd just need set_reset?

For the n_bits default value override, this just goes through to the uvm_reg constructor. I think we can just use the larger value (with a comment)?

For the address field, I think we could just use the larger value again. It's the top field in the register, so I don't think that would cause a problem.

Could you help me in doing that on this PR?

hw/ip/lc_ctrl/dv/tb.sv

rswarbrick · 2024-10-22T12:24:07Z

hw/ip/lc_ctrl/dv/tb.sv

@@ -96,6 +98,7 @@ module tb;

 `DV_ALERT_IF_CONNECT()

+`ifdef USE_DMI_INTERFACE


Would it make sense to pull this into a localparam further up in the file with something like

`ifdef USE_DMI_INTERFACE localparam bit use_dmi_interface = 1'b1; `else localparam bit use_dmi_interface = 1'b0; `endif

That would cut down the amount of stuff that needed ifdef code.

rswarbrick · 2024-10-22T12:26:31Z

hw/top_earlgrey/doc/design/README.md

@@ -404,6 +404,7 @@ For the purpose of `top_earlgrey`, the first option has been chosen to benefit s
 | otp_ctrl | otp_ctrl | 0x40130000 (core) |
 | | | 0x40138000 (prim) |
 | lc_ctrl | lc_ctrl | 0x40140000 (regs) |
+| | | 0 (dmi) |


It clearly doesn't matter, but maybe worth getting the "(dmi)" bit to line up with the other items? Maybe by expressing the address as 0x00000000?

I changed the address to 0x00000000 but it will stick there with the lcctrl.

The TL-UL-based DMI is accessible through ports of `top_darjeeling`. Opens: - Pinmux has not been touched; in its instantiation in Darjeeling the JTAG output port to `lc_ctrl` is left unconnected and the corresponding input port is tied off. - `lc_ctrl`'s block-level DV has not been updated yet. - Darjeeling's testbench and UVM environment have not been updated yet. - Darjeeling's Verilator top has not been updated yet. - Darjeeling's FPGA top has not been updated yet. Co-authored-by: Michael Schaffner <msf@opentitan.org> Signed-off-by: Andreas Kurth <adk@lowrisc.org>

In preparation for converting DMI to be TL-UL, making use of common xbar infrastructure, and adding a separate reg block for DMI to connect to, give the usual lc_ctrl reg block a name. Co-authored-by: Michael Schaffner <msf@opentitan.org> Co-authored-by: Alexander Williams <awill@opentitan.org> Signed-off-by: Robert Schilling <rschilling@rivosinc.com>

Co-authored-by: Michael Schaffner <msf@opentitan.org> Co-authored-by: Alexander Williams <awill@opentitan.org> Signed-off-by: Robert Schilling <rschilling@rivosinc.com>

Signed-off-by: Robert Schilling <rschilling@rivosinc.com>

Razer6 requested review from a team as code owners September 4, 2024 12:36

Razer6 requested review from rswarbrick, cfrantz, pamaury, a-will and andreaskurth and removed request for a team, rswarbrick, cfrantz and pamaury September 4, 2024 12:36

Razer6 force-pushed the sync-lc-ctrl branch from 678f339 to ec0d6dd Compare September 4, 2024 12:47

johngt requested a review from rswarbrick October 7, 2024 08:32

rswarbrick reviewed Oct 7, 2024

View reviewed changes

Razer6 commented Oct 7, 2024

View reviewed changes

hw/top_earlgrey/sw/autogen/top_earlgrey.h Outdated Show resolved Hide resolved

Razer6 force-pushed the sync-lc-ctrl branch 5 times, most recently from 9a5f531 to 2b00f24 Compare October 9, 2024 09:22

qmn mentioned this pull request Oct 9, 2024

[top_darjeeling] DMI address shifting convention #24759

Open

Razer6 added IP:lc_ctrl Component:MultiTop labels Oct 11, 2024

Razer6 force-pushed the sync-lc-ctrl branch 4 times, most recently from 18a6b51 to 9e4087a Compare October 11, 2024 06:57

Razer6 force-pushed the sync-lc-ctrl branch 13 times, most recently from 826cdca to 4238861 Compare October 12, 2024 09:24

Razer6 force-pushed the sync-lc-ctrl branch 4 times, most recently from 4986aec to 9094c99 Compare October 21, 2024 21:39

rswarbrick reviewed Oct 22, 2024

View reviewed changes

andreaskurth and others added 4 commits October 22, 2024 15:47

[lc_ctrl] Convert DMI to real TL-UL interface

9512a17

Co-authored-by: Michael Schaffner <msf@opentitan.org> Co-authored-by: Alexander Williams <awill@opentitan.org> Signed-off-by: Robert Schilling <rschilling@rivosinc.com>

[hw,lc_ctrl] Bump version number to 2.1.0

a94430a

Signed-off-by: Robert Schilling <rschilling@rivosinc.com>

Razer6 force-pushed the sync-lc-ctrl branch from 9094c99 to a94430a Compare October 22, 2024 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hw,lc_ctrl,rtl] Unified LC CTRL for JTAG TAP and TLUL-based DMI #24507

[hw,lc_ctrl,rtl] Unified LC CTRL for JTAG TAP and TLUL-based DMI #24507

Razer6 commented Sep 4, 2024

rswarbrick left a comment

rswarbrick left a comment

rswarbrick Oct 22, 2024

rswarbrick Oct 22, 2024

Razer6 Oct 22, 2024

rswarbrick Oct 22, 2024

Razer6 Oct 22, 2024

rswarbrick Oct 22, 2024

Razer6 Oct 22, 2024

rswarbrick Oct 22, 2024

Razer6 Oct 22, 2024

rswarbrick Oct 22, 2024

rswarbrick Oct 22, 2024

Razer6 Oct 22, 2024


		lc_ctrl_reg_top u_reg_tap (
		// Statically MUX DMI TLUL port and the one coming from the JTAG TAP

		@@ -96,6 +98,7 @@ module tb;

		`DV_ALERT_IF_CONNECT()

		`ifdef USE_DMI_INTERFACE

[hw,lc_ctrl,rtl] Unified LC CTRL for JTAG TAP and TLUL-based DMI #24507

Are you sure you want to change the base?

[hw,lc_ctrl,rtl] Unified LC CTRL for JTAG TAP and TLUL-based DMI #24507

Conversation

Razer6 commented Sep 4, 2024

rswarbrick left a comment

Choose a reason for hiding this comment

rswarbrick 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