-
Notifications
You must be signed in to change notification settings - Fork 8.9k
lib: open-amp: initialize the resource_table.cm_trace.da at runtime #96554
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,7 +85,8 @@ struct fw_resource_table { | |
| #define CM_TRACE_ENTRY \ | ||
| .cm_trace = { \ | ||
| RSC_TRACE, \ | ||
| (uint32_t)ram_console_buf, CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0,\ | ||
| 0, /* Will be initialized at runtime */ \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you check impact in case of firmware that are loaded with a static const table? I do not verify by a test but seems to me that this update can lead to a Linux crash. Indeed initialise the address at runtime is to late, the resource table is parsed by the Linux remoteproc driver before loading the remoteproc firmware (https://elixir.bootlin.com/linux/v6.17/source/drivers/remoteproc/remoteproc_core.c#L571)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your comments!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please tell me if I'm wrong but the Linux kernel calls rproc_handle_trace before loading the and booting up the Zephyr Do you load and start the firmware with the Linux remoteproc driver or just use the attach mechanism? I will try to find time end of this week to test your patch on Stm32mp157F-DK board to confirm ( or not) my hypothesis...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had a run on imx95evk, start the Zephyr on a CA55 core and cat the resource table immediately, and can get the correct ram_console_buf[] address. [ 1637.292640] Install rpmsg tty driver! Vring 0 Vring 1 Entry 1 is of type trace
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What Linux are you using? Have you made any changes to the remoteproc subsystem?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @iuliana-prodan and @arnopo The initialization of cm_trace.da (a uint32_t type) violates the following requirement:
The operand
While
This case seems fell into the 6.6 Constant Expressions, Clause 14: The current expression When change to So, the current expression And any idea to fix this violation?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Zhiqiang-Hou, could you please share the west command you are using to reproduce the issue?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@Zhiqiang-Hou @arnopo I propose this solution: In this case, if we have the Can you, please, check?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
west build -p always -b imx95_evk/mimx9596/a55 samples/hello_world/ -DCONFIG_OPENAMP_RSC_TABLE=y -DCONFIG_RAM_CONSOLE=y
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Here it looks to me that the issue is related to 64-bit arch build that does not allow to cast a 64-bit address to 32-bit address in a static way. In such case I would use the An alternative would be to create a new entry that support the 64-bit addressing but this would request a patch in Linux also. |
||
| CONFIG_RAM_CONSOLE_BUFFER_SIZE, 0, \ | ||
| "Zephyr_log", \ | ||
| }, | ||
| #else | ||
|
|
||
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.
IMO, a simple solution, with small changes is:
This way:
DT_REG_ADDR(DT_CHOSEN(zephyr_ram_console))will resolve to the compile-time constant from snippet;Uh oh!
There was an error while loading. Please reload this page.
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.
There are 2 config options that we have to take into account:
Your solution is only for the option 1. Do you have any idea for option 2.
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.
@iuliana-prodan and @arnopo I think we can use the runtime-initialization solution, it can handle both of the 2 options, and won't lead Linux crash. Any comments?
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 tested the above solution on the Xtensa and ARM32 architectures, and their toolchains are more permissive, allowing undefined macros to default to 0.
To fix also ARM64, we should do something like:
With this change, if
CONFIG_RAM_CONSOLE=yis set, theram_console_bufwill be placed in.bss. If a snippet defines thezephyr,ram-consolenode, the buffer will be located in the specified memory region.Here’s the location of
ram_console_buffrom three different builds:Can you please check this and let me know if it works for you?
Uh oh!
There was an error while loading. Please reload this page.
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.
In this case, the cm_trace.da is initialized with '0', so still need runtime initialization to assign it the ram_console_buf[] address?
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.
Yes, the feature is disabled even though the buffer exists.
However, since we don’t have a snippet that defines the address of the
ram_console_buf, I assumed this was the expected behavior.Am I mistaken?
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.
This changes the functionality against the current code. The current code initializes the
cm_trace.dano matter theram_console_buflocates in dedicated section or the address arragned by toolchain, this change only handle the former. I think we also need to handle the latter, since it affects all architectures/platforms.