lowRISC / lowRISC/opentitan

[otbn] OTBN autogenerated headers do not have value defines for CMD, STATUS

Open
#16,754 5 comments 0 reactions 1 assignee View on GitHub

@GregAC is already working on this.

Since Apr 2, 2024.

Component:DV Earlgrey-PROD Candidate IP:otbn Priority:P3
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

See https://github.com/lowRISC/opentitan/pull/16119#discussion_r1043471466

The autogenerated `otbn_regs.h` file does not have defines for field values in the `CMD` or `STATUS` fields. The fields are defined but their values are not enumerated, e.g.:
```c
// Command Register
#define OTBN_CMD_REG_OFFSET 0x10
#define OTBN_CMD_REG_RESVAL 0x0
#define OTBN_CMD_CMD_MASK 0xff
#define OTBN_CMD_CMD_OFFSET 0
#define OTBN_CMD_CMD_FIELD \
((bitfield_field32_t) { .mask = OTBN_CMD_CMD_MASK, .index = OTBN_CMD_CMD_OFFSET })

// Control Register
#define OTBN_CTRL_REG_OFFSET 0x14
...
```

I think this can be fixed by adding an `enum` field to `otbn.hjson` [here](https://github.com/lowRISC/opentitan/blob/7411ac9c6fc12949199bfe70fe43e7fd205c7ef3/hw/ip/otbn/data/otbn.hjson#L326). This seems to produce the desired result for e.g. [KMAC](https://github.com/lowRISC/opentitan/blob/7411ac9c6fc12949199bfe70fe43e7fd205c7ef3/hw/ip/kmac/data/kmac.hjson#L496).

I think I understand what's required here and am happy to make this change myself. But I wanted to check if there's a specific reason these values weren't included. @GregAC do you know?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.