Date: Sat, 23 Jul 2016 11:35:40 -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: <4e7a3e8f-cc21-f5f2-e3e0-4dbd554a4cd0@freebsd.org> In-Reply-To: <57934ABD.6010807@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
> 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.
> This is, imho, only one difference between us.
One of many, yes.
>
>> You could, for instance, set it to one of the structures introduced
>> in r301453 if you wanted to.
>>
>> I would have zero problems with changing the prototype of the
>> existing dev/ofw function to something more generic in name, like:
>>
>> bus_map_intr(device_t dev, uintptr_t iparent, size_t intrlen, void *intr)
>>
>> instead of the existing equivalent:
>>
>> ofw_bus_map_intr(device_t dev, phandle_t iparent, int icells, pcell_t
>> *intr)
>>
> Our bus_map_intr() method is not indeed as replacement of
> ofw_bus_map_intr(). Its evolution of "how we can store more complex
> data to resource list (from bus enumerator) and transfer it to
> bus_allocate_resource() and/or to bus_setup_intr()" in driver
> independent way. We found no reasonable way to do it, so we postponed
> reading of properties to bus_allocate_resource() time.
Right, but that is (a) a solved problem with ofw_bus_map_intr() and (b)
this code doesn't solve it as completely. What does it let you do that
the existing code does not? There has not been a single concrete example
of something anyone wants to do on actual hardware so far in this
discussion.
> But now jhb@ gives us alternative and I must say that this looks like
> a clean and elegant way how to make this (assuming that we can expand
> resource_list_entry by pointer to alloc_resource_args)
Except that jhb@'s suggestion doesn't actually work for interrupts for
the reasons I and others have pointed out. We could make such a system,
but it would be a different one.
>
>>> By this, one byte string in OFW encoding can describe one IRQ and
>>> exactly same string in UEFI encoding can describe different IRQ.
>>> Or, in reverse, OFW and UEFI can describe same (and compatible) IRQ
>>> by two different strings.
>>> This is exact reason, why we discards virtual IRQ idea and I think
>>> that this fact is root issue of this clash.
>>> Probably it doesn't make sense to talk about others, unless we can
>>> find consensus on this.
>>
>> You have the larger problem if you end up in this situation that you
>> are enumerating the hardware by two different and incompatible
>> techniques. There simply is no way to solve this unless you either
>> (a) segregate the system into an ACPI-enumerated domain and an
>> OF-enumerated domain, in which case the problem vanishes, (b) discard
>> one enumeration, which is what arm64 does and will always do,
>> according to Andrew in another post, or (c) make some incredibly
>> complex merging code that would naturally handle interrupts with
>> everything else. So I don't think this is an actual, real problem.
>>
> I think that above proposed solution resolves this gracefully.
Assuming you are talking about jhb's decoration plan in the bus
hierarchy, it doesn't actually work for interrupts at all because of
lateral connections and circular dependencies.
>
>>
>>>>
>>>>>
>>>>>> 2. It partially duplicates the functionality of
>>>>>> OFW_BUS_MAP_INTR(), but is both problematically more general and
>>>>>> less flexible (it has requirements on timing of PIC attachment
>>>>>> vs. driver resource allocation)
>>>>> OFW_BUS_MAP_INTR() can parse only OFW based data and expect that
>>>>> parsed data are magicaly stored within the call.
>>>>> The new method, bus_map_intr(), can parse data from multiple
>>>>> sources (OFW, UEFI / ACPI, synthetic[gpio device + pin number]).
>>>>> It also returns parsed data back to caller.
>>>>
>>>> That is not true. It works as long as you can specify the interrupt
>>>> state as a 32-bit key of some kind for the PIC and a string of
>>>> arbitrary data, which works with all of those. You could even make
>>>> the interrupt data be a pointer to exactly the structs you have
>>>> chosen to define here.
>>> Nope, in heterogeneous world, same string can describe two different
>>> IRQs and/or two different strings can describe single IRQ in
>>> compatible manner.
>>
>> Can you give *any* concrete example of this that doesn't involve
>> mixed ACPI/FDT enumeration of a single system where devices appear in
>> both trees, which doesn't actually ever happen?
> GPIO - its interrupt function can be accessed by regular "interrupts"
> property, or it can be derived from GPIO pin.
The GPIO controller has one or more interrupts assigned to it that are
part of its parent's interrupt domain. It cascades them to one or more
virtual interrupts that belong to its own domain. Where is the ambiguity
or complication here?
>>
>>>>
>>>>>
>>>>>> 3. It is not fully transparent to end code. Since it happens at
>>>>>> bus_alloc_resource() time, it is complicated to get the
>>>>>> appropriate values for IRQs constructed by composite techniques
>>>>>> (interrupt-map vs. interrupts vs. hand allocation vs. PCI
>>>>>> routing, for example).
>>>>> I don't see any limitation - can you be more exact? Why is not
>>>>> transparent? Why is more complicated ?
>>>>
>>>> Suppose that a PCI device adds more IRQs to its resource list or
>>>> modifies the ordering. How is whatever bus layer supposed to do
>>>> something sensible at allocation time? It requires that RID numbers
>>>> mean something to the parent bus after assignment, which is not
>>>> guaranteed by anything and is, in more than handful of cases I
>>>> think of, not true in practice.
>>> Sure. And since the new code allows delivering resources in RL, so I
>>> don't see any limitation here.
>>
>> It indexed mapping by RID and then searches interrupt lists by that
>> to get the interrupt-parent. This is fundamentally a broken design if
>> the child needs to, say, add a second interrupt to its RL on a
>> different interrupt-parent.
> ?? I don't understand. The new code doesn't need this.
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);
This works either by rid or by rman_res_t. In the event of a
self-assigned interrupt from the child, which is a fairly common case,
neither of these values are meaningfully parseable by the bus parent,
especially if the interrupt is on a different controller. The main issue
is that this code goes back to the device tree according to the
*parent*'s idea of how it should be parsed and there are many cases
(GPIO interrupts, for example) when the *child* needs to amend the list
in a different way.
>
>>
>>>
>>>>
>>>>>> 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.
>
>>>
>>>>
>>>>>
>>>>>> (1) is easy to fix without API changes, but (2) and (3) are
>>>>>> fundamental architectural problems that will bite us immediately
>>>>>> down the road and cause a permanent schism between OF support on
>>>>>> different platforms.
>>>>>>
>>>>>> Let me describe how this is handled on PowerPC (Linux on PPC
>>>>>> solves the problem the same way). When constructing a resource
>>>>>> list, bus drivers that construct them from OF properties call
>>>>>> ofw_bus_map_intr() with the interrupt parent phandle and the
>>>>>> array of cells corresponding to the interrupt. This thunks
>>>>>> immediately to nexus, which connects to code in intr_machdep.c.
>>>>>> Code there assigns a unique made-up virtual IRQ and returns it,
>>>>>> caching the interrupt parent ID and opaque interrupt data (if the
>>>>>> same string of data reappears later, you get back the same
>>>>>> virtual IRQ of course).
>>>>>>
>>>>>> When PIC drivers attach and register themselves with the
>>>>>> interrupt handling layer, all the interrupts for that PIC are
>>>>>> passed to it along with the virtual IRQ. The PIC driver is
>>>>>> supposed to know what its interrupt data mean, which can be
>>>>>> safely guaranteed, and it presents the assigned virtual IRQ
>>>>>> number to the kernel when dispatching interrupts. (IRQs
>>>>>> configured after PIC attachment are passed through immediately).
>>>>>>
>>>>>> This accomplishes the following things:
>>>>>> 1. Parsing interrupt data is moved to the PIC driver, which is
>>>>>> the only place it can be done safely.
>>>>> I don't see anything different comparing with INTRNG.
>>>>
>>>> What I am advocating *is* INTRNG, at least as originally conceived
>>>> and implemented.
>>>>
>>>>>> 2. There is no ordering requirement on PIC attachment vs. the
>>>>>> attachment of anything else.
>>>>> I think thats is not a true - PIC must exist before
>>>>> bus_alloc_resource() / bus_setup_intr() is called.
>>>>
>>>> It does not with the IRQ mapping infrastructure. Interrupts are set
>>>> up at PIC attachment, whenever that occurs.
>>>>
>>> Assuming that bus_alloc_resource and bus_setup_intr() are close
>>> thorougher and in linear piece of code, can i assume that you can
>>> call bus_setup_intr()
>>> without PIC attached ?
>>
>> Yes.
> So driver can request and/or setup any random IRQ and gets success
> from bus_alloc_resource() or bus_setup_intr()?
> Do you think that is this right behavior?
Yes. And it is a behavior required by newbus.
>>
>>>>>
>>>>>> 3. Changes are extremely minimal relative to the "standard"
>>>>>> interrupt flow: you only have to patch code that is already
>>>>>> directly dealing with OF interrupts.
>>>>> I don't see anything different comparing with INTRNG.
>>>>
>>>> Again, this was the original INTRNG architecture and is already
>>>> implemented. As such, there are *no* changes required on ARM to get
>>>> it. bus_map_intr() adds a bunch of new code, in parallel with the
>>>> old code that also solves the problem, to no purpose.
>>> So, on PPC, how i can get interrupt for GPIO pin described by this
>>> property:
>>> https://svnweb.freebsd.org/base/head/sys/gnu/dts/arm/tegra124-jetson-tk1.dts?revision=295436&view=markup#l1691
>>>
>>
>> The GPIO controller registers itself as an interrupt domain and
>> decodes those strings as IRQ specifiers. When interrupts are
>> configured, it does whatever it needs to do to configure them
>> appropriately and dispatches them to the kernel when they occur. It's
>> a pretty trivial cascaded interrupt configuration. And, since the
>> VIRQ code does not need the interrupt controller attached in advance,
>> you don't need to worry about attach order of wherever &gpio points
>> and the SDHCI driver.
>>
>>>>
>>>>>> 4. It happens at bus enumeration time, when results can be
>>>>>> guaranteed self-consistent.
>>>>> Where do you see any potential source of inconsistency in INTRNG?
>>>>
>>>> See the example above about modified interrupt lists. There is also
>>>> no obvious way for a child device to construct an interrupt not
>>>> assigned to it by the parent device from device tree properties
>>>> without knowing in some detail what kind of interrupt needs to be
>>>> built.
>>>>
>>>>>
>>>>>> 5. It combines naturally with ofw_bus_lookup_imap() and friends
>>>>>> in the interrupt-map case (e.g. for PCI).
>>>>> Again, I don't see anything different. Proper parsing of interrupt
>>>>> property is not a problem of INTRNG (but must be fixed, of course).
>>>>
>>>> But it is *already* fixed by the standard code that already exists.
>>>> You are introducing a less-functional parallel code path here.
>>> NO, its not fixed, at least not for ARM.
>>
>> Why not, concretely? I'm happy to write whatever code is missing if
>> there's a bug. It can't be more than a few tens of lines in arm/intr.c.
> Interrupt maps are not covered by current ARM code.
This suggests otherwise:
files.arm:dev/ofw/ofwpci.c optional fdt pci
But it's quite possible that code is not being used as widely as it
should, since it's a new introduction to the ARM tree. That the existing
code is not being used broadly enough is hardly a reasonable to invent
something new.
>>>>
>>>>>>
>>>>>> I'm not sure what the right path forward is, but this code needs
>>>>>> to be fixed. The PowerPC code is fully MI, and was the template
>>>>>> for the original INTRNG, so it shouldn't be too bad to replace.
>>>>>> -Nathan
>>>>>>
>>>>>
>>>>> So, new INTRNG:
>>>>> - Introduces new more general bus method that can parse interrupt
>>>>> configuration
>>>>> data from any source. Is this step backward?
>>>>
>>>> Yes, since it is more general in some sense, while simultaneously
>>>> handling fewer cases than code that already exists and is implemented.
>>>>
>>>>>
>>>>> - Old INTRNG and PPC code stores unparsed and/or parsed interrupt
>>>>> data in
>>>>> INTRNG and each consumer must query for them. This data sharing
>>>>> also causes
>>>>> significant locking issues. New INTRNG stores interrupt
>>>>> configuration data into
>>>>> given resource, so each relevant bus method can access it
>>>>> immediately.
>>>>> Is this step backward?
>>>>
>>>> Which locking issues? And yes, it is.
>>>>
>>>>>
>>>>> - New INTRNG is not OFW centric, it can works with virtually
>>>>> unlimited number
>>>>> of configuration data sources. Is this step backward?
>>>>
>>>> Also yes, because it makes the interrupt handles less opaque, which
>>>> makes the infrastructure less flexible.
>>>>
>>>>> - New INTRNG correctly uses standard system infrastructure. Real
>>>>> IRQ number
>>>>> is reserved in rman within bus_alloc_resource() call, interrupt
>>>>> HW is
>>>>> configured (only!) within bus_setup_intr() call. Is this step
>>>>> backward?
>>>>
>>>> The "real" IRQ number is not well defined always, so requiring that
>>>> is a step backwards, yes.
>>>>
>>>>> - New INTRNG completely eliminates huge and not always working virtual
>>>>> IRQ concept.
>>>>
>>>> When does it "not always work"? It seems to, in fact, always work
>>>> on multiple platforms and have for a long time in the face of all
>>>> kinds of totally bizarre topologies and system architectures.
>>>>
>>>>>
>>>>>
>>>>> Don’t take me bad, I’m open to any change. But no, at this time,
>>>>> I’m not ready to completely revert someone else's work – although
>>>>> I am a co-author.
>>>>
>>>> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4e7a3e8f-cc21-f5f2-e3e0-4dbd554a4cd0>