-
Notifications
You must be signed in to change notification settings - Fork 7.6k
open-amp: resource table: Add support for vendor-specific features #87296
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question regarding turning this option on and off, otherwise looks good to me.
lib/open-amp/Kconfig
Outdated
config OPENAMP_RSC_TABLE_VENDOR_RSC_LENGTH | ||
int "Vendor specific length of the resource" | ||
default 0 | ||
range 0 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question instead of using the size to actually reserve space in the rsc_table, why not create a specific option let's ADD_VENDOR_SPEFICIC_AREA?
I'm afraid that implementors may use for example 0 - sized values and it silently have hard time to debug when this value is badly populated
Having a switch like option enforces the implementor to search the documentation to refer the "right" way to populate the vendor reserved area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of CONFIG_OPENAMP_RSC_TABLE_VENDOR_DATA_FEATURES_0, which means the vendor-specific feature is disable, I can add:
config CONFIG_OPENAMP_RSC_TABLE_VENDOR_RSC_FEATURE
bool "Vendor specific resource entry"
depends on OPENAMP_RSC_TABLE
help
Enable vendor-specific features supported by remote processor.
Will that be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @iuliana-prodan , I like that approach and it sounds more explicit on what this symbol does :)
lib/open-amp/resource_table.c
Outdated
#if (CONFIG_OPENAMP_RSC_TABLE_VENDOR_RSC_LENGTH > 0) | ||
/* Initialize vendor-specific resource entry */ | ||
.vendor_hdr = { | ||
RSC_VENDOR_START, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to define a resource ID different from RSC_VENDOR_START
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I wanted this to be generic, I used RSC_VENDOR_START.
I can add a config, something like:
config RSC_VENDOR_TYPE
int "Vendor specific type of the resource entry"
default 128
range 128 512
help
Type of vendor-specific resource entry.
For example, for NXP:
- in board overlay I can add my NXP type:
CONFIG_RSC_VENDOR_TYPE=129
- in Linux driver (imx_dsp_rproc) I'll check for this value.
The same can be done for other vendors.
lib/open-amp/resource_table.c
Outdated
, CONFIG_OPENAMP_RSC_TABLE_VENDOR_DATA_FEATURES_3, | ||
#endif | ||
}, | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the implementation I wonder if you should not define your own table in you SoC Adapting this for all vendors could become a nightmare.
This one could be the default one without specific vendor resources
An example of implementation would be to define a OPENAMP_VENDOR_ RSC_TABLE
config that would include a vendor_resource_table.h
instead of defining the enum rsc_table_entries
and struct fw_resource_table
Some other/better probably exist...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want this to be generic, like struct fw_rsc_trace
, with CONFIG_RAM_CONSOLE
.
That's why I used those 4 CONFIG_OPENAMP_RSC_TABLE_VENDOR_DATA_FEATURES_X
, so a vendor can enable as many features as needed.
This will be very simple to enable it for a vendor. Here's how I do for i.MX8ULP:
diff --git a/samples/subsys/ipc/openamp_rsc_table/boards/imx8ulp_evk_mimx8ud7_adsp.conf b/samples/subsys/ipc/openamp_rsc_table/boards/imx8ulp_evk_mimx8ud7_adsp.conf
index 88f6a2df451..e9e2c5d7099 100644
--- a/samples/subsys/ipc/openamp_rsc_table/boards/imx8ulp_evk_mimx8ud7_adsp.conf
+++ b/samples/subsys/ipc/openamp_rsc_table/boards/imx8ulp_evk_mimx8ud7_adsp.conf
@@ -9,3 +9,6 @@ CONFIG_MBOX_NXP_IMX_MU=y
CONFIG_OPENAMP_ADDR_TRANSLATION=y
CONFIG_SHELL=n
+
+CONFIG_OPENAMP_RSC_TABLE_VENDOR_RSC_LENGTH=1
+CONFIG_OPENAMP_RSC_TABLE_VENDOR_DATA_FEATURES_1=0x0
(the diff is based on this PR that is still in review: https://github.com/zephyrproject-rtos/zephyr/pull/83049/files#diff-143b27ae2e5e64ca6cbb40b17c473439f177882a711361d97aa2f081b3b4cd7bR1)
I also have a Linux PR for i.MX DSP remoteproc driver: https://lore.kernel.org/all/[email protected]/ which works perfect with this change from remote processor side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concerns with this implementation are:
- What happens if a vendor needs to set CONFIG_OPENAMP_RSC_TABLE_VENDOR_RSC_LENGTH to 20?
- What happens if a vendor needs 4 vendor IDs?
That's why I suggest keeping this generic and implementing your own resource table for your platform.
Of course, this is just a suggestion. If the maintainer is okay with this implementation, let's keep it and address the issue when/if someone needs new vendor ID structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlocaione Can you please review since you are the assignee & maintainer?
Thank you!
0874db4
to
146c275
Compare
Changes in v2:
|
An example on how this will be used is the i.MX8ULP support for
I haven't created a PR yet, for this change since there are still some dependencies that are not merged. |
146c275
to
7c1ba25
Compare
Changes in
An example on how to use this, and how to enable it is:
This diff will be appended to this PR, when PR #89317 will be merged. |
7c1ba25
to
6f6ba58
Compare
lib/open-amp/resource_table.c
Outdated
"Zephyr_log", | ||
}, | ||
#endif | ||
#include "rsc_table_init.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that you can do that. it working but it is not clean.
you should have
#include <zephyr/kernel.h>
#include <resource_table.h>
#include "rsc_table_init.h"
#ifdef CONFIG_OPENAMP_COPY_RSC_TABLE
#define RSC_TABLE_ADDR DT_REG_ADDR(DT_CHOSEN(zephyr_ipc_rsc_table))
#define RSC_TABLE_SIZE DT_REG_SIZE(DT_CHOSEN(zephyr_ipc_rsc_table))
BUILD_ASSERT(sizeof(struct fw_resource_table) <= RSC_TABLE_SIZE);
#endif
extern char ram_console_buf[];
#define __resource Z_GENERIC_SECTION(.resource_table)
static struct fw_resource_table __resource resource_table = RESOURCE_TABLE_INIT
with RESOURCE_TABLE_INIT
defined as a macro in lib/open-amp/rsc_table_init.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried
static struct fw_resource_table __resource resource_table = RESOURCE_TABLE_INIT
I cannot use macro since in the initialization of resource table we have a few defines, like:
...
.offset = {
#if (CONFIG_OPENAMP_RSC_TABLE_NUM_RPMSG_BUFF > 0)
offsetof(struct fw_resource_table, vdev),
#endif
#if defined(CONFIG_RAM_CONSOLE)
offsetof(struct fw_resource_table, cm_trace),
#endif
...
lib/open-amp/rsc_table_init.h
Outdated
.vendor_type = CONFIG_OPENAMP_VENDOR_RSC_TYPE, | ||
#endif | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra line to remove
lib/open-amp/rsc_table_init.h
Outdated
#ifndef RESOURCE_TABLE_INIT_H__ | ||
#define RESOURCE_TABLE_INIT_H__ | ||
|
||
.hdr = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be a Macro
# define RESOURCE_TABLE_INIT \
{ \
.hdr = { \
....
}
6f6ba58
to
0771820
Compare
0bc5dd2
to
3497f10
Compare
Changes in
|
depends on OPENAMP_VENDOR_RSC_TABLE | ||
help | ||
Name of a source file containing vendor-specific | ||
resource table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look to me that there are some redundancies in configs
Could we do it in this way:
OPENAMP_RSC_TABLE
should specify if there is a resource table
OPENAMP_VENDOR_RSC_TABLE_FILE
point to the resource table file that is the legacy file by default
config OPENAMP_VENDOR_RSC_TABLE_FILE
string "Source file containing vendor-specific resource table"
default ""
depends on OPENAMP_RSC_TABLE
help
Name of a source file containing vendor-specific resource table.
and then in CMakeLists.txt
zephyr_include_directories_ifdef(CONFIG_OPENAMP_RSC_TABLE .)
# include vendor-specific resource table files
if(CONFIG_OPENAMP_VENDOR_RSC_TABLE STREQUAL "")
# include generic resource table
zephyr_sources(resource_table.c)
ielseif(CONFIG_OPENAMP_VENDOR_RSC_TABLE )
zephyr_include_directories(vendor)
zephyr_sources(vendor/${CONFIG_OPENAMP_VENDOR_RSC_TABLE_FILE})
endif()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look to me that there are some redundancies in configs
Here are the reasons why I wrote the code as I did:
Using CONFIG_OPENAMP_RSC_TABLE_NUM_RPMSG_BUFF
in vendor rsc table is not right.
For the generic table this config enables the VDEV_ENTRY that is mandatory for the sample.
I've made the CONFIG_OPENAMP_RSC_TABLE
and CONFIG_OPENAMP_VENDOR_RSC_TABLE
exclusive so a vendor can use only what is needed in their resource table.
The only configs that are needed in both generic and vendor rsc table will have the CONFIG_OPENAMP_RSC_TABLE_PRESENT
.
if(CONFIG_OPENAMP_VENDOR_RSC_TABLE STREQUAL "")
In C, you can't directly check whether a Kconfig string is empty or not to conditionally include code. This limitation affects what I would need to implement here or here if I go with your proposed solution.
3497f10
to
b46b986
Compare
Changes in
In this PR I've introduced only 2 new configuration options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a last comment, with that LGTM
lib/open-amp/CMakeLists.txt
Outdated
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
# | ||
|
||
zephyr_include_directories_ifdef(CONFIG_OPENAMP_RSC_TABLE .) | ||
zephyr_sources_ifdef(CONFIG_OPENAMP_RSC_TABLE resource_table.c) | ||
zephyr_include_directories_ifdef(CONFIG_OPENAMP_VENDOR_RSC_TABLE vendor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here CONFIG_OPENAMP_RSC_TABLE
and CONFIG_OPENAMP_VENDOR_RSC_TABLE
can be defined move the
zephyr_include_directories_ifdef
in codition below replacung by zephyr_include_directories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
b46b986
to
de5d9c5
Compare
Changes in
|
Switching to void allows greater flexibility in supporting vendor-specific resource tables. This change reverts commit 39863b6, and adds some fixes. Signed-off-by: Iuliana Prodan <[email protected]>
Enhance flexibility by allowing vendors to define their own resource tables. This is achieved by moving the vdev and vring functions into the source file, requiring each vendor to implement these functions within their specific resource table definitions. Signed-off-by: Iuliana Prodan <[email protected]>
Introduce macros to define initialization routines for OpenAMP resource table entries. This will allow to extend the default OpenAMP resource table with vendor-specific data structures. Signed-off-by: Iuliana Prodan <[email protected]>
Add config options for vendor-specific resource tables. Signed-off-by: Iuliana Prodan <[email protected]>
Modify the build system to conditionally compile vendor-specific resource table support based on configuration. Signed-off-by: Iuliana Prodan <[email protected]>
Implement NXP-specific support for the remote processor resource table, including custom definitions and initialization logic to meet NXP's communication requirements. Signed-off-by: Iuliana Prodan <[email protected]>
Update the imx8ulp_evk_mimx8ud7_adsp board configuration to enable vendor-specific resource table features. Signed-off-by: Iuliana Prodan <[email protected]>
de5d9c5
to
e5fb03d
Compare
|
This update introduces support for vendor-specific features within the OpenAMP remote processor resource table.
To facilitate the integration of custom data structures, new macros have been added to define initialization routines for resource table entries.
The core resource table has been refactored to include hooks that enable vendor-specific initialization logic. As part of this enhancement, NXP-specific support has been implemented, incorporating custom definitions and initialization routines tailored to NXP’s communication requirements.
To demonstrate this capability, the imx8ulp_evk_mimx8ud7_adsp board configuration has been updated to activate vendor-specific resource table features.
This enhancement allows better flexibility for remote processors requiring custom features configurations.