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

Define a platform hook interface and cleanup misuse of private calls #61547

Closed
wants to merge 16 commits into from

Conversation

nashif
Copy link
Member

@nashif nashif commented Aug 16, 2023

An attempt to cleanup misuse of private API call to define platform hooks and generalize the definition of hooks implemented by platforms overriding architecture implementation.

This also generalizes some hook implementations done for some architectures and made such implementation arch agnostic, allowing hooks to be implemented by any architecture.

Relates to #58007

@nashif nashif added the Architecture Review Discussion in the Architecture WG required label Aug 16, 2023
@carlescufi carlescufi requested a review from gmarull August 16, 2023 11:38
@nashif nashif changed the title Define a platform hook interface and cleanup misused of private calls Define a platform hook interface and cleanup misuse of private calls Aug 16, 2023
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

changes make sense to me. we should document reserved internal prefixes like arch, platform, etc. Some CI checks would also be nice to prevent misuses (e.g. forbid arch_/platform_ calls in samples/ or drivers/)

Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

any reason this can't be done with weak functions?

@nashif
Copy link
Member Author

nashif commented Aug 22, 2023

any reason this can't be done with weak functions?

This has been the approch for some interfaces. This PR is mostly changing the naming and moving a fewthings upo to the architecture level, but not changing how the interfaces are implemented.
We had a few change proposals floating around removing usage of _weak given it is considered not stanard but we still have many usages in the tree. Whether we use _weak or some other mechanism is something we need decide on separately I guess.

@@ -0,0 +1,16 @@
.. _platform_cusomizations:

Hardware with Custom Interfaces
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good first step towards closing the documentation gap between "board porting" and "soc" or "arch" porting.

@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Oct 29, 2023
@nashif nashif removed the Stale label Oct 31, 2023
@nashif nashif marked this pull request as ready for review November 29, 2023 12:32
@zephyrbot zephyrbot added area: ARM ARM (32-bit) Architecture platform: NXP NXP area: RISCV RISCV Architecture (32-bit & 64-bit) platform: Intel ADSP Intel Audio platforms area: Xtensa Xtensa Architecture area: Memory Management area: Architectures area: ARM64 ARM (64-bit) Architecture labels Nov 29, 2023
@amr-sc
Copy link
Contributor

amr-sc commented Dec 7, 2023

any reason this can't be done with weak functions?

Is there a particular use for weak functions in this case?

Hi @cfriedt , I think the weak functions could be useful to override the default arch level implementations (which follow the Spec) in case some vendors doesn't conform to the Spec, or the Spec is ambiguous.

Sure - it's just that weak symbols are not part of ISO C or ISO C++ and are very much ELF specific.

We can achieve the same thing with a well-defined interface and having a Kconfig option to choose between the default (arch) and overridden (platform) solution.

Oh, I've got your point.
The explicit policy with default arch_ and overridden platform_ API is for sure better than the _weak approach.
Thanks!

P.S.: looks like this may provide a guideline for #65824 as well

Move from private to internal call. This should probably be promoted to
an API and implementation abstracted similar to what we do with other
cache functions.

Signed-off-by: Anas Nashif <[email protected]>
use BIT macro where possible.

Signed-off-by: Anas Nashif <[email protected]>
84b86d9 introduced the ability of defining custom sys_io functions for
riscv. This is however useful for all other architectures and there is
nothing riscv specific about it. So instead of having this be riscv
specific, move implementation to the common architecture code and make
it available for everyone using PLATFORM_HAS_CUSTOM_SYS_IO.

Signed-off-by: Anas Nashif <[email protected]>
Move from private hook names to internal using a common prefix for
platform hooks.

Signed-off-by: Anas Nashif <[email protected]>
Move from private hook names to internal using a common prefix for
platform hooks.

Signed-off-by: Anas Nashif <[email protected]>
Use platform_ instead of z_soc_ and make the feature generic supporting
all architectures with CONFIG_PLATFORM_HAS_CUSTOM_IRQ_LOCK_OPS.

Signed-off-by: Anas Nashif <[email protected]>
This call is not being used anywhere.

Signed-off-by: Anas Nashif <[email protected]>
Rename platform hooks enabled with Kconfig from z_soc_ to platform_.

Move from private hook names to internal using a common prefix for
platform hooks.

Signed-off-by: Anas Nashif <[email protected]>
Define a set of platform hooks.

Signed-off-by: Anas Nashif <[email protected]>
Update with some fixes to API usage.

Signed-off-by: Anas Nashif <[email protected]>
Although this option is now being used only by ARM, it is needed by
other architectures, so make it global and change all occurances.

Signed-off-by: Anas Nashif <[email protected]>
group all custom interface configs into one menu.

Signed-off-by: Anas Nashif <[email protected]>
Rename all z_/z_arm_ call for setting irq priority to a new architecture
interface shared by all architectures.

Signed-off-by: Anas Nashif <[email protected]>
define additional routines that can be shared by other architectures.
Right now this is only for ARM and will be moved up in the future.

Signed-off-by: Anas Nashif <[email protected]>
Reorganize functions in header file for better fit.

Signed-off-by: Anas Nashif <[email protected]>
To allow other interrupt controllers to be called.

Signed-off-by: Anas Nashif <[email protected]>
@nashif
Copy link
Member Author

nashif commented Dec 13, 2023

the more I look into things and try to fix existing inconsistencies and interfaces, I realize how messy interrupt implementation is and how many ways exist to enable interrupt controllers that we need to fix ASAP, so for example, I do not think we need to support platform specific irq handling as this PR does and instead allow for this to be done using dedicated interrupt drivers, the same way we do this already on many architectures.

When the provided support in Zephyr does not cover certain hardware platforms,
some interfaces can be implemented directly by the platform or a custom
architecture interface can be used and resulting in more flexibility and
hardware support of differebt variants of the same hardware.
Copy link
Member

Choose a reason for hiding this comment

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

typo differebt


When the provided support in Zephyr does not cover certain hardware platforms,
some interfaces can be implemented directly by the platform or a custom
architecture interface can be used and resulting in more flexibility and
Copy link
Member

Choose a reason for hiding this comment

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

".. can be used and resulting .."
This whole sentence is a bit too tough to read.

@nashif
Copy link
Member Author

nashif commented Dec 13, 2023

the more I look into things and try to fix existing inconsistencies and interfaces, I realize how messy interrupt implementation is and how many ways exist to enable interrupt controllers that we need to fix ASAP, so for example, I do not think we need to support platform specific irq handling as this PR does and instead allow for this to be done using dedicated interrupt drivers, the same way we do this already on many architectures.

just seen this #66505

Copy link

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.

@github-actions github-actions bot added the Stale label Feb 12, 2024
@nashif nashif removed the Stale label Feb 14, 2024
Copy link

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.

@github-actions github-actions bot added the Stale label Apr 15, 2024
@github-actions github-actions bot closed this Apr 30, 2024
@ycsin
Copy link
Member

ycsin commented Jun 26, 2024

@nashif any update for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Review Discussion in the Architecture WG required area: Architectures area: ARM ARM (32-bit) Architecture area: ARM64 ARM (64-bit) Architecture area: Memory Management area: RISCV RISCV Architecture (32-bit & 64-bit) area: Xtensa Xtensa Architecture manifest manifest-sof platform: Intel ADSP Intel Audio platforms platform: NXP NXP Stale
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants