-
Notifications
You must be signed in to change notification settings - Fork 14
[top_darjeeling_no_ibex] Generate Ibex-less version of top_darjeeling #30
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
base: master
Are you sure you want to change the base?
Conversation
davidschrammel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a first set of comments based on reviewing the changes in this PR and comparing the top_darjeeliing_no_ibex files to top_darjeeling. But I have not (yet) tried to instantiate/lint/test any of this.
| name: "cpu_dump", | ||
| act: "rcv", | ||
| package: "rv_core_ibex_pkg", | ||
| % if topname == "darjeeling_no_ibex": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more generic way to do that without referring to the topname?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without modifying topgen.py. I can add an argument to pass before topgen renders an IP template using ipgen (for e.g. including the list of modules from the top, so that it can be checked if 'rv_core_ibex' is in top["modules"]).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took out the if statement. You're right; if the default is set empty it will always default to that when Ibex is taken out instead of generating a signal from the rv_core_ibex_pkg.
| load("//hw/ip/uart:defs.bzl", "UART") | ||
|
|
||
| <<<<<<< HEAD | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this isn't intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not. Removed as of now.
util/topgen.py
Outdated
| 'alert_handler') is not None | ||
| with_alert_handler = has_ibex and has_alert_handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the has_ibex necessary here and what are its implications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just removes the alert_handler_pkg dependency for the reset manager when there is no ibex. Used to fix a linting issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a use case for an ibex-less top that still has an alert_handler. Is there a problem with getting this to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so; this fix was made specifically because of a linting issue where a package could not be found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But please don't change the semantics of with_alert_handler: its meaning is whether or not there is an alert_handler module instantiated. What was the lint error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint error corresponded to the one related to reset manager:
#### Flow Errors
%Error: src/lowrisc_systems_top_darjeeling_no_ibex_0.1/rtl/autogen/top_darjeeling_no_ibex.sv:1367:37: Can't find definition of scope/variable: 'CPU_CRASH_DUMP_DEFAULT'
ERROR: %Warning-VARHIDDEN: src/lowrisc_constants_top_darjeeling_ibex_pmp_reset_pkg_0/rtl/ibex_pmp_reset_pkg.sv:12:24: Declaration of signal hides declaration in upper scope: 'PmpCfgRst'
ERROR: Failed to build lowrisc:systems:top_darjeeling_no_ibex:0.1 : '['make', 'lint-only']' exited with an error: 2
I currently realized that the change in the rstmgr.hjson.tpl was the one that removed this error; that being said, this falls under the issue regarding taking out crash dumps and alerts when an ibex is not present. Removed.
topgen.out
Outdated
| @@ -0,0 +1,344 @@ | |||
| OrderedDict([('name', 'rv_core_ibex.corei'), ('type', 'host'), ('addr_space', 'hart'), ('clock', 'clk_main_i'), ('reset', 'rst_main_ni'), ('pipeline', False), ('xbar', False), ('stub', False), ('inst_type', ''), ('req_fifo_pass', True), ('rsp_fifo_pass', True)]) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this file coming from? Is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was used for debugging. Removed as of now.
| @@ -0,0 +1,137 @@ | |||
| // Copyright lowRISC contributors (OpenTitan project). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of changes in the hw/top_darjeeling directory. I'm not sure they are intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to topgen.out, a lot of it came from experimentation using topgen to generate two tops within the same directory. Removed as of now.
| space, i.e. ROM, main SRAM, and mbx SRAM are excluded but retention SRAM or | ||
| spi_device are included. | ||
| ''' | ||
| nodes: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to the top_darjeeling.hjson rom_ctrl0.regs and rom_ctrl1.regs is missing here. This is unexpected to me. Also the mbx*.core are missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They only exists as nodes in the address space for rv_dm.sba. They can be taken out as they don't perform any other functions in top_darjeeling_no_ibex.
Then again, as mentioned in another comment, if there needs to be communication with these devices externally (from another rv_dm), another TL-UL host port needs to be generated that allows for that.
| // For device accesses to memories (ram / rom / flash), performance is a concern, | ||
| // so use pipeline false where permissible by timing. If not, find a combination | ||
| // that works. | ||
| nodes: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to top_darjeeling's version there's a lot of unexpected removals. Why is that? (See also my comment on top_darjeeling_no_ibex.hjson)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this commit, the modules removed were not just rv_core_ibex but several other Ibex-related modules, such as rv_dm. Once these two are removed, topgen.py will not allow the generation of any "empty" device nodes; in other words, if a device such as mbx_pcie0.core is only connected to rv_dm as a host, and rv_dm is removed, then mbx_pcie0.core has no reason to exist (else, topgen will have an address range error).
A discussion would need to be had about what the host for these nodes would be, and how would they be wired up. If, say, the debug manager of a regular top_darjeeling is to communicate as a host for these devices, then an "empty" host TL-UL port must be generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference in this discussion, the following were removed in this file:
csrng
dma
edn0
edn1
entropy_src
mbx_jtag.core
mbx_pcie0.core
mbx_pcie1.core
mbx0.core
mbx1.core
mbx2.core
mbx3.core
mbx4.core
mbx5.core
mbx6.core
rom_ctrl0.regs
rom_ctrl0.rom
rom_ctrl1.regs
rom_ctrl1.rom
rv_core_ibex.cfg
rv_core_ibex.cored
rv_core_ibex.corei
rv_dm.mem
rv_dm.regs
rv_dm.sba
rv_plic
soc_proxy.core
sram_ctrl_main.regs
sram_ctrl_mbox.regs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the listed IPs should probably be removed from top_darjeeling_no_ibex.hjson.
However it would be great if the plic could still exist to collect all of the top-internal interrupts. csrng/edn/entropy_src need to be discussed whether other IPs still need that and if those IPs make sense without an ibex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the corresponding IPs would probably not be necessary if we want them to continue to communicate (e.g. dma.host is still connected to otbn; the mailboxes are connected to sram_ctrl_mbox.ram).
The collection for the interrupts makes sense; I believe the best solution then will be to reintroduce the rv_plic module and include its rv_plic.irq and rv_plic.msip as externals.
| @@ -0,0 +1,23 @@ | |||
| // Copyright lowRISC contributors (OpenTitan project). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised to see rv_core_ibex and rv_plic in this ip_autogen directory here. Where they auto-generated? If so, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not auto-generated from the current HJSON. These were generated from previous iterations that still had rv_core_ibex or rv_plic in some form. Removed.
bef13b6 to
1f73c2d
Compare
|
@LouisTheLuis You might want to delete the |
1f73c2d to
2e1d920
Compare
Oops, got it removed. |
2dd6cf3 to
1d424f1
Compare
…TL-UL connections This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
…TL-UL connections This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
| name: "cpu_dump", | ||
| act: "rcv", | ||
| package: "rv_core_ibex_pkg", | ||
| default: " ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this default? Frankly, we seem to be dancing around these dumps: I think we should open an issue to completely remove the input crash dump and corresponding CSRs based on topgen. We should also open an issue to completely remove the corresponding alert dump artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on previous discussions; this is a fix to be made on topgen. Removed the default statement.
| @@ -0,0 +1,10 @@ | |||
| # Copyright lowRISC contributors (OpenTitan project). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add extra line with zeroRISC copyright, here and in any other file you change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device - `inter_signal_list`, used for defining the TL-UL signal. For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
f9331f3 to
868255c
Compare
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
This commit builds upon a previous PR (zerorisc#30). Introduced some attributes that allow for an external TL-UL port to be added to a particular top. For a `host` port, it requires defining the external device (e.g. `top_darjeeling_no_ibex`) as a module in the top configuration file; include the device in the address space for the top and include the following additional fields: - `external`, indicating that it is a external device - `external_size`, indicating the byte size of the address space of the device For a `device` port, adding the node in the corresponding `xbar` with the `external` field will generate the corresponding port connection.
fd97d20 to
327ea23
Compare
This PR generates a new top named `top_darjeeling_no_ibex` which has the following modules removed from both the main instantiation as well as the crossbar connections: - `rv_core_ibex` - `rv_timer` - `rv_dm` - `rv_plic` The auto-generated files corresponding to this new top can be found within the `/hw/top_darjeeling_no_ibex` directory. Signed-off-by: Luis Martinez <luism@zerorisc.com>
327ea23 to
c3e0584
Compare
This PR generates a new top named
top_darjeeling_no_ibexwhich has the following modules removed from both the main instantiation as well as the crossbar connections:rv_core_ibexrv_timerrv_dmrv_plicThe auto-generated files corresponding to this new top can be found within the
/hw/top_darjeeling_no_ibexdirectory and can be generated through the Makefile at the top of the repo, usingmake -C hw top_and_cmdgen.