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

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

Conversation

iuliana-prodan
Copy link
Collaborator

@iuliana-prodan iuliana-prodan commented Jan 17, 2025

Now, in openamp_rsc_table sample we support only the devices
that have the same value for driver and device physical address.
But, in case they are different, we need to convert from driver
to device address.

Therefore, add ranges property in device tree in case we need to
translate the addresses. If this property is found in device tree,
take both the driver and device address, and size.
Otherwise it works as before, take the device physical address and
size from reg property.

Fixes: #84168

An example on how to use the ranges property for a target which has different physical addresses for driver and device: #83049

uLipe
uLipe previously approved these changes Jan 17, 2025
#define SHM_PHYS_START_ADDR COND_CODE_1(DT_NUM_REGS(SHM_NODE), \
(DT_REG_ADDR(SHM_NODE)), \
(DT_REG_ADDR_BY_NAME(SHM_NODE, 1))) \

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

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" :)

Now, in open_amp_rsc_table sample we support only the devices
that have the same value for driver and device physical address.
But, in case they are different, we need to convert from driver
to device address.

Therefore, add ranges property in device tree in case we need to
translate the addresses. If this property is found in device tree,
take both the driver and device address, and size.
Otherwise it works as before, take the device physical address and
size from reg property.

Signed-off-by: Iuliana Prodan <[email protected]>
@iuliana-prodan iuliana-prodan force-pushed the openamp_rsc_table_va_to_pa branch from 4d31ecc to 7c04f82 Compare February 3, 2025 15:52
Copy link
Contributor

@mbolivar mbolivar left a comment

Choose a reason for hiding this comment

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

If I understand you are trying to use ranges to signal virtual and physical address differences.

This is contrary to the DT specification. The ranges property is basically syntactic sugar that makes it easier to write down physical addresses and it shall not be used for virtual addressing.

@iuliana-prodan
Copy link
Collaborator Author

If I understand you are trying to use ranges to signal virtual and physical address differences.

This is contrary to the DT specification. The ranges property is basically syntactic sugar that makes it easier to write down physical addresses and it shall not be used for virtual addressing.

@mbolivar both addresses are physical addresses, as explained here.

Here's an example on how I use the ranges property for i.MX8ULP where we need to translate from driver physical address (Cortex A) to device physical address (DSP).

If ranges is not suitable for this case, do you have any other suggestion?

Thanks!

@mausys
Copy link
Contributor

mausys commented Feb 4, 2025

I think the solution to this problem would be to use carveouts in the resource table, so the host can store its physical address and the OpenAMP library can translate it to its own device address. This was the reason for my pull request #83754, but I have yet to rewrite the example to finish the pull request.

@iuliana-prodan
Copy link
Collaborator Author

iuliana-prodan commented Feb 4, 2025

I think the solution to this problem would be to use carveouts in the resource table, so the host can store its physical address and the OpenAMP library can translate it to its own device address. This was the reason for my pull request #83754, but I have yet to rewrite the example to finish the pull request.

This solution was agreed in the OpenAMP technical meeting. See also here.
libmetal update here: OpenAMP/libmetal#322

@mbolivar
Copy link
Contributor

mbolivar commented Feb 5, 2025

If ranges is not suitable for this case, do you have any other suggestion?

Hi @iuliana-prodan , before we get into the specifics of your use case, can you please confirm that you've read the core DT spec's sections on reg and ranges, in particular sections 2.3.5 through 2.3.8 in https://devicetree-specification.readthedocs.io/en/latest/index.html ?

I am happy to try to help you with the specifics, but I unfortunately do not have time to educate every zephyr maintainer about the core specification itself. Happy to help if anything in the spec is confusing or if you have further questions after you've confirmed you read the spec.

Thanks!

@arnopo
Copy link
Collaborator

arnopo commented Feb 6, 2025

If ranges is not suitable for this case, do you have any other suggestion?

Hi @iuliana-prodan , before we get into the specifics of your use case, can you please confirm that you've read the core DT spec's sections on reg and ranges, in particular sections 2.3.5 through 2.3.8 in https://devicetree-specification.readthedocs.io/en/latest/index.html ?

I am happy to try to help you with the specifics, but I unfortunately do not have time to educate every zephyr maintainer about the core specification itself. Happy to help if anything in the spec is confusing or if you have further questions after you've confirmed you read the spec.

Thanks!

In Linux for such translation dma_ranges is used.

@iuliana-prodan
Copy link
Collaborator Author

If ranges is not suitable for this case, do you have any other suggestion?

Hi @iuliana-prodan , before we get into the specifics of your use case, can you please confirm that you've read the core DT spec's sections on reg and ranges, in particular sections 2.3.5 through 2.3.8 in https://devicetree-specification.readthedocs.io/en/latest/index.html ?

I am happy to try to help you with the specifics, but I unfortunately do not have time to educate every zephyr maintainer about the core specification itself. Happy to help if anything in the spec is confusing or if you have further questions after you've confirmed you read the spec.

Thanks!

@mbolivar I've read the specs, but for this openamp_rst_table sample it doesn't make a difference. Also, there is no driver associated with this device tree node.
Would dma-ranges work?

@arnopo based on your comments from libmetal here I will add 2 new members: metal_io_ops::remote_phys_to_offset and metal_io_ops::offset_to_remote_phys.
This new code will be implemented in a driver libmetal_addr_translation which will be enabled on demand using dts overlay.
The driver should return the metal_io_ops that will be passed to metal_io_init.
For initialization will use the data from dma-ranges property.

Is this proposal acceptable?
Is there another way to have these platform specific code in another location (open-amp, libmetal, or somewhere else in Zephyr)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants