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.

Sign up for GitHub

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

Jump to bottom

[top] Integrate ast into FPGA #8194

Merged
merged 1 commit into from
Sep 17, 2021
Merged

[top] Integrate ast into FPGA #8194

merged 1 commit into from
Sep 17, 2021

Conversation

tjaychen
Copy link

@tjaychen tjaychen commented Sep 15, 2021 •
edited
Loading

Integrate AST into fpga (cw305/cw310/nexysvideo)

@tjaychen tjaychen changed the title Ast on fpga2 [top] Integrate AST into FPGA Sep 15, 2021
@tjaychen tjaychen changed the title [top] Integrate AST into FPGA [top] Integrate ast into FPGA Sep 15, 2021
@tjaychen tjaychen marked this pull request as ready for review September 15, 2021 22:38
@tjaychen tjaychen requested a review from vogelpi September 15, 2021 22:39
@tjaychen
Copy link
Author

I need to clean-up the commits a bit, but can you guys have a look at the final result and let me know what you think?
I had to add ast into englishbreakfast...which was a bit awkward..but i think that's slightly better than the alternative of having a different clocking structure.

@tjaychen tjaychen requested a review from imphil September 15, 2021 23:51
msfschaffner
msfschaffner reviewed Sep 16, 2021
View reviewed changes
Copy link
Contributor

@msfschaffner msfschaffner left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab at this! A few comments, LGTM otherwise.

hw/top_earlgrey/ip/ast/rtl/rglts_pdm_3p3v.sv Outdated
@@ -47,7 +47,8 @@ initial begin
init_start = 1'b0;
end

always_ff @( init_start, posedge vcc_pok_h_i, negedge vcc_pok_h_i ) begin
always_ff @( posedge init_start, negedge init_start,
posedge vcc_pok_h_i, negedge vcc_pok_h_i ) begin
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to make sure that Nuvoton is aware of this change in AST...

Copy link
Author

Choose a reason for hiding this comment

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

yep, so this change is for verilator (the other pr) and @Jacob-Levy is aware of them.

hw/top_earlgrey/rtl/autogen/chip_earlgrey_nexysvideo.sv


assign ext_clk = '0;
assign pad2ast = '0;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a TODO here and below and say that this needs to be connected to the padring (once that module is instantiated at this level also?)

Copy link
Author

Choose a reason for hiding this comment

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

yeah i can do that. I actually think ext_clk should already be connected now.

hw/top_earlgrey/rtl/chip_earlgrey_verilator.sv Outdated
@@ -221,14 +451,18 @@ module chip_earlgrey_verilator (
.dio_attr_o ( ),

// Memory attributes
// This is different between verilator and the rest of the platforms right now
.ram_1p_cfg_i ('0),
.ram_2p_cfg_i ('0),
.rom_cfg_i ('0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't these technically come from AST as well?

Copy link
Author

Choose a reason for hiding this comment

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

yeah they do, i just go lazy because this is the verilator specific one that i intend to migrate into chiplevel.sv.tpl eventually.
o this is also meant for the other PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sounds good!

util/topgen/templates/chiplevel.sv.tpl Outdated Show resolved Hide resolved
util/topgen/templates/chiplevel.sv.tpl
.es_rng_rsp_i ( es_rng_rsp ),
.es_rng_fips_o ( es_rng_fips ),
.ast2pinmux_i ( ast2pinmux ),
.ast_init_done_i ( ast_init_done ),
Copy link
Contributor

Choose a reason for hiding this comment

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

just a nit: maybe align these brackets a bit...

Copy link
Author

Choose a reason for hiding this comment

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

but i like them misaligned!
yes will do.

@msfschaffner
Copy link
Contributor

msfschaffner commented Sep 16, 2021 •
edited
Loading

The related issue is here: #295 (yes, pretty old :)).
@tjaychen do you mind ticking off some of the FPGA / Verilator tasks on that issue once this has been merged?

@msfschaffner msfschaffner self-requested a review September 16, 2021 23:13
msfschaffner
msfschaffner approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@msfschaffner msfschaffner left a comment

Choose a reason for hiding this comment

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

LGTM!

eunchan
eunchan approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@eunchan eunchan left a comment

Choose a reason for hiding this comment

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

just a nit for the empty line. As verilator PR merged, this PR shouldn't be a problem in my opinion.

hw/top_earlgrey/ip/ast/ast.core Outdated
@@ -114,4 +114,3 @@ targets:
vcs:
vcs_options: [-timescale=1ns/1ps -l vcs.log]
toplevel: ast

Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to have one empty line at the end of the file AFAIK,

[top] Integrate ast into fpga
b178055 
- Integrate ast for cw305/cw310/nexysvideo
- Add ast to top_englishbreakfast for compatibility
- Remove BUFG usage inside ast to avoid cascading BUFG's

Signed-off-by: Timothy Chen <[email protected]>

[top] Auto generate

Signed-off-by: Timothy Chen <[email protected]>
@tjaychen
Copy link
Author

i've addressed the comments, merging.

@tjaychen tjaychen merged commit e441882 into lowRISC:master Sep 17, 2021
@tjaychen tjaychen deleted the ast_on_fpga2 branch September 17, 2021 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msfschaffner msfschaffner msfschaffner approved these changes

@eunchan eunchan eunchan approved these changes

@vogelpi vogelpi Awaiting requested review from vogelpi

@imphil imphil Awaiting requested review from imphil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tjaychen @msfschaffner @eunchan