[edn/rtl] Adapt sw_cmd_sts register #20873

h-filali · 2024-01-18T11:47:46Z

This PR contains changes for the sw_cmd_sts register. The register now includes four fields.
Namely: sw_cmd_rdy, sw_cmd_reg_rdy, sw_cmd_ack and sw_cmd_sts.
Through this PR the documentation should now be consistent within itself and with the RTL (regarding the sw_cmd_sts reg).
For further information please see the individual commit messages.

Some of the commits in this PR are already contained in #20293

This PR is a precursor of the fix for #15561

vogelpi

Great work @h-filali , this was a major piece of work! I just have a few questions and nits.

I've reviewed RTL, DV and the DIF as well as the cryptolib changes and it looks good to me. But I think it would be great if @jadephilipoom could also review the software changes.

hw/ip/edn/data/edn.hjson

hw/ip/edn/rtl/edn.sv

hw/ip/edn/rtl/edn_main_sm.sv

hw/ip/edn/rtl/edn_core.sv

hw/ip/edn/dv/env/edn_scoreboard.sv

hw/ip/edn/dv/env/seq_lib/edn_genbits_vseq.sv

hw/ip/edn/dv/edn_sim_cfg.hjson

sw/device/tests/BUILD

vogelpi · 2024-01-19T12:25:13Z

sw/device/lib/crypto/drivers/entropy.c

-    abs_mmio_write32(kBaseCsrng + CSRNG_INTR_STATE_REG_OFFSET, reg);
+    abs_mmio_write32(base_address + CSRNG_INTR_STATE_REG_OFFSET, reg);


Nit: this change seems not required as the if is only true for CSRNG anyway.

h-filali · 2024-01-19T14:10:41Z

Great work @h-filali , this was a major piece of work! I just have a few questions and nits.

I've reviewed RTL, DV and the DIF as well as the cryptolib changes and it looks good to me. But I think it would be great if @jadephilipoom could also review the software changes.

Thanks @vogelpi for your review, some very helpful inputs. I believe all your points should be addressed now.

jadephilipoom

Looked over the changes in cryptolib and dif files and it looks good to me! Just a few small comments.

jadephilipoom · 2024-01-19T14:12:51Z

sw/device/lib/crypto/drivers/entropy.c

+// passes to CSRNG. In this case, the check_completion register should only be
+// true for non-generate commands issued to the SW register.


nit: I think here the comment refers to the argument to the function below

Suggested change

// passes to CSRNG. In this case, the check_completion register should only be

// true for non-generate commands issued to the SW register.

// passes to CSRNG. In this case, the check_completion argument should only be

// true for non-generate commands issued to the SW register.

You're right, I'll change that.

jadephilipoom · 2024-01-19T14:15:49Z

sw/device/lib/crypto/drivers/entropy.c

@@ -361,10 +390,11 @@ static status_t csrng_send_app_cmd(uint32_t reg_address,
    return OTCRYPTO_RECOV_ERR;
  }

-  if (check_completion) {
+  if (check_completion && (cmd_type == kEntropyCsrngSendAppCmdTypeCsrng)) {


In this and the following check, is check_completion actually serving a purpose now? Should we just remove that argument from the function and use cmd_type instead to determine where and whether to look for the done bit?

I guess this could still have a use case if one would like to send a CSRNG/EDN_SW command without waiting for the acknowledgement.

jadephilipoom · 2024-01-19T14:20:10Z

sw/device/lib/crypto/drivers/entropy.c

+  uint32_t cmd_reg;
+  uint32_t sts_reg;
+  uint32_t rdy_bit;


nit: I'd rename these slightly so that it's clear they hold register addresses, not register contents; this will better match naming elsewhere in the cryptolib drivers.

Suggested change

uint32_t cmd_reg;

uint32_t sts_reg;

uint32_t rdy_bit;

uint32_t cmd_reg_addr;

uint32_t sts_reg_addr;

uint32_t rdy_bit_addr;

Good point, thanks!

h-filali · 2024-01-19T14:43:50Z

Looked over the changes in cryptolib and dif files and it looks good to me! Just a few small comments.

Thanks a lot for your inputs @jadephilipoom ! I hope I was able to address everything properly.

This commit changes the sw_cmd_sts register. The register now contains the following fields: cmd_reg_rdy, cmd_rdy, cmd_sts and cmd_ack. An explanation for these fields can be found in the contents of this commit. The command reg ready signal should be high when the EDN is ready to accept new SW command data and the output FIFO is ready to accept data. This commit should make it easier for firmware to check whether a command has been acked by CSRNG. Now software shouldn't need to check the intr signal any more and it should be possible to find out whether the ack has happened and the sts is succesful with a single read. Signed-off-by: Hakim Filali <[email protected]>

… vseq This commit fixes some rare instances where the edn_err_vseq fails because the data is unstable after the EDN has been disabled. The assertions can be disabled here, since the specification says that the CSRNG and ES have to be disabled and re-enabled before the EDN is turned back on. Signed-off-by: Hakim Filali <[email protected]>

This commit adds new checks to the scoreboard for the new cmd_ack and cmd_sts fields of the sw_cmd_sts register. The cmd_rdy and cmd_reg_rdy fields are not checked yet since there is another HW change planned where we remove the output FIFO. It would be fairly complicated to track those two fields and since we plan on changing the behaviour for those two regs a bit it would not be worth to write the tracking code in the scoreboard yet. Signed-off-by: Hakim Filali <[email protected]>

This commit aligns the documentation with the new behavior of the sw_cmd_sts register. This commit also gets rid of the existing amiguity in our documentation regarding the sw_cmd_sts register. Signed-off-by: Hakim Filali <[email protected]>

This commit adds two new cover groups. One for each time the sw_cmd_sts register is read. The other for the sts signal that gets recorded each time the edn receives an acknowledgement from the CSRNG agent. Signed-off-by: Hakim Filali <[email protected]>

This commit sets the time out for two of the chip level EDN tests to long. These tests were failing or are now failing with the added HW changes, because the timeout values were too low. This commit fixes this issue and removes the broken tag from one of the tests. Signed-off-by: Hakim Filali <[email protected]>

This commit aligns the EDN difs with the HW changes in prior commits. Some of the difs are shared between the EDN and CSRNG. The main difference is that now the difs can see when a command is done by looking at the ack bit in the sw_cmd_sts register. Signed-off-by: Hakim Filali <[email protected]>

This commit aligns the crypto library to the new behavior of the EDN's sw_cmd_sts register. This commit is similar to the previous commit for the dif's. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the edn-align-sw-cmd-sts branch from fb673dd to 0293f15 Compare January 18, 2024 13:04

h-filali marked this pull request as ready for review January 18, 2024 13:05

h-filali requested review from a team as code owners January 18, 2024 13:05

h-filali requested review from jdonjdon and jadephilipoom and removed request for a team January 18, 2024 13:05

h-filali force-pushed the edn-align-sw-cmd-sts branch 8 times, most recently from 765ed1c to 576f15a Compare January 19, 2024 09:17

vogelpi requested review from andreaskurth and vogelpi January 19, 2024 09:29

h-filali force-pushed the edn-align-sw-cmd-sts branch from 576f15a to e6af7aa Compare January 19, 2024 10:29

h-filali added the kokoro:rebuild label Jan 19, 2024

opentitan-github-bot removed the kokoro:rebuild label Jan 19, 2024

vogelpi approved these changes Jan 19, 2024

View reviewed changes

h-filali force-pushed the edn-align-sw-cmd-sts branch from e6af7aa to 64c5fd3 Compare January 19, 2024 14:06

h-filali force-pushed the edn-align-sw-cmd-sts branch from 64c5fd3 to 489cc5a Compare January 19, 2024 14:16

jadephilipoom approved these changes Jan 19, 2024

View reviewed changes

h-filali force-pushed the edn-align-sw-cmd-sts branch from 489cc5a to 254d8ce Compare January 19, 2024 14:40

h-filali force-pushed the edn-align-sw-cmd-sts branch 2 times, most recently from ada8810 to b0c327d Compare January 19, 2024 16:47

h-filali force-pushed the edn-align-sw-cmd-sts branch from b0c327d to fb97c5b Compare January 22, 2024 09:46

Hakim Filali added 8 commits January 22, 2024 09:49

[edn/cryptolib] Adapt the crypto lib to the new sw_cmd_sts register

57be4c4

This commit aligns the crypto library to the new behavior of the EDN's sw_cmd_sts register. This commit is similar to the previous commit for the dif's. Signed-off-by: Hakim Filali <[email protected]>

h-filali force-pushed the edn-align-sw-cmd-sts branch from fb97c5b to 57be4c4 Compare January 22, 2024 09:50

vogelpi merged commit 0a79acc into lowRISC:master Jan 22, 2024
32 checks passed

vogelpi mentioned this pull request Mar 29, 2024

[edn] D2S Signoff #20982

Closed

h-filali deleted the edn-align-sw-cmd-sts branch October 7, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[edn/rtl] Adapt sw_cmd_sts register #20873

[edn/rtl] Adapt sw_cmd_sts register #20873

h-filali commented Jan 18, 2024

vogelpi left a comment

vogelpi Jan 19, 2024

h-filali commented Jan 19, 2024 •

edited

Loading

jadephilipoom left a comment

jadephilipoom Jan 19, 2024

h-filali Jan 19, 2024

jadephilipoom Jan 19, 2024

h-filali Jan 19, 2024

jadephilipoom Jan 19, 2024

h-filali Jan 19, 2024

h-filali commented Jan 19, 2024 •

edited

Loading

		abs_mmio_write32(kBaseCsrng + CSRNG_INTR_STATE_REG_OFFSET, reg);
		abs_mmio_write32(base_address + CSRNG_INTR_STATE_REG_OFFSET, reg);

		// passes to CSRNG. In this case, the check_completion register should only be
		// true for non-generate commands issued to the SW register.

[edn/rtl] Adapt sw_cmd_sts register #20873

[edn/rtl] Adapt sw_cmd_sts register #20873

Conversation

h-filali commented Jan 18, 2024

vogelpi left a comment

Choose a reason for hiding this comment

vogelpi Jan 19, 2024

Choose a reason for hiding this comment

h-filali commented Jan 19, 2024 • edited Loading

jadephilipoom left a comment

Choose a reason for hiding this comment

jadephilipoom Jan 19, 2024

Choose a reason for hiding this comment

h-filali Jan 19, 2024

Choose a reason for hiding this comment

jadephilipoom Jan 19, 2024

Choose a reason for hiding this comment

h-filali Jan 19, 2024

Choose a reason for hiding this comment

jadephilipoom Jan 19, 2024

Choose a reason for hiding this comment

h-filali Jan 19, 2024

Choose a reason for hiding this comment

h-filali commented Jan 19, 2024 • edited Loading

h-filali commented Jan 19, 2024 •

edited

Loading

h-filali commented Jan 19, 2024 •

edited

Loading