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

[top,tlgen] fabric multi-clock support #903

Merged
merged 2 commits into from Dec 4, 2019

Conversation

tjaychen
Copy link

@tjaychen tjaychen commented Nov 6, 2019

Add the ability for fabric to support multiple clocks / resets
Also separate clock / reset connectivity at top.

@tjaychen
Copy link
Author

tjaychen commented Nov 6, 2019

new PR from #519

@tjaychen tjaychen force-pushed the tlul_multi_clocks branch 2 times, most recently from b4c36fa to ad71cc9 Compare November 8, 2019 23:00
@tjaychen tjaychen marked this pull request as ready for review November 8, 2019 23:04
@tjaychen tjaychen requested a review from sjgitty November 8, 2019 23:04
@tjaychen
Copy link
Author

@eunchan / @sjgitty can you have a look?

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.

Looks OK.

I am wondering if we could try multi xbar in this PR. Only concern is how the main xbar figures out the address range to the downstream (second tier crossbar). It could be manually done at this time but later the tool can recognize.

@@ -0,0 +1,334 @@
// Copyright lowRISC contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

This file shouldn't be checked in. @weicaiyang has been preparing the automatic TB wrapper for xbar test.

Copy link
Author

Choose a reason for hiding this comment

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

o is that ready? Or will @weicaiyang remove the checked in stuff?

Copy link
Contributor

Choose a reason for hiding this comment

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

The top_earlgray xbar TB is currently at hw/ip/tlul/dv, but this one is not auto-generated
When we finish the TB automation, will check in generated files in hw/top_earlgrey/dv

@tjaychen
Copy link
Author

o i actually wanted to do multi-xbar in a different PR. Basically the experiment to split up domains.
I didn't want to make this one too muddied.

@tjaychen
Copy link
Author

tjaychen commented Nov 13, 2019 via email

@weicaiyang
Copy link
Contributor

ah so should i remove in this PR

Yes, please remove it
as discussed with @eunchan, hw/top_earlgrey/dv/xbar_main_tb.sv is a simple TB. If designer wants to try some simple tests, generate it locally, but don't need to check it in.

@tjaychen
Copy link
Author

i've removed the tb file.

@tjaychen
Copy link
Author

could someone approve if this looks okay?

@sjgitty
Copy link
Contributor

sjgitty commented Nov 14, 2019 via email

Copy link
Contributor

@sjgitty sjgitty left a comment

Choose a reason for hiding this comment

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

From my understanding of what you're trying to do, this all makes sense,
so LGTMing, but I would prefer to get +1 from @eunchan as well since he's
the one whose world is changing here.

Also note this will knock us back to D1/V1 or D2/V2 for xbar (which is fine,
just we should mark that on the .prj.hjson file. @weicaiyang can do that
in a separate PR. I'll be working on the multi-entry .prj.hjson so that we can
keep the signed off one available.

@weicaiyang
Copy link
Contributor

@sjgitty

Also note this will knock us back to D1/V1 or D2/V2 for xbar (which is fine,
just we should mark that on the .prj.hjson file. @weicaiyang can do that
in a separate PR. I'll be working on the multi-entry .prj.hjson so that we can
keep the signed off one available.

Sure, will create a PR to change TLUL back to D1/V1

Copy link
Contributor

@weicaiyang weicaiyang 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
Copy link
Contributor

eunchan commented Nov 15, 2019

LGTM with comments.

I think at this moment, it is not necessary to split the clock in the crossbar yet. But this change definitely let me engage the multi crossbars in the top sooner :) I will take a look at the multi xbar implementation after this is merged. If the feature works, it can reduce many Async FIFOs.

Timothy Chen added 2 commits December 4, 2019 10:58
Add the ability for fabric to support multiple clocks and resets.

There is currently no defined relationship between clocks, so
anything that is different is treated as asynchronous. This can be
improved in the future.

Signed-off-by: Timothy Chen <timothytim@google.com>
Also add multiple resets sync'ed to each clock.  The multi-clock
and resets are still connected to the same source, a separate PR will
bring true clock and reset sources.

Signed-off-by: Timothy Chen <timothytim@google.com>
@tjaychen tjaychen merged commit 65d7425 into lowRISC:master Dec 4, 2019
@tjaychen tjaychen deleted the tlul_multi_clocks branch December 4, 2019 19:25
weicaiyang added a commit to weicaiyang/opentitan that referenced this pull request Dec 5, 2019
New feature was just added lowRISC#903
Current TB doesn't support multi-clock, use same clock to avoid failure
Signed-off-by: Weicai Yang <weicai@google.com>
weicaiyang added a commit that referenced this pull request Dec 5, 2019
New feature was just added #903
Current TB doesn't support multi-clock, use same clock to avoid failure
Signed-off-by: Weicai Yang <weicai@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants