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

drivers: Add external address translation module and TI RAT support #60028

Merged

Conversation

KarthikL1729
Copy link
Contributor

@KarthikL1729 KarthikL1729 commented Jul 5, 2023

This PR adds support for external address translation modules by using the sys_mm_drv_page_phys_get() function in the system_mm API. Texas Instruments SoCs (specifically the K3 family) require a Region based Address Translation (RAT) module to be able to access peripherals such as UART, and this PR includes the driver for RAT as well, which utilises the aforementioned API.

Signed off by: l-lakshmanan@ti.com

Copy link
Collaborator

@carlocaione carlocaione left a 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 this is the way we should go.

Either we fix / adapt the current system_mm API and we add a new RAT driver using this API so that we can simply use sys_mm_drv_map_region() in device_mmio.h or we add an entirely new class for this use-case, outside the system_mm API (and directories).

@dcpleung the system_mm is a bit of an odd-ball, is this a use case that you envisioned for this API?

@povergoing
Copy link
Member

I am not sure, does RAT play a role like an SMMU but based on Regions?

@KarthikL1729
Copy link
Contributor Author

RAT is a TI specific module that a few TI SoCs require to be able to access peripherals due to the change in bus size from a 48 bit SoC address to a 32 bit local address (for example in the Cortex M4F in the AM62X SoC).

@povergoing
Copy link
Member

RAT is a TI specific module that a few TI SoCs require to be able to access peripherals due to the change in bus size from a 48 bit SoC address to a 32 bit local address (for example in the Cortex M4F in the AM62X SoC).

So, it translates the accesses from the core to the peripherals, right? The initiator is still core? Like, add another region based translation layer under MMU/MPU?

@carlocaione
Copy link
Collaborator

So, it translates the accesses from the core to the peripherals, right? The initiator is still core? Like, add another region based translation layer under MMU/MPU?

Yes, but this must me somehow integrated into our current MMIO API. Also, we could do that but we do not currently have a proper MMU/MPU API so that should be again some custom / soc-specific code so I was wondering whether we could instead leverage the system_mm API for this case to have a proper abstraction layer.

@KarthikL1729
Copy link
Contributor Author

RAT is a TI specific module that a few TI SoCs require to be able to access peripherals due to the change in bus size from a 48 bit SoC address to a 32 bit local address (for example in the Cortex M4F in the AM62X SoC).

So, it translates the accesses from the core to the peripherals, right? The initiator is still core? Like, add another region based translation layer under MMU/MPU?

The initiator is the core, yes. However, the main requirement in the case of RAT is being able to get a certain local address when given a physical address using the mapping function.

"It translates a 32-bit input address into a 48-bit output address. Any input transaction that starts inside of a programmed region will have its address translated, if the region is enabled." -> This is from the TRM linked in the commit here.

It works on a transaction basis, and hence, a function like z_get_local_addr is what it requires? I was not able to find another function that could serve the same purpose in the current API, and so went ahead with this.

@povergoing
Copy link
Member

The initiator is the core, yes. However, the main requirement in the case of RAT is being able to get a certain local address when given a physical address using the mapping function.

"It translates a 32-bit input address into a 48-bit output address. Any input transaction that starts inside of a programmed region will have its address translated, if the region is enabled." -> This is from the TRM linked in the commit here.

It works on a transaction basis, and hence, a function like z_get_local_addr is what it requires? I was not able to find another function that could serve the same purpose in the current API, and so went ahead with this.

Thank you, I got it now.

@povergoing
Copy link
Member

So, it translates the accesses from the core to the peripherals, right? The initiator is still core? Like, add another region based translation layer under MMU/MPU?

Yes, but this must me somehow integrated into our current MMIO API. Also, we could do that but we do not currently have a proper MMU/MPU API so that should be again some custom / soc-specific code so I was wondering whether we could instead leverage the system_mm API for this case to have a proper abstraction layer.

My only concern is, if there will be an issue when this RAT and MMU exist at the same time? And will RAT be configured dynamically? I mean configuring it at runtime, mapping the same 32-bit address to 2 devices alternatively?

@dcpleung
Copy link
Member

dcpleung commented Jul 5, 2023

@dcpleung the system_mm is a bit of an odd-ball, is this a use case that you envisioned for this API?

That API is for interfacing with external MMU/MPU-like hardware that is not architecture specific (e.g. MMU/MPU for ARM and x86).

@carlocaione
Copy link
Collaborator

@dcpleung the system_mm is a bit of an odd-ball, is this a use case that you envisioned for this API?

That API is for interfacing with external MMU/MPU-like hardware that is not architecture specific (e.g. MMU/MPU for ARM and x86).

So I guess it is a good match for the RAT then?

@dcpleung
Copy link
Member

dcpleung commented Jul 5, 2023

Hm... from what I can skim through the doc... for example, K3 is based on Cortex-R5, which has MPU. How would the RAT module and MPU work together? Unless you add a depends on !MPU on the Kconfig, there is always a chance someone will enable them together.

Correct me if I am wrong (as I have only skimmed through the doc), RAT module seems like a limited scope MMU where you can map some (limited) memory region to another memory region (so... "translating" the address from one to another). However, there is no access control, and this is pure address translation. It would help to know what kind of use cases are out there, which would help us to review this.

@dcpleung
Copy link
Member

dcpleung commented Jul 5, 2023

@dcpleung the system_mm is a bit of an odd-ball, is this a use case that you envisioned for this API?
That API is for interfacing with external MMU/MPU-like hardware that is not architecture specific (e.g. MMU/MPU for ARM and x86).
So I guess it is a good match for the RAT then?

Possibly... I would like to see use cases to help me understand the bigger picture.

@vaishnavachath
Copy link
Collaborator

@dcpleung @povergoing @carlocaione , Thank you for the review, feedback, adding some notes to address your questions:

@povergoing

My only concern is, if there will be an issue when this RAT and MMU exist at the same time? And will RAT be configured dynamically? I mean configuring it at runtime, mapping the same 32-bit address to 2 devices alternatively?

RAT will be primarily be used by the M4 cores in the K3 SoCs where there is no MMU available, but from a hardware perspective the R5 in these devices which has MPU can also make use of RAT, but there is no use case where both MPU/MMU and RAT is configured at same time and we would like to support use cases where RAT is enabled independently like for M4. From hardware perspective dynamic reconfiguration of RAT is possible, but again this is also not a valid use case we have seen in the past.

@dcpleung

Hm... from what I can skim through the doc... for example, K3 is based on Cortex-R5, which has MPU. How would the RAT module and MPU work together? Unless you add a depends on !MPU on the Kconfig, there is always a chance someone will enable them together.

Correct me if I am wrong (as I have only skimmed through the doc), RAT module seems like a limited scope MMU where you can map some (limited) memory region to another memory region (so... "translating" the address from one to another). However, there is no access control, and this is pure address translation. It would help to know what kind of use cases are out there, which would help us to review this.

Just clarifying a few pieces, your description is correct, RAT can be thought of as a limited scope MMU which performs only address translation.
K3 (Keystone 3) SoCs is a family of heterogeneous multi-core application processors from TI which usually contains multicore ARM64 application processors (A53, A72), co-processors(R5, M4), DSPs .etc. The device under discussion here AM62 has Quad A53 cores and a single M4 core for which @KarthikL1729 is trying to add support for here: #58722 and the RAT support is a dependency as the M4 does not have a MMU/MPU and the core needs to access memory region outside the 32-bit memory space.

This FAQ also describes the primary use cases with RAT : https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1192281/faq-am62x-am64x-updating-the-region-based-address-translation-rat-settings

Looking at the system_mm description you and @carlocaione provided and https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/mm/system_mm.h#L12 , it seems like that is where the RAT implementation belongs, please let us know if the understanding is correct.

@carlocaione
Copy link
Collaborator

Looking at the system_mm description you and @carlocaione provided and https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/drivers/mm/system_mm.h#L12 , it seems like that is where the RAT implementation belongs, please let us know if the understanding is correct.

In my opinion (not sure what @dcpleung thinks) but you can use the system_mm API to abstract RAT away.

@dcpleung
Copy link
Member

dcpleung commented Jul 5, 2023

It would be great if you can make it work with the existing system_mm API.

include/zephyr/sys/mem_manage.h Outdated Show resolved Hide resolved
drivers/mm/Kconfig Outdated Show resolved Hide resolved
drivers/mm/rat/CMakeLists.txt Outdated Show resolved Hide resolved
drivers/mm/rat/CMakeLists.txt Outdated Show resolved Hide resolved
include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
drivers/mm/rat/ti_rat.c Outdated Show resolved Hide resolved
drivers/mm/rat/ti_rat.c Outdated Show resolved Hide resolved
drivers/mm/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

Please, properly squash the commits.

@KarthikL1729
Copy link
Contributor Author

@carlocaione Apologies, I pushed the new files from my local system without squashing, hence the multiple changes in files created in this commit. Will rebase and push.

@KarthikL1729
Copy link
Contributor Author

KarthikL1729 commented Jul 6, 2023

I have made the changes to use the system_mm API, and this implementation should be functional. It is all in one commit as there is no extension to an API now. @carlocaione I will rebase and push a fix for the typedefs after implementing a workaround. Apologies for the previous commits.

drivers/mm/CMakeLists.txt Outdated Show resolved Hide resolved
drivers/mm/Kconfig Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this file select KERNEL_VM_SUPPORT should be moved out of menuconfig now?

include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
drivers/mm/mm_drv_ti_rat.c Outdated Show resolved Hide resolved
drivers/mm/mm_drv_ti_rat.c Outdated Show resolved Hide resolved
@@ -101,8 +105,11 @@ static inline void device_map(mm_reg_t *virt_addr, uintptr_t phys_addr,
#else
ARG_UNUSED(size);
ARG_UNUSED(flags);

#ifdef CONFIG_EXTERNAL_ADDRESS_TRANSLATION
sys_mm_drv_page_phys_get((void *) phys_addr, virt_addr);
Copy link
Member

Choose a reason for hiding this comment

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

I have no objection to this since this is the only use case for now?

Just raise my concern for the future as this is a system API:

  1. MMU/MPU and RAT (or alternative technologies) exist at the same time which is hard to handle.
  2. Suppose we remove the exclusive macro (like z_phys_map firstly, and then sys_mm_drv_page_phys_get, it's still annoying that if we want to remap RAT only, however, we inevitably call MMU remap again.
  3. How do we deal with similar techniques like Arm ATU?

Fortunately, we could think about it later. MMU and RAT existing simultaneously seem like a 2 stage translation which is definitely a tricky thing.

drivers/mm/Kconfig Outdated Show resolved Hide resolved
drivers/mm/mm_drv_ti_rat.c Outdated Show resolved Hide resolved
drivers/mm/mm_drv_ti_rat.c Outdated Show resolved Hide resolved
include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
include/zephyr/drivers/mm/rat.h Outdated Show resolved Hide resolved
include/zephyr/sys/device_mmio.h Outdated Show resolved Hide resolved
@KarthikL1729 KarthikL1729 changed the title kernel,drivers: Add external address translation module and RAT drivers: Add external address translation module and TI RAT support Jul 7, 2023
carlocaione
carlocaione previously approved these changes Jul 10, 2023
Copy link
Collaborator

@carlocaione carlocaione left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments.

drivers/mm/Kconfig Outdated Show resolved Hide resolved
drivers/mm/Kconfig Outdated Show resolved Hide resolved
drivers/mm/mm_drv_ti_rat.c Outdated Show resolved Hide resolved
drivers/mm/mm_drv_ti_rat.c Outdated Show resolved Hide resolved
Added Region based Address Translation (RAT) module driver. Required by
a few Texas Instruments SoCs to fucntion. Uses
sys_mm_drv_page_phys_get() API with device_map() for address translation.

Signed-off-by: L Lakshmanan <l-lakshmanan@ti.com>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi @KarthikL1729!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

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

Successfully merging this pull request may close these issues.

8 participants