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.

Sign up for GitHub

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

[DNM][RFC] dts: Get SoC from dts and not from kconfig generated autoconfig.h #9925

Closed
wants to merge 1 commit into from
Closed

[DNM][RFC] dts: Get SoC from dts and not from kconfig generated autoconfig.h #9925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions boards/arm/disco_l475_iot1/disco_l475_iot1.dts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@
};
};

&soc {
compatible = "STM32L475XG", "simple-bus";
Copy link
Member

Choose a reason for hiding this comment

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

We already have :

compatible = "st,stm32f207zg-nucleo", "st,stm32f207";

Can't we re-use that instead?

Copy link
Member

Choose a reason for hiding this comment

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

If aim is to add information in boards.dts in order to remove usage of DT_SRAM_SIZE and DT_FLASH_SIZE,
can't we just add matching information :

/ {
	sram0: memory@20000000 {
		reg = <0x20000000 0x10000>;
	};

	soc {
		flash-controller@40023c00 {
			flash0: flash@8000000 {
				reg = <0x08000000 0x40000>;
			};
		};
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

@erwango

You may have a look at the extended flash property extraction to EDTS in 733ecb0 of #5799.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I guess I'll go with the direct route of having stm32f207zg.dtsi and including that in the board.dts.

Copy link
Member

Choose a reason for hiding this comment

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

I was hoping we could do that in the board and spare the stm32f207zg to avoid multiplication of dtsi file. Can't we ?

Copy link
Member

@erwango erwango Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Like done in linux (https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/stm32f429-disco.dts):

	memory {
		reg = <0x90000000 0x800000>;
	};

So we keep compatibility..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to avoid having boards that use the same SoC having to duplicate that info. While we could do it in the board.dts I think having a SoC specific .dtsi is the cleanest.

};

&usart1 {
current-speed = <115200>;
pinctrl-0 = <&usart1_pins_a>;
Expand Down
59 changes: 58 additions & 1 deletion cmake/dts.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,67 @@ if(CONFIG_HAS_DTS)
-isystem ${ZEPHYR_BASE}/include
-isystem ${ZEPHYR_BASE}/dts/${ARCH}
-isystem ${ZEPHYR_BASE}/dts
-include ${AUTOCONF_H}
${DTC_INCLUDE_FLAG_FOR_DTS} # include the DTS source and overlays
-I${ZEPHYR_BASE}/dts/common
-I${ZEPHYR_BASE}/drivers
-DCONFIG_SOC_DUMMY
-undef -D__DTS__
-P
-E ${ZEPHYR_BASE}/misc/empty_file.c
-o ${BOARD}.dts.pre.tmp
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
RESULT_VARIABLE ret
)
if(NOT "${ret}" STREQUAL "0")
message(FATAL_ERROR "command failed with return code: ${ret}")
endif()

# Run the DTC on *.dts.pre.tmp to create the intermediary file *.dts_compiled
execute_process(
COMMAND ${DTC}
-O dts
-o ${BOARD}.dts_compiled
-b 0
${BOARD}.dts.pre.tmp
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
RESULT_VARIABLE ret
)
if(NOT "${ret}" STREQUAL "0")
message(FATAL_ERROR "command failed with return code: ${ret}")
endif()

set(CMD_EARLY_EXTRACT_COMPAT ${PYTHON_EXECUTABLE}
${ZEPHYR_BASE}/scripts/dts/extract_early_compat.py ${BOARD}.dts_compiled)

# Run extract_dts_includes.py to create a .conf and a header file that can be
# included into the CMake namespace
execute_process(
COMMAND ${CMD_EARLY_EXTRACT_COMPAT}
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
OUTPUT_VARIABLE COMPAT_DEFINES
RESULT_VARIABLE ret
OUTPUT_STRIP_TRAILING_WHITESPACE
)
if(NOT "${ret}" STREQUAL "0")
message(FATAL_ERROR "command failed with return code: ${ret}")
endif()
separate_arguments(COMPAT_DEFINES)

# Run the C preprocessor on an empty C source file that has one or
# more DTS source files -include'd into it to create the
# intermediary file *.dts.pre.tmp
execute_process(
COMMAND ${CMAKE_C_COMPILER}
-x assembler-with-cpp
-nostdinc
-I${ZEPHYR_BASE}/arch/${ARCH}/soc
-isystem ${ZEPHYR_BASE}/include
-isystem ${ZEPHYR_BASE}/dts/${ARCH}
-isystem ${ZEPHYR_BASE}/dts
${DTC_INCLUDE_FLAG_FOR_DTS} # include the DTS source and overlays
-I${ZEPHYR_BASE}/dts/common
-I${ZEPHYR_BASE}/drivers
${COMPAT_DEFINES}
-undef -D__DTS__
-P
-E ${ZEPHYR_BASE}/misc/empty_file.c
Expand Down
5 changes: 4 additions & 1 deletion dts/arm/st/mem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@
#elif defined(CONFIG_SOC_STM32L073XZ)
#define DT_FLASH_SIZE __SIZE_K(192)
#define DT_SRAM_SIZE __SIZE_K(20)
#elif defined(CONFIG_SOC_STM32L475XG)
#elif defined(CONFIG_SOC_STM32L475XG) || defined(SOC_STM32L475XG)
#define DT_FLASH_SIZE __SIZE_K(1024)
#define DT_SRAM_SIZE __SIZE_K(96)
#elif defined(CONFIG_SOC_STM32L476XG)
Expand All @@ -124,6 +124,9 @@
#elif defined(CONFIG_SOC_STM32L433XC)
#define DT_FLASH_SIZE __SIZE_K(256)
#define DT_SRAM_SIZE __SIZE_K(64)
#elif defined(CONFIG_SOC_DUMMY)
Copy link
Contributor

Choose a reason for hiding this comment

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

From a quick read, this approach feels kind of ad hoc:

  • it's specific to SoC management
  • it seems to introduce extra defines we need to test for
  • it's got yet another "extract" script which handles certain keys in DTS in magic ways that users won't understand
  • it seems to require special magic in a bunch of places to handle the CONFIG_SOC_DUMMY step

As discussed on IRC, the only reason we are doing C preprocessor checks in our application DTS overlays is specifically to handle the MCUboot partitions, which I don't think belong in DT in the first place.

There's also this example here of defining memory sizes based on the SoC. But I don't see why those defines can't be generated from the SoC compatible itself in a clean and generic way -- for example, if the YAML binding language was extended so you could generate defines with the right sizes based on which SoC compatible was specified or chosen (not sure if I'm getting the DT lingo right here, but you get what I mean).

So from my perspective, some more generic bindings and solving the MCUboot problem by moving flash partitions to Kconfig defaults would potentially mean there's no need for ifdeffery in the SOC dts overlays at all, and this sort of multi-pass approach wouldn't be needed.

Copy link
Member

Choose a reason for hiding this comment

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

From a quick read, this approach feels kind of ad hoc

I thought that was intended to be ad-hoc, this PR is just to illustrate the discussion we had on IRC. I agree on all the points you list above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's also this example here of defining memory sizes based on the SoC. But I don't see why those defines can't be generated from the SoC compatible itself in a clean and generic way -- for example, if the YAML binding language was extended so you could generate defines with the right sizes based on which SoC compatible was specified or chosen (not sure if I'm getting the DT lingo right here, but you get what I mean).

Not sure I follow what you are suggesting. Where would we encode the memory & flash sizes? The proper way to handle this is to have different DTS for each SoC variant.

So yes, this is specific to SoC handling for flash & sram sizes.

We should be avoiding #ifdef in DTS, its not typically the proper way to handle things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow what you are suggesting. Where would we encode the memory & flash sizes? The proper way to handle this is to have different DTS for each SoC variant.

So yes, this is specific to SoC handling for flash & sram sizes.

We should be avoiding #ifdef in DTS, its not typically the proper way to handle things.

I'm confused about the purpose of this RFC if the goal is to get rid of ifdef in DTS, as this introduces additional preprocessor logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm confused about the purpose of this RFC if the goal is to get rid of ifdef in DTS, as this introduces additional preprocessor logic.

take a look at #9946 to see how I plan on going forward. The idea was to remove the need for autoconf.h

Copy link
Contributor

Choose a reason for hiding this comment

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

take a look at #9946 to see how I plan on going forward. The idea was to remove the need for autoconf.h

That one removes some CONFIG_SOC_xxx ifdeffery by adding SoC-specific dtsi files which the board files have to include now, deleting files like dts/arm/ti/mem.h entirely. This PR, on the other hand, adds additional logic to the very same type of mem.h file.

So I don't get how they are related or complementary, sorry; what am I missing? They seem to conflict, actually.

I also don't follow how or if 9946 points out a way forward for users who are doing CONFIG_SOC_xxx ifdeffery in their dts overlays today to do the same thing in a future where DT is run before kconfig.

#define DT_FLASH_SIZE __SIZE_K(0)
#define DT_SRAM_SIZE __SIZE_K(0)
#else
#error "Flash, RAM, and CCM sizes not defined for this chip"
#endif
Expand Down
2 changes: 1 addition & 1 deletion dts/arm/st/stm32l475.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include <st/stm32l4.dtsi>

/ {
soc {
soc: soc {
pinctrl: pin-controller@48000000 {

gpiod: gpio@48000c00 {
Expand Down
55 changes: 55 additions & 0 deletions scripts/dts/extract_early_compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#!/usr/bin/env python3
#
# Copyright (c) 2017, Linaro Limited
# Copyright (c) 2018, Bobby Noelte
#
# SPDX-License-Identifier: Apache-2.0
#

# vim: ai:ts=4:sw=4

import sys
import pprint
from devicetree import parse_file

def convert_string_to_label(s):
# Transmute ,-@/ to _
s = s.replace("-", "_")
s = s.replace(",", "_")
s = s.replace("@", "_")
s = s.replace("/", "_")
# Uppercase the string
s = s.upper()
return s

def main(args):
if len(args) == 1:
print('Usage: %s filename.dts' % args[0])
return 1

with open(args[1], "r") as fd:
dts = parse_file(fd)
root_compat = dts['/']['props']['compatible']
soc_compat = dts['/']['children']['soc']['props']['compatible']

if not isinstance(soc_compat, list):
soc_compat = [soc_compat]

if not isinstance(root_compat, list):
root_compat = [root_compat]

for k in root_compat:
if k != 'simple-bus':
sys.stdout.write('-D%s ' % convert_string_to_label(k))

for k in soc_compat:
if k != 'simple-bus':
sys.stdout.write('-DSOC_%s ' % convert_string_to_label(k))

sys.stdout.write('\n')
sys.stdout.flush()

return 0

if __name__ == '__main__':
sys.exit(main(sys.argv))