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

Test chip features #128

Merged
merged 6 commits into from
Apr 1, 2021
Merged

Test chip features #128

merged 6 commits into from
Apr 1, 2021

Conversation

jerryz123
Copy link
Contributor

@jerryz123 jerryz123 commented Mar 30, 2021

A set of useful features for testchip

  • The +init_write=<addr>:<val> argument lets the user write to arbitrary SCRs before hart0 leaves the bootrom. Useful for setting up clocks, putting things in and out of reset, etc. You can string together an arbitrary sequence of init_write flags to set up the SCRs in any way you want.
  • The bootrom now references a value stored in a BootAddrReg memory-mapped register, instead of always sending the harts to DRAM_BASE. The default is still DRAM_BASE
  • The new CustomBootPin functionality exposes a ChipTop pin that, when asserted, writes some custom address to the BootAddrReg, and then writes MSIP for hart0. This lets us simulate untethered boot, since we can redirect hart0 to secondary boot media without relying on TSI for anything.
  • The +no_hart0_msip argument prevents the tsi widget from breaking hart0 out of the bootrom by writing MSIP. When coupled with the CustomBootPin functionality, this lets us simulate untethered boot
  • TileResetCtrl control registers are restored to word-packed, as opposed to byte-packed. There is a longstanding bug in upstream riscv-fesvr where read_chunk and write_chunk does not work with sub-word accesses. https://github.com/riscv/riscv-isa-sim/blob/21684fd9b073cf9bd8f8d23cfc5f94ce361f170c/fesvr/tsi.cc#L55 . Making the ResetCtrl registers word-packed lets them be manipulated through TSI, which is useful.

Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

I think this looks nice. The one assumption that I think you are making is that you will write to all SCRs in batch. In my test chip I write SCRs in phases (I have some scripts that write SCRs to reset... wait a bit... set clocks.... wait a bit / calc stuff... set more stuff). However, this seems like a useful 1st step for simulation purposes.

src/main/resources/testchipip/csrc/testchip_tsi.cc Outdated Show resolved Hide resolved
src/main/resources/testchipip/csrc/testchip_tsi.cc Outdated Show resolved Hide resolved
src/main/resources/testchipip/csrc/testchip_tsi.cc Outdated Show resolved Hide resolved
@jerryz123 jerryz123 changed the title Add TSI feature to write arbitrary memory through command line | make tile-reset-ctrl word-packed Test chip features Mar 30, 2021
@jerryz123
Copy link
Contributor Author

I think this looks nice. The one assumption that I think you are making is that you will write to all SCRs in batch.

Yeah. For anything more complicated this simple solution breaks down. But it is useful for simulation in this form

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.

I think the init_writes feature is useful but as Abe mentioned for an actual chip it is probably insufficient and people would end up writing their own my_chip_tsi_t class or something more custom like that to solve this issue. It is worth at least mentioning that in the documentation for this new feature on what to do when this in insufficient.

The CustomBootPin feature seems to be missing the part of self-boot where the bootrom is updated to read from some off-chip storage before jumping to it. Is the idea that one would use yet-another-boot-loader(YABL) in some other ROM/RAM for the reading from off-chip storage?

My other question about CustomBootPin is why do we choose to make this a hardware state machine vs the traditional way we have done this in bootrom SW?

And finally compressing the reset regs doesn't seem very useful to me and removes some functionality.

src/main/resources/testchipip/bootrom/bootrom.S Outdated Show resolved Hide resolved
src/main/scala/BootAddrReg.scala Outdated Show resolved Hide resolved
src/main/scala/CustomBootPin.scala Outdated Show resolved Hide resolved
src/main/scala/CustomBootPin.scala Outdated Show resolved Hide resolved
src/main/scala/CustomBootPin.scala Show resolved Hide resolved
val r_tile_resets = (0 until nTiles).map({ i =>
withReset (asyncResetSinkNode.in.head._1.reset) {
Module(new AsyncResetRegVec(w=1, init=(if (params.initResetHarts.contains(i)) 1 else 0)))
}
})
node.regmap((0 until nTiles).map({ i =>
i -> Seq(RegField.rwReg(1, r_tile_resets(i).io)),
i * 4 -> Seq(RegField.rwReg(1, r_tile_resets(i).io)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This change still seems silly to me.
The only benefit seems to be the user doesn't need to know that state of the cores to affect a change, i.e. they can just write a 0 or 1 to a specific word without reading first or keeping of track of which cores are in reset.
The down side is the user cannot bring multiple cores out of reset at once. This can be problematic if the user has cores that are designed to come up together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that byte-wide writes through TSI don't work is the real motivation for this change. This just seems safer for now, as its more intuitive to do a series of word-writes, instead of doing byte-mask manipulation.

Also if some cores are designed to come up together with reset, then some other widget should be used instead of this one. Even if we byte-packed them, this widget wouldn't support bringing them out of reset together if their hartids are more than 8 apart, or not aligned properly.

@jerryz123
Copy link
Contributor Author

jerryz123 commented Mar 30, 2021

Is the idea that one would use yet-another-boot-loader(YABL) in some other ROM/RAM for the reading from off-chip storage?

Yes. This functionality gives us the flexibility to put YABL off-chip as well, instead of on a on-chip ROM.
We could even have TSI set it to an arbitrary address other than DRAM_BASE, if, for-example, parts of DRAM were broken and inaccessible.

My other question about CustomBootPin is why do we choose to make this a hardware state machine vs the traditional way we have done this in bootrom SW?

One benefit is that the bootrom code can stay the same, regardless of whether or not this pin is present. Instead of having the bootrom check if this pin is set, have this pin control when hart0 wakes up and where it tries to boot from.

Also the HW state machine for this is very simple.

Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

This seems fine. I rather have SW do all of this... but the fact of the matter is that the BootROM and this are the "same" in the sense that they are both "hardcoded". This should give a bit more flexibility while allowing the BootROM to stay the same-ish.

src/main/scala/BootAddrReg.scala Outdated Show resolved Hide resolved
src/main/scala/BootAddrReg.scala Show resolved Hide resolved
src/main/scala/CustomBootPin.scala Outdated Show resolved Hide resolved
@jerryz123
Copy link
Contributor Author

This has all been tested by me using these features to simulate various boot sequences.

Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

LGTM on the condition that you post an issue or track somewhere that we need to document all tapeout items (I'm pulling a bit of an Alon here).

@jerryz123
Copy link
Contributor Author

Issue for docs opened here: ucb-bar/chipyard#844

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.

I'm ok with this. I'd prefer software for the boot pin detection but don't care that much given the small size of the state machine.

@jerryz123 jerryz123 merged commit 89b9d66 into master Apr 1, 2021
@jerryz123 jerryz123 deleted the fesvr-init-writes branch April 19, 2023 18:43
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.

3 participants