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.

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

Conversation

galak
Copy link
Collaborator

@galak galak commented Sep 11, 2018

Example of how we can remove the dep between Kconfig & dts. We still
need to deal with MCUBOOT define, but at least this shows how to deal
with the SoC defines.

Signed-off-by: Kumar Gala kumar.gala@linaro.org

Example of how we can remove the dep between Kconfig & dts.  We still
need to deal with MCUBOOT define, but at least this shows how to deal
with the SoC defines.

Signed-off-by: Kumar Gala <kumar.gala@linaro.org>
@galak galak added area: Devicetree RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Sep 11, 2018
@galak
Copy link
Collaborator Author

galak commented Sep 11, 2018

The idea here is to remove the inclusion of autoconf.h and thus needing to run Kconfig before dts. Right now we need Kconfig to get CONFIG_SOC_* and CONFIG_BOOTLOADER_MCUBOOT.

To address the CONFIG_SOC_* we can have the .dts tell us which SOC it is via the compatible in the SoC node and use that to create a define to let us pick flash/sram sizes.

So we do a two pass through the dts. First pass, we set -DCONFIG_SOC_DUMMY to create a dts with flash/sram sizes set to 0 so we can compile the dts. We than extract the compats from the top level and soc nodes and turn those into -D defines which we pass to the second pass.

@ulfalizer
Copy link
Collaborator

ulfalizer commented Sep 11, 2018

Fine by me, if it gets us closer to doing DTS before Kconfig. Not a DTS expert.

Is there some limitation to doing this by the way?

  1. You select a board (-DBOARD=...)
  2. It gets mapped to a SoC (via a simple table somewhere)
  3. The board/SoC combo is mapped to a DTS file (or files), which gets processed
  4. Kconfig runs

That would get rid of the loopiness at least. Not sure how the mapping is done now...

@@ -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.

@@ -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

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.

@galak
Copy link
Collaborator Author

galak commented Sep 12, 2018

Take a look at #9946 is how we should do this going forward.

@galak
Copy link
Collaborator Author

galak commented Sep 14, 2018

Closing this PR, as we figure this out, will at least move forward in the direction of #9946.

@galak galak closed this Sep 14, 2018
@galak galak deleted the dts-soc-hack branch September 14, 2018 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree DNM This PR should not be merged (Do Not Merge) RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants