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

[opentitantool] Introduce JSON config for teacup shield #20902

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

jesultra
Copy link
Contributor

The HyperDebug "shield" has a number of MUX'es, buffers and other ICs (current sensors), which can be controlled through some signals in addition to what is used when e.g. simulating OpenTitan on an FPGA.

This PR introduces a new hyperdebug_teacup.json file, to be used with --interface=hyperdebug (not hyper310). Both this file and existing FPGA-specific files refer to a common opentitan.json.

@jesultra jesultra marked this pull request as ready for review January 19, 2024 21:58
@jesultra jesultra requested a review from a team as a code owner January 19, 2024 21:58
"alias_of": "CN7_10"
},
{
"name": "SW_STRAP0_WEAK",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a _WEAK suffix for the SW straps?

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 SW straps are designed to have four different values per pin. Either strongly driven high or low, or weakly (~1MOhm) pulled up or down. On production hardware this will be achieve through physical resistors to a power rail. For simulation when connected to HyperDebug, the each SW strap pin of OpenTitan is simultaneously connected to two HyperDebug pins, one of them through a 1M resistor. That is, SW_STRAP0 and SW_STRAP0_WEAK are two separate pins on HyperDebug, and the former will be configured as either PushPull/true, PushPull/false or Input, with the last option allowing the pulling the SW strap weakly, by setting `SW_STRAP0_WEAK' either high or low.

Copy link
Contributor

@engdoreis engdoreis left a comment

Choose a reason for hiding this comment

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

To be consistent with the existing names hyper310 and hyper340 we could call the new target hyperteacup.

"alias_of": "CN10_20"
},
{
// Controls if CS signal from OT QSPI device is propagated to onboard SPI
Copy link
Contributor

Choose a reason for hiding this comment

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

json does not support comments really, should we change the file extension to json5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds as a reasonable refactoring. I am not familiar with the various supersets of json, or what extension would be the most appropriate. If there is general agreement that json5 is the extension that matches what our parser accepts, then by all means.
opentitan_verilator.json already uses // for comments, so I was hoping to merge this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, it should be a future refactoring.

@jesultra jesultra requested review from pamaury and engdoreis January 22, 2024 22:39
@jesultra
Copy link
Contributor Author

To be consistent with the existing names hyper310 and hyper340 we could call the new target hyperteacup.

My stance is that there is conceptual difference between "interface", which is kind of the debugger hardware, and then the configuration for the combined setup of debugger and device under test.

Until now, it has been a bit conflated, since the "debugger hardware" included both HyperDebug and Sam3X on either CW310 or CW340, which also fully described the device under test.

The "pure" hyperdebug debugger hardware, however, can be connected to a variety of devices to be tested. The Teacup board is the first OpenTitan test board to use the pure "hyperdebug" debugger. I foresee that the OpenTitan project in the future will have other test boards, which may feature newer revisions of the OT chip, but could also feature additional current sensors, mux'es, LEDs, connectors and other things, that require slight variations in the JSON configuration file.

The Google hardware security team already uses two other teacup-like boards on top of a pure HyperDebug, one for each of two previous generations of legacy Titan chips. We have a configuration file for each of them, but use the generic interface "hyperdebug".

In short, there will be a one-to-many relationship between interfaces and configuration files, and this Teacup-specific file should not be called "hyperdebug.json", neither do I think that we should have an interface called "hyperteacup", which would use the same exact driver as "hyperdebug", the only difference being that the default configuration file is this new hyperdebug_teacup.json. I would rather work towards build scripts that explicitly pass a --conf argument to opentitantool, instead of passing --interface, since as I describe above, there is a many-to-one relationship betwen the two, and we might as well pass a pointer to the "many" side, rather than artificially adding more entries to the "one"-side in order to try to keep up and make it one-to-one.

The HyperDebug "shield" has a number of MUX'es, buffers and other
ICs (current sensors), which can be controlled through some signals in
addition to what is used when e.g. simulating OpenTitan on an FPGA.

This PR introduces a new `hyperdebug_teacup.json` file, to be used with
`--interface=hyperdebug` (not `hyper310`).  Both this file and existing
FPGA-specific files refer to a common `opentitan.json`.

Change-Id: I15390cd3ac93a149ea65635c3c3f298e7a08cea0
Signed-off-by: Jes B. Klinke <[email protected]>
@engdoreis
Copy link
Contributor

To be consistent with the existing names hyper310 and hyper340 we could call the new target hyperteacup.

My stance is that there is conceptual difference between "interface", which is kind of the debugger hardware, and then the configuration for the combined setup of debugger and device under test.

Until now, it has been a bit conflated, since the "debugger hardware" included both HyperDebug and Sam3X on either CW310 or CW340, which also fully described the device under test.

The "pure" hyperdebug debugger hardware, however, can be connected to a variety of devices to be tested. The Teacup board is the first OpenTitan test board to use the pure "hyperdebug" debugger. I foresee that the OpenTitan project in the future will have other test boards, which may feature newer revisions of the OT chip, but could also feature additional current sensors, mux'es, LEDs, connectors and other things, that require slight variations in the JSON configuration file.

The Google hardware security team already uses two other teacup-like boards on top of a pure HyperDebug, one for each of two previous generations of legacy Titan chips. We have a configuration file for each of them, but use the generic interface "hyperdebug".

In short, there will be a one-to-many relationship between interfaces and configuration files, and this Teacup-specific file should not be called "hyperdebug.json", neither do I think that we should have an interface called "hyperteacup", which would use the same exact driver as "hyperdebug", the only difference being that the default configuration file is this new hyperdebug_teacup.json. I would rather work towards build scripts that explicitly pass a --conf argument to opentitantool, instead of passing --interface, since as I describe above, there is a many-to-one relationship betwen the two, and we might as well pass a pointer to the "many" side, rather than artificially adding more entries to the "one"-side in order to try to keep up and make it one-to-one.

Thanks for explaining that.
So from my understanding, we call hyper310 and hyper340 these names due to the use of separated transport to load bitstreams, which is not the case with hyperdebug + any shield. Therefore, the --interface maps to the transport rather than the board configuration.

@pamaury
Copy link
Contributor

pamaury commented Jan 24, 2024

I agree that calling the interface hyperdebug make sense. There are already a number of assumptions baked into opentitantool that will have to be lifted in the future anyway: the tool always assume earlgrey for example so we might need a CLI option to specify the soc. It might make sense to have a board option in the future to specify how the interface is connected to the soc (that would be teacup here).

@jesultra
Copy link
Contributor Author

jesultra commented Jan 24, 2024

So from my understanding, we call hyper310 and hyper340 these names due to the use of separated transport to load bitstreams, which is not the case with hyperdebug + any shield. Therefore, the --interface maps to the transport rather than the board configuration.

Yes exactly. It is a bit of shame that for some reason both terms "interface" and "transport" are used in the source, meaning the same thing. If done again, we should probably have called it --transport, to make it clear, and dropped the term "interface".

I agree that calling the interface hyperdebug make sense. There are already a number of assumptions baked into opentitantool that will have to be lifted in the future anyway: the tool always assume earlgrey for example so we might need a CLI option to specify the soc. It might make sense to have a board option in the future to specify how the interface is connected to the soc (that would be teacup here).

The new file in this PR has a line "interface": "hyperdebug", meaning that you can specify --conf hyperdebug_teacup.json and omit --interface=hyperdebug". I think it would make sense to later on add "soc": "earlgrey" to this file, so that all of transport, soc, as well as the details of their interconnection would be specified through a single --conf argument. We would still have --interface and a new --soc if someone wanted to manually specify everything on the command line, but I would like to move towards the default being specifying a --conf argument, and not specifying --interface. This will require some changes to BUILD files, but it should be rather mechanical for now, since hyper310 and hyper340 are one-to-one.

@jesultra jesultra merged commit 17d5a97 into lowRISC:master Jan 24, 2024
32 checks passed
@timothytrippel timothytrippel added the CherryPick:earlgrey_es_sival This PR should be cherry-picked to earlgrey_es_sival label Feb 7, 2024
Copy link

github-actions bot commented Feb 7, 2024

Successfully created backport PR for earlgrey_es_sival:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPick:earlgrey_es_sival This PR should be cherry-picked to earlgrey_es_sival
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants