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

RFC: Enable device by using dts, not Kconfig #10621

Closed
qianfan-Zhao opened this issue Oct 16, 2018 · 10 comments
Closed

RFC: Enable device by using dts, not Kconfig #10621

qianfan-Zhao opened this issue Oct 16, 2018 · 10 comments
Assignees
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features

Comments

@qianfan-Zhao
Copy link
Collaborator

The most drivers use Kconfig to select which device compiled and which not currently. eg:
(CONFIG_GPIO_STM32_PORTA is a variable defined in Kconfig)

#ifdef CONFIG_GPIO_STM32_PORTA
GPIO_DEVICE_INIT_STM32(a, A);
#endif /* CONFIG_GPIO_STM32_PORTA */

#ifdef CONFIG_GPIO_STM32_PORTB
GPIO_DEVICE_INIT_STM32(b, B);
#endif /* CONFIG_GPIO_STM32_PORTB */

#ifdef CONFIG_GPIO_STM32_PORTC
GPIO_DEVICE_INIT_STM32(c, C);
#endif /* CONFIG_GPIO_STM32_PORTC */

And the 'status = "ok"' in dts seems useless because the drivers doesn't need the defines generated from dts. Drivers use the peripheral's base address, irq number and other informations provide by HAL.

#define GPIO_DEVICE_INIT_STM32(__suffix, __SUFFIX)			\
	GPIO_DEVICE_INIT("GPIO" #__SUFFIX, __suffix,			\
			 GPIO##__SUFFIX##_BASE, STM32_PORT##__SUFFIX,	\
			 LL_APB2_GRP1_PERIPH_AFIO |			\
			 STM32_PERIPH_GPIO##__SUFFIX,			\
			 STM32_CLOCK_BUS_GPIO)

In the above code, GPIOA_BASE,_STM32_PERIPH_GPIOA... are provided by stm32cube.

How about replace this informations by using the marcos generated from dts? Remove all Kconfig variables such as CONFIG_GPIO_STM32_PORTB, use the peripheral base address generated from dts to determined which device should compiled.

Here is a example in drivers/spi/spi-sam.c:

#define SPI_SAM_DEFINE_CONFIG(n)					\
	static const struct spi_sam_config spi_sam_config_##n = {	\
		.regs = (Spi *)CONFIG_SPI_##n##_BASE_ADDRESS,		\
		.periph_id = CONFIG_SPI_##n##_PERIPHERAL_ID,		\
		.pins = PINS_SPI##n,					\
	}

#define SPI_SAM_DEVICE_INIT(n)						\
	SPI_SAM_DEFINE_CONFIG(n);					\
	static struct spi_sam_data spi_sam_dev_data_##n = {		\
		SPI_CONTEXT_INIT_LOCK(spi_sam_dev_data_##n, ctx),	\
		SPI_CONTEXT_INIT_SYNC(spi_sam_dev_data_##n, ctx),	\
	};								\
	DEVICE_AND_API_INIT(spi_sam_##n,				\
			    CONFIG_SPI_##n##_NAME,			\
			    &spi_sam_init, &spi_sam_dev_data_##n,	\
			    &spi_sam_config_##n, POST_KERNEL,		\
			    CONFIG_SPI_INIT_PRIORITY, &spi_sam_driver_api)

#if CONFIG_SPI_0_BASE_ADDRESS
SPI_SAM_DEVICE_INIT(0);
#endif

#if CONFIG_SPI_1_BASE_ADDRESS
SPI_SAM_DEVICE_INIT(1);
#endif

So we don't need select which spi port we used in Kconfig, can use dts select spi port directly.

&spi0 {
    status = "ok";
};
@erwango
Copy link
Member

erwango commented Oct 16, 2018

Hi @qianfan-Zhao

Thanks for this proposal. Indeed STM32 GPIO driver should be moved to dts information usage.
I've raised #10629 to follow the change. You're welcome to do it if interested.

About using using dts to select instances to activate, there has been discussions about that (you could check issue #8499 which lists all the open points) already and for now decision is to keep it as Kconfig responsibility, mainly because there there is a UI available for Kconfig (menuconfig) and not for dts.

@nashif nashif added the Enhancement Changes/Updates/Additions to existing features label Oct 16, 2018
@mbolivar-nordic
Copy link
Contributor

mbolivar-nordic commented Apr 23, 2020

I'm reviving this old issue to continue discussion on how to handle per-instance Kconfigs.

I can see the following options:

  1. Keep per-instance Kconfig options. Do not do any devicetree integration at all.
  2. Keep per-instance Kconfig options whenever an SoC driver maintainer wants them. These should default to 'y' if the right node label is enabled. We cannot use DT_INST instance numbers here because their relationship to nodes is undefined. This also likely means the device driver will not use DT_INST APIs unless we do something more clever to establish this relationship.
  3. Remove per-instance Kconfig options as a general policy; let node status properties control which device instances get created.

Thoughts on these? Did I miss anything?

Edit: option 3. also relates to #19448 (which in turn relates to #24318). I'm being intentionally vague when I say "let node status properties control" -- we need to consider if going with 3. requires refinements to the way status works.

cc @carlescufi @MaureenHelm @henrikbrixandersen @erwango @mnkp @galak @anangl @pabigot @nashif

@galak
Copy link
Collaborator

galak commented Apr 23, 2020

I feel like the ship has sailed on #1. I'd argue that #2 should not be allowed and that we are moving towards #3 already.

For example - here's a rough idea for serial drivers, the majority of cases that are under 15 lines are just singletons to enable/disable the driver.

[galak@spiff zephyr]$ wc -l drivers/serial/Kconfig*
  128 drivers/serial/Kconfig
    8 drivers/serial/Kconfig.altera_jtag
   27 drivers/serial/Kconfig.cc13xx_cc26xx
   10 drivers/serial/Kconfig.cc32xx
   12 drivers/serial/Kconfig.cmsdk_apb
   11 drivers/serial/Kconfig.esp32
   14 drivers/serial/Kconfig.gecko
   13 drivers/serial/Kconfig.imx
   14 drivers/serial/Kconfig.leuart_gecko
   13 drivers/serial/Kconfig.litex
   12 drivers/serial/Kconfig.mcux
   18 drivers/serial/Kconfig.mcux_flexcomm
   18 drivers/serial/Kconfig.mcux_lpsci
   12 drivers/serial/Kconfig.mcux_lpuart
   11 drivers/serial/Kconfig.miv
   10 drivers/serial/Kconfig.msp432p4xx
   70 drivers/serial/Kconfig.native_posix
  353 drivers/serial/Kconfig.nrfx
   42 drivers/serial/Kconfig.ns16550
    7 drivers/serial/Kconfig.nsim
   23 drivers/serial/Kconfig.pl011
   23 drivers/serial/Kconfig.psoc6
  104 drivers/serial/Kconfig.rtt
   39 drivers/serial/Kconfig.rv32m1_lpuart
   19 drivers/serial/Kconfig.sam0
   72 drivers/serial/Kconfig.sifive
   39 drivers/serial/Kconfig.stellaris
   14 drivers/serial/Kconfig.stm32
   13 drivers/serial/Kconfig.uart_sam
   12 drivers/serial/Kconfig.usart_sam
   12 drivers/serial/Kconfig.xlnx

@henrikbrixandersen
Copy link
Member

I would definitely go for 3) as well.

I would like to stress, however, that it must still be possible to disable a driver (not its instances, those should be handled through devicetree) through Kconfig.

@tbursztyka
Copy link
Collaborator

I would like to stress, however, that it must still be possible to disable a driver (not its instances, those should be handled through devicetree) through Kconfig.

+1

@gmarull
Copy link
Member

gmarull commented Apr 24, 2020

I would definitely go for 3) as well.

I would like to stress, however, that it must still be possible to disable a driver (not its instances, those should be handled through devicetree) through Kconfig.

+1

@galak galak added the dev-review To be discussed in dev-review meeting label Apr 29, 2020
@galak
Copy link
Collaborator

galak commented Apr 30, 2020

DEV-REVIEW: In general no issues with the general direction here (option #3), need to deal with some issues associated with mis-matched configuration between removal of Kconfig and what dts enables, also some power mgmt issues in that we might enable things sub-optimally now (we can look to use reference count here as well as warning checks for various controllers enabled w/o any deps).

@mbolivar-nordic to add general policy to DTS docs.

@galak galak removed the dev-review To be discussed in dev-review meeting label May 7, 2020
carlescufi pushed a commit that referenced this issue Sep 3, 2020
Instantiate RTT UART instances from devicetree nodes instead of from
Kconfig symbols. While RTT is implemented using software, not hardware,
it is emulating a hardware device, and thus should be configured through
devicetree. This allows the simulated UART device to be selected via
devicetree aliases and chosen nodes.

The following devicetree snippet will instantiate RTT channels 0 and 2
as UART devices.
```
/ {
	rtt0: rtt_terminal {
		compatible = "segger,rtt-uart";
		label = "rtt_terminal";
		status = "okay";
	};

	rtt2: rtt_secondary {
		compatible = "segger,rtt-uart";
		label = "rtt_app_specific";
		status = "okay";
	};
};
```

Fixes the RTT portion of #10621.

Signed-off-by: Jordan Yates <[email protected]>
@mbolivar-nordic
Copy link
Contributor

I think we can safely call this issue closed at this point 🎉

If anyone is aware of a device driver that isn't allocating instances from devicetree, I think we should treat that as a bug or deficiency in that particular driver.

@galak
Copy link
Collaborator

galak commented May 26, 2021

If anyone is aware of a device driver that isn't allocating instances from devicetree, I think we should treat that as a bug or deficiency in that particular driver.

Fair, there are still some driver classes like IEEE 802154 that dont utilize devicetree, but I'm fine w/closing this and opening specific bugs for those drivers.

@mbolivar-nordic
Copy link
Contributor

Yeah, I've been looking at the list in #10516 to understand what's still left to do. I think that tracker covers the remaining work although a lot of drivers have moved around or gotten deleted since that initial list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests

9 participants