x-heep icon indicating copy to clipboard operation
x-heep copied to clipboard

regtool generates a `reg_steer` signal that is one bit too short

Open cousteaulecommandant opened this issue 1 year ago • 1 comments

https://github.com/esl-epfl/x-heep/blob/a639054570419fb706da38fd0c054e401dfb14d4/hw/vendor/pulp_platform_register_interface/vendor/lowrisc_opentitan/util/reggen/reg_top.sv.tpl#L192

The above line should say ${num_wins_width}, not ${num_wins_width-1}, since the definition of num_wins_width already includes the -1 (and in newer OpenTitan versions has been renamed to steer_msb which is a less misleading name):

https://github.com/esl-epfl/x-heep/blob/a639054570419fb706da38fd0c054e401dfb14d4/hw/vendor/pulp_platform_register_interface/vendor/lowrisc_opentitan/util/reggen/reg_top.sv.tpl#L16

The following files are affected:

  • hw/vendor/pulp_platform_register_interface/vendor/lowrisc_opentitan/util/reggen/reg_top.sv.tpl
  • hw/vendor/pulp_platform_gpio/util/reggen/reggen/reg_top.sv.tpl
  • hw/vendor/lowrisc_opentitan/util/reggen/reg_top.sv.tpl (not affected by -1 issue, but see comment below about superfluous +1)

cousteaulecommandant avatar May 13 '24 12:05 cousteaulecommandant

https://github.com/lowRISC/opentitan/blob/bfed7763827d338e014aba80884273c23355009d/util/reggen/reg_top.sv.tpl#L26

In fact, in OpenTitan they have ditched the +1 in num_wins+1 since reg_steer only ever needs to take a value between 0 and num_wins (0 to num_wins-1 for a window, or num_wins for the plain register interface, i.e., num_wins+1 possible values). This difference made the code work for num_wins=1 (as well as 3, 7, 15, etc) since it was erroneously allocating a larger than needed range, and then cutting down that range by 1 bit, and the two errors cancelled out each other in the case of powers of 2 minus 1 (1, 3, 7...) but not for any other number of windows.

In short, this error does not surface if you only have 1 window, which I guess was the most widely tested use case, but it does surface e.g. for 2 or 4 windows.

cousteaulecommandant avatar May 13 '24 13:05 cousteaulecommandant

@cousteaulecommandant sorry for the late reply - if you have a way to fix it, feel free as usual to submit a PR - as this is touching a vendorized repository, you have two choices:

  1. PR to the pulp_platform register interface repository - then revendorize it here if it gets accepted
  2. PR to this repository by patching the script above
  3. PR to this repository by removing the pulp script and using a new one that will be maintained in X-HEEP

My favorites are in order 1, 2, and 3.

davideschiavone avatar Jan 29 '25 15:01 davideschiavone

Good news. It looks like pulp-platform/register_interface already uses the logic [${steer_msb}:0] reg_steer; definition from OpenTitan, so it's just a matter of revendorizing it here. I don't have much experience with revendorizing though, so for now I'll leave it to the experts if possible :) (if not, I might give it a try in the future)

(The change was introduced in pulp-platform/register_interface@7cf6ae7 from February 2023, in case you don't want to upgrade all the way to the newest version.)

cousteaulecommandant avatar Jan 31 '25 00:01 cousteaulecommandant