Date: Sat, 6 Aug 2016 09:44:27 -0700 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Michal Meloun <mmel@FreeBSD.org> Cc: "freebsd-arm@freebsd.org" <freebsd-arm@FreeBSD.org>, Svatopluk Kraus <skra@FreeBSD.org>, "freebsd-arch@freebsd.org" <freebsd-arch@FreeBSD.org> Subject: Re: INTRNG (Was: svn commit: r301453....) Message-ID: <1d63e3aa-1a2f-992f-ae83-656eb185d386@freebsd.org> In-Reply-To: <57A5F480.20309@FreeBSD.org> References: <201606051620.u55GKD5S066398@repo.freebsd.org> <CANCZdfpz=z3gc3pyb_Qssa3vGJSnPv_r6J-SWDPPpE9zPYB9=w@mail.gmail.com> <ab44ddb1-515b-94ac-6b12-673b7c53d658@freebsd.org> <57976867.6080705@FreeBSD.org> <f2edac8f-2859-cd98-754e-881e2b2d1e63@freebsd.org> <5798E104.5020104@FreeBSD.org> <a5d43044-1733-6cc7-2e99-e85b60b0fcf3@freebsd.org> <579A25BB.8070206@FreeBSD.org> <30790e40-58b4-3371-c0f0-b7545571f389@freebsd.org> <579AFFC5.1040005@FreeBSD.org> <eb603349-eb88-866d-7a26-9e026518fd39@freebsd.org> <579CD355.1050203@FreeBSD.org> <460fa0b3-ddb7-6247-2412-3d75a589d5e7@freebsd.org> <579CF7C8.1040302@FreeBSD.org> <24107713-6d50-c21d-ccf1-7dbdb36cc484@freebsd.org> <579E1BE2.7020500@FreeBSD.org> <7f053bb8-ab03-e46c-1c72-d757348e4e54@freebsd.org> <cefdfaab-a95f-2a92-89bd-3d0cef2a75ab@freebsd.org> <57A09F34.4050400@FreeBSD.org> <ad1e6337-468e-f35d-7454-444a561cb103@freebsd.org> <57A30B72.7070809@FreeBSD.org> <1946069a-d0f9-2c19-80a5-0b490682574b@freebsd.org> <57A5F480.20309@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 08/06/16 07:30, Michal Meloun wrote: > Dne 04.08.2016 v 14:34 Nathan Whitehorn napsal(a): > [snip] >>>>>>> ------------ The following is a large parenthesis >>>>>>> ------------------- >>>>>>> >>>>>>> One other possible route here that would also work well would be to >>>>>>> make ofwbus.c MD (it's a trivial piece of code anyway, so we don't >>>>>>> gain a lot by sharing it) and implement ofw_bus_map_intr() >>>>>>> locally as >>>>>>> an ofwbus bus method. Then you could have the mapping table >>>>>>> stored in >>>>>>> ofwbus's softc -- the API was designed for this initially. You would >>>>>>> need MD extensions for doing PIC registration there (which is fine), >>>>>>> but that segregates all the OFW-specific information into >>>>>>> OFW-specific code and would let the bus methods and the OFW >>>>>>> interrupt >>>>>>> mapping table interact cleanly in the same place. This still >>>>>>> preserves the pre-r301453 API, makes PowerPC work, and maybe >>>>>>> address's skra@'s concern about extensibility and letting core >>>>>>> interrupt code know about FDT (or ACPI). I'd be happy to mock >>>>>>> this up >>>>>>> as well if you think it's a good approach. >>>>>>> >>>>>> [snip] >>>>>>> This has the following features: >>>>>>> - Existing OFW API and semantics unchanged >>>>>>> - As such, PowerPC, PCI, etc. work fine with no changes >>>>>>> - Details encapsulated in MD code, so individual platforms can >>>>>>> implement this however they like >>>>>>> - arm/arm/intr.c (or whatever) only needs a method to allocate a >>>>>>> fresh interrupt, with no state, and anoter to set the device_t >>>>>>> for an >>>>>>> interrupt sometime later. >>>>>>> - The internal table in the platform interrupt code has no knowledge >>>>>>> of any mappings whatsoever except having the appropriate device_t >>>>>>> for >>>>>>> the PIC stored with the interrupt vector. >>>>>>> - Device tree bits handled purely in device tree code >>>>>>> - No action need be taken on any mapping until the interrupt is >>>>>>> actually allocated/set up, like r301453 >>>>>>> - Easy to add more mapping mechanisms (e.g. ACPI) by having similar >>>>>>> enumeration-mechanism-specific code in the root bus for that mapping >>>>>>> mechanism. >>>>>>> >>>>>>> -------------- End parenthesis ------------------------------- >>>>>> Here's an implementation of the parenthesis I wrote on an airplane >>>>>> this afternoon. It should be complete, though has not been tested. >>>>>> The >>>>>> code is short and simple (+70 lines in ofwbus.c). This preserves the >>>>>> pre-r301453 API and semantics relative to drivers, which means >>>>>> PowerPC >>>>>> and PCI work out of the box, while keeping the semantics relative to >>>>>> the interrupt layer of r301453 (PIC methods only called on resource >>>>>> allocation, no allocatable IRQs on unattached PICs, encapsulation of >>>>>> OFW-specific code in OFW-specific bits of the tree). It turns out >>>>>> those two things are compatible, somewhat to my surprise, and that >>>>>> makes the result very clean. I like this approach and would be happy >>>>>> to move forward with it. There are five functions of interest: >>>>>> >>>>>> 1. OFW_BUS_MAP_INTR(). This has the semantics and API it has now: you >>>>>> pass an interrupt specifier and parent, you get back an IRQ. No >>>>>> changes. This is the core of the normal OFW interrupt API. >>>>>> >>>>>> 2. OFW_BUS_REGISTER_PIC(device_t pic, phandle_t phandle). This is a >>>>>> new function that PIC drivers are supposed to use to register control >>>>>> of an interrupt domain. This replaces machine-specific code like >>>>>> powerpc_register_pic() to allow the PIC table to be in a bus parent >>>>>> rather than in the interrupt core. >>>>>> >>>>>> 3. PIC_MAP_OFW_INTR(device_t pic, int irq, interrupt specifier). This >>>>>> is a new function that PIC drivers that know how to handle >>>>>> device-tree >>>>>> interrupt descriptors implement (analogous to various existing ones >>>>>> that vary by platform). It tells the PIC that the given abstract IRQ >>>>>> means the given opaque interrupt specifier. >>>>>> >>>>>> 4. arm_allocate_irq(int suggested). This allocates a new IRQ number >>>>>> not (yet) attached to a PIC, as in r301453. I've added a parameter >>>>>> that lets you pass a suggested number to try in case it is >>>>>> possible to >>>>>> make it match an interrupt pin or something for human-readability. >>>>>> >>>>>> 5. arm_set_pic_for_irq(int irq, device_t). This tells the MD >>>>>> interrupt >>>>>> layer to associate a given PIC device_t with an IRQ. That is all the >>>>>> information the MD layer ever has about the IRQ mapping. >>>>>> >>>>>> Functions #1 and #2 are now implemented completely in ofwbus.c: there >>>>>> are no callouts anywhere else and the interrupt mapping table is >>>>>> maintained entirely internally to ofwbus (in its softc). In order to >>>>>> implement ACPI, or some other strategy, you would implement >>>>>> analogs to >>>>>> functions #1 and #2 that live somewhere in the bus hierarchy that is >>>>>> guaranteed to be above all devices that might want that type of >>>>>> interrupt (e.g. in acpi0), and some analog to #3 that PIC drivers >>>>>> implementing the mapping scheme would provide. >>>>>> >>>>>> Since the system interrupt code has no knowledge at all of interrupt >>>>>> mapping, of any type, in this scheme, adding new mapping types is >>>>>> trivial and can be done on a driver-by-driver basis if necessary >>>>>> without changing KPI and without any other part of the system even >>>>>> being aware. For example, GPIOs can use a completely different >>>>>> mechanism if they want and can do setup purely in the GPIO controller >>>>>> driver. You could have a method GPIO_GET_INTERRUPT_FOR_PIN() on a >>>>>> GPIO >>>>>> controller in which the GPIO controller allocates a generic IRQ, >>>>>> assigns through some internal table just in the GPIO driver, and >>>>>> returns to it to a consumer in some other device driver -- without a >>>>>> GPIO mapping type, new bus functions, or modifications to the >>>>>> platform >>>>>> interrupt code. >>>>>> >>>>>> The control flow goes like this: >>>>>> - Bus driver enumerates children, parses interrupts properties, calls >>>>>> OFW_BUS_MAP_INTR() to get IRQs for them (as pre-r301453), adds to >>>>>> interrupt list. >>>>>> - ofwbus receives the OFW_BUS_MAP_INTR() call, allocates a blank >>>>>> disconnected IRQ from the MD interrupt layer, and stores the mapping >>>>>> from the new IRQ to the given interrupt specifier and phandle in an >>>>>> internal table in ofwbus's softc. >>>>>> NB: Nothing else happens here, like post-r301453. Changing >>>>>> this does >>>>>> not change any semantics of the API pre-r301453, which means it >>>>>> remains fully compatible with PCI and PowerPC. Also, like >>>>>> post-r301453, there is no involvement of nexus. >>>>>> - PICs attach and call OFW_BUS_REGISTER_PIC(). ofwbus receives these >>>>>> messages and adds a (device_t, phandle_t) mapping to a second >>>>>> internal >>>>>> table. Note that the interrupt layer does not need to handle PIC >>>>>> registration anymore at all (except for the root PIC). >>>>>> - Bus child eventually calls a function that tries to set the >>>>>> interrupt up (e.g. bus_setup_intr()). That propagates up the bus >>>>>> hierarchy, eventually getting to ofwbus. ofwbus notes the IRQ number, >>>>>> looks it up in the table, looks up the appropriate PIC from the PIC >>>>>> table, then: >>>>>> A) calls arm_set_pic_for_irq(irq, pic_device_t) -- this is the >>>>>> interrupt layer's only interaction with the mapping code. All it >>>>>> deals >>>>>> with is device_ts and abstract IRQ numbers. >>>>>> B) calls PIC_MAP_OFW_INTR(pic, irq_number, interrupt-cells, >>>>>> interrupt-specifier) to tell the PIC that the interrupt layer's IRQ >>>>>> irq_number means the given specifier >>>>>> C) finally, passes the call onto nexus, which will do whatever >>>>>> would >>>>>> normally happen (unmasking the interrupt, setting handlers, etc.) in >>>>>> terms only of the abstract IRQ and the device_t assigned by ofwbus. >>>>>> >>>>>> You would implement ACPI just by doing a s/OFW/ACPI/g >>>>>> search-and-replace above -- since the interrupt layer doesn't know >>>>>> about OFW or ACPI or anything else, there is no need to touch it. >>>>>> This >>>>>> seems clean, simple, compartmentalized, preserves the existing API, >>>>>> and should work on all of our various hardware. PowerPC can't quite >>>>>> work with it yet without some multipass foo, but, since the API is >>>>>> preserved, that transition can happen gradually without KPI changes. >>>>>> For the same reason that it is API-preserving, I think this code is >>>>>> also MFC-able. >>>>>> -Nathan >>>>> I think that we are slightly diverges in this place: >>>>> - please note that PIC API (in kern/pic_if.m) is different from the >>>>> one >>>>> that PPC uses. >>>> Yes, which is fine (this is machine-dependent code anyway). On >>>> consideration, the PIC_MAP_OFW_INTR() function should probably be >>>> called OFW_BUS_PIC_MAP_INTR() and live in ofw_bus_if.m anyway, then be >>>> implemented by PICs. >>>> >>>>> - we must be able to store configuration data (original interrupt >>>>> specifier) in all cases, not only for OFW. That's reason why I have >>>>> proposed >>>>> to create 'mapping table'. >>>> It is stored in all cases, just not in the core interrupt code. I've >>>> only mocked this up for OFW here. For ACPI, you would have the >>>> equivalent table in acpi.c, along with ACPI_MAP_INTR(), >>>> ACPI_REGISTER_PIC(), etc. in acpi_if.m and implemented in >>>> dev/acpica/acpi.c. >>>> >>>> For GPIOs, you would have a mechanism that just traces however you are >>>> representing GPIOs anyway. >>>> >>>>> Anyway, i think that we both talking about +/- same solution, only i >>>>> want to move it from OFW specific level implemented at OFW bus >>>>> level to >>>>> bus/interrupt specifier format independent level implemented in >>>>> subr_intr.c. >>>> This implements the same API in any case (identical to that >>>> pre-r301453), so the implementation doesn't really matter at all in >>>> terms of my needs. >>> Perfect. >>>> That said, having it in the root bus for the mapping domain (ofwbus0, >>>> acpi0) etc. seems cleaner to me, with no loss of functionality. The >>>> core interrupt code (subr_intr.c or whatever) doesn't have any obvious >>>> need to know anything at all about the mapping information so long as >>>> it knows the PIC device_t that corresponds to every abstract IRQ so >>>> that it can mask it or do other operations. Since, presumably, nothing >>>> in an ACPI device tree references an interrupt in the OFW device tree >>>> (if you even had both -- and how would you specify that, anyway?), >>>> implementing the relevant bits one step up the bus hierarchy doesn't >>>> change any behavior. >>>> >>>> And nothing can possibly be more flexible in terms of the mapping >>>> table in subr_intr.c than not having a mapping table: you don't need >>>> enums for mapping types, or unions that need to be expanded, breaking >>>> KBI, or anything. You can delete all the PIC registration (and MSI) >>>> code, all the switches, and all the unions from subr_intr.c and have a >>>> totally mapping-mechanism-agnostic KPI. >>>> >>> Where you see "all the switches, and all the unions" in subr_intr.c? >>> subr_intr.c uses nested structures approach, in exactly same way as is >>> used in OFW bus. Moreover, subr_intr.c *IS* currently totally >>> mapping-mechanism-agnostic KPI, and any form of mapping table must >>> follow this rule. >> That was hyperbolic; my apologies. >> >> The point was that you don't need intr_map_data, or the >> intr_map_data_type enum, this way. One of the nice things about that >> is that you don't need to add more entries to the enum to add new >> one-off mapping types. For example, the PS3 platform on PPC has its >> own non-device-tree, non-ACPI bus description and interrupt system. >> Needing to make new enums (or anything) like INTR_MAP_DATA_PS3 in >> sys/bus.h seems a little silly, though hardly very bad. > Hmm, right. > Is something like <machine/_intr.h> solution for you? Or just the existing machine/intr_whatever_it_is.h. >> I've attached a version of PowerPC's intr_machdep.c and pic_if.m that >> fully implements this distributed-table scheme. The code ends up being >> very short and clean (a third the number of lines of code as >> kern/subr_intr.c, for example) and the diff to PowerPC, including the >> new ofwbus.c code, ends up being net-negative. > Yep, I read it very carefully. >>> But allow me to make short summary: >>> - we must support 3 'main' cases - OFW, ACPI and HINTS (for older arm >>> and mips boards) based systems >>> - single kernel must support all above cases, but only one can by >>> 'active' (kernel must be able to select right one at runtime). >>> - we must support 'direct' (interrupt specifier is defined by one of >>> above methods) or 'indirect' (where interrupt is associated with other >>> function - GPIO pin, but don't have direct description). >>> - we must be able to access single physical pin by both methods, >>> 'direct' and/or 'indirect'. >>> - we must be able to add new interrupt type as simple as possible >> Agreed with all of this. >> >>> For interrupt controllers: >>> - single controller must be able to parse multiple formats. 'direct' or >>> 'indirect'. OFW or ACPI. ARM GIC must accept OFW or ACPI arguments (in >>> single kernel), GPIO based interrupt controller must accept 'direct' or >>> 'indirect' formats. >>> >>> For interrupt consumers: >>> - 'direct' interrupts are easy >>> - driver must be able to consume 'indirect' interrupt in 'root >>> bus'/'mapping-mechanism' agnostic manner. >>> For example, MMC driver must be able to get interrupt from CD gpio >>> pin in all possible cases/combinations . >>> Are we still connected? >> And this. >> >>> I don't think that all this is possible without single universal format >>> of interrupt mapping specifier. >>> And, if we must have universal format then why we needs different >>> mapping databases? >> It actually works just fine in this mapping-table-in-the-root-bus >> model. If you have an OFW-type of interrupt, it is set up by ofwbus.c. >> Since everything that would use an OFW interrupt is a child of ofwbus, >> this is totally indistinguishable from a global table from the >> perspective of client code. For ACPI interrupts, they are set up by >> acpi.c, which is indistinguishable from a global table for the same >> reason. For hints -- and single-domain systems generally -- you can >> just have the machine-dependent code assume that interrupts it doesn't >> explicitly know about belong to the root PIC. This is what you call >> the "direct" case. >> >> For the "indirect" case, there are as usual a few things you could do. >> They match the set of things you would do in any implementation: >> 1. Set up one or more global registries for "indirect" PICs, with >> corresponding mapping methods, either as bus methods on some >> high-level bus or on the machine-specific nexus, or just a global >> function that you call. >> 2. Use the global registry that you already have to have for GPIO >> controllers and provide a mapping method on the GPIO controller >> (GPIO_GET_INTERRUPT_FOR_PIN() or something) that returns a mapped IRQ. >> > I still don't think that this can works. > Assume that i have driver with 2 interrupts, one is standard (OFW > property based), second is GPIO, targeting same interrupt controller. > First is allocated by bus_alloc_resource(), second by > gpio_alloc_intr_resource(), so each resource is stored in different table. > Then bus_setup_intr() is called, on both. So bus_setup_intr() must be > able to select right table, right? But how? The interrupt defined by the GPIO controller from a GPIO property (as opposed to an interrupt property) could be defined, in general, in four ways: 1. You have a bus method on the GPIO controller that returns a pre-mapped IRQ. Something like GPIO_INTERRUPT_FOR_PIN(). The GPIO properties I have seen don't have standard names so this would need to be done by the child. As a result, you don't need it to work before PIC attachment (or GPIO attachment) and can directly call methods on the GPIO controller. This kind of API has the nice feature that the GPIO controller can return errors early if that pin doesn't actually support interrupts, which is not a concern with normal interrupts property. The IRQ would be mapped internally by the controller when you do GPIO_INTERRUPT_FOR_PIN() and so ofwbus.c would pass it through to nexus (you'd need to remove a KASSERT from my code), which would then go to the interrupt code, which would see the device_t assigned earlier and pass through all operations appropriately. 2. Identical to the first paragraph of (1), except the bus method resolves to an interrupt-parent, specifier pair passed to OFW_BUS_MAP_INTR() rather than an internal mapping in the GPIO controller. This proceeds as normal for OF interrupts thereafter. API is the same as (1). 3. Some global helper function that transforms GPIO specifiers into some other domain (OF, ACPI) without talking to the GPIO controller. I think this is fragile and a bad idea in any case, though would also work as well (or as badly) here as in any other scheme. 4. A GPIO interrupt domain with specifier, like you have currently. You'd have to implement this mapping table in nexus (potentially as a thin wrapper for some code in intr.c or intr_gpio.c or whatever) as a global table as now or as in the first patch I sent. ofwbus.c would, like (1) pass through such requests. This is a perfectly good interface. So I don't think you lose anything by putting the table in ofwbus.c. All four cases would have the same API no matter where the mapping table is. The only non-idiomatic case is #4, which would require some shims, but those shims are wholly identical to the shims you would need if you didn't do OF/FDT interrupts this way. The larger point is that, since all four cases would have identical APIs in either case, it doesn't much matter how it is implemented now. If we picked one method and wanted to change later, we lose nothing and the change can be MFC-ed at will since it has no effect on KPI. > >> One of the nice things about distributing the tables this way is that >> you have lots of flexibility with things like these "indirect" >> interrupts and are free to do things like #2 at will locally in some >> machine-specific set of drivers. For example, in the PS3 code, I don't >> need to add a new "PS3" mapping type *anywhere*. The ps3bus root >> driver just has a table when it hands out interrupts and talks to >> ps3pic internally. >> -Nathan >>>>> Most of your control flow can be implemented at general level as >>>>> is, or >>>>> already exist [intr_map_irq(), intr_pic_register()] . >>>>> Also, impact for current PPC code is, by me, minimal. >>>>> I can sketch my idea in more details, if you found it acceptable. >>>> All ideas are fine -- and the solution does not need to apply to all >>>> platforms anyway, so long as the OFW_BUS_MAP_INTR() semantics (and, >>>> ideally, API) are preserved. > I have prepared working patch (it's not full, but it works on Tegra), > https://github.com/strejda/tegra/commit/2cf72e248877fb917c4fc618bcb6e46b7c1058a4 > can you, please, take look on it? Change looks great to me at first pass. Thank you for working on it! > Also, please, take in account: > - we have, currently, 20+ interrupt controllers converted to new INTRNG > PIC API. > - universal format of interrupt resources is generic part of this API. > - I'm not ready to commit any PIC API change together with above patch - > i hope that this is understandable. Yes, of course. Also, as far as I can tell, there's no reason to change the PIC API on ARM/MIPS anyway from the current INTRNG API. I think there are some minor simplifications you could do, but there's no need for them, and you can implement absolutely any of the APIs we have discussed in this thread in terms of the current pic_if.m with no problems. > > also, > "we must be able to add new interrupt type as simple as possible" rule > is very important for me. Adding new table (with implementation), one > bus method and one PIC method for each new type is not *simple*. Fair enough! I don't think we need that for, e.g., GPIOs (see cases 1-2 above), just for bus enumeration schemes (ACPI, OFW are probably the only ones) that usually require a ton of this kind of thing anyway. But, fundamentally, it doesn't matter. There are three important things from my end: 1. That it is possible to, at bus enumeration time, permanently assign an IRQ to an interrupt specifier from OFW/ACPI. 2. That that assignment not depend on having the PIC attached yet. 3. That the implementation details of that mechanism be reasonably abstracted so that they can change later or vary platform to platform. Whether mapping tables are in some central place (subr_intr.c) or in the parent bus, how the PIC API works, whether they are stored in that table in the form of a union or in different tables, doesn't matter for those three at all. And, with a constant API (3) we can even change our minds later without a lot of hassle. > In any case, can we concentrate to above patch first? I'm ready to > finish it in next few hours, then put it to phabricator for real review. Sounds good. As I said, first pass looks perfect to me. I'll look in detail once it hits phabricator. -Nathan > > Michal >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1d63e3aa-1a2f-992f-ae83-656eb185d386>