Date: Sun, 24 Jul 2016 08:32:03 -0700 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: Michal Meloun <mmel@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: <8bfd8668-bc49-e109-e610-b5cd470be3ec@freebsd.org> In-Reply-To: <5794720F.4050303@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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?8bfd8668-bc49-e109-e610-b5cd470be3ec>