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

AXI Memory Over TL Serial Link #812

Merged
merged 34 commits into from
Mar 23, 2021
Merged

AXI Memory Over TL Serial Link #812

merged 34 commits into from
Mar 23, 2021

Conversation

abejgonzalez
Copy link
Contributor

@abejgonzalez abejgonzalez commented Mar 3, 2021

Related issue: ucb-bar/testchipip#121

Type of change: new feature

Impact: rtl change

Release Notes
This PR adds support for simulating an AXI memory interface over the default TL serial link in Chipyard/FireSim.

  • Add WithSerialTLBackingMemory config frag. to move ExtMem params to SerialTLKey
  • New harness binders to connect the Serial Link + Passthrough Clock (for AXI clock domain)
  • New example configs to demonstrate setup (MulticlockAXIOverSerialConfig, FireSimMulticlockAXIOverSerialConfig)
  • Modified config. fragment to specify tile clocks (overall or just by hart id)
  • Split FireSim "tweaks" into 2 fragments... without clocking information and overall. This lets users ignore FireSim clock defaults if they want.
  • Add support in both Chipyard and FireSim to request a clock in the harness (and harness binder). Chipyard provides both a clock/reset while FireSim just provides a clock. Note that (AXI Memory Over TL Serial Link #812 (comment)) applies to both FireSim and Chipyard's test harnesses.

This was tested in FireSim @ 30MHz with the added config.

generators/chipyard/src/main/scala/IOBinders.scala Outdated Show resolved Hide resolved
@@ -99,6 +82,23 @@ class WithFireSimConfigTweaks extends Config(
new chipyard.config.WithNoDebug
)

// Tweaks that are generally applied to all firesim configs
class WithFireSimConfigTweaks extends Config(
// Optional*: Removing this will require adjusting the UART baud rate and
Copy link
Contributor

Choose a reason for hiding this comment

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

Like this change.
Maybe call this WithFireSimClockingTweaks, and keep the name of the old one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, its probably safer to rename the old one as well, WithFireSimDesignTweak, perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This WithFireSimConfigTweaks combines the clocking with the "design" tweaks. In order to avoid changing FireSim itself (changing the build recipes), I will at least keep this name WithFireSimConfigTweaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make a WithFireSimDefaultFrequencies or something and compose the two of them to produce the old tweaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to unresolve this.

generators/chipyard/src/main/scala/ConfigFragments.scala Outdated Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Mar 6, 2021

There is a distinction that needs to be made in the harness with these changes:

harnessClock/harnessReset =/= clock/reset in the TestHarness

In the case that the reference clock coming from ChipTop isn't the overall reference clock of the entire simulation (i.e. isn't the clock/reset coming into the TestHarness module), the harnessClock/harnessReset can differ from the implicit TestHarness clock/reset. Most of the time this won't be the case... but the current MulticlockAXIOverSerialConfig exposes this behavior. Here is a snippet of the clock summaries:

image

Here you can see that the reference clock for ChipTop is 500MHz but the reference clock for the entire system is actually 1GHz. So the implicit TestHarness clock/reset will be 1GHz while the harness<Clock/Reset> will be 500MHz.

@jerryz123
Copy link
Contributor

In the example you provide, the mem_over_serial_tl_clock is the clock for the AXI memory?

@abejgonzalez
Copy link
Contributor Author

In the example you provide, the mem_over_serial_tl_clock is the clock for the AXI memory?

Yup that's correct.

@alonamid
Copy link
Contributor

alonamid commented Mar 8, 2021

Will this setup come with a docs section under the "Advanced Concepts" section?

@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Mar 8, 2021

Will this setup come with a docs section under the "Advanced Concepts" section?

Sure. Though I think I might re-work some of the "Communicating with the DUT" section a bit instead of adding a completely new page.

Edit: Probably need to have a separate page telling people that they can request clocks in the harness...

@abejgonzalez
Copy link
Contributor Author

Ok this PR is ready for a final review. One thing to note though. If not using the load_mem feature in FireSim, loading a binary is extremely slow (have to send the binary over the serial link to the chip, then back over the serial link to DRAM). While this is slow to begin with, this is exacerbated by FESVR step size in FireSim being very very large (https://github.com/firesim/firesim/blob/7e32128bea5390ca528052dcdffb13656ee41162/sim/firesim-lib/src/main/cc/bridges/serial.cc#L20) which slows down the binary loading. A future PR can address this since this PR is fairly big to begin with.

@jerryz123
Copy link
Contributor

This LGTM, although I think there can be some more deduplication that we can do later.

Copy link
Contributor

@alonamid alonamid left a comment

Choose a reason for hiding this comment

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

LGTM (minor nit comment)

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

Looks pretty good, a couple of comments/potential issues.
Also did we already have some discussion about what to do about images and the source that generated them? If so what was the decision?

docs/Advanced-Concepts/Chip-Communication.rst Outdated Show resolved Hide resolved
docs/Advanced-Concepts/Chip-Communication.rst Outdated Show resolved Hide resolved
docs/Advanced-Concepts/Chip-Communication.rst Show resolved Hide resolved
docs/Advanced-Concepts/Chip-Communication.rst Show resolved Hide resolved
docs/Advanced-Concepts/Chip-Communication.rst Outdated Show resolved Hide resolved
docs/Advanced-Concepts/Chip-Communication.rst Outdated Show resolved Hide resolved
docs/Advanced-Concepts/Harness-Clocks.rst Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved
sims/vcs/Makefile Show resolved Hide resolved
@abejgonzalez
Copy link
Contributor Author

Also did we already have some discussion about what to do about images and the source that generated them? If so what was the decision?

I think we had one in passing. And we didn't make a definitive decision. For now, I think the sources should not be put here since Chipyard is already big (we could just give a Drive link with the relevant sources and update them there).

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

LGTM.
I think the image source size issue doesn't make sense to me as I bet the source for those pngs is smaller than the output, but I'm not willing to die on this bike shed.

sims/vcs/Makefile Show resolved Hide resolved
@abejgonzalez abejgonzalez merged commit fa275b6 into dev Mar 23, 2021
@abejgonzalez abejgonzalez deleted the offchip-axi-setup branch May 3, 2021 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants