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.

Sign up for GitHub

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

samples: Add support for different base address of the shared mem in the memory map of each core #84170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −7
Open

samples: Add support for different base address of the shared mem in the memory map of each core #84170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doc/build/dts/api/api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,8 @@ device.
* - zephyr,ipc_shm
- A node whose ``reg`` is used by the OpenAMP subsystem to determine the
base address and size of the shared memory (SHM) usable for
interprocess-communication (IPC)
interprocess-communication (IPC).
A ``ranges`` property is used in case the driver and device physical addresses are different.
* - zephyr,itcm
- Instruction Tightly Coupled Memory node on some Arm SoCs
* - zephyr,log-uart
Expand Down
22 changes: 16 additions & 6 deletions samples/subsys/ipc/openamp_rsc_table/src/main_remote.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,18 @@ LOG_MODULE_REGISTER(openamp_rsc_table, LOG_LEVEL_DBG);

/* Constants derived from device tree */
#define SHM_NODE DT_CHOSEN(zephyr_ipc_shm)
#define SHM_START_ADDR DT_REG_ADDR(SHM_NODE)
#define SHM_SIZE DT_REG_SIZE(SHM_NODE)

#if DT_NODE_HAS_PROP(SHM_NODE, ranges)
static const uint32_t shm_start_addr = DT_RANGES_CHILD_BUS_ADDRESS_BY_IDX(SHM_NODE, 0);
static const uint32_t shm_size = DT_RANGES_LENGTH_BY_IDX(SHM_NODE, 0);

static metal_phys_addr_t val = DT_RANGES_PARENT_BUS_ADDRESS_BY_IDX(SHM_NODE, 0);
static metal_phys_addr_t *shm_physmap_drv = &val;
#else
static const uint32_t shm_start_addr = DT_REG_ADDR(SHM_NODE);
static const uint32_t shm_size = DT_REG_SIZE(SHM_NODE);
static metal_phys_addr_t *shm_physmap_drv;
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something is not clear to me in this patch. It looks like you want to define a virtual address in the device tree, which does not seem like a good idea.

In the device tree, I would expect that the reg property gives only the physical address, which you then map in the driver or application to obtain a virtual address.

But perhaps I am missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arnopo you understood correctly, in the dts overlay, in reg property I'm giving both physical and virtual address.

To do the mapping in the application I'm afraid I might break compatibility for the other boards that have the same physical and virtual address.

Another idea is to take the physical address from a CONFIG which will be defined only if CONFIG_OPENAMP=y and we will configure it only in the board conf that supports address mapping.

@arnopo what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two points here.

1)Can we define a virtual address in the device tree? For me, it does not make sense because the hardware reference is the physical address, and the device tree describes the hardware. A virtual address is computed from a physical address, usually with an offset. I don't think there is a precedent for a virtual address defined in the device tree, but perhaps I'm wrong?

  1. mapping in the application
    Could you use K_MEM_VIRT_ADDR, which seems to return the physical address if CONFIG_MMU is not enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both addresses are physical addresses - one from DSP memory space and the other one from main core (Cortex A) memory space - here is the address in Linux dts for 8ULP.

Virtual address is improper said because DSP doesn't have MMU.
Using K_MEM_VIRT_ADDR wouldn't help because it can't get the correct address, this is nowhere computed in Zephyr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification.

What you are trying to do here is convert your Cortex-A memory physical address to the local physical address, which is not related to the virtual address that could be applied on top of it.

In my point of view, you are misusing the OpenAMP API by defining the local physical address as a virtual address.

Memory translation is generally managed by Linux itself using dma-range in the Linux remoteproc. However, this is not implemented for the RPMsg VirtIO buffers, and I suppose that is your issue.

One solution would be to address this on the Linux side. Another would be to implement metal_io_ops to perform the translation if a dma-range is defined in the Zephyr device tree.

I propose we discuss this point in the next RP meeting with the Linux maintainer."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would choose to implement metal_io_ops to do the translation (on Zephyr side), but sure, we can talk to Linux maintainer on RP meeting.

Copy link
Collaborator Author

@iuliana-prodan iuliana-prodan Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@arnopo I've created this PR in libmetal for address translation: OpenAMP/libmetal#322

In this PR, I've updated the openamp_rsc_table sample.

And in this PR is an example on how to use the ranges property for a target which has diffrent physical addresses for driver and device: #83049

I need to figure out a way to link these PRs to make the Zephyr CI "happy" :)

#define APP_TASK_STACK_SIZE (1024)

Expand All @@ -52,7 +62,7 @@ static struct k_thread thread_tty_data;
static const struct device *const ipm_handle =
DEVICE_DT_GET(DT_CHOSEN(zephyr_ipc));

static metal_phys_addr_t shm_physmap = SHM_START_ADDR;
static metal_phys_addr_t shm_physmap = shm_start_addr;
static metal_phys_addr_t rsc_tab_physmap;

static struct metal_io_region shm_io_data; /* shared memory */
Expand Down Expand Up @@ -151,15 +161,15 @@ int platform_init(void)
}

/* declare shared memory region */
metal_io_init(shm_io, (void *)SHM_START_ADDR, &shm_physmap,
SHM_SIZE, -1, 0, NULL);
metal_io_init(shm_io, (void *)shm_start_addr, &shm_physmap,
shm_size, shm_physmap_drv, -1, 0, NULL);

/* declare resource table region */
rsc_table_get(&rsc_table, &rsc_size);
rsc_tab_physmap = (uintptr_t)rsc_table;

metal_io_init(rsc_io, rsc_table,
&rsc_tab_physmap, rsc_size, -1, 0, NULL);
&rsc_tab_physmap, rsc_size, NULL, -1, 0, NULL);

/* setup IPM */
if (!device_is_ready(ipm_handle)) {
Expand Down
Loading