lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.58k stars 776 forks source link

[pinmux] Earl Grey open drain and pullup #22802

Closed jesultra closed 5 months ago

jesultra commented 6 months ago

Description

With Z1 silicon, I am observing that if I enable od_en on pin R7, then it seems that the chip also enables internal pullup, no matter how I configure pull_en/pull_select. (I have not tried other pins, but have no reason to assume they would behave differently.)

ChromeOS uses open drain signals between different power domains, and we want to control which power rail the pullup is connected to. Being forced to have a pullup to the GSC power rail could mean that while a Chromebook is off, the battery is unnecessailty being discharged through a pin of the unpowered AP.

Does enabling "true" open drain allow the voltage to go beyond the upper voltage rail? That is, does it disable the clamping diodes? Even if it did, if not enabled under/after reset, clamping would still be a problem. Where do we find such documentation, which I assume does not belong with Earl grey top, but is specific to NuvoTitan.

If not, what is the use of "true" open drain, as opposed to alway using "virtual open drain"?

Further. This issue makes me wonder again, what is the difference between virtual_od_en and od_en. I understand that od_en is the enable signal to a feature of the pad circuit/macro supplied by Nuvoton, whereas virtual_od_en is implemented in "our" logic, (by fiddling with output enable in sync with the output value I assume). I still question the need to have two separate bits for these features, as opposed to the FPGA and silicon logic being different, so that each had a slightly differnt feature enabled by the same bit. We would not need to synthesize the virtual open drain logic on any real chip. For now though, it seems that we can use the virtual open drain as a workaround to get open drain without being forced to have internal pullup. I do not think we want to do this in production Chromebooks, unless we are sure that the virtual open drain works without glitches.

a-will commented 6 months ago

I'll just note that the pad attributes originate rather directly from CSRs:

https://github.com/lowRISC/opentitan/blob/90f79898a7aef54110511d8a9c54eb54720aa233/hw/ip/pinmux/rtl/pinmux.sv#L213-L218

The CSR values get masked through a technology-specific prim_pad_attr: https://github.com/lowRISC/opentitan/blob/90f79898a7aef54110511d8a9c54eb54720aa233/hw/ip/pinmux/rtl/pinmux.sv#L265-L273

And they are then passed directly to the technology-specific prim_pad_wrapper (with a couple overrides for JTAG TCK and TRST_N if JTAG is enabled, but IOR7 is not one of those).

Supported pad features and the logic that connects them would be part of the technology-specific implementation.

jesultra commented 6 months ago

Yes, that is why I originally filed this with NuvoTitan in the title, rather than Earlgrey.

moidx commented 6 months ago

Hi @jesultra, I checked on the silicon side and based on the current IO configuration, the only pads that support od_en are as follows:

All other IO don't support od due to limitations in the pad. Instead, they support virtual_od which relies on digital logic to perform the following as described in the spec:

[for pads with virtual_od support] standard bidirectional pads are employed, but instead of driving the output high for a logic 1 the pad is put into tristate mode.

The specification should be updated to make this more clear.

So in summary, I believe we do need to ensure that virtual_od works without any glitches for the IO pads that don't support open drain natively.

andreaskurth commented 6 months ago

Thanks for checking this, @moidx.

The specification should be updated to make this more clear.

I created PR #23031 that should do this.

So in summary, I believe we do need to ensure that virtual_od works without any glitches for the IO pads that don't support open drain natively.

Would someone on your side be able to look at this? Or shall I try to find someone else?

andreaskurth commented 6 months ago

Related to this, in Earlgrey's SDC we restrict STA to the case where od_en is disabled: https://github.com/lowRISC/opentitan/blob/ab878b5d3578939a04db72d4ed966a56a869b2ed/hw/top_earlgrey/syn/chip_earlgrey_asic.sdc#L1073-L1078

I understand that we don't need to close timing for switching od_en, but shouldn't we also include od_en == 1 in STA?

a-will commented 6 months ago

Related to this, in Earlgrey's SDC we restrict STA to the case where od_en is disabled:

https://github.com/lowRISC/opentitan/blob/ab878b5d3578939a04db72d4ed966a56a869b2ed/hw/top_earlgrey/syn/chip_earlgrey_asic.sdc#L1073-L1078

I understand that we don't need to close timing for switching od_en, but shouldn't we also include od_en == 1 in STA?

I don't believe there are any timing-critical paths where this is meaningful or would somehow apply a tighter constraint than we already have between the relevant nodes (i.e. the pinmux and the pad). That said, for final timing sign-off, it probably would be best if all the functional modes were represented.

jesultra commented 6 months ago

I have limited knowledge of logic synthesis, so what I describe below could be way off.

What I am concerned about is logic such as this: https://github.com/lowRISC/opentitan/blob/03c10d2e2e2fd8c9849b3787e987b6e48d503a60/hw/ip/prim_generic/rtl/prim_generic_pad_wrapper.sv#L78-L81

I assume that the Nuvoton pad logic is asynchronous (does not take clock input), and with the logic above, any transition of out_i on a pad with attr_i.virt_od_en enabled will result in both out and oe signals to the pad toggling in the same cycle. If these two signals go though a clocked buffer before the pad, the possible skew is going to be small, and probably would not be an issue, even though the asynchronous pad itself could have skewed response times on the two signals. If the two signals do not go through any clocked buffers, then the skew could be larger. I would suggest changing the logic as below:

    assign out = (out_i ^ attr_i.invert) & ~attr_i.virt_od_en;
    assign oe  = oe_i & ((attr_i.virt_od_en & ~out) | ~attr_i.virt_od_en);

That way, a transition of out_i will only result in transition of the oe signal to the pad, not two almost simultaneous transitions. If there is no clocked buffer after the above logic, there could still be a problem even with the above change, depending on how exactly the logic is synthesized, if the "compiler" is free to rearrange (which I suppose it is), and chose to implement the above as well as more logic in a PLA or something, then maybe a transition of out_i could still result in transients on out. I do not know enough about Verilog to tell.

I am not certain that the above piece of logic is what is used for the Nuvoton chip, but I hope that we can do a similar review if is not.

By the way, the above logic has another bug, I would say. If both attr_i.invert and attr_i.virt_od_en are set, we get a behavior where setting out_i to 0 results in strong drive high, and setting out_i to 1 results in high-Z. This is not what I would have expected from "inverted open drain". I would suggest the below logic instead:

    assign oe  = oe_i & ~(attr_i.virt_od_en & (out_i ^ attr_i.invert));
a-will commented 6 months ago

prim_generic_pad_wrapper is for simulation only.

But the point about hazards with mixing open-drain and signal level inversion is an interesting one. The original code is correct here, though.

I think it doesn't generally make sense for an I/O IP to use open-drain signaling, non-flopped outputs, and an output-enable, though. That's just a flawed design.

It's unusual to combine open-drain signaling at the pad and an IP-controlled output-enable, since the nature of open-drain signaling already handles collisions. In a single IP, these would likely only combine for multi-modal types (like I3C), and in those cases, the IP would do its own virtual open-drain with the output-enable instead.

a-will commented 6 months ago

This reminds me... We have IPs that do virtual open-drain internally, and the logic connecting it to a pad in either open-drain mode would need to understand that. This can be an area where bugs might surface.

For example, i2c only signals via the output-enable:

https://github.com/lowRISC/opentitan/blob/03c10d2e2e2fd8c9849b3787e987b6e48d503a60/hw/ip/i2c/rtl/i2c.sv#L126-L130

jesultra commented 6 months ago

Signalling with output-enable only, as the I2C IP does, would be exactly what we want, right? That way, there is no opportunity for skew, as only one signal changes.

jesultra commented 5 months ago

I am ready to close this issue with the observation that the original problem reported by me turned out to be a misunderstanding about what virtual open drain vs. true open drain was. Updating the Ti50 codebase to make use of the virtual open drain bit at least for the pins that do not support true open drain should work around that issue.

I have a minor thing, which is that on the engineering silicon, I observe true open drain behavior on pin B8, contrary to what Miguel claimed. (It could be that Miguel quoted the A1 silicon specifications, which could have differed from Z1.) I have not attempted to do a comprehensive examination of all pins. Only observing that the pins R6 and R7 does not have true open drain capability, but setting the VIRTUAL_OPEN_DRAIN bit from Ti50 makes them behave as we expect, that is, never actively driving high.

moidx wrote:

Hi @jesultra, I checked on the silicon side and based on the current IO configuration, the only pads that support od_en are as follows:

IOA[6-8] IOB[9-12] IOC[10-12] IOR[8-13]

cdgori commented 5 months ago

@jesultra do you want to open a separate low-priority issue for the behavior of pin B8 so this doesn't get lost? (I suspect that as this is closed it will be lost to the sands of time)

jesultra commented 5 months ago

Good point. I have created #23270 to get clarity on B8, (and more clarity on the virtual open drain in general.)