Skip to content

Xlnx add trace buf and update address space#98

Open
tnmysh wants to merge 4 commits into
OpenAMP:mainfrom
tnmysh:xlnx_add_trace_buf_and_update_address_space
Open

Xlnx add trace buf and update address space#98
tnmysh wants to merge 4 commits into
OpenAMP:mainfrom
tnmysh:xlnx_add_trace_buf_and_update_address_space

Conversation

@tnmysh
Copy link
Copy Markdown
Collaborator

@tnmysh tnmysh commented Mar 13, 2026

  • Add trace buffer support
  • Update current DDR addresses to match latest system device-tree standards
  • fix trivial compiler warning during building matrix multiply demo

@tnmysh tnmysh requested review from arnopo and edmooring March 13, 2026 04:00
@tnmysh tnmysh force-pushed the xlnx_add_trace_buf_and_update_address_space branch from f6a6178 to 833a6b4 Compare March 13, 2026 21:26
Comment thread examples/legacy_apps/examples/echo/freertos/main.c
Comment thread examples/legacy_apps/machine/xlnx/zynqmp_r5/CMakeLists.txt
Comment thread examples/legacy_apps/machine/xlnx/zynqmp_r5/platform_info.c
Comment thread examples/legacy_apps/machine/xlnx/zynqmp_r5/platform_info.c
Comment thread examples/legacy_apps/machine/xlnx/zynqmp_r5/platform_info.c Outdated
@tnmysh tnmysh force-pushed the xlnx_add_trace_buf_and_update_address_space branch from 833a6b4 to b59ecf4 Compare March 27, 2026 03:45
@tnmysh tnmysh requested a review from arnopo March 27, 2026 03:48
collect (APP_INC_DIRS "${CMAKE_CURRENT_SOURCE_DIR}")

if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${PROJECT_SYSTEM}/gic_init.c")
collect (APP_COMMON_SOURCES ${PROJECT_SYSTEM}/gic_init.c)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

s/exampples/examples/ in commit message

That said you can remove examples: as done for other patches

int32_t len;
va_list args;


Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

useless double blank line


#define NUM_TABLE_ENTRIES 1
#define NUM_TABLE_ENTRIES 2
#define RSC_TRACE_SZ (4*1024)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpicking. The trace contain char, why not just just

Suggested change
#define RSC_TRACE_SZ (4*1024)
#define RSC_TRACE_SZ 4096

@tnmysh tnmysh force-pushed the xlnx_add_trace_buf_and_update_address_space branch from b59ecf4 to 1e20941 Compare April 3, 2026 20:28
@tnmysh
Copy link
Copy Markdown
Collaborator Author

tnmysh commented Apr 7, 2026

Hi @arnopo,

I found bug in this patch series when using internal tools, please do not merge it until next update.

Thanks,
Tanmay

@tnmysh tnmysh force-pushed the xlnx_add_trace_buf_and_update_address_space branch from 1e20941 to 6796ba0 Compare May 18, 2026 15:19
@tnmysh
Copy link
Copy Markdown
Collaborator Author

tnmysh commented May 18, 2026

@arnopo, @edmooring this change is ready for review.

tnmysh added 4 commits May 20, 2026 08:01
platform_info.h was changed to platform_info_common.h, replace it
accordingly for echo demo.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
fix following compiler warning:

warning: passing argument 1 of 'rpmsg_matrix_app' fr
om incompatible pointer type [-Wincompatible-pointer-types]
   47 |                 rpmsg_matrix_app(rpdev, platform);
      |                                  ^~~~~

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
As per latest SDT standard AMD-xilinx EDF yocto is released with fixed
memory regions for each core to use. Update the default address for
cluster-A core0 to use as per this standard.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
trace buffer is a circular buffer to store logs. Linux remoteproc
subsystem creates a debugfs file that can be used to print these logs on
linux side.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
@tnmysh tnmysh force-pushed the xlnx_add_trace_buf_and_update_address_space branch from 6796ba0 to 9456f3e Compare May 20, 2026 15:02
@tnmysh tnmysh requested a review from arnopo May 20, 2026 15:12
@arnopo arnopo added this to the Release V2026.04 milestone May 20, 2026
Copy link
Copy Markdown

@edmooring edmooring left a comment

Choose a reason for hiding this comment

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

This comes down to two questions:

Which is the correct size for the TCM lockstep region, 0x40000 or 0x 60000?
Why is the log level escalated for levels above "debug"?

MEMORY
{
psu_ddr_S_AXI_BASEADDR : ORIGIN = 0x3ED00000, LENGTH = 0x00040000
psu_ddr_S_AXI_BASEADDR : ORIGIN = 0x9800000, LENGTH = 0x00060000
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This now conflicts with the comment in line 18 above, Size has changed from 0x40000 to 0x60000 without changing the comment. Which is correct?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@edmooring, actually the memory modified here is not TCM, but DDR. So the firmware code can be split and some sections can be loaded into TCM, and some are loaded in the DDR.

The TCM memory is represented here with: psu_r5_tcm_ram_0_S_AXI_BASEADDR

Which I think is 64KB. I agree that the comment should be modified, as the TCM is 64-KB chunks. I will update comment.

MEMORY
{
psu_ddr_S_AXI_BASEADDR : ORIGIN = 0x3ED00000, LENGTH = 0x00040000
psu_ddr_S_AXI_BASEADDR : ORIGIN = 0x9800000, LENGTH = 0x00060000
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same as above. Either the comment in line 18 is wrong or the code in line 21 is wrong.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same explanation as above.

#ifndef SHARED_MEM_PA
#if XPAR_CPU_ID == 0
#define SHARED_MEM_PA 0x3ED40000UL
#define SHARED_MEM_PA 0x9860000UL
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again, why the size change from 0x40000 to 0x60000? This also applies to line 106 below.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This SHARED_MEM_PA is different region than mentioned in the linker script.

This is where vring buffers are located. This address is moved in the new spec.

} circ;

static void rsc_trace_putchar(char c)
{
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A very small nit:
The circular buffer will be left in a logically inconsistent state after c_len calls to rsc_trace_puchar(), where c_pos actually points past the end of the buffer. This might cause problems for readers of the buffer. Switching the order so the put to the buffer is done first, and then the check for the end of the buffer would keep the state of the buffer logically consistent between calls to rsc_trace_putchar().

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ack.

int32_t len;
va_list args;

if (level > METAL_LOG_DEBUG)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This needs an explanatory comment. Why are we escalating the warning level?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants