boards: creating a pinmux library for pinmux source files #38608

tejlmand · 2021-09-16T13:46:11Z

With the driver library cleanup in #37512 all drivers are now placed in
dedicated zephyr libraries and not directly in libzephyr.a.

This commit follows up on this by placing all pinmux.c files in a
dedicated pinmux zephyr library.

Signed-off-by: Torsten Rasmussen [email protected]

With the driver library cleanup in zephyrproject-rtos#37512 all drivers are now placed in dedicated zephyr libraries and not directly in libzephyr.a. This commit follows up on this by placing all pinmux.c files in a dedicated pinmux zephyr library. Signed-off-by: Torsten Rasmussen <[email protected]>

tejlmand · 2021-09-16T13:47:01Z

this is continuation of: #38471
to avoid CI timeout.

galak

This should probably be zephyr_library_named(board) instead.

Extending zephyr_library_amend() to take an optional library name argument, like `zephyr_library_amend(foo)`. This allows the build system to amend to a zephyr library created elsewhere in the build tree in a consistent way. A use-case for such feature would be pinmux source files located both in the board folder but also with files under drivers/pinmux. By using `zephyr_library_amend(<name>)` such source files can now be placed together in the same library. Signed-off-by: Torsten Rasmussen <[email protected]>

Fixes: zephyrproject-rtos#38403 A board might already have created a pinmux driver library. If such library has already been created then amend to that library, else the zephyr library is created in the drivers/pinmux CMakeLists.txt file. This ensures that pinmux source files from the board and drivers/pinmux are placed in the same library. Furthermore this avoids empty pinmux driver libraries and thus avoids the following warning: > No SOURCES given to Zephyr library: drivers__pinmux > > Excluding target from build. Signed-off-by: Torsten Rasmussen <[email protected]>

tejlmand · 2021-09-17T09:54:59Z

This should probably be zephyr_library_named(board) instead.

@galak As I understand, all those pinmux.c are actually pinmux driver files / pinmux setup, and as such they belong together with other pinmux driver files located in drivers/pinmux.

When you viewed this PR initially, I had only taken a single commit from #38471 instead of current 3 commits.
Having all 3 commits should hopefully provide a better understanding of the purpose.

This PR ensures that all pinmux driver related files are always placed in a pinmux library.
This is especially valuable to users of IDEs that organize the source tree according to the libraries in CMake.
Today, it's hard to guess which library contains files related to pinmux.
For example on zephyr/main, if I build for:

stm32f0_disco I will have
- drivers__pinmux library containing: pinmux_stm32.c.obj
hifive1 I will have
- drivers__pinmux library containing: pinmux_sifive.c.obj
- boards__riscv__hifive1 library containing: pinmux.c.obj
mps2_an385 I will have:
- boards__arm__mps2_an385 library containing: pinmux.c.obj

Now, with this PR, what we will have instead is:

stm32f0_disco I will have
- pinmux library containing: pinmux_stm32.c.obj
hifive1 I will have
- pinmux library containing: pinmux_sifive.c.obj and pinmux.c.obj
mps2_an385 I will have:
- pinmux library containing: pinmux.c.obj

Thus pinmux always goes to a pinmux library.
And on top of that, we avoid the drivers__pinmux library being empty, and hence the Zephyr CMake warning is no longer present.

Such a cleanup is also valuable in IDEs that organize code according to the library in which they are located.
See: #8825
as it is now clearer where to find pinmux related files.

gmarull

I'm not sure if this is the right solution, please check my comments.

gmarull · 2021-09-22T10:51:59Z

boards/arm/adafruit_feather_m0_basic_proto/CMakeLists.txt

@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: Apache-2.0

 if(CONFIG_PINMUX_SAM0)
-  zephyr_library()
+  zephyr_library_named(pinmux)


just one comment: why do we need to name the library in this case? Is name relevant? The board pinmux.c files are not part of the pinmux driver/library, they are board related files that setup pinmux at system init.

that depends on how we want to solve this issue.

If creating a common library for pinmux functionality, then the best is to have a known name so that we can refer to the same library when appending files to it from different folders.

gmarull · 2021-09-22T10:57:47Z

drivers/pinmux/CMakeLists.txt

@@ -1,6 +1,11 @@
 # SPDX-License-Identifier: Apache-2.0

-zephyr_library()


this CMakeLists is only included if CONFIG_PINMUX=y, so if this library gets no sources, it means that there must be an issue somewhere else.

yes, and that issue is because several boards are defining CONFIG_PINMUX=y, even when not selecting a driver from drivers/pinmux.

Maybe because of this guide:
https://docs.zephyrproject.org/latest/guides/porting/board_porting.html?highlight=config_pinmux#create-your-board-directory

CMakeLists.txt: if you need to add additional source files to your build.

One common use for this file is to add a pinmux.c file in your board directory to the build, which configures pin > controllers at boot time. In that case, CMakeLists.txt usually looks like this:

if(CONFIG_PINMUX) zephyr_library() zephyr_library_sources(pinmux.c) endif()

so following that guide will in most cases result in the mess this PR is trying to clean up.

Now, one way of fixing that would be to avoid having CONFIG_PINMUX=y just because you add pinmux.c, and that would be identical to option 4 here:
#38608 (comment)

galak · 2021-09-22T13:01:04Z

There is option 5 which is to call the library board.

galak · 2021-09-22T13:03:03Z

Because, all boards with a pinmux.c file seems to select the PINMUX symbol, but that symbol is a pinmux driver symbol.

zephyr/drivers/pinmux/Kconfig

Lines 9 to 10 in a9a01d4

menuconfig PINMUX

bool "Enable board pinmux driver"

I'm not sure I follow here, the board code is using the pinmux driver so why shouldn't they select it in Kconfig?

tejlmand · 2021-09-24T15:34:44Z

There is option 5 which is to call the library board.

@galak no, because in that case you end up with the library defined here being empty:

zephyr/drivers/pinmux/CMakeLists.txt

Lines 3 to 19 in 2637c0e

    
           zephyr_library() 
        
           # Board initialization 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_CC13XX_CC26XX   pinmux_cc13xx_cc26xx.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_ESP32           pinmux_esp32.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_HSDK            pinmux_hsdk.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_INTEL_S1000     pinmux_intel_s1000.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_ITE_IT8XXX2     pinmux_ite_it8xxx2.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_LPC11U6X        pinmux_lpc11u6x.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_MCUX            pinmux_mcux.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_MCUX_LPC        pinmux_mcux_lpc.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_RV32M1          pinmux_rv32m1.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_SAM0            pinmux_sam0.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_SIFIVE          pinmux_sifive.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_STM32           pinmux_stm32.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_TELINK_B91      pinmux_b91.c) 
        
           zephyr_library_sources_ifdef(CONFIG_PINMUX_XEC             pinmux_mchp_xec.c)

thus generating the warning described in #38403.
This happens because the board have CONFIG_PINMUX=y but the driver is not in that library, because you want it inside the board library.
So your proposal 5 is identical to propsal 2.

Today the board library you propose is autonamed boards__arm__mps2_an385

$ ninja -t query zephyr/boards/arm/mps2_an385/libboards__arm__mps2_an385.a
ephyr/boards/arm/mps2_an385/libboards__arm__mps2_an385.a:
  input: C_STATIC_LIBRARY_LINKER__boards__arm__mps2_an385_
    zephyr/boards/arm/mps2_an385/CMakeFiles/boards__arm__mps2_an385.dir/pinmux.c.obj
....

so the proposal 5 leads us nowhere when it comes to fixing #38403.

tejlmand · 2021-09-24T15:50:22Z

I'm not sure I follow here, the board code is using the pinmux driver so why shouldn't they select it in Kconfig?

if pinmux.c is a driver it should be in the drivers library, not in a board library.

If I need to a library that provides me with certain functionality, like math, encryption, etc. I would link to a known library providing such functionality, like a math lib, or a security lib.

Similar to a pinmux driver.
If I need to link to a pinmux driver lib, then such static library should always provide me with needed functionality, regardless of the internal structure of that project.
Your proposal is like saying:

building for mps2_an385: link to board library to have pinmux functionality.
building for stm32f0_disco: link to drivers__pinmux library to have pinmux functionality.

that sounds wrong to me.

Note: I know the library structure and organization in Zephyr is messy.

gmarull · 2021-09-27T14:54:06Z

Another option we should consider is to leave pinmux as it was before the creation of libraries for drivers, i.e. revert. The reason I mention this option is that the future pinctrl API will likely replace pinmux, and pinctrl drivers are/will be designed to avoid such kind of problems.

tejlmand · 2021-09-28T08:55:40Z

Please see alternative fix: #38908

I'm fine with either solution, just not happy with a CMake warning in an LTS.

tejlmand · 2021-10-13T11:54:23Z

Closed in favor of: #38908

tejlmand added the bug The issue is a bug, or the PR is fixing a bug label Sep 16, 2021

tejlmand requested review from ABOSTM, abrodkin, dcpleung, dleach02, erwango, evgeniy-paltsev, FRASTM, fvincenzo, galak, kgugala, Mani-Sadhasivam, MaureenHelm, mmahadevan108, nandojve, parthitce, pgielda, ruuddw, sathishkuttan and vanti as code owners September 16, 2021 13:46

github-actions bot added area: Boards platform: NXP NXP labels Sep 16, 2021

dcpleung approved these changes Sep 16, 2021

View reviewed changes

galak requested changes Sep 16, 2021

View reviewed changes

tejlmand added 2 commits September 17, 2021 09:28

tejlmand requested a review from nashif as a code owner September 17, 2021 07:29

zephyrbot requested review from GTLin08, IRISZZW, jackrosenthal, katsuster, keith-zephyr, mgielda, mnkp, scottwcpg, sjg20 and VenkatKotakonda September 20, 2021 14:23

zephyrbot assigned tejlmand Sep 20, 2021

jackrosenthal approved these changes Sep 20, 2021

View reviewed changes

carlescufi requested review from gmarull and removed request for albertofloyd, andyross, brockus-zephyr, katsuster, keith-zephyr, mgielda and VenkatKotakonda September 22, 2021 10:49

gmarull reviewed Sep 22, 2021

View reviewed changes

tejlmand mentioned this pull request Sep 28, 2021

Revert "drivers: pinmux: build as static library" #38908

Merged

tejlmand closed this Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

boards: creating a pinmux library for pinmux source files #38608

boards: creating a pinmux library for pinmux source files #38608

tejlmand commented Sep 16, 2021

tejlmand commented Sep 16, 2021

galak left a comment

tejlmand commented Sep 17, 2021

gmarull left a comment

gmarull Sep 22, 2021

tejlmand Sep 24, 2021

gmarull Sep 22, 2021

tejlmand Sep 24, 2021

galak commented Sep 22, 2021

galak commented Sep 22, 2021

tejlmand commented Sep 24, 2021

tejlmand commented Sep 24, 2021

gmarull commented Sep 27, 2021

tejlmand commented Sep 28, 2021

tejlmand commented Oct 13, 2021

		@@ -1,6 +1,11 @@
		# SPDX-License-Identifier: Apache-2.0

		zephyr_library()

boards: creating a pinmux library for pinmux source files #38608

boards: creating a pinmux library for pinmux source files #38608

Conversation

tejlmand commented Sep 16, 2021

tejlmand commented Sep 16, 2021

galak left a comment

Choose a reason for hiding this comment

tejlmand commented Sep 17, 2021

gmarull left a comment

Choose a reason for hiding this comment

gmarull Sep 22, 2021

Choose a reason for hiding this comment

tejlmand Sep 24, 2021

Choose a reason for hiding this comment

gmarull Sep 22, 2021

Choose a reason for hiding this comment

tejlmand Sep 24, 2021

Choose a reason for hiding this comment

galak commented Sep 22, 2021

galak commented Sep 22, 2021

tejlmand commented Sep 24, 2021

tejlmand commented Sep 24, 2021

gmarull commented Sep 27, 2021

tejlmand commented Sep 28, 2021

tejlmand commented Oct 13, 2021