RTimothyEdwards / caravel_openframe_project

Example digital project for the Efabless Caravel "openframe" harness
Apache License 2.0
7 stars 10 forks source link

Select out of bounds on signal `gpio_dat_o[*]` #1

Closed kareefardi closed 1 year ago

kareefardi commented 1 year ago

In the following section, in picorv32.v:

    for (i = 0; i < 32; i = i + 1) begin
        assign gpio_all_dat_o[i] = |(gpio_dat_o[i][`OPENFRAME_IO_PADS-1:0]);
    end

[`OPENFRAME_IO_PADS-1:0] is out of bound for the 32 bus gpio_data_o[i]. I believe the intention behind the for loop was to OR all i'th bit of each PADS gpio bus. The current for loop doesn't accomplish that.

RTimothyEdwards commented 1 year ago

@kareefardi : iverilog seemed to be okay with it but I can't get the synthesis tools to understand it. The intent seems perfectly clear but apparently somehow is not valid verilog (except to iverilog). Do you know a way to restate it properly?

RTimothyEdwards commented 1 year ago

@kareefardi : Note that very little of this project has actually been tested through verilog testbenches. There's only one testbench, but it tests the GPIO vector setting, so it actually validates iverilog's understanding of the "for" loop above, and iverilog handles it correctly.

RTimothyEdwards commented 1 year ago

@kareefardi : There is a caravel_openframe_wrapper.def file in caravel_openframe_project/openlane/openframe_project_wrapper/fixed_dont_change/ which ultimately should reside in the caravel repository. It is incomplete; I copied it from the original user project wrapper and modified it for the correct openframe pins. However, there are tracks and sites defined in it that correspond to the original user project wrapper and are limited to the smaller area that it has compared to the openframe area. However, when I ran openlane, it appeared to honor the DIEAREA and used the entire area for placement. I assume that Openlane/OpenROAD can rewrite a corrected DEF at some point in the flow?

So far, my attempt to run this through Openlane (and ignoring the for() loop error just to see if I could get through the entire flow) gets through placement, at which point it fails to complete placement and I'm not sure how to proceed from there. Looking at the placement result, it looked fine, and the macros were placed correctly, but the number of logic gates looked way too low (and the sparse placement doesn't sync with the failure to complete placement).

Please bear in mind that the current state of this project is where I was just trying to get it to complete synthesis, place, and route. I have made no attempt to verify anything or apply timing constraints, other than copying some files from the caravel management SoC that undoubtedly need editing.

kareefardi commented 1 year ago

gpio_dat_o is a 2D array of 44 rows and 32 columns. The loop is attempting to access the i'th 32 bit bus while selecting a range of size 44. This results in the yosys warning of out of bounds.

RTimothyEdwards commented 1 year ago

@kareefardi : If you can suggest what the double loop would look like to concatenate the bits of an array instead of using the "|" operator which yosys will not apply to an array, then please post the solution here.

M0stafaRady commented 1 year ago

I can see the same warning in VCS.

Warning-[SIOB] Select index out of bounds /home/rady/caravel/openframe/caravel_openframe_user_project//verilog/rtl/picosoc.v, 588 "gpio_dat_o[31][(44 - 1):0]" The select index is out of declared bounds : [31:0]. In module instance : openframe_example In module : picosoc.

RTimothyEdwards commented 1 year ago

Yes, it needs to be rewritten, if someone can suggest a valid way to rewrite it without running the "|" operator on the array.

M0stafaRady commented 1 year ago

Can you review the fix at https://github.com/RTimothyEdwards/caravel_openframe_project/pull/5 ?