[i3c] Output driver abstraction, top-level fixes

[alees24]

Introduce Target switch to cover all intended roles. Document in design_parameters.md which roles are currently supported, i.e. available in this first version.

Indicate the presence of Controller and/or Target logic.

Introduce Virtual Open Drain driver implementation, and tidy up the selection of driver style, leaving the build to choose the appropriate RTL design file.

Correct the spelling of RSTACT.

Update: migrated files because, as I thought may happen, linting doesn't like the mismatch between filename and package name.
Update 2: CI seems happy with the restructuring, squashed and pushed again.

[mkj121]

One query to be explained

[mkj121]

LGTM now

[andreaskurth]

Thanks @alees24, good to see ifdef inside the package replaced by a proper file split for the different driver variants, and the drv_state_t helpers should make the transceiver code considerably easier to follow.

The approach seems sensible to me, but I have a few questions/suggestions.

I also checked the RSTACT fix, the EXT_IBI_QUEUE_EN correction and the new clamp limits against HCI Table 139, Table 44 and Table 94 respectively, and they all look right.

[andreaskurth]

This looks like it will collide with #30998, which removes the enable_i port from i3c_hci_ibi altogether and drops this connection. Was the intention to keep the port after all, and should that PR be updated to match (or this change dropped)?

If enable_i does stay: ibi_read comes from i3c_queues, which has no enable of its own, so a read of IBI_PORT while PIO_CONTROL.ENABLE is 0 still pops the FIFO but doesn't advance rd_words_left. That would desynchronise the status/data interleaving until a software reset. Would it be safer to gate the read itself (or report it via hciq_access_err) rather than only the counter?

[alees24]

No, it's intended to go as annotated in that later PR; this one has been sitting for a bit longer.

[andreaskurth]

Are "Primary controller only" and "Target only" (line 16) really available yet? Target doesn't currently gate any logic: i3c_target is instantiated unconditionally and targ_enabled comes only from TARG_CONTROL.EN, so a Target=0 build still contains a fully working target that software can enable, while TARG_STATUS.PRESENT reads 0. That seems to contradict the note added at i3c_core.sv:1592 ("neither can presently be unconfigured, since all instances require all functionality").

Would it be clearer to mark these two rows - as well until the parameter actually gates the logic?

[alees24]

I think that should wait until a later review of the documentation. The parameters do not, in the visible RTL, do very much at present, you're correct. I've a local branch which started to scope out the work involved because even in the first deployment I suspect that it will not be the case that both instances require all functionality; proper use of the parameters may occur before first release.

[andreaskurth]

This file isn't listed in SUMMARY.md and nothing links to it, so it won't appear on the docs site. The link checker only validates on-disk paths, so it won't flag the omission either. Could it be added alongside Integration Notes?

[alees24]

OK. Since the other blocks have tended to follow a template, I've not regarded those links as being so 'free form.'

[andreaskurth]

Should these be the post-move paths, i.e. rtl/io_sep/i3c_io_pkg.sv here and rtl/io_vod/i3c_io_pkg.sv at line 91? i3c_io_vod_pkg.sv no longer exists.

While touching the headings - would it be worth naming the styles rather than numbering them, e.g. "Separated Push-Pull and Open Drain Enables" and "Virtual Open Drain"? "Style 1"/"Style 2" doesn't convey anything on its own, and the surrounding text (and DrvSeparatedEn) already uses the descriptive terms.

[alees24]

Thanks for the first response about pathnames; it looks as though that change may hold, but it was far from certain that it would be accepted when I tentatively made it.
I'll keep Style 1/2 - referenced in many places - for now, because I expect all of this still to change. I was aware that numbers are less than helpful. There's an ongoing discussion about even what EG2 requires.

[andreaskurth]

Same here: these are now rtl/io_sep/i3c_io_pkg.sv and rtl/io_vod/i3c_io_pkg.sv.

[andreaskurth]

Did the TODO removed above ("These values are restricted to 8 bits by the HCI; we have 10 bit used/avail values") not apply to these fields too? used/avail are BufAddrW+1 = 11 bits, and RESPONSE_BUFFER_LVL, CMD_QUEUE_FREE_LVL, RX_BUF_LVL and TX_BUF_FREE_LVL (lines 2086-2087) are all 8 bits, so they still wrap for exactly the misconfiguration the new comment guards against. Should they be clamped the same way?

[alees24]

I've started something here, haven't I? This is probably another case of 'software should get what it requests' if misconfiguring things, and assertions in the software should catch misconfiguration. I've introduced a function that clamps the values to the maximum representable levels, but only because this is inexpensive for 2^n.

[andreaskurth]

fifo_lvl_clamp does not appear to be defined anywhere: it is called 12 times here but there is no definition in i3c_fifo_pkg.sv (which this commit does not touch), nowhere else in the tree, and not in #31147/#31148. Was the i3c_fifo_pkg.sv change left out of the commit?

[andreaskurth]

i3c.sv now defaults SecondaryCtrl to 1'b0; should this one match? As it stands the core's standalone default (1,1,1) is the row design_parameters.md marks as not yet available.

[alees24]

I should probably stop supplying default parameter values in submodules; they are, of course, overridden in the parent, although in this case the 'i3c.sv' parent is supposed to be optional, and OT-specific.

[andreaskurth]

Now that Target is explicit, are the SecondaryCtrl | !PrimaryCtrl terms still needed? Per the new table both of those cases already imply Target=1, and the !PrimaryCtrl term makes an all-zero configuration report a target as present.

[andreaskurth]

This comment looks copy-pasted from the VOD variant - this is the package with separate pp_en/od_en enables.

[andreaskurth]

The PR description mentions "leaving the build to choose the appropriate RTL design file", but this is hard-coded and there's no fileset or target for the VOD variant, so choosing still means editing this file. Would a second fileset (e.g. files_rtl_io_vod) selected by a target be worth adding? Happy for that to be a tracked follow-up rather than done here.

[alees24]

Well the description meant builds generally, not just OT EG2; I'm anticipating that EG2 will have its OT-specific '.core' files set for what it needs, in the same way that the hjson cannot support all possible I3C IP configurations, just the one that OT EG2 uses.

[etterli]

Thanks for the refactoring. The VOD now looks like we are almost CIO compatible :)

The problem is that the combined signal struct is not nicely supported by the framework, there needs always to be a data and enable signal. This is what we now have for the VOD case but the signals are grouped to one struct at the IP boundary. This raises the following question to me:
How does topgen know which signal of the struct has to be connected to which pad/pinmux port? AFAIU, this would require manual changes in the top RTL file to correctly wire the data and enable signals. And these changes would depend on the chosen IO mode.

Even if we find a solution, having both IO options is not easily supported in the hjson. We cannot parametrise parts of the hjson.

I've created a CIO compatible proposal on https://github.com/etterli/opentitan/tree/i3c-io. This keeps the combined i3c_targ_bus_drv_t structs but adds the required wiring such that only CIOs can be used. The arbitration logic there is now probably no longer required when the IP is used in the VOD mode/setting.

Just for my understanding, do we see any use case where we require the separated IO scheme? I'm not against having multiple options but just want to understand the context.

[etterli]

NIT: Both packages have the same filename. Just the folder is named differently.

[alees24]

Yes, thanks. Updated. I expected to be slapped down in CI for having subfolders, and I shall not be surprised if down the line something in our tooling gets upset. It seems to be without precedent.

[etterli]

Both packages have the same filename. Just the folder is named differently.

[etterli]

why must the pull up be activated when the IO should be open_drain? Is this controller specific?

General I3C question: Isn't it intended that the bus pull ups are on the PCB? Is this in the responsibility of the active controller?

[alees24]

High-keepers are always on, but pull-ups need to be switched on/off. In open drain mode, the pull-up is required to produce the 'high' state of the bus; otherwise the signal voltage is defined. In push-pull mode, having the pull-up enabled, whilst it may not break function, it would at least be creating a resistance against which the NMOS driver (pulling low) would have to fight, and increase the power consumption.

Even with external pull-ups, the Active Controller is still in charge of them. Ditto for the high-keepers. Although the high-keepers are usually left enabled, they may prove inadequate for a given physical bus, so the I3C Basic specification recommends that it be possible to enable/disable them.

[etterli]

I see, makes sense. Thanks.

[andreaskurth]

Thanks @alees24, you fixed many points I raised, and I'm OK with the ones you deferred or declined.

Two things on the new clamping code, otherwise this LGTM.

[andreaskurth]

TARG_TX_QUEUE_STATUS_LEVEL.TX_DESC_FREE_LVL is bits 23:16, i.e. 8 bits (i3c_reg_pkg.sv:2607-2609), so the 12-bit clamp and cast here get truncated to 8 anyway and the field still wraps above 255. Should this be 8'(fifo_lvl_clamp(..., 8))?

Its sibling tx_buf_free_lvl on the next line genuinely is 12 bits, which is presumably how the two came to be paired. The other 12-bit uses look fine (no-ops, since used/avail are 11 bits, but harmless as a precaution).

[alees24]

Thanks; actually this has prompted to me to check a bit more carefully - the TTI probably isn't yet complete since there have never been any proper requirements and nothing uses it fully - and I discovered that the rx_desc_lvl field was inappropriately set to 12 bits.

For data Tx and Rx from/to the Virtual Targets, 2^12 should be more than enough for the data words, and 2^8 should be more than ample for the transfer descriptors. For IBI and Async Events I stick to 2^12 because these have inline descriptors rather than separated queues for descriptors and data.

[andreaskurth]

fifo_lvl_clamp does not appear to be defined anywhere: it is called 12 times here but there is no definition in i3c_fifo_pkg.sv (which this commit does not touch), nowhere else in the tree, and not in #31147/#31148. Was the i3c_fifo_pkg.sv change left out of the commit?

[alees24]

Thanks @alees24, you fixed many points I raised, and I'm OK with the ones you deferred or declined.

Two things on the new clamping code, otherwise this LGTM.

Yes, fifo_lvl_clamp was I'm afraid omitted from the modified PR; I did realize but not in time to fix it before you discovered this yourself.

[andreaskurth]

Thanks @alees24 for addressing my feedback. The new fifo_lvl_clamp function seems to return one bit fewer than it takes; pls see below.

[andreaskurth]

The return type is one bit narrower than the argument: entries is [DepthW:0] (11 bits, 0-1024) but the result is [DepthW-1:0], so the maximum value wraps rather than passing through.

used/avail do reach 1024, since fifo_size = (max - min) + 1 is 1024 for min=0, max=1023, so a queue given the whole message buffer would report 0 when completely full (used) or 0 free when completely empty (avail). It affects the four 12-bit callers - async_evt_lvl, targ_buf_status_level.rx_buf_lvl, ibi_free_lvl and tx_buf_free_lvl; the 5- and 8-bit ones are safe because the guard catches those values first.

Suggested change

	function automatic bit [DepthW-1:0] fifo_lvl_clamp(input [DepthW:0] entries, input int n);
	function automatic bit [DepthW:0] fifo_lvl_clamp(input [DepthW:0] entries, input int n);

One related detail, in case it looks like the mask should do the work instead: ('b1 << n) - 'b1 is evaluated at the width of the enclosing DepthW'() cast, so for n >= DepthW it saturates at 1023 rather than 2**n - 1. That is unreachable today (the guard is false there), but it is why widening the return type seems the cleaner fix.

[alees24]

Och, branch-juggling again; the version that you're seeing is not correct. I briefly confused myself thinking about the values as byte counts rather than DWORDs. It was meant to be [DepthW:0] as you suggest, although in fact it does not matter. The 12-bit fields I've created in the register API are that width to leave a bit of headroom for larger buffers, but nothing is intended to support a buffer that large, and something else would surely break first. The 12-bit fields therefore don't require use of fifo_lvl_clamp() so I've dropped that complication.

In fact no queue can in practice be allocated the entire message buffer, so even 1024 entries does not need to be accommodated; as a though exercise, I suppose one could conceivably create a Target that does nothing more than send IBIs, but anything that actually requires Read/Write Transfers must have a descriptor queue alongside the data buffer.

Re the last point, about expression width, rest assured that I am aware of that.

[andreaskurth]

LGTM, thank you @alees24!