drivers: syscon: Set default for CONFIG_SYSCON to y

[yperess]

Currently for large projects with many boards, using the syscon driver
is a bit of a pain. It has the convinience of using dt_compat_enabled
for CONFIG_GENERIC_SYSCON but that only works if CONFIG_SYSCON is
also enabled.

Set the default for CONFIG_SYSCON=y so that the driver can be enabled
by simply setting the devicetree node to status = "okay";

Signed-off-by: Yuval Peress [email protected]

[yperess]

For reference, https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3177749 is the change that integrates syscon. If we don't have this default value for CONFIG_SYSCON then we would need to also make changes to all the boards under zephyr/boards/arm AND make sure that every new board also has this in the _defconfig. It's not impossible, but seems that the driver is already gated by the compatible string so there's no need to have double gating.

[henrikbrixandersen]

How about using simple changing the default for CONFIG_SYSCON to y in the soc/<arch>/<soc>/Kconfig.defconfig.series for the SoCs containing a SYSCON device?

[carlocaione]

This is ugly. What about using something like

default y if $(dt_compat_enabled,$(DT_COMPAT_SYSCON))

or something like that

[yperess]

My initial approach was actually to collapse the two into a single config SYSCON (which in a way is very similar to what you said @carlocaione). It would remove SYSCON_GENERIC since it's basically redundant and move the default dt_compat_enabled up to the SYSCON config, it would look something like:

DT_COMPAT_SYSCON := syscon

menuconfig SYSCON
	bool "SYSCON (System Controller) drivers"
	default $(dt_compat_enabled,$(DT_COMPAT_SYSCON))
	help
	  SYSCON (System Controller) drivers. System controller node represents
	  a register region containing a set of miscellaneous registers. The
	  registers are not cohesive enough to represent as any specific type
	  of device. The typical use-case is for some other node's driver, or
	  platform-specific code, to acquire a reference to the syscon node and
	  extract information from there.
if SYSCON

module = SYSCON
module-str = syscon
source "subsys/logging/Kconfig.template.log_config"

config SYSCON_INIT_PRIORITY

Would that work for you? I was thinking it was too big of a change and decided to go with the minimal approach originally.

[henrikbrixandersen]

Would that work for you? I was thinking it was too big of a change and decided to go with the minimal approach originally.

This will require changes if we bring in a SoC specific SYSCON driver in the future. I think we need to keep the "top-level" Kconfig for enabling SYSCON support and then have the drivers underneath that (as it is today).

[yperess]

Would that work for you? I was thinking it was too big of a change and decided to go with the minimal approach originally.

This will require changes if we bring in a SoC specific SYSCON driver in the future. I think we need to keep the "top-level" Kconfig for enabling SYSCON support and then have the drivers underneath that (as it is today).

I see what you're saying, I would argue that CONFIG_SYSCON then is obsolete. We could do something similar to the logging template where CONFIG_SYSCON is enabled automatically if CONFIG_SYSCON_GENERIC is enabled (eventually other implementations). Should I update the PR so it's easier to see?

[henrikbrixandersen]

I see what you're saying, I would argue that CONFIG_SYSCON then is obsolete. We could do something similar to the logging template where CONFIG_SYSCON is enabled automatically if CONFIG_SYSCON_GENERIC is enabled (eventually other implementations). Should I update the PR so it's easier to see?

As far as I can tell, this would go against how all other drivers (except maybe interrupt controllers, where we recently had issues due to this exact construct (no top-level gating, see #37910 (comment))) handles Kconfig.

[yperess]

Ping, I went ahead with the original suggestion of defaulting the CONFIG_SYSCON the same as CONFIG_GENERIC_SYSCON. Thoughts?

[tejlmand]

Make sense to default the syscon drivers menu based on the devicetree settings.
Downstream users with custom drivers can do similar, and use zephyr_library_amend() to place a custom driver in the lib.

Just a minor nit.

[yperess]

Accidentally force pushed the wrong commit, all better now :)

[henrikbrixandersen]

My initial comment still stands:

How about using simple changing the default for CONFIG_SYSCON to y in the soc/<arch>/<soc>/Kconfig.defconfig.series for the SoCs containing a SYSCON device?

Why does this need to be handled differently than all other drivers classes?

[tejlmand]

How about using simple changing the default for CONFIG_SYSCON to y in the soc/<arch>/<soc>/Kconfig.defconfig.series for the SoCs containing a SYSCON device?

Why does this need to be handled differently than all other drivers classes?

But wouldn't that result in the same issue that @mbolivar-nordic tries to cleanup here ?
#38510

Having a config SYSCON\r default y in board series Kconfig will result in one of those two things:

• The driver being enabled always for that board, even when not needed
  see Some boards enable non-minimal peripherals by default #30694
• SYSCON is enabled, but no driver is placed in that lib
  resulting in: Cleanup No SOURCES given to Zephyr library warnings  #38403

Maybe we need to rethink how we enable drivers in general and where we place them in libraries.
Does each driver needs its own library if that library contains only a single file ?

[carlocaione]

While I agree with @henrikbrixandersen I wanted to point out that something similar is already used in zephyr, see https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/i2c/Kconfig.stm32#L7

[yperess]

While I agree with @henrikbrixandersen I wanted to point out that something similar is already used in zephyr, see https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/i2c/Kconfig.stm32#L7

I think there's precedence for most different ways to enable things. I would propose that we create an issue to discuss this and once there are several comments on it we can bring it up at the weekly dev meeting?

[yperess]

Ping

[carlocaione]

I think there's precedence for most different ways to enable things. I would propose that we create an issue to discuss this and once there are several comments on it we can bring it up at the weekly dev meeting?

Feel free to bring this to the next dev-meeting. Add a label for that in this case.

[yperess]

I think there's precedence for most different ways to enable things. I would propose that we create an issue to discuss this and once there are several comments on it we can bring it up at the weekly dev meeting?

Feel free to bring this to the next dev-meeting. Add a label for that in this case.

Done

[yperess]

Discussed at dev meeting, no longer needed.