drivers: intc: renesas_ra: Handle interrupts based on event numbers #75946
Conversation
soburi
force-pushed
the
ra_icu_isr_table
branch
4 times, most recently
from
d52ac91
to
dcd9671
Compare
zephyrbot
added
area: UART
zephyrbot
requested review from
57300,
carlocaione,
duynguyenxa,
galak,
ithinuel,
jeremybettis,
KhiemNguyenT,
MaureenHelm,
microbuilder,
nordicjm,
stephanosio,
tejlmand and
thaoluonguw
|
Adding a DNM while I review |
There was a problem hiding this comment.
This PR seems to be solving mapping from the interrupt line number of the UIC to the interrupt line number of the NVIC. This is typically done using interrupt maps in the devicetree. Has this been considered as an option? It seems quite extreme to rewrite the vector generation table and IRQ macros just to introduce this mapping.
See section 2.4.3 of https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.4 (or search for interrupt-map in the source code for examples)
Not all RA ICUs have m:n mapping, but some of them (RA2 family) have m: to set of subsets of n mapping. I.e. UART0 RX ICU line can be assigned to 0, 4, 8, 12, 16, 20, 24 and 28 NVIC lines, but for UART9 RX only 4 lines are valid: 4, 12, 24 and 28. So using the (modified) script is much easier. |
The Renesas RA ICU allows any function to be assigned to each interrupt, but this is a mechanism for efficiently using a small number of interrupts.
Therefore, if we tried to solve this by mapping, we would be required to redefine the mapping for each case, which would be an extremely painful task for users. |
|
I see, let's use the RTC as an example: We want to connect RTC_ALM, RTC_PLD, and RTC_CUP (38, 39, 40) to NVIC interrupts 0, 1 and 2: The RTC uses the IRQ number (event) defined by the UIC The proper way of managing this is an explicit interrupt-map. The UIC node presents an interrupt map which maps interrupts 38, 39 and 40 to an interrupt line of the parent: this provides complete flexibility in defining shared interrupts, and mapping UIC IRQ numbers to other nexus like DTC and DMAC, for example, routing GPT0_CCMPA (87) to DTC (this is kind of sudo code though since I don't know what the DTC is exactly, I'm just following the UIC block diagram) This solution is already supported in-tree. It requires no changes to the IRQ infrastructure, and is compliant with the devicetree specification. I don't believe there is any "getting around" the complexity of the interrupt infrastructure. The best option IMO is to model it correctly using the devicetree. Then, on top of the devicetree, a script can be added to automatically generate a naive The script would generate the following devicetree snippet which replaces the need for a custom |
|
Removed DNM as nack is sufficient while discussing |
Hi @bjarki-andreasen it looks like you have missed my previous comment. So let me explain my opinion in more detail:
So, to summarize, if customer's application is complex enough, and is using almost all NVIC's lines, then make a mistake with wrong IRQ assignment is a piece of cake. Nevertheless if you have an idea on how to implement the above mapping without significant difficulty for customers, you're welcome to share it. I've given up and would prefer to modify the PS Btw using the |
|
This is because the interrupt controller of the Renesas RA has a different abstraction model from the one assumed by the Zephyr, and it is a problem that requires a new design rather than an extension of the existing design.
I got how to use this feature.
The problem with this proposal is that the dts fragment cannot be generated until the interrupts that will actually be used are determined, that is until generating zephyr_pre0.elf, making it practically impossible or requiring a fundamental reconstruction of Zephyr's build sequence. In the development environment provided by Renesas, this is handled by automating it on the IDE side. This SoC implicitly demands such a solution.
We need to find a "workaround" to not impose an excessive burden on at least the users of this SoC. In this PR, the changes are almost all concentrated in an independent Python script, |
|
@avolkov-1221 Regarding the map This is how the interrupt map could be created: This is the parent domain: it is mapped through the ICU, which is the child domain Let's create the lookup table. We have two interrupt cells in the parent domain, in this order:
We have three interrupt cells in the child domain, in this order:
With this interrupt map, the following will route event 1 to nvic line 0 and the following will map event 0 to nvic line 0 which, if you don't support shared interrupts, will produce a build error as both foo and bar are trying to use the same nvic line. It also allows for mapping multiple events to multiple potential nvic lines, like event 4 which can be routed to nvic lines 0 and 1: The devicetree is already designed to handle this properly, and you will have to keep the information regarding which line of the icu goes to which lines of the nvic somewhere anyway. On a more general note: The solution in this PR presents a layering violation... The ISR table generation has nothing to do with the UIC... The ISR table is generated for the top level interrupt controller, the NVIC. enabling irq number 0 means enabling the NVICs IRQ number 0. If you need to do some mapping from the UIC domain to the NVIC domain, that is done with a map. If you need to configure the UIC to route its interrupts to the parent NVIC, that is done in SoC/driver level code, using the devicetree. On top of the layering violation, allowing every vendor to add a completely custom script for generating ISRs will lead to a huge amount of code debt and duplicate code. |
@bjarki-andreasen Yes, you are repeating exactly my path with ints maps and masks :). Unfortunately this is wrong one: Let's consider the following example: some application needs 3 SCI modules (SCI0, SCI1 and SCI3), 1 CAN, 1 ADC input, 1 GPT0 CCMPA input and PORT0 pin0 edge interrupt. And it will be using, let's say, only 7 lines from these IPs (the real life situation is more complex): SCI0_RXI, SCI1_RXI, SCI3_RXI, CAN0_ERS, ADC120_ADI, GPT0_CCMPA and PORT_IRQ0. All these peripherals have common NVIC's group0, and some of them also belongs to the group4, so if you try to use your proposed DTS interrupt mapping, the group0 or group4 will overflow, and you'll get error. What's wrong: the second group still have free lines. The interrupts assignment must be more smart: actually groups have different weights defined by the number of all used lines connected to them. AFAIK this information can't be used by the current device tree infrastructure, fix me please, if I wrong. And the assignment must be started from the group with less weight. In this case, the result of the assignment will look like this (the lines within the group are assigned in order from low to high): For different sets of IPs and for the different peripherals initialization sequences, the mapping will vary, and the complexity of IRQs manually assignment mapping grows non-linearly with the number of peripherals. So, for RA2 family, due to lack of above logic implementation in Regarding additional 3rd parties scripts, we're on the same page. Specifically, I have already shared my point of view here: #77531. However, this case unfortunately seems to be an exception so far. Making changes in global |
|
@avolkov-1221 To summarize my point, the is issue in fact not at the ISR table generation level, and the devicetree specification already provides the interrupt-map, which is the way to map interrupts from one domain to another. Yes, it may be inconvenient for the user to have to check the reference manual to figure out the routing, but that is the way to do it. This is common for DMA channels, memory regions, pinctrl maps and other "non user friendly" configs. Practically, a vendor using this SoC will provide drivers for specific peripherals, and a default layout with some of them enabled, and routed validly. A user who wants to use other peripherals than the default ones, will have to make a conscious decision which peripherals the user can live without. Have you considered what happens if a user tries to enable conflicting peripherals? In this case, the user must look into the reference manual to figure out how to solve it in any case. If you truly want to help the user, create a SoC level script which looks through the interrupt-map for an SoC, and uses this to generate some visualization of the existing peripherals and possible combinations. You need to store the interrupt-map somewhere anyway (for validation and use in your script), might as well take it directly from the devicetree. |
@bjarki-andreasen Sorry, but it's obvious. It's the responsibility of any embedded software engineer. And I disagree with what the vendors have provided. For example, Renesas officially actively ignored Zephyr's existence until last year (btw I've been happily living without Windows for decades, but vendors often think that 'Windows based' is a killer feature of their BSPs). Besides copy and paste evaluation boards hardware are rare in the embedded world of entry/middle-level chips. Also the quality of drivers provided by vendors, well, is not always 'good enough'. I remember a case when Freescale broke my driver from the stable Linux kernel in their BSP. So the developer will create/modify the DTS (and driver) provided by vendor by himself. In the case of this chip, our team have been end users for years. And modification interrupt mappings is a headache (certainly we have some tools for it), and this is despite the fact that our projects are quite close in hardware.
But this is exactly the scope of this PR. The script (located in soc/renesas) is executed only for one precise task: to create the LUT and generate the static mapping. Ie to fill the functionality gap of device tree infrastructure. Initially, I thought subclassing from the global
Indeed. The group mapping information is encoded in the IRQ numbers. More precisely, in the new header And this info is used by my modified script derived from this PR. Besides the device tree for the RA2, based on this numbering scheme become very close to any other device tree: As result the generic RA serial port driver |
This is where I disagree, there is not a gap of functionality in the device tree infrastructure. The devicetree is fully capable of creating the map, and mapping out the interrupt lines, and IRQ_CONNECT will indeed be able to get the correctly mapped IRQ number. This PR is adding, not just a new way of defining and configuring interrupts, but opens up the door for all vendors to add their own custom solutions for something that is indeed generic, and well supported by the existing infrastructure. The reason zephyr is useful at all, is that it supports multiple platforms using common infrastructure. The fact that you have to break that infrastructure to provide support for your SoC should be an indication that you may be doing something wrong, it works for every other platform... |
The current devicetree, as I tried to explain earlier, is not capable of mapping to the pool of interrupts, resolving assignment conflicts, or helping the customer avoid bugs in complex cases. Btw, could you explain in more detail your idea about the 'SoC level script'? Is it a post-/pre-processor for DTS, As a compromise (and probably worst) solution, this PR's script also could be moved to the HAL. IMHO, a huge manual handled
Could you also add your opinion as a comment to #77531? Because the current situation with modules is exactly what concerns you, and unfortunately, it has already become a reality. Your statements regarding this PR should ideally be discussed too. That and/or this topic(s) may be discussed at one of the upcoming TCS meetings. |
bjarki-andreasen
added
the
Architecture Review
|
We definately need to discuss this in a larger forum, added the arch review label.
I believe you may be overstating the complexity of the interrupt-map, and the amount of work needed for the hardcoded items. The seemingly autogenerated header renesas-ra2-icu.h is also hardcoded, just like the interrupt-map would be. From the perspective of a device which is a child of the UIC, they only information needed is the group, event id and flags, which is gotten from the reference manual, (or by looking at the interrupt-map itself which contains the exact same information).
It would use the interrupt-map (which is "hardcoded") as an input, just like a user and any SoC level driver for the UIC would, and could help a user identify which specific groups can be used for which peripherals. Validating that the specific interrupts defined by the user are valid would be done by static asserts in the SoC level driver for the UIC. I'm not sure this script would be of much use, a good README or reference manual explaining the groups and how they map to the peripherals may be more then enough for a user. |
Nice. Thank you. Sept, 03 or a week later?
The main difference between the map and the header (which is certainly based on information extracted from the datasheet using some scripts) is that the header is generated only once and remains almost constant 'till SOC's EOL, whereas the map is regenerated with each hardware modification and updated manually (welcome the human-factor related bugs). Don't forget also about the map sizes for the topmost members of the RA family (up to 96 items for RA8m1).
Small correction: an ICU line can belong to multiple NVIC's groups. And the groups info can't be used at the devicetree level. It's merely an additional condition for selecting from the pool of target IRQs. If assignment from the IRQ pool is not supported, then the group is useless either. Ah, and flags are not supported by IRQ_CONNECT, issue #75851
I've just figured that some devicetree intermediate postprocessor script, which generates a map based on the interrupts used in the DTS, could be a reasonable compromise. |
Should be Sept, 03
I don't believe this is the case, the This map remains constant for the lifetime of the hardware. INTA can be connected to IRQ 1 of PCI slot 1, and IRQ 1 of PCI slot 2, these are the possible physical connections. Modifying the sample to be a bit closer to the UIC (the first child interrupt specifier is group, second is line): All of the information is constant, and only possible combinations of group/line can be selected from the map. Unless shared interrupts are enabled, attempting to route two ICU lines to the same NVIC IRQ will produce a build error, and can be detected by the UIC SoC level driver as well. The only change made by a user, if required, is switching which mapping is used: or trying to route UIC line 0 to NVIC IRQ 1 would fail: |
|
I have done my best to make the devicetree snippet match the table you showed earlier here: The specifiers in the map are: With this, we can only apply valid mappings according to the groups, let's connect event PORT_IRQ0 to NVIC IRQ line 4, and DTC_COMPLETE to NVIC 0. Note here that PORT_IRQ0 can be connected to NVIC IRQ 0 (group 0), 4 (group 4), 8 (group 0), 16 (group 0) and 24 (group 0). We choose to use NVIC IRQ 4 since we also want to use DTC_COMPLETE. |
You are wrong here: you continue to treat the ICU as an n:1 controller, but it's actually a 1:n one. To differentiate the source lines, a bitmask of valid groups should be added as a part of source line ID. Then, the map of the first 14 lines would look like this The problem here is that there are 154 ICU lines, but only 32 grouped valid NVICs, so adding or replacing any peripheral makes the map invalid, since each ICU interrupt line has predefined groups combination, and removing one IP doesn't mean that another can be used instead (even of the same type). Some hardware combinations make the mapping impossible. Updating it correctly manually without a special tool (like the Renesas-provided e2 with FSP) is not so easy task. Therefore, if we add a mapping script, the above mapping can be generated and verified automatically in time of DTS postprocessing, or, easier, in linking time. NB: this nightmare exists only for RA2 series, all other (RA4, RA6 and RA8) have more adequate ICU, where any ICU line can be assigned to any free NVIC one. Limitation only in relation of ICU to NVIC lines numbers (474:96 accordingly in one of the topmost RA8m1). I.e. for them, the script should perform the mapping until an NVIC overflow occurs. But changing the peripheral set will invalidate the map in any case: you'll need to manage the free lines counter somehow, and you can't use a static NVIC mapping, as the NVIC line numbers depend solely on the number of chosen IPs. New IPs combination == new NVIC table. |
IMO the situation here is worse than before: you have a static map, but variable IP's nodes probably spreaded over many .overlay and .dtsi files. And the problem is the same: NVIC's lines are constant. What's not true, the ICU lines can be static, not NVIC's ones (check my previous example for the first 14 lines). |
Exactly, that's an accurate model of this SoCs hardware. As I mentioned before, this is the case for pinctrl, DMAs, memory regions, etc. These are also modified by the "user" to match their usecases, and have even more strange constraints like which memory region can a specific DMA read from, and which specific peripheral can use which specific DMA channels. Can we at least agree that the above interrupt-map is a viable solution to correctly routing any peripherals event to any valid NVIC channel? |
Yes sure, and I didn't really claimed otherwise :). I have stated and continue to state that:
Sorry, but updating pinctrl and DMA maps doesn't require rewriting most of the items; usually, only those related to the changed hardware are enough, and it's not as painful as dealing with RA2 IRQs. So using them as an example here is not quite appropriate. Thus, the question about of some auxiliary script remains unresolved. Rewriting a significant part of the very error-prone map, with every small change of hardware is not something that will attract customers. Especially if we want to have "zephyr is useful at all" :). Btw it seems that the script could be more universal and not so RA-specific, making it usable for other interrupt controllers where 'IRQ pool' assignment is required. Besides I don't have a better term for this type of assignment, and I'm not sure that it's correct. If anyone has one, please correct me. |
I simply disagree with this statement. This is where a DMA channel is selected and configured, in a single location, on a single device node: This is where a free NVIC line is selected and configured, in a single location, on a single device node: This is where free pins are selected and configured, in a single location, on a single device node: True for all of them is that selecting a specific DMA/IRQ/PINs must be done with consideration to the rest of the devices in the system, and validity of the configuration (can a specific pin be routed to RX of USART3 at all). The interrupt-map actually makes it easier to select valid configurations than DMA channels and PINs which have no constraints on them at all in the devicetree. |
@bjarki-andreasen Well, I've tried to implement the |
If we agree to use the devicetree interrupt-map, I will definitely help ensure edtlib functions as intended :) I'm learning as we discuss here as well, it will be nice to try and actually implement it :) |
I've created an issue: #77890. To simplify the discussion. |
|
Architecture WG:
|
The Renesas RA interrupt controller does not correspond to an interrupt number and function but is configurable by the user.
Zephyr's isr_table is designed under the assumption that interrupt numbers are assigned to specific functions, making this difficult to support.
We will extend gen_isr_table.py to create a table by specifying the function(event) number instead of the interrupt number.
When creating a table with function(event) numbers, interrupt handlers are assigned starting from interrupt number 0 in the order they are found during linking. At this time, it will also create a table that allows us to reference the assigned interrupt number from the function(event) number.
This is similar behavior as Renesas FSP's code generator.
By receiving event numbers instead of IRQ numbers and converting them internally to IRQ numbers, a unified interface based on event numbers can be created.
That is, it make API calls using event numbers rather than raw interrupt numbers.
This allows interrupt definitions in the device tree to also be described semantically by event numbers.
Related to #75440