-
Notifications
You must be signed in to change notification settings - Fork 7
Support multiple CSRs interface types; Improve User Guide. #95
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
Conversation
Currently supported CSR interfaces: "IOb", "AXIL", "Wishbone", "APB", "AXI". Update simulation wrapper to include iob_universal_converter to convert to the corresponding CSRs interface.
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.
Pull request overview
This pull request adds support for multiple CSR interface types (IOb, AXI-Lite, Wishbone, APB, AXI) and significantly improves documentation by adding comprehensive Doxygen comments to bare-metal driver headers and enhancing the User Guide.
Key Changes:
- Added Doxygen documentation to all bare-metal driver header files with detailed descriptions of macros, functions, and constants
- Implemented CSR interface selection via
iob_universal_converterin both the core and simulation wrapper - Enhanced User Guide with new sections for simulation, instantiation, features, and deliverables
- Updated build system to support CSR_IF parameter selection
Reviewed changes
Copilot reviewed 19 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| software/src/iob_eth_rmac.h | Added file-level doxygen documentation explaining RMAC placeholder functionality |
| software/src/iob_eth_macros.h | Converted all macro comments to doxygen format with grouped documentation sections |
| software/src/iob_eth_defines.h | Added doxygen documentation for macro functions with parameter descriptions |
| software/src/iob_eth.h | Added comprehensive doxygen function documentation with parameter and return value descriptions |
| software/src/iob_eth.c | Fixed function signature to match header declaration (removed void parameter) |
| software/src/eth_mem_map.h | Added doxygen documentation for register offset macros |
| software/src/eth_frame_struct.h | Added doxygen documentation for frame structure constants |
| iob_eth.py | Added CSR_IF parameter support and improved signal descriptions |
| hardware/simulation/iob_eth_sim/iob_eth_sim.py | Added universal converter to support multiple CSR interface types in testbench |
| document/tsrc/*.tex | Added new documentation sections for simulation, instantiation, features, and deliverables |
| document/figures/*.odg | Added binary diagram files for documentation |
| document/doc_build.mk | Added DOXYGEN and CONFIDENTIAL configuration flags |
| default.nix | Updated py2hwsw dependency version |
| Makefile | Enhanced to support CSR_IF parameter configuration |
| README.md | Fixed function signature consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "name": "inta_o", | ||
| "width": 1, | ||
| "descr": "Interrupt Output A", |
Copilot
AI
Dec 19, 2025
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 interrupt output signal description was changed from "Interrupt Output A" to "Interrupt Output", but the signal name remains "inta_o". The description in the nested signal still says "Interrupt Output A". For consistency, either both descriptions should match, or the nested signal description should be updated to match the port-level description.
| "descr": "Interrupt Output A", | |
| "descr": "Interrupt Output", |
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 inta_o signal description still mentions "Interrupt Output A".
I removed the 'A' from the description of the port (group of signals), so that 'A' does not appear in the LaTeX table description. It will only appear on the description of the signal itself, inside the table.
| ifeq ($(PY_PARAMS),) | ||
| ifneq ($(CSR_IF),) | ||
| PY_PARAMS:=$(PY_PARAMS):csr_if=$(CSR_IF) | ||
| endif | ||
| # Remove first char (:) from PY_PARAMS | ||
| PY_PARAMS:=$(shell echo $(PY_PARAMS) | cut -c2-) | ||
| endif # ifndef PY_PARAMS | ||
|
|
||
| BUILD_DIR ?= $(shell nix-shell --run "py2hwsw $(CORE) print_build_dir --py_params '$(PY_PARAMS)'") |
Copilot
AI
Dec 19, 2025
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 construction and reuse of PY_PARAMS here allows shell command injection via both CSR_IF and PY_PARAMS, since their values are interpolated unquoted into $(shell echo $(PY_PARAMS) | cut -c2-) and then into nix-shell --run as --py_params '$(PY_PARAMS)'. An attacker who can influence CSR_IF or PY_PARAMS (e.g., via environment variables or build parameters) can inject shell metacharacters such as ; or ' to execute arbitrary commands during make, potentially compromising the build environment. To fix this, avoid passing untrusted values through $(shell ...) and shell-quoted arguments; instead, pass them as environment variables or ensure they are rigorously escaped before use in shell commands.
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.
This does not apply.
Any user that runs the Makefile already has access to the system and can modify the Makefile to run any commands.
These interfaces are not currently desired for the CSR bus. They will likely still work if a user chooses them, but they will be undocumented. Update py2hwsw and prebuilt ug.pdf
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
P-Miranda
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.
Move eth components into their own python modules. This allows subblocks list of main iob_eth module to be complete, generating the UG's blocks table correctly. WIP: Eth simulation stuck
|
avoid bent lines in the block diagram such as CPU IF |
|
RX buffer and MII managemnt also have bents |
As suggested in: IObundle#95 (comment)
Remove outdated comments.
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.
User Guide seems ok.
Possible improvements include moving CDC logic to dedicated module (example: iob_eth_cdc) and remaining top level logic - like remaining iob_regs for no-auto CSRs - to another dedicated module (example: iob_eth_logic.v)
| # Synchronizers | ||
| # | ||
|
|
||
| synchronizers = { |
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.
Usually we place all CDC in a dedicated verilog module (for example iob_eth_cdc.v), instead of placing then at the top module level.
Not sure if we want to do the same for this core.
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.
This would be worth doing. The internal objective of this milestone is to train people in designing IP.
Ideally, the top should be just the modules in the block diagram, interconnected by wires.

Currently supported CSR interfaces: "IOb", "AXI-Lite", "Wishbone".