[opentitan] Integrate OpenTitan Peripherals #11

viv-eth · 2024-11-29T11:13:32Z

This PR introduces support for OpenTitan peripherals and adjusts the build system to improve integration and build consistency.

OpenTitan Integration and Build System Improvements

Integration of OpenTitan Peripherals
- OpenTitan is now added as an external project using sparse checkout. This method ensures that only the necessary files are pulled from the repository, reducing overhead.
- When the repository already exists locally, adding new sources to the SPARSE_PATTERNS will only fetch the additional files, avoiding a full repository download. Take care to ensure everything is up to date.
- ⚠️ Warning: When adding new sources, ensure OPENTITAN_SOURCES and SPARSE_PATTERNS are updated accordingly.
Compilation and Linking
- The pulled sources are compiled into a public library and linked against the runtime.

Scheremo

Left some comments - quite a bit in opentitan.cmake doesn't intuitively make sense to me, maybe you can elaborate. IMO what you're doing in there is a mix of installation / setup and build flow. I would take this apart and only keep the build flow in the build flow.

CMakeLists.txt

Scheremo · 2024-12-17T10:12:29Z

cmake/opentitan.cmake

+ExternalProject_Add(
+  opentitan
+  DOWNLOAD_COMMAND
+    git clone --no-checkout https://github.com/lowRISC/opentitan.git ${OPENTITAN_DIR}
+    COMMAND git -C ${OPENTITAN_DIR} config core.sparseCheckout true
+    COMMAND ${CMAKE_COMMAND} -E copy ${SPARSE_CHECKOUT_FILE} ${OPENTITAN_DIR}/.git/info/sparse-checkout
+    COMMAND git -C ${OPENTITAN_DIR} checkout master
+  UPDATE_COMMAND ""  # Disable updates to avoid unexpected changes
+  CONFIGURE_COMMAND ""  # No configure step needed
+  BUILD_COMMAND ""  # No build step needed
+  INSTALL_COMMAND ""  # No install step needed
+  SOURCE_DIR ${OPENTITAN_DIR}
+)


I'd strongly suggest picking a commit hash / version number that corresponds to the RTL groundtruth. Most likely should be set in targets.

It makes perfect sense; I am now cloning it based on a target-specific commit hash. C.f. b0f9d16.

Scheremo · 2024-12-17T10:12:59Z

cmake/opentitan.cmake

+if(EXISTS ${OPENTITAN_DIR})
+  file(REMOVE_RECURSE ${OPENTITAN_DIR})
+endif()
+
+set(SPARSE_CHECKOUT_FILE ${CMAKE_CURRENT_BINARY_DIR}/sparse-checkout)
+
+set(SPARSE_PATTERNS
+  "util/*"
+  "hw/ip/*"
+  "sw/device/lib/base/*"
+  "sw/device/lib/base/internal/*"
+  "sw/device/lib/dif/*"
+  "sw/device/lib/dif/autogen/*"
+)
+
+file(WRITE ${SPARSE_CHECKOUT_FILE} "")
+foreach(PATTERN ${SPARSE_PATTERNS})
+  file(APPEND ${SPARSE_CHECKOUT_FILE} "${PATTERN}\n")
+endforeach()


Why do this?

View comment below.

Scheremo · 2024-12-17T10:15:19Z

cmake/opentitan.cmake

+add_custom_command(
+  OUTPUT ${UART_REGS_H}
+  COMMAND ${Python3_EXECUTABLE} ${REGTOOL_PY} ${UART_HJSON} -D > ${UART_REGS_H}
+  DEPENDS ${REGTOOL_PY} ${UART_HJSON}
+  COMMENT "Generating uart_regs.h"
+)
+
+add_custom_target(
+  generate_uart_regs ALL
+  DEPENDS ${UART_REGS_H}
+)


Can we externalize this? I would do this in a setup procedure, not the main build flow.

Thanks for the feedback! I see the concern about keeping the uart_regs.h generation within the main build flow, and I’d like to clarify why this step is currently integrated and how externalizing it might affect the workflow.

The uart_regs.h is necessary for building the opentitan_lib. Without it, the library cannot be compiled. Externalizing this process entirely (e.g., as a manual pre-build step) would introduce additional steps for developers, like running a separate script before invoking the main build. Or maybe I am missing the point of your comment 😅

If you’re suggesting a more decoupled approach (e.g., fully externalizing the process to a pre-build script or tool), let me know what you envision.

Would you prefer having a dedicated setup.sh or similar to prepare the repository before building?

Or is the suggestion to make the build system more modular while retaining the automation?

Let me know what you think @Scheremo .

Seems like a perfect use case for a makefile, or an installation step in your cmake flow imo. Redownloading the entire repo and building everything from scratch requires an internet connection, but it's not really necessary. I'd add a very simple setup step (note: makefile, not shell script) if necessary to generate these headers. Another fairly reasonable option is to check in generated headers in a patch.

Another gripe of mine is that CMake is not meant to run python scripts, and having it do that makes it much harder to understand what's going on. Debugging my C build flow should not entail debugging Python. The installation process, however, is a different story :)

Scheremo · 2024-12-17T10:16:11Z

cmake/opentitan.cmake

+add_dependencies(generate_uart_regs opentitan)
+
+ExternalProject_Get_Property(opentitan SOURCE_DIR)
+
+file(GLOB_RECURSE OPENTITAN_SOURCES
+    "${OPENTITAN_SW_DIR}/device/lib/base/*.c"
+    "${OPENTITAN_SW_DIR}/device/lib/dif/*.c"
+    "${OPENTITAN_SW_DIR}/device/lib/dif/autogen/*.c"
+    "${OPENTITAN_SW_DIR}/device/lib/runtime/*.c"
+)
+
+list(FILTER OPENTITAN_SOURCES EXCLUDE REGEX ".*crc32.*")
+list(FILTER OPENTITAN_SOURCES EXCLUDE REGEX ".*hardened.*")


There is some brute-forcing going on here. What's up with crc32 and hardened?

I’ve noticed that the opentitan repository includes a considerable number of files that are either unnecessary or unsupported on our platforms. As you might recall from my discussion with Pirmin from OpenTitan, he suggested using the OT_PLATFORM_RV32 flag for 32-bit RISC-V platforms. However, this flag also enables crc32 instructions (cyclic redundancy check), which are part of the Bitmanip extensions that our toolchain does not support.

Given this, I don’t see the benefit of including the entire repository at this stage. Doing so introduces some drawbacks, such as:

Slowing down the cloning process and the building of opentitan_lib.

Including files with instructions that we currently cannot support.

For now, I believe it makes sense to keep the repository to a minimal set of essential files. As we progress, we can always revisit and add more files if the need arises.

Definitely a discussion worthy of a comment - if I see these stray lines of filtering I start to wonder :)

Point 1, however, should be mute, as built files should not be rebuilt (read: I would set up an installation and reuse things that can be reused).
Point 2 is fair enough. Adding it here is a decent choice imo, and I am a bit dubious whether this approach will scale to targets with different peripheral configurations - the drivers to be used are target-specific after all.

Scheremo · 2024-12-17T10:17:11Z

cmake/opentitan.cmake

+target_link_libraries(opentitan_lib
+    PUBLIC
+    runtime
+)


Not sure this is not the other way around... What's the thought process?

Indeed! This is addressed in a2974ab.

Xeratec

Looks good in principle but I suggest a different folder structure. Additionally, unfortunately, the build flow only works for the chimera-open target as the opentitan.cmake is only included there, but drivers/CMakeLists.txt runtime_host is always linked against opentitan_lib. My suggestions should also fix this issue.

I suggest a more centralized folder structure where you put the opentitan.cmake inside drivers/opentitan, rename it to CMakeLists.txt.
Then the only thing a target needs to do is to add_subfolder(opentitan) (handled via the DRIVER_MAPPINGS). In this case, you then can clone the external source files into drivers/opentitan/third_party).

I also just saw that the cloned files are not included in the .gitignore. Please make sure that no external files will be accidentally check in.

Xeratec · 2024-12-19T13:05:03Z

cmake/opentitan.cmake

+  message(FATAL_ERROR "Please set a commit hash in your target.")
+endif()
+
+message(STATUS "[OT Drivers] Using OpenTitan commit hash: ${OPENTITAN_COMMIT_HASH}")


NIT, but please make sure the message aligns with the other ones.
Also, I would use a [CHIMERA-SDK] tag to be consistent and avoid polluting the messages. This way all messages from the SDK can be easily spotted.

Xeratec · 2024-12-19T13:07:15Z

drivers/CMakeLists.txt

+# VIVIANEP: Link to the opentitan library
+target_link_libraries(runtime_host
+    PUBLIC
+    opentitan_lib
+)


This is the wrong place to do this. Please look at the summary comment.

Xeratec · 2024-12-19T13:08:11Z

cmake/opentitan.cmake

I would move this file into drivers/opentitan. Please look at the summary comment.

Xeratec · 2024-12-19T13:10:04Z

targets/chimera-open/CMakeLists.txt

+
+# VIVIANEP: Select target-specific commit hash of the OT lib
+set(TARGET_OPENTITAN_COMMIT_HASH 22168a3d42febfca725b36dd0d1554b68cf5783e)
+
+set(OPENTITAN_COMMIT_HASH ${TARGET_OPENTITAN_COMMIT_HASH})
+
+# VIVIANEP: Add OpenTitan as an external project
+include(${CMAKE_SOURCE_DIR}/cmake/opentitan.cmake)
+


This is the wrong place to do this. Please look at the summary comment.

viv-eth mentioned this pull request Dec 2, 2024

Snitch Cluster Offloading #13

Merged

viv-eth requested review from Scheremo and Xeratec and removed request for Scheremo December 2, 2024 15:54

viv-eth marked this pull request as draft December 3, 2024 16:05

Scheremo reviewed Dec 17, 2024

View reviewed changes

Xeratec assigned viv-eth Dec 17, 2024

Xeratec added the enhancement New feature or request label Dec 17, 2024

viv-eth added 9 commits December 19, 2024 11:56

[toolchain] Move to LLVM toolchain to support OpenTitan peripherals

2fbef9f

[drivers] Add OpenTitan as external project to SDK

fac4a51

[drivers] Sparse checkout of OpenTitan sources

941360a

[drivers] Compile OpenTitan sources into STATIC lib

dd00c19

[opentitan] Remove superfluous CXX Standard flag

98f1f64

[cmake] Remove duplicate lines

6c3b394

[drivers] Improve OpenTitan Integration and Adjust Build System

97c0487

[drivers] Recursively add .c files to OpenTitan sources

858b0d9

[drivers] Link OT drivers against host runtime

2c298d9

viv-eth force-pushed the vivianep/opentitan-peripherals branch from d7f9aab to 2c298d9 Compare December 19, 2024 11:00

viv-eth added 3 commits December 19, 2024 12:24

[OT] remove superfluous dependency

a2974ab

[OT] Select commit hash on target-level for OT drivers

b0f9d16

[cmake] Remove OT drivers from top level

f149ee4

Xeratec requested changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[opentitan] Integrate OpenTitan Peripherals #11

[opentitan] Integrate OpenTitan Peripherals #11

viv-eth commented Nov 29, 2024 •

edited

Loading

Scheremo left a comment •

edited

Loading

Scheremo Dec 17, 2024

viv-eth Dec 19, 2024

Scheremo Dec 17, 2024

viv-eth Dec 19, 2024

Scheremo Dec 17, 2024

viv-eth Dec 19, 2024

Scheremo Dec 19, 2024 •

edited

Loading

Scheremo Dec 17, 2024

viv-eth Dec 19, 2024

Scheremo Dec 19, 2024

Scheremo Dec 17, 2024

viv-eth Dec 19, 2024 •

edited

Loading

Xeratec left a comment •

edited

Loading

Xeratec Dec 19, 2024

Xeratec Dec 19, 2024

Xeratec Dec 19, 2024

Xeratec Dec 19, 2024

[opentitan] Integrate OpenTitan Peripherals #11

Are you sure you want to change the base?

[opentitan] Integrate OpenTitan Peripherals #11

Conversation

viv-eth commented Nov 29, 2024 • edited Loading

OpenTitan Integration and Build System Improvements

Scheremo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scheremo Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viv-eth Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Xeratec left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viv-eth commented Nov 29, 2024 •

edited

Loading

Scheremo left a comment •

edited

Loading

Scheremo Dec 19, 2024 •

edited

Loading

viv-eth Dec 19, 2024 •

edited

Loading

Xeratec left a comment •

edited

Loading