efabless / caravel

Caravel is a standard SoC harness with on chip resources to control and read/write operations from a user-dedicated space.
https://caravel-harness.readthedocs.io/
Apache License 2.0
263 stars 63 forks source link

`make gpio_defaults` incorrectly handles some Verilog constant patterns #540

Open amm-efabless opened 2 months ago

amm-efabless commented 2 months ago

make gpio_defaults run on a caravel_user_project can incorrectly handle some Verilog constant/literal patterns in verilog/rtl/user_defines.v because it assumes a 4-character hex value.

`define USER_CONFIG_GPIO_36_INIT 13'h1137              // OK:  mag/gpio_defaults_block_1137.mag
`define USER_CONFIG_GPIO_37_INIT 13'b1_0001_0011_0111  // BAD: mag/gpio_defaults_block_0111.mag; should be _1137.mag also

Also, case matters, as this example should probably just create one normalized ..._1eee.mag file, but instead creates two files:

`define USER_CONFIG_GPIO_36_INIT 13'h1eee              // OK:  mag/gpio_defaults_block_1eee.mag
`define USER_CONFIG_GPIO_37_INIT 13'h1EeE              // BAD: mag/gpio_defaults_block_1EeE.mag also made beside _1eee.mag

These same problems occur indirectly, i.e. when using an intermediate `define:

`define MY_DUMMY_1 13'h1cff
`define MY_DUMMY_2 13'h1cFF
`define MY_DUMMY_3 13'b1110011111111 // same as 1cff in hex

`define USER_CONFIG_GPIO_35_INIT `MY_DUMMY_1
`define USER_CONFIG_GPIO_36_INIT `MY_DUMMY_2
`define USER_CONFIG_GPIO_37_INIT `MY_DUMMY_3

...which will create:

gpio_defaults_block_1cff.mag -- correct
gpio_defaults_block_1cFF.mag -- probably should be _1cff.mag
gpio_defaults_block_1111.mag -- definitely should be _1cff.mag
RTimothyEdwards commented 2 months ago

The script was not designed to be a verilog parser; it is only doing a simple search in the file.

amm-efabless commented 1 month ago

@DavidRLindley FYI