CMake refactoring for system DT #55293

mbolivar-nordic · 2023-03-01T01:03:27Z

Nothing in here directly adds system DT support; it's all just cleanups and rearranging things to make adding it easier.

No functional changes expected.

tejlmand

Thanks for taking a look a cleaning up this.

A first look through the changes raised some questions / comments to address.
Will take a 2nd look at some of the specifics later.

cmake/modules/generated_file_directories.cmake

cmake/modules/pre_dt.cmake

cmake/modules/extensions.cmake

nordicjm

Minor change request

cmake/modules/dts.cmake

mbolivar-nordic · 2023-03-28T19:34:42Z

@nordicjm fixed, thanks!

The name of each commented section should match the name in the "table of contents", for consistency and so people can jump from contents to implementations more easily with their editors' search functions. Signed-off-by: Martí Bolívar <[email protected]>

Due to Hyrum's Law, there are users out in the wild depending on this directory existing to place their own generated files, so it'd break things unnecessarily to not do this if we don't have a DTS, as explained in a source code comment. However, this doesn't really have anything to do with DTS processing, so split it into its own module to separate concerns. This isn't really a CMake module in the usual sense of something that defines functions you can use, and is therefore a form of technical debt. The decision was made to accept this because fixing this is a larger task for the files in cmake/modules/, since there are multiple other examples of this in here. This also paves the way for inserting another module in between the generated_file_directories and dts modules that itself depends on these directories existing. Signed-off-by: Martí Bolívar <[email protected]>

Create a separate module that sets up all our devicetree handling, by setting up common variables that would apply to any and all DT processing. This is then included in the regular dts module that we include in zephyr_default.cmake. The separation of this code from dts.cmake is groundwork for enabling system devicetree in Zephyr, which will need the same definitions included into its scope. Signed-off-by: Martí Bolívar <[email protected]>

When the build system was being split up into modules under cmake/modules, most of the resulting cmake modules had their inputs and outputs documented in top-of-file comments. The dts module is an exception, which makes it harder to use since its contracts aren't defined. Fix this by adding a contract. Signed-off-by: Martí Bolívar <[email protected]>

This list processing procedure will be useful elsewhere, so prep for not repeating ourselves. Put it in a new zephyr_list() function whose signature has room to grow if we keep adding list processing extensions. Signed-off-by: Martí Bolívar <[email protected]>

Tighten up the interface boundaries by adding an extension function that separates *how* we preprocess the DTS from *what* DTS files we are preprocessing. This involves a functional change to the pre_dt module. This is useful cleanup but is also groundwork for relying on this helper function when adding system devicetree support. Signed-off-by: Martí Bolívar <[email protected]>

nordicjm · 2023-04-11T08:08:52Z

cmake/modules/dts.cmake

-  if(i EQUAL 0)
-    message(STATUS "Found BOARD.dts: ${dts_file}")
-  else()
+  if(dts_file MATCHES ".*[.]overlay$")


What if someone provides a file with a different file extension? What if someone on windows provides a file with the extension in CAPS LOCK mode?

tejlmand

sorry for a late discovery of changed printing behavior.

I believe it is important to always print dts files that we will be using.
Only printing the files in certain conditions can lead to user confusions / concerns that files are not processed, even though they are.

tejlmand · 2023-04-11T08:41:32Z

cmake/modules/dts.cmake

-  if(i EQUAL 0)
-    message(STATUS "Found BOARD.dts: ${dts_file}")
-  else()
+  if(dts_file MATCHES ".*[.]overlay$")


this actually changes printing behavior.

Existing on zephyr/main, if you specify -DDTS_SOURCE=<path>/foo.dts, the following is printed:

-- Found BOARD.dts: /<path>/foo.dts

but with those changes, nothing is printed when specifying: -DDTS_SOURCE= because this line only prints for files ending with .overlay.
and board file is now printed here when DTS_SOURCE is not given by caller:

zephyr/cmake/modules/dts.cmake

Line 131 in 668ded1

message(STATUS "Found BOARD.dts: ${DTS_SOURCE}")

especially is system-dt and sysbuild is expected in future to generate and use DTS_SOURCE I believe that it is important that the list of dts_files being used are always printed, as they are today.
Also for the case when the files specified by the caller is not ending in .overlay cause the files are still used by the build system.

tejlmand

proposal to adjust printing by simply checking dts_file against list content.

This minimize the need for bookeeping and remove the risk of flaws if lists are passed using DTS_SOURCE.

tejlmand · 2023-04-12T07:54:31Z

cmake/modules/dts.cmake

+set(BOARD_DTS ${BOARD_DIR}/${BOARD}.dts)
+set(FOUND_BOARD_DTS FALSE)
+set(HAD_DTS_SOURCE FALSE)
+if(DEFINED DTS_SOURCE)


technically this changes behavior as it is now possible to pass a list of files and not just a single file.
The old if(EXISTS ...) code fails if a list is passed as it can only check existence of a single file / folder.

but when looking at our docs then DTS_SOURCE is not described, so I wonder if this is in something we should worry about.
Do we plan to pass a list of files in future ?

Another thing to notice is that this test later will fail if a list was provided:

zephyr/cmake/modules/dts.cmake

Line 176 in 753c5fd

if(HAD_DTS_SOURCE AND ("${dts_file}" STREQUAL "${ORIGINAL_DTS_SOURCE}"))

tejlmand · 2023-04-12T08:07:58Z

cmake/modules/dts.cmake

+  if(FOUND_BOARD_DTS AND ("${dts_file}" STREQUAL "${BOARD_DTS}"))
+    continue()                  # We already printed a message above.
  endif()
-
-  math(EXPR i "${i}+1")
+  if(HAD_DTS_SOURCE AND ("${dts_file}" STREQUAL "${ORIGINAL_DTS_SOURCE}"))
+    continue()
+  endif()


We know that above code would have printed any file present in DTS_SOURCE list.

So why not simply skip if the dts_file is part of that list, and avoid all the other book keeping.

Suggested change

if(FOUND_BOARD_DTS AND ("${dts_file}" STREQUAL "${BOARD_DTS}"))

continue() # We already printed a message above.

endif()

math(EXPR i "${i}+1")

if(HAD_DTS_SOURCE AND ("${dts_file}" STREQUAL "${ORIGINAL_DTS_SOURCE}"))

continue()

endif()

if(dts_file IN_LIST DTS_SOURCE)

# Any file present in DTS_SOURCE has already be printed above

continue()

endif

tejlmand · 2023-04-12T08:18:36Z

cmake/modules/dts.cmake

+    if(BOARD_REVISION AND EXISTS ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)
+      list(APPEND DTS_SOURCE ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)


If adding a message print here, then we can remove all the extra book keeping as any file present in DTS_SOURCE has been print when reaching code below:

Suggested change

if(BOARD_REVISION AND EXISTS ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)

list(APPEND DTS_SOURCE ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)

set(board_rev_overlay ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)

if(BOARD_REVISION AND EXISTS ${board_rev_overlay})

message(STATUS "Found devicetree overlay: ${board_rev_overlay}")

list(APPEND DTS_SOURCE ${board_rev_overlay})

mbolivar-nordic · 2023-04-12T08:30:49Z

@tejlmand I just dropped the last patch since it's causing controversy. My goal here is just to isolate non-controversial patches that can go in now. We'll deal with printing in the system DT support PRs.

mbolivar-nordic requested review from tejlmand and nashif as code owners March 1, 2023 01:03

mbolivar-nordic requested a review from nordicjm March 1, 2023 01:03

zephyrbot added the area: Build System label Mar 1, 2023

zephyrbot requested a review from jeremybettis March 1, 2023 01:03

zephyrbot assigned tejlmand Mar 1, 2023

mbolivar-nordic force-pushed the cmake-dts-prep-for-sysdt-20230228 branch from 4f2f745 to 55792b2 Compare March 1, 2023 04:27

mbolivar-nordic mentioned this pull request Mar 1, 2023

[RFC] System devicetree demo for mps2 an521 board #52272

Closed

tejlmand requested changes Mar 1, 2023

View reviewed changes

cmake/modules/generated_file_directories.cmake Show resolved Hide resolved

cmake/modules/pre_dt.cmake Show resolved Hide resolved

cmake/modules/extensions.cmake Outdated Show resolved Hide resolved

nordicjm requested changes Mar 28, 2023

View reviewed changes

cmake/modules/dts.cmake Outdated Show resolved Hide resolved

mbolivar-nordic force-pushed the cmake-dts-prep-for-sysdt-20230228 branch from 55792b2 to b5ae998 Compare March 28, 2023 19:34

nordicjm previously approved these changes Mar 29, 2023

View reviewed changes

mbolivar-nordic added 4 commits April 10, 2023 19:10

mbolivar-nordic dismissed nordicjm’s stale review via caf15af April 11, 2023 02:29

mbolivar-nordic force-pushed the cmake-dts-prep-for-sysdt-20230228 branch 2 times, most recently from caf15af to 668ded1 Compare April 11, 2023 02:43

mbolivar-nordic added 2 commits April 10, 2023 19:43

nordicjm reviewed Apr 11, 2023

View reviewed changes

tejlmand requested changes Apr 11, 2023

View reviewed changes

mbolivar-nordic force-pushed the cmake-dts-prep-for-sysdt-20230228 branch from 668ded1 to 753c5fd Compare April 12, 2023 05:44

nordicjm previously approved these changes Apr 12, 2023

View reviewed changes

tejlmand requested changes Apr 12, 2023

View reviewed changes

mbolivar-nordic dismissed nordicjm’s stale review via a089d60 April 12, 2023 08:30

mbolivar-nordic force-pushed the cmake-dts-prep-for-sysdt-20230228 branch from 753c5fd to a089d60 Compare April 12, 2023 08:30

tejlmand approved these changes Apr 12, 2023

View reviewed changes

nordicjm approved these changes Apr 12, 2023

View reviewed changes

carlescufi merged commit d395719 into zephyrproject-rtos:main Apr 12, 2023

tejlmand mentioned this pull request May 25, 2023

sysbuild: generate .config.sysbuild for controlling build settings #55871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMake refactoring for system DT #55293

CMake refactoring for system DT #55293

mbolivar-nordic commented Mar 1, 2023 •

edited

Loading

tejlmand left a comment

nordicjm left a comment

mbolivar-nordic commented Mar 28, 2023

nordicjm Apr 11, 2023

tejlmand left a comment

tejlmand Apr 11, 2023

tejlmand left a comment

tejlmand Apr 12, 2023

tejlmand Apr 12, 2023

tejlmand Apr 12, 2023

mbolivar-nordic commented Apr 12, 2023

		if(BOARD_REVISION AND EXISTS ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)
		list(APPEND DTS_SOURCE ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)

-    if(BOARD_REVISION AND EXISTS ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)
-      list(APPEND DTS_SOURCE ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)
+    set(board_rev_overlay ${BOARD_DIR}/${BOARD}_${BOARD_REVISION_STRING}.overlay)
+    if(BOARD_REVISION AND EXISTS ${board_rev_overlay})
+      message(STATUS "Found devicetree overlay: ${board_rev_overlay}")
+      list(APPEND DTS_SOURCE ${board_rev_overlay})

CMake refactoring for system DT #55293

CMake refactoring for system DT #55293

Conversation

mbolivar-nordic commented Mar 1, 2023 • edited Loading

tejlmand left a comment

Choose a reason for hiding this comment

nordicjm left a comment

Choose a reason for hiding this comment

mbolivar-nordic commented Mar 28, 2023

nordicjm Apr 11, 2023

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Apr 11, 2023

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Apr 12, 2023

Choose a reason for hiding this comment

tejlmand Apr 12, 2023

Choose a reason for hiding this comment

tejlmand Apr 12, 2023

Choose a reason for hiding this comment

mbolivar-nordic commented Apr 12, 2023

mbolivar-nordic commented Mar 1, 2023 •

edited

Loading