[sysrst_ctrl] Misleading documentation and reset default for PIN_ALLOWED_CTL

[jesultra]

Description

The section Flash Write Protect Output contains the following two sentences:

[...] The software can release flash_wp_l_o explicitly by setting PIN_OUT_CTL.FLASH_WP_L to 0 when needed. [...] the value of flash_wp_l_o defaults to logic 0 when it is not explicitly driven via the override function. [...]

Keeping in mind that flash_wp_l is a low-active signal, the first sentence gives the impression that if the override control bit for flash_wp_l is disabled, that the signal will revert to its deasserted high level. However, the second sentence states that in the absense of overriding, the signal will be logic 0, that is, asserted.

In practice, the Z1 silicon seems to behave as the second sentence describes, that is, contradicting the first one.

I think that it would make more sense that the "default" level of flash_wp_l, in absence of any override would be logic 1, that is, deasserted. This would match how the other signal ec_rst_l behaves, in the sense that in absence of override, the level of that signal is controlled by the key combination detection logic, which defaults to logic 1.

Furthermore, the default value for the register PIN_ALLOWED_CTL is to have bits flash_wp_l_0 and ec_rst_l_0 set, and nothing else. This seems consistent with the intention behind the first sentence in the documentation (which is contrary to how Z1 silicon behaves.) If the default level of flash_wp_l is not changed to logic 1, then the PIN_ALLOWED_CTL ought to also have flash_wp_l_1 set by default.

[pamaury]

I agree that the sentence

The software can release flash_wp_l_o explicitly by setting PIN_OUT_CTL.FLASH_WP_L to 0 when needed.

seems contrary to the rest of the documentation and how it actually works.

It would indeed be more logical that the default value of flash_wp_l is 1 when there is no override, that would be consistent with the reset value of PIN_OUT_CTL and PIN_ALLOWED_CTL.

By the way, there is useful code in the repo in case in helps:
https://github.com/lowRISC/opentitan/blob/master/sw/device/lib/testing/sysrst_ctrl_testutils.c#L63
You can also look at the various sysrst_ctrl tests as well.

I don't know if it's possible to change the RTL at this point though. @andreaskurth might have an answer to that?

[andreaskurth]

So the actual behavior is: The post-reset value of flash_wp_l_o is low (i.e., asserted), and SW can override that to high (i.e., deasserted) -- correct? If so, I think that matches the intention of having write protection by default and having to enable writes explicitly.

Following that logic, we should update/correct our documentation (see PR #24485), but I don't see a need for an RTL change. Do you disagree?

[pamaury]

So the actual behavior is: The post-reset value of flash_wp_l_o is low (i.e., asserted), and SW can override that to high (i.e., deasserted) -- correct? If so, I think that matches the intention of having write protection by default and having to enable writes explicitly.

I think the point that @jesultra is making is that both ec_rst_l and flash_wp_l seem to have the same expected behaviour per the documentation (be asserted low on boot, and there is a way to release after that) but for some reason they use a completely different mecanism:

• ec_rst_l_o is 1 by default so to hold it low, the registers have a default value that specify an override to0,
• flash_wp_lo is 0 by default, so in fact with no override it's already asserted to low on boot, but for some reason, there also an override to 0 in the default register value (which is redundant).

This suggests that the intention was for both pins to work the same way (otherwise why specify a default override for the flash_wp_l pin?), but for some reason it ended up not being the case.

Anyway, this is really more of a software issue at this point I would say, but maybe the RTL can be changed for future release?

[jesultra]

Exactly. For consistency I would like that a future B1 chip would have the WP signal default to 1, with a power on override to 0.
I can fully work around the issue in software for A1.

[andreaskurth]

OK, let's track this under future release then

[pamaury]

@andreaskurth Can you review #24485 when you have the time? It's just a documentation change