Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Numerous bug fixes, ending in clean full LVS for both caravel and caravan. #76

Merged
merged 4 commits into from
Apr 20, 2022

Conversation

RTimothyEdwards
Copy link
Contributor

(1) Modified the .magicrc file to set a default for PDK if not set in
environment. (2) Fixed the user ID programming layout to not leave holes
behind when the script moves the vias around (similar to the handling of
the GPIO defaults block). (3) Added substrate isolation to gpio_control_block
and fixed the path references to the standard cells. (4) Fixed the four
missing routes on the Caravan top level. (5) Reinstated the large rendered
labels for the pads on both caravel and caravan. (6) Corrected the top
level gate-level netlist for caravan to add the missing pins to the
management core wrapper. (7) Did the same for the caravan top level RTL.
(8) Created scripts to run full LVS including extracting the management
core wrapper and reading all gate-level verilog submodules. (9) Moved all
of the LVS scripts to the scripts directory.

RTimothyEdwards and others added 2 commits April 19, 2022 08:41
… the

environment.  (2) Fixed the user ID programming layout to not leave holes
behind when the script moves the vias around (similar to the handling of
the GPIO defaults block).  (3) Added substrate isolation to gpio_control_block
and fixed the path references to the standard cells.  (4) Fixed the four
missing routes on the Caravan top level.  (5) Reinstated the large rendered
labels for the pads on both caravel and caravan.  (6) Corrected the top
level gate-level netlist for caravan to add the missing pins to the
management core wrapper.  (7) Did the same for the caravan top level RTL.
(8) Created scripts to run full LVS including extracting the management
core wrapper and reading all gate-level verilog submodules.  (9) Moved all
of the LVS scripts to the scripts directory.
@RTimothyEdwards
Copy link
Contributor Author

@d-m-bailey : I'd appreciate it if you could review these changes, especially around the user ID programming block.
Attn: @jeffdi

Copy link
Contributor

@d-m-bailey d-m-bailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things you might want to take a look at.

  1. In the scripts/run_carav*_lvs_full.sh files, the readnet verilog lines are all relative paths. Maybe $CARAVEL_ROOT and $MCW_ROOT would be better. I believe the management core is installed as mcw_core_wrapper by default, but the scripts reference caravel_mgmt_soc_litex.

  2. The changes to scripts/set_user_id.py have not been included, so that will crash.

@d-m-bailey
Copy link
Contributor

3 ) simple_por still uses a common ground. See #31

@RTimothyEdwards
Copy link
Contributor Author

@jeffdi , @d-m-bailey : I don't get why scripts/set_user_id.py is not changed. That was in pull request #73, which was stated to have been merged 12 hours ago. But I pulled the latest main branch and it's unchanged there. What happened?

@RTimothyEdwards
Copy link
Contributor Author

@jeffdi : Did pull request #73 get blocked by failing the CI or something? That's what's suggested in the Actions tab. That's the only explanation I can come up with. But I don't know why github claims that #73 was merged because clearly it wasn't. It needs to be merged before this pull request can be merged.

@RTimothyEdwards
Copy link
Contributor Author

@d-m-bailey : I understand that the scripts are somewhat ad hoc, but my main goal was to get both projects to show LVS clean, and then move the scripts to the scripts/ directory so that they can be standardized and made more universally useful. I can raise a separate issue to get them cleaned up, preferably with a hook to the Makefile so that one can do something like "make lvs-full" to initiate a complete LVS.

The simple POR fix is a separate issue, although since I can do the fix in probably an hour, max, it is well worth considering getting that done for this MPW.

@d-m-bailey
Copy link
Contributor

@RTimothyEdwards Haven't completed LVS yet, but these are the temporary changes I made to simple_por (for reference).
simple_por_after

@d-m-bailey
Copy link
Contributor

@RTimothyEdwards I had a problem trying to create a pull request where it didn't appear that the previous updates were included. Finally, this is what I did. mpw5-set-user-id is my local branch.

git checkout -b efabless-main mpw5-set-user-id
git pull https://github.com/efabless/caravel.git main
<fix conflicts>
git checkout mpw5-set-user-id
git merge --no-ff efabless-main
git push origin mpw5-set-user-id

@RTimothyEdwards
Copy link
Contributor Author

@d-m-bailey :
(1) Add the comments about the simple_por to issue #31
(2) Please direct comments about merging to @jeffdi , as I do not have authority to do a merge myself.

@RTimothyEdwards
Copy link
Contributor Author

@d-m-bailey : Rather than try to get Jeff to work through the merge issues, I'm just going to make the changes from PR #73 in this PR so Jeff should only need to do the one merge.

RTimothyEdwards and others added 2 commits April 19, 2022 14:40
successfully, and if merged now they will generate conflicts with
this pull request in scripts/set_user_id.py.  So it's easier to
just manually add them to this pull request.
@d-m-bailey
Copy link
Contributor

@RTimothyEdwards the merge suggestions weren't for Jeff, but for your local repo.
This is how I think it works.
You created the all_lvs_clean branch from main.
After that, the main github branch was updated and you pulled changes, but all_lvs_clean does not get updated it that case. I think you need to git merge --no-ff.

@RTimothyEdwards
Copy link
Contributor Author

@d-m-bailey : I am not well versed in any of the more obscure git options. Anyway, I see your point, but on my laptop I had pulled the main branch and I was still not seeing the changes from your PR #73. Anyway, I have merged those changes into this pull request, so they should get properly merged into main as soon as Jeff has the free time to do it.

@jeffdi jeffdi merged commit 99518ac into main Apr 20, 2022
@jeffdi jeffdi deleted the all_lvs_clean branch April 20, 2022 02:05
@antonblanchard
Copy link
Contributor

Hi @RTimothyEdwards, the removal of -v $(PWD)/..:$(PWD)/.. broke tape outs for me, was that change intentional?

diff --git a/openlane/Makefile b/openlane/Makefile
index 8fd607d..ca6b7ed 100644
--- a/openlane/Makefile
+++ b/openlane/Makefile
@@ -44,8 +44,9 @@ endif
        @if [ -f ./$*/interactive.tcl ]; then\
                docker run --rm -v $(OPENLANE_ROOT):/openlane \
                -v $(PDK_ROOT):$(PDK_ROOT) \
-               -v $(PWD)/..:$(PWD)/.. \
+               -v $(MCW_ROOT):$(MCW_ROOT) \
                -v $(CARAVEL_ROOT):$(CARAVEL_ROOT) \
+               -e MCW_ROOT=$(MCW_ROOT) \
                -e PDK_ROOT=$(PDK_ROOT) \
                -e CARAVEL_ROOT=$(CARAVEL_ROOT) \
                -e PDK=$(PDK) \
@@ -56,8 +57,9 @@ endif
        else\
                docker run --rm -v $(OPENLANE_ROOT):/openlane \
                -v $(PDK_ROOT):$(PDK_ROOT) \
-               -v $(PWD)/..:$(PWD)/.. \
                -v $(CARAVEL_ROOT):$(CARAVEL_ROOT) \
+               -v $(MCW_ROOT):$(MCW_ROOT) \
+               -e MCW_ROOT=$(MCW_ROOT) \
                -e PDK=$(PDK) \
                -e PDK_ROOT=$(PDK_ROOT) \
                -e CARAVEL_ROOT=$(CARAVEL_ROOT) \

M0stafaRady pushed a commit that referenced this pull request Sep 30, 2022
…avan. (#76)

* (1) Modified the .magicrc file to set a default for PDK if not set in the
environment.  (2) Fixed the user ID programming layout to not leave holes
behind when the script moves the vias around (similar to the handling of
the GPIO defaults block).  (3) Added substrate isolation to gpio_control_block
and fixed the path references to the standard cells.  (4) Fixed the four
missing routes on the Caravan top level.  (5) Reinstated the large rendered
labels for the pads on both caravel and caravan.  (6) Corrected the top
level gate-level netlist for caravan to add the missing pins to the
management core wrapper.  (7) Did the same for the caravan top level RTL.
(8) Created scripts to run full LVS including extracting the management
core wrapper and reading all gate-level verilog submodules.  (9) Moved all
of the LVS scripts to the scripts directory.

* Apply automatic changes to Manifest and README.rst

* Made the changes from pull request #73 as they did not get merged
successfully, and if merged now they will generate conflicts with
this pull request in scripts/set_user_id.py.  So it's easier to
just manually add them to this pull request.

* Apply automatic changes to Manifest and README.rst

Co-authored-by: RTimothyEdwards <RTimothyEdwards@users.noreply.github.com>
@RTimothyEdwards RTimothyEdwards added error Something isn't working RTL Verilog source code changed PnR Gate level verilog and/or layout changed flow Makefile or in-repository flow script changed labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error Something isn't working flow Makefile or in-repository flow script changed PnR Gate level verilog and/or layout changed RTL Verilog source code changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants