search

Juniper / open-register-design-tool

Tool to generate register RTL, models, and docs using SystemRDL or JSpec input
Apache License 2.0
195 stars 69 forks source link

Bug: adding a reset signal breaks ordt #64

Closed sjalloq closed 5 years ago

sjalloq commented 5 years ago

I'm assuming this is a bug as I'm copying an example from the RDL spec. I'm trying to specify a reset signal name of reset_n that is active low. Adding this causes ordt to output the error message:

Open Register Design Tool, version=190524.01, input=regfile.rdl.pp.final Ordt: reading parameters from rdl.params... Ordt: building verilog... *** ERROR ***: A root addrmap is required for verilog generation

Example attached: regfile.rdl.pp.final.txt

sdnellen commented 5 years ago

There are several tool ideosyncracies/bugs affecting behavior here...

  1. by default, the the tool expects a single top level addrmap instance and an instanced signal counts here - can move the signal instance to the addrmap to address this (the def can still be at top level)
  2. if intent is to define a new reset signal for all generated logic, add the cpuif_reset property to the signal definition (no explicit resetsignal assign is required in this case)

  3. the tool very tightly constrains variable scope (eg, reset_n defined at top is not visible in a field define). if intent is to use a specific reset on a reg, you can use dynamic assigns, eg

    regfile my_regs { reset_n reset_n; irq_0 my_irq_0 ; my_irq_0.*->resetsignal = reset_n; }; Unfortunately, it looks like there is a bug that causes the activelow property to be lost in this case.

sjalloq commented 5 years ago

Thanks for the description. I've now had a play with it and can see what's going on.

I've defined an activelow reset_n signal at the top level and am instancing it in my addrmap. However, it seems to have respected the activelow property. The only thing it doesn't respect is async which I think you already mark as not being supported.

Is there a way to get rid of the "sig_" prefix to the signal though? I wanted to use an active low "reset_n" input to avoid having to invert and assign a local reset signal at the hierarchy above. I still have to do that if the compiler prefixes reset_n to sig_reset_n.

Finally, is there an option to force the tool to generate a toplevel Verliog file with the same name as the toplevel module. I use Emacs and Verilog-Mode AUTOs and it searches for Verilog files with the same name as the module you're instantiating. At the moment I force the output filename in my Makefile but obviously have to remember to update this when I update the addrmap name.

Thanks.

sdnellen commented 5 years ago

There's definitely an issue with use of activelow in user defined reset signals that do not use cpuif_reset or logic_reset options - fix is pending.

The asyc rdl property isnt supported on a signal basis, but there is a global systemverilog parameter use_async_resets that if set to true will make reset of all sequential elements async.

Currently no option to avoid 'sig_' prefix on user defined signals - it's a shortcut to guarantee avoidance of aliasing with tool-generated signal names and provides easy way to identify user-defined signals.

The tool will output one file per verilog module if the specified commend line destination ends with "/", eg -verilog myOutputDirectory/

sjalloq commented 5 years ago

OK, fantastic. Now implemented those options and I get what I want.