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

Add support for RAL generation with alternate Hjson path for AST, with other fixes #9325

Merged
merged 5 commits into from
Nov 23, 2021

Conversation

sriyerg
Copy link
Contributor

@sriyerg sriyerg commented Nov 21, 2021

Fixes #9323

Srikrishna Iyer added 2 commits November 20, 2021 21:07
- Use dashes in switches instead of underscores
- Fix a log msg

Signed-off-by: Srikrishna Iyer <sriyer@google.com>
- Kept most of the lint cleanups suggested by util/lintpy.py.
- No functional change.

Signed-off-by: Srikrishna Iyer <sriyer@google.com>
Srikrishna Iyer added 3 commits November 21, 2021 10:08
This commit was an attempt at a fix for lowRISC#9323 which was unsuccessful.
The idea was to provide per-sub-RAL block creation knobs which are
controlled at runtime instead. So for instance, the generic AST register
block within the chip RAL model could be skipped from creation at
runtime. The provided callback functions `pre|post_build_ral_settings()`
would be used to achieve this. The extended (external) chip env cfg
class could skip the generic RAL blocks for instance and maintain
separate actual RAL blocks in their env instead. Chose to keep these
updates but not execute this idea.

Signed-off-by: Srikrishna Iyer <sriyer@google.com>
- This will eventually be needed for Nuvoton to extend the base (open
source) chip env classes with the actual RAL_T type where they create
the chip RAL model with the actual AST. More type-parameterizations may
be needed, but we will start with this one.

Signed-off-by: Srikrishna Iyer <sriyer@google.com>
Followup update to lowRISC#9184 for fixing lowRISC#8207/lowRISC#9323.

- Adds support in ralgen to wire up `hjson_path` setting in FuseSoC core
file as `--hjson-path` switch to topgen when generating the chip RAL
model with a different AST spec.
- Added FuseSoC flag to skip the open source RAL generation via FuseSoC
flag that can be set in the DVSim HJson file.
- Fixes lowRISC#9323.
- See lowRISC#9323 for more details.

Signed-off-by: Srikrishna Iyer <sriyer@google.com>
Copy link
Contributor

@sha-ron sha-ron left a comment

Choose a reason for hiding this comment

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

It works thanks:)

// We apply those fixes here - please note these fixes need to be reflected in the scoreboard
protected virtual function void apply_ral_fixes();
// fix access policies & reset values
// Set pre-build RAL knobs.

Choose a reason for hiding this comment

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

any reason you didn't decide to continue with this idea?
It sounds like this approach could get rid of the dependencies the other solution has? Or at least we wouldn't need a flag?

Copy link
Contributor Author

@sriyerg sriyerg Nov 22, 2021

Choose a reason for hiding this comment

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

(+ @sha-ron, FYI)
This solution would work in the following way:

  1. In the closed env, they will use this function to set the knob create_ast = 0 (which is added in commit 3).
  2. This will remove the generic AST completely from the chip RAL. The chip RAL will contain everything else.
  3. They will create the AST RAL separately in ext chip env, outside of chip RAL.
  4. The AST RAL will need to be hooked up to TL agent in stub CPU mode so that it can use the TL sequences to read and write AST registers.
  5. The closed env now has 2 RALs that needs to be supplied to everything (automated test sequences, scoreboard etc.).
  6. Our base classes have support for multiple RALs, but we have not fully tested everything because we have never had to so far (but likely everything will work ok).

I thought having all sub-blocks including AST under one roof (i.e. a single chip RAL) would be easier to work with down the road. But yes, this would have been the fall back solution if the ralgen fix hadn't worked.

The ralgen/topgen fix feels hacky and not super well defined, but it makes the SV coding easier. The SV fix suggested here makes tooling less hacky, but may make coding a bit difficult down the road (due to multiple RAL models). I am not 100% sure which is better, but I have a mild inclination towards the latter just from the perspective of SV coding ease.

Choose a reason for hiding this comment

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

sounds good. Thanks @sriyerg
do you want to file this approach as a separate issue in case we need it?
Because it does seem like the current solution could fall over if the dependency order isn't what we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Sri that one RAL might be easier, we see in Flash/OTP cases where the 2 RALs issue some problems. But it is good to remember the second solution as backup option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, this runtime solution may not work after all. The open source chip_ral_pkg will still have ast_ral_pkg created with the open source ast.hjson, which will collide with the standalone ast_ral_pkg created in the closed source with the 'real' ast.hjson. It may work if we change the name of the 'real' ast hjson to something else, but topgen does not support it (and who knows how many cans of worms it ends up opening even if we did add support for it!).

Copy link

@tjaychen tjaychen left a comment

Choose a reason for hiding this comment

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

lgtm

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

@sriyerg sriyerg merged commit 2dc0711 into lowRISC:master Nov 23, 2021
@sriyerg sriyerg deleted the dvsim-alt-hjson branch November 23, 2021 19:13
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.

[util] ralgen connectivity to topgen
6 participants