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: cmake: add nrf_pin_report target #24132

Conversation

mbolivar-nordic
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic commented Apr 6, 2020

This target prints the devicetree's pin map to standard out. It only works for Nordic SoCs.

This is just an RFC for now because I'm playing dirty tricks, but it's working so far:

$ west build -b nrf52840dk_nrf52840 -s samples/sensor/bme280/ -t nrf_pin_report
[snip]
-- west build: running target nrf_pin_report
[snip]
===== =========================== ========
Pin   User                        Property
===== =========================== ========
P0.05 uart0 (/soc/uart@40002000)  rts-pin 
P0.06 uart0 (/soc/uart@40002000)  tx-pin  
P0.07 uart0 (/soc/uart@40002000)  cts-pin 
P0.08 uart0 (/soc/uart@40002000)  rx-pin  
P0.11 button0 (/buttons/button_0) gpios   
P0.12 button1 (/buttons/button_1) gpios   
----- --------------------------- --------
P0.13 pwm0 (/soc/pwm@4001c000)    ch0-pin 
      led0 (/leds/led_0)          gpios   
----- --------------------------- --------
P0.14 led1 (/leds/led_1)          gpios   
P0.15 led2 (/leds/led_2)          gpios   
P0.16 led3 (/leds/led_3)          gpios   
P0.19 qspi (/soc/qspi@40029000)   sck-pin 
P0.24 button2 (/buttons/button_2) gpios   
P0.25 button3 (/buttons/button_3) gpios   
P0.26 i2c0 (/soc/i2c@40003000)    sda-pin 
P0.27 i2c0 (/soc/i2c@40003000)    scl-pin 
P0.30 spi1 (/soc/spi@40004000)    mosi-pin
P0.31 spi1 (/soc/spi@40004000)    sck-pin 
P1.01 uart1 (/soc/uart@40028000)  rx-pin  
P1.02 uart1 (/soc/uart@40028000)  tx-pin  
P1.08 spi1 (/soc/spi@40004000)    miso-pin
P1.12 spi3 (/soc/spi@4002f000)    cs-gpios
P1.13 spi3 (/soc/spi@4002f000)    mosi-pin
P1.14 spi3 (/soc/spi@4002f000)    miso-pin
P1.15 spi3 (/soc/spi@4002f000)    sck-pin 
===== =========================== ========

I hate hunting for pins, and I hate having to grep zephyr.dts to look for conflicts, so this is useful enough for me to maintain out of tree for myself. But I thought I'd share in case we can find a way to mainline this if it's interesting to others.

@mbolivar-nordic
Copy link
Contributor Author

@galak @nashif please ignore for now, just a Nordic thing.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 6, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@mbolivar-nordic mbolivar-nordic force-pushed the nrf-pin-report branch 2 times, most recently from e06dc44 to 8f4c14c Compare April 6, 2020 23:51
@mbolivar-nordic
Copy link
Contributor Author

Tweaked the output a bit. Updated the description.

@tejlmand what do you think about putting the cmake target into soc/arm/nordic_nrf/CMakeLists.txt or so?

Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Think we should find a better location for the CMake code.

Comment on lines +249 to +259
add_custom_target(
nrf_pin_report
DEPENDS
${ZEPHYR_BASE}/scripts/dts/nrf_pin_report.py
COMMAND
${PYTHON_EXECUTABLE}
${ZEPHYR_BASE}/scripts/dts/nrf_pin_report.py
--dts ${ZEPHYR_DTS}
--bindings-dirs ${DTS_ROOT_BINDINGS}
USES_TERMINAL
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is too nRF specific at current stage, to be a build target we want on all boards.
For example, having a ninja nrf_pin_report if my board is sam4s_explained is a bit weird.

Could we place this code in a dedicated cmake file, and only include it for nRF boards ?

Similar to how boards/common/nrfjprog.board.cmake is included ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit: somehow I hit the GitHub keyboard shortcut for 'send' too soon

I think this is too nRF specific at current stage, to be a build target we want on all boards.

To be clear, this is just an RFC to show what we can do for Nordic.

That is why I wrote this in the description:

This is just an RFC for now because I'm playing dirty tricks

And for emphasis, I do not want to solve this for other boards. Nordic SoCs are the only ones whose devicetrees are used in this way to encode pin mux information and there are a lot of changes going on in other SoCs in this regard right now, so it's not stable.

This is why I named the target nrf_pin_report instead of something else.

Now, the question is, is this worth having in mainline, just for Nordic SoCs and properly scoped and organized, at all, or not?

FInding what others think about the answer to this question is the purpose of the RFC.

If "yes", we can work on cleaning it up. If "no", I will continue to maintain this out of tree. But I've had users ask me how they can figure out pin maps and pin conflicts, so I personally think this is generally useful.

@tejlmand
Copy link
Collaborator

tejlmand commented Apr 7, 2020

@tejlmand what do you think about putting the cmake target into soc/arm/nordic_nrf/CMakeLists.txt or so?

think this is more a board thing, and we already have board specific cmake files in boards/common so that might be a better place.

@mbolivar-nordic
Copy link
Contributor Author

For sure, none of the code is in the right place. I just wanted to get something working for an RFC.

This is more an SoC thing than a board thing, though, isn't it?

The SoC pins depend just as much on the application as the board. But the possible pins are determined by the SoC, not board or application.

@tejlmand
Copy link
Collaborator

tejlmand commented Apr 8, 2020

For sure, none of the code is in the right place. I just wanted to get something working for an RFC.

yeah, and I think it's a great start.

This is more an SoC thing than a board thing, though, isn't it?

The SoC pins depend just as much on the application as the board. But the possible pins are determined by the SoC, not board or application.

This highly depends on perspective.
The SoC itself is highly versatile when it comes to pin configuration, but the actual routing on the board is for sure a limiting factor on the configuration.
As example, the nRF53840 dk has four leds on the board, routed to P0.13-p0.16, so configuration wise, it doesn't make sense to configure P0.11 as a led.
So from configuration perspective, the board place a vital role.

However, reading out the actual configuration, then I agree, this is a SoC thing, cause in that case, you don't care on what is possible. You care about actual configuration.

Which brings up next part. Do we want to divide the config perspective from the reading perspective ?

I think both SoC and board can be reasoned to be the right choice, and have no strong opinion on one over the other.

Now, the question is, is this worth having in mainline, just for Nordic SoCs and properly scoped and organized, at all, or not?

I think it is worth having this in mainline.
Especially as it can act as inspiration, or even be extended to cover other boards.
I haven't looked the details of the implementation, but as it parses dts files, I assume it is should be fairly simple to extend to cover any board, right ?
If not, I'm happy with the nRF name, and then it can be made more generic in future.

One concern I do have, is that this only covers dts.
For nRF SoCs, those are very flexible so you can reassing pins to other peripheral run-time, in which case the dts only shows part of the truth.

@pabigot
Copy link
Collaborator

pabigot commented Apr 8, 2020

I think it's worth having (though it's not something I've specifically looked for in Zephyr yet), and it should start as Nordic-specific because I suspect users of pinmux-based SOCs will need a very different sort of information.

@galak
Copy link
Collaborator

galak commented Apr 8, 2020

I'd rather see us produce something like this that can be used on the majority of boards/SoCs. I think having a 'port' & 'pin' concept in EDT would be more generic and we could than work up a tool across multiple SoCs.

@mbolivar-nordic
Copy link
Contributor Author

mbolivar-nordic commented Apr 8, 2020

I'd rather see us produce something like this that can be used on the majority of boards/SoCs. I think having a 'port' & 'pin' concept in EDT would be more generic and we could than work up a tool across multiple SoCs.

That's a worthy long term goal, for sure, but I don't see that as being a blocker for getting something like this enabled for SoCs that support it today.

We can always rework it in the future so nrf_pin_report runs a generic soc_pin_report target and prints a warning that it's deprecated, once soc_pin_report is available.

@mbolivar-nordic
Copy link
Contributor Author

I haven't looked the details of the implementation, but as it parses dts files, I assume it is should be fairly simple to extend to cover any board, right ?

You'd think so, but it isn't simple at all to do this for other SoCs.

People have been working on DTS-based pinmux for other platforms for years; it's still ongoing work. Nordic's hardware design makes it uniquely easy to do pinmux with existing DTS infrastructure.

Hopefully we will start to see code for other platforms getting merged within the next release or two, but I don't want to wait for that if Nordic is able to do this now, especially since we can have this as example code / a source of inspiration for others.

If not, I'm happy with the nRF name, and then it can be made more generic in future.

Thanks.

One concern I do have, is that this only covers dts.
For nRF SoCs, those are very flexible so you can reassing pins to other peripheral run-time, in which case the dts only shows part of the truth.

There is simply no way to predict what an application will do at runtime. This is purely meant to be a check of what the build-time pinmux is.

@tejlmand
Copy link
Collaborator

tejlmand commented Apr 8, 2020

I haven't looked the details of the implementation, but as it parses dts files, I assume it is should be fairly simple to extend to cover any board, right ?

You'd think so, but it isn't simple at all to do this for other SoCs.

Then i'm happy with the proposal.

If not, I'm happy with the nRF name, and then it can be made more generic in future.

Thanks.

The nRF name, it is.

One concern I do have, is that this only covers dts.
For nRF SoCs, those are very flexible so you can reassing pins to other peripheral run-time, in which case the dts only shows part of the truth.

There is simply no way to predict what an application will do at runtime. This is purely meant to be a check of what the build-time pinmux is.

Exactly. And those participating in this review knows that.
My (small) concern is other users thinking this is a nice command, but fails to realize that this is not the truth in case the app does all the pin handling in c-code.

But this does not reduce the value of the command.
It's more a small concern, and a hint that we should think about how to present the pin-report to users, so they are also aware of this fact.

@mbolivar-nordic
Copy link
Contributor Author

It's more a small concern, and a hint that we should think about how to present the pin-report to users, so they are also aware of this fact.

Sure, I can make the output say something like "Build-time pin report" or so -- will that address the concern?

@github-actions
Copy link

github-actions bot commented Jul 1, 2020

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

This target prints the devicetree's pin map to standard out.
It only works for Nordic SoCs.

Signed-off-by: Martí Bolívar <marti.bolivar@nordicsemi.no>
@mbolivar-nordic
Copy link
Contributor Author

I still think this can be worked into mergeable shape, even if it's buried in an SoC specific part of the build system, so rebased and removed stale label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants