Date: Sun, 24 Jul 2016 19:09:23 +0200 From: Michal Meloun <mmel@FreeBSD.org> To: Nathan Whitehorn <nwhitehorn@freebsd.org>, Svatopluk Kraus <skra@FreeBSD.org>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r301453 - in head/sys: arm/arm arm64/arm64 dev/fdt dev/gpio dev/iicbus dev/ofw dev/pci dev/vnic kern mips/mips sys Message-ID: <5794F643.4070207@FreeBSD.org> In-Reply-To: <8bfd8668-bc49-e109-e610-b5cd470be3ec@freebsd.org> References: <201606051620.u55GKD5S066398@repo.freebsd.org> <b9606755-69cb-2cb0-04d7-6be32e4cb89e@freebsd.org> <578E0B5D.3070105@FreeBSD.org> <e026f6fc-76ed-5dbe-00fc-365b6d7bcf94@freebsd.org> <578F6075.7010500@FreeBSD.org> <05a80ac6-4285-ec9d-36e9-2f92c609f746@freebsd.org> <57907B0F.9070204@FreeBSD.org> <9d2a224c-b787-2875-5984-a7a2354e8695@freebsd.org> <57934ABD.6010807@FreeBSD.org> <4e7a3e8f-cc21-f5f2-e3e0-4dbd554a4cd0@freebsd.org> <5794720F.4050303@FreeBSD.org> <8bfd8668-bc49-e109-e610-b5cd470be3ec@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Dne 24.07.2016 v 17:32 Nathan Whitehorn napsal(a): > > > On 07/24/16 00:45, Michal Meloun wrote: >> Dne 23.07.2016 v 20:35 Nathan Whitehorn napsal(a): >>> >>> >>> On 07/23/16 03:45, Michal Meloun wrote: >>>> Dne 21.07.2016 v 17:53 Nathan Whitehorn napsal(a): >>>>> >>>>> On PowerPC, GENERIC64 supports FDT systems (some IBM hardware), >>>>> OFW systems (Macs, some IBM hardware), systems with no device >>>>> trees at all (old-style PS3), and systems with a mixture of device >>>>> tree and no device tree (new-style PS3). On these, there is a >>>>> mixture of "real" interrupts and GPIO-type interrupts. There is no >>>>> limitation that this be used only for device-tree-type systems. >>>>> >>>>> The system requires two things: an interrupt domain key and an >>>>> arbitrary unique byte string describing the interrupt. When >>>>> running with a device tree, these are set to the phandle of the >>>>> interrupt-parent and the contents of the device tree interrupt >>>>> specifier, respectively, and the system was of course developed >>>>> with that in mind. But they don't need to be, and often aren't. >>>>> You could make the domain an element of an enum (where "ACPI" is a >>>>> choice, for instance -- this is what PS3 does), or set it to a >>>>> pointer to a device_t, or really anything you like. Similarly, the >>>>> interrupt specifier is totally free-form. >>>> >>>> Yes, I agree. and i think that we followed the same direction. But >>>> i see two problems with this approach. >>>> - in some cases (OFW, device_t) you don't have control over >>>> domain key value, so you cannot guarantee its uniqueness. >>>> Of course, probability of collision is low, but it is. >>> >>> We could solve this in a number of ways, for example widening to 64 >>> bits, or adding another value (domain type, for example). You could >>> also make an acpi_bus_map_intr() to go with the OFW one that connect >>> in some machine-dependent code if you have fundamentally >>> incompatible bus enumeration mechanisms that you expect to exist >>> simultaneously -- but, of course, no systems like this seem to >>> actually exist, so the problem is both easily solved and totally >>> theoretical. >>> >>>> - within ofw_bus_map_intr() (or later - at the time when byte >>>> string must be decoded) you are not able (easily) to differentiate >>>> between different formats, thus you are not able to select >>>> appropriate decoder. The GPIO controller, for example, >>>> must be able to handle interrupts defined by standard OFW >>>> property, or by <device_t, pin number> pair concurrently. >>> >>> In principle, you could solve that as above, or by registering a >>> second interrupt domain for the same controller. >>> >>> In practice, it doesn't matter since, in the GPIO case, for example, >>> the GPIO controller is never itself also a normal interrupt >>> controller (i.e. the GIC and GPIO controller are always different >>> devices). As such, the theoretical does not occur in practice. >> form >> https://svnweb.freebsd.org/base/head/sys/gnu/dts/arm/tegra124-jetson-tk1.dts?revision=295436&view=markup#l1380 >> "interrupts = <TEGRA_GPIO(H, 4) GPIO_ACTIVE_HIGH>; " >> Do you want more examples ? > > Those have the identical format to the GPIO properties, because they > are the same thing. So it works out of the box. Do you have examples > of something that *doesn't work*? > >> >>> >>>> For this reason we makes domain key composite, in our model, the >>>> domain key consist of "domain key type" >>>> and "domain key value". This composite key guarantees uniqueness >>>> and it also allows to select proper parser for byte string. >>> >>> Yes, but this solves what is a nonexistant problem by making the >>> system substantially less flexible and more invasive. Which is not a >>> good tradeoff. >>> >> I think that existence of problem is confirmed in the above example . >> Quote from previous paragraph: >> "We could solve this in a number of ways, ... , or adding another >> value (domain type, for example)." >> What can I say more ... > > Except that the example you gave *is not an example* of the problem > you are describing. You would only end up with a problem if: > 1) You had interrupts in a GPIO property rather than in an interrupts > property (or equivalent). > 2) *And* you had interrupts on GPIOs in an interrupts property (or > equivalent) > 3) *and* those are encoded differently > 4) *and* the different encodings use the same number of cells > 5) *and* are not otherwise distinguishable > > Does that ever actually happen, in the real world, on any device tree? > You could imagine any kind of messed up thing you want, but we > shouldn't structure APIs around them, especially given that the > current alternative you are proposing has real, concrete problems on > real hardware that actually exists. > >> >> > > [snip] > >>>> >>>>> >>>>>> >>>>>>> >>>>>>>>> It is much easier to do this correctly at bus attach time when >>>>>>>>> the resource lists are made (how PPC does it). >>>>>>>>> >>>>>>>> I don't agree. I don't agree. Making this at bus attach time >>>>>>>> leads into complicated 'virtual' IRQ infrastructure, with many >>>>>>>> unresolved corner cases. >>>>>>> >>>>>>> Which unresolved corner cases? This has been working correctly >>>>>>> on a number of platforms in both FreeBSD and Linux for many years. >>>>>> Nope, it doesn't work for ARM yet (for GPIO interrupts for >>>>>> example) and Linux uses EPROBE_DEFER mechanism for a long time. >>>>>> See: http://lxr.free-electrons.com/source/drivers/base/platform.c#L87 >>>>> >>>>> There is some missing code on ARM (probably about 30 minutes of >>>>> work to make it match PowerPC) to make it work in an ideal case, >>>>> sure, but there is no reason you could not go out right now, with >>>>> the existing code, and implement GPIO interrupts by declaring the >>>>> GPIO driver as an interrupt controller. >>>>> >>>>> Can you give any concrete case of something that doesn't work? >>>> GPIO again. How you can allocate interrupt associated with given >>>> gpio pin installed by "cd-gpios = <&gpio TEGRA_GPIO(V, 2) >>>> GPIO_ACTIVE_LOW>;" >>> >>> You parse that as an interrupt on a interrupt domain associated with >>> the GPIO controller referenced by &gpio. In pseudo-code: >>> >>> int irq = ofw_bus_map_intr(dev, <&gpio>, ncells, <TEGRA_GPIO(V, 2) >>> GPIO_ACTIVE_LOW>); >>> The GPIO controller, meanwhile, has registered an interrupt domain >>> for <&gpio> and is asked to decode and configure the interrupt >>> defined by <TEGRA_GPIO(V, 2) GPIO_ACTIVE_LOW>, which it knows how to >>> parse. This is simple and straightforward. >> And again and again: >> We have >> "cd-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_LOW>;" >> and >> interrupt-parent = <&gpio>; >> "interrupts = <TEGRA_GPIO(H, 4) GPIO_ACTIVE_HIGH>; " >> in one tree. And we need to get a interrupt in both variants. > > Where, again, those are completely identical and there are not even > two variants, so you are presenting this an example of a problem *that > it does not exemplify*. > >> >>>>>>> I would urge, in the strongest possible terms, that this be >>>>>>> backed out from stable/11 at least. We can add the new API back >>>>>>> for 11.1 if we want it, but we totally lose the ability to >>>>>>> change it later in the stable/11 cycle if it stays in now. >>>>>>> -Nathan >>>>>>> >>>>>> The API is part of still unstable, experimental INTRNG, so its >>>>>> not fixed we we can change it at any time, I think. >>>>>> But yes, we forget to wrap new bus_map_intr() method (and >>>>>> associated code) by #ifdef INTRNG. Is this sufficient for you? >>>>> >>>>> For HEAD, yes. I would like it out of stable/11 entirely until >>>>> this discussion converges. >>>>> -Nathan >>>> The current code (in stable/11) works and is tested. I simple >>>> cannot commit any untested change for stable tree mainly if is in >>>> BETA2 stage. And I cannot fully test the requested change, at this >>>> time i have access to single ARM platform. >>>> But we're in the same situation - both have the same commit bit, >>>> neither one of us is the author of the disputed commit and neither >>>> of us are not able to fully test it. >>>> So feel free to commit what you want, if you have courage to commit >>>> untested code. I haven't it...\ >>>> Michal >>>> >>> >>> Well, the code isn't actually used anywhere in stable/11, or HEAD, >>> so it can't possibly be either tested or important for the >>> functionality of any current code. As such, I will revert from HEAD >>> on Monday and request an MFC from re@ following that if the author >>> has not appeared by that time. >>> -Nathan >> >> The code is, of course, already used by each of INTRNG enabled platforms. >> >> ------------------------------------------------------------------------------------------------------ >> diff --git a/sys/dev/ofw/ofwbus.c b/sys/dev/ofw/ofwbus.c >> index 1e048c4..14eb507 100644 >> --- a/sys/dev/ofw/ofwbus.c >> +++ b/sys/dev/ofw/ofwbus.c >> @@ -323,7 +323,7 @@ ofwbus_map_intr(device_t bus, device_t child, int >> *rid, rman_res_t *start, >> int ncells, rv; >> u_int irq; >> struct intr_map_data_fdt *fdt_data; >> - >> +printf(" *** %s: bus: %p\n", __func__, bus); >> node = ofw_bus_get_node(child); >> rv = ofw_bus_intr_by_rid(child, node, *rid, &iparent, >> &ncells, &cells); >> if (rv != 0) >> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c >> index af3ca57..b381163 100644 >> --- a/sys/kern/subr_bus.c >> +++ b/sys/kern/subr_bus.c >> @@ -3960,6 +3960,7 @@ int >> bus_generic_map_intr(device_t dev, device_t child, int *rid, >> rman_res_t *start, >> rman_res_t *end, rman_res_t *count, struct intr_map_data **imd) >> { >> +printf(" *** %s: dev: %p\n", __func__, dev); >> /* Propagate up the bus hierarchy until someone handles it. */ >> if (dev->parent) >> return (BUS_MAP_INTR(dev->parent, child, rid, start, >> end, count, >> --------------------------------------------------------------------------------------------------- >> Result: >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** bus_generic_map_intr: dev: 0xc4ce5780 >> *** bus_generic_map_intr: dev: 0xc4ce0980 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** bus_generic_map_intr: dev: 0xc4e52080 >> *** bus_generic_map_intr: dev: 0xc4e52480 >> *** bus_generic_map_intr: dev: 0xc4e52780 >> *** bus_generic_map_intr: dev: 0xc4ce1500 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** bus_generic_map_intr: dev: 0xc4ce0300 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> *** ofwbus_map_intr: bus: 0xc4ce1880 >> >> So its hard to say that code is unused. >> >> And, just for info, last "clean in PPC way" is initial commit INTRNG >> at r289529 (9 months ago). >> Michal >> >> > > Ah, I see, it is called from bus_extend_resource(). Could you describe > this one? It takes an arbitrary resource, but only actually works on > interrupts and seems to be called from #ifdef-ed parts of the MI > bus_alloc_resource(). Could you describe what is going on here some > more? There doesn't appear to be any documentation of any of this. > > This will make this much harder to untangle, unfortunately, and > probably means we are stuck with this as a rump API in stable/11. > -Nathan > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5794F643.4070207>