[top_darjeeling] DMI address shifting convention

[qmn]

Commit 87d5764 brought rv_dm and lc_ctrl to be on the same xbar_dbg, which makes use of the existing TL-UL infrastructure. My recent PR #24758 returns the JTAG DPI interface back to the Darjeeling Verilator toplevel, but I noticed that accesses to the lc_ctrl part of the address space (starting at 0x20000) don't work; instead, they alias into rv_dm.

For example, the lifecycle state is located at 0x20038, but attempting to access that through riscv dmi_read 0x20038 on OpenOCD returns rv_dm's system bus access control and status register at 0x38 instead. This is because DMI accesses from JTAG get left-shifted by two before going into xbar_dbg...

opentitan/hw/ip/tlul/rtl/tlul_jtag_dtm.sv

Line 119 in 2585f4a

.addr_i ( top_pkg::TL_AW'({dmi_req.addr, 2'b00}) ),

... and then truncated to 0x38, because the current NumDmiByteAbits parameter to tlul_jtag_dtm, at 18, isn't wide enough. The only reason accesses to rv_dm worked was because of a corresponding right shift in tlul_adapter_dmi:

opentitan/hw/ip/tlul/rtl/tlul_adapter_dmi.sv

Lines 63 to 64 in 2585f4a

	// We expect a word-aligned address here, otherwise an error is returned (see further below).
	assign dmi_req_o.addr = {2'b00, tl_h2d_i.a_address[top_pkg::TL_AW-1:2]};

Even if the number of DMI address bits were set correctly, it still wouldn't work because the corresponding right shift doesn't happen at lc_ctrl. In fact, a riscv dmi_read 0x800e (i.e. 0x20038 >> 2) gets correctly routed to lc_ctrl and returns the lifecycle state correctly. It is relatively easy to replicate that address shift at lc_ctrl as done at rv_dm, but I think there is a broader issue I'd like feedback on:

As far as I can tell, the shifting is needed because the RISC-V Debug Module has word-size debug registers that are accessed at byte granularity. However, the existing TL-UL infrastructure expects word-aligned addresses for word-sized accesses in several places (e.g., tlul_adapter_dmi, tl_err, tlul_adapter_host). There are a few comments that obliquely note the requirement for word-aligned addresses, but don't make it clear it's because rv_dm's byte-addressed debug module registers need them. Addressing this issue sooner will help make for less surprising additions to the debug crossbar (especially now as I've seen increasing momentum in efforts to backport 87d5764 and other changes to master, e.g. #24507).

I see two ways forward: 1) make it an explicit convention that all DMI addresses are internally left-shifted by 2 before entering the TL-UL infrastructure, or 2) extend the TL-UL infrastructure to specially support DMI semantics (that is, full-word-size accesses on non-word-aligned addresses), and don't shift addresses anymore. Should it be the first, I think it would be worth calling this out more explicitly across the codebase and in documentation. And, of course, right-shifting addresses just before they get to lc_ctrl so that it functions properly. The second is more aesthetically appealing, but would call for pretty invasive changes to TL-UL.

Thanks for your consideration.

[a-will]

Hm... I'm not sure I see where the problem is here. riscv dmi_read is defined to take 32-bit word addresses (the DMI protocol explicitly defines registers as being 32 bits), and the bug above was using a byte address as the argument. TL-UL is byte-addressed, so the DTM would need to appropriately adjust the address (as it does in tlul_jtag_dtm).

Before we switched to TL-UL as the DMI implementation, did we map lc_ctrl's byte addresses directly to the address field in the dmi register in the DTM? If so, that's kind of a wasteful mapping, and we ought to use word addresses so the CSRs aren't unnecessarily sparse. For the JTAG DTM, we can adopt the new use of the address bits based on the IDCODE that identifies this combined TAP.

[qmn]

Hi, thanks so much for taking a look.

did we map lc_ctrl's byte addresses directly to the address field in the dmi register in the DTM?

By my read, yes, with some caveats: when lc_ctrl had its own JTAG TAP, the original dmi_jtag module put out word addresses which were reconstituted into byte addresses before entering tlul_adapter_host on their way to lc_ctrl_reg_top:

opentitan/hw/ip/lc_ctrl/rtl/lc_ctrl.sv

Lines 238 to 246 in 73ae262

	tlul_adapter_host #(
	.EnableDataIntgGen(1)
	) u_tap_tlul_host (
	.clk_i,
	.rst_ni,
	// do not make a request unless there is room for the response
	.req_i ( dmi_req_valid & dmi_resp_ready ),
	.gnt_o ( dmi_req_ready ),
	.addr_i ( top_pkg::TL_AW'({dmi_req.addr, 2'b00}) ),

Currently, TL DMI signals get pushed straight through lc_ctrl to lc_ctrl_reg_top:

opentitan/hw/ip/lc_ctrl/rtl/lc_ctrl.sv

Lines 152 to 159 in 87d5764

	// Ignore the window and use the regs interface directly.
	lc_ctrl_regs_reg_top u_reg_dmi (
	.clk_i,
	.rst_ni,
	.tl_i ( dmi_tl_i ),
	.tl_o ( dmi_tl_o ),
	.reg2hw ( dmi_reg2hw ),
	.hw2reg ( dmi_hw2reg ),

In any case, completely agreed on not having lc_ctrl's addresses be sparse. At the same time, this code as-is effectively makes lc_ctrl's DMI address space "dense". If I know to access lc_ctrl's registers by using word addresses rather than byte addresses (e.g. LC_CTRL_ALERT_TEST by issuing riscv dmi_read 0x8000, not 0x20000; LC_CTRL_STATUS by issuing riscv dmi_read 0x8001, not 0x20004; etc.) then this does work. I feel like some leads in the code / documentation would be nice, though, since I read 87d5764 to mean that lc_ctrl should be accessed from riscv dmi_read 0x20000 and up.

I hope that makes it clearer.