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

dts: arm: rpi_pico: remove #define from dts #81196

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

Siliconsignals
Copy link
Contributor

we can not use #define. Instead, include a dt-bindings header including the definitions.

Fixes: #79719

@zephyrbot zephyrbot added the platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) label Nov 10, 2024
@zephyrbot zephyrbot requested review from decsny and soburi November 10, 2024 14:55
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Whilst the move for the defines is fine, how is this fixing the original issue of RPI_PICO_DEFAULT_IRQ_PRIORITY?

@Siliconsignals
Copy link
Contributor Author

Hi @nordicjm,

Thank you for your input.

This PR addresses just issue #79719 by removing direct #define usage in the DTS file and converting these definitions to use a dt-bindings header instead.

Regarding RPI_PICO_DEFAULT_IRQ_PRIORITY, if you recommend, we could consider adding it to the Kconfig file for the RPI_PICO board. Alternatively, please let me know if you have any other suggestions on how to handle RPI_PICO_DEFAULT_IRQ_PRIORITY.

@nordicjm
Copy link
Collaborator

Kconfig cannot be used from dts. Have spoken with Gerard who suggests to use the way it is done with nordic devices, the reset file change can stay but without the IRQ part, use the equivalent override here and define below it with the comment: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/common/nordic/nrf_common.dtsi#L15 add an override file similar to https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/arm/nordic/override.dtsi then all should work

@Siliconsignals
Copy link
Contributor Author

Kconfig cannot be used from dts. Have spoken with Gerard who suggests to use the way it is done with nordic devices, the reset file change can stay but without the IRQ part,

okay

use the equivalent override here and define below it with the comment: https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/common/nordic/nrf_common.dtsi#L15 add an override file similar to https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/arm/nordic/override.dtsi then all should work

sure, will do

@Siliconsignals
Copy link
Contributor Author

Hi @nordicjm,

Could you please confirm if this change aligns with your requirements? I’d appreciate any feedback to ensure it meets your expectations.

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Thanks

@nordicjm
Copy link
Collaborator

@Tarangraval21 can you check compliance and fix issues?

@Siliconsignals
Copy link
Contributor Author

@nordicjm,

@Tarangraval21 can you check compliance and fix issues?

ohh okay , The subject line "Update dts/arm/rpi_pico/rp2040.dtsi" is not in the correct format, and the patch is missing the signed-off-by tag.

i will fix it.

@nordicjm
Copy link
Collaborator

The merge commit needs to be removed, rebases are used for pulling in latest zephyr changes (which you only need to do if there is a file conflict)

@Siliconsignals Siliconsignals force-pushed the fix_rpi_pico_dts branch 3 times, most recently from 3e802a7 to 95eb08d Compare November 20, 2024 08:56
@Siliconsignals
Copy link
Contributor Author

@nordicjm

I am encountering build failures in the "Run tests with twister / twister-build(2) (pull_request_target)

error log:
INFO    - The following issues were found (showing the top 10 items):
INFO    - 1) samples/psa/its/sample.psa.its.tfm on mps3/corstone300/fvp/ns error (Build failure)
INFO    - 2) samples/psa/persistent_key/sample.psa.persistent_key.tfm on mps3/corstone300/fvp/ns error (Build failure)
INFO    - 3) samples/psa/its/sample.psa.its.tfm on mps3/corstone310/fvp/ns error (Build failure)
INFO    - 4) samples/psa/persistent_key/sample.psa.persistent_key.tfm on mps3/corstone310/fvp/ns error (Build failure)
INFO    - 
INFO    - To rerun the tests, call twister using the following commandline:
INFO    - west twister -p <PLATFORM> -s <TEST ID>, for example:
INFO    - 
INFO    - west twister -p mps3/corstone310/fvp/ns -s samples/psa/persistent_key/sample.psa.persistent_key.tfm
INFO    - or with west:
INFO    - west build -p -b mps3/corstone310/fvp/ns samples/psa/persistent_key -T sample.psa.persistent_key.tfm
INFO    - -+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
INFO    - Run completed

Error: Error: failed to run script step: command terminated with non-zero exit code: Error executing in Docker Container: 1
Error: Process completed with exit code 1.
Error: Executing the custom container implementation failed. Please contact your self hosted runner administrator.

I am unable to determine how these build failures are related to the current patch. Could you please help clarify or provide guidance on troubleshooting these issues?

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Can you squash these commits? In rebase, mark the second as fixup. Also do a rebase on main and hopefully CI should pass

Removing direct #define usage in the DTSI file and converting these
definitions to use a dt-bindings header instead.

Relocates the RPI_PICO_DEFAULT_IRQ_PRIORITY definition to a DTSI file and
introduces an override.dtsi file. The override file is used when no other
override file is present, allowing for better flexibility and compliance
with Zephyr’s DTS structure.

Fixes: zephyrproject-rtos#79719

Signed-off-by: Tarang Raval <[email protected]>
@nashif nashif merged commit 31eee15 into zephyrproject-rtos:main Nov 20, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DTS files should not directly contain #define
6 participants