lowRISC / lowRISC/opentitan

[reggen] Adding index to register when there's only a single register

Open
#2,254 13 comments 0 reactions 3 assignees View on GitHub

@msfschaffner is already working on this.

Since Apr 1, 2022.

Component:Tooling Earlgrey-PROD Triaged Priority:P2 Tool:reggen Type:Bug
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

Currently in reggen where you have a multireg where the `count` isn't large enough that it needs to generate multiple registers it doesn't add an index to the register name when generating them. This causes an issue when trying to write something that uses the generated register definitions that is intended to work for any `count`.

As a concrete example in the padctrl DIF (currently in a PR so link is to my fork) `PADCTRL_DIO_PADS0_REG_OFFSET` is used and this becomes `PADCTRL_DIO_PADS_REG_OFFSET` depending upon the number of DIO pads:

https://github.com/GregAC/opentitan/blob/1b9ce558ddb0c440950525a890b1917773923576/sw/device/lib/dif/dif_padctrl.c#L64-L70

Fixing this in the C is a bit messy as it would require some `#ifdef` to determine if `PADCTRL_DIO_PADS0_REG_OFFSET` or `PADCTRL_DIO_PADS_REG_OFFSET` should be used.

Fixing this in reggen is easy enough, but disruptive, the python that does this is here:

https://github.com/lowRISC/opentitan/blob/7a98ea292907689b15bcb7897f7163b8e6508680/util/reggen/validate.py#L1140-L1152

However removing this behaviour alters various autogened headers which breaks existing code. This can be fixed but I wanted to see what people thought before I went and did this. Perhaps there's a reason to preserve this behaviour I haven't considered?

As an alternative we could have reggen generated some additional define that gives you the 'base offset' who's name doesn't change (which in the example above would be equal to `PADCTRL_DIO_PADS0_REG_OFFSET`/`PADCTRL_DIO_PADS_REG_OFFSET`), ultimately code that can work with any `count` will want some base it can then compute an appropriate offset to so referring to something like `PADCTRL_DIO_PADS_REG_OFFSET_BASE` (as opposed to something that's meant to be a specific register) may make more sense.

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.