On 07/19/16 04:13, Michal Meloun wrote:
Dne 19.07.2016 v 2:11 Nathan Whitehorn napsal(a):
Hi Nathan,
I’m afraid that skra is on vacation, for next 2 weeks (at minimum), so
please don’t expect quick response.
Could you please describe what this change is in more detail?
Short description is appended.
It breaks a lot of encapsulations we have worked very hard to maintain,
moves ARM code into MI parts of the kernel, and the OFW parts violate
IEEE 1275 (the Open Firmware standard). In particular, there is no
guarantee that the interrupts for a newbus (or OF) device are encoded in
a property called "interrupts" (or, indeed, in any property at all) on
that node and there are many, many device trees where that is not the
case (e.g. ones with interrupt maps, as well as Apple hardware). By
putting that knowledge into the OF root bus device, which we have tried
to keep it out of, this enforces a standard that doesn't actually exist.
Imho, this patch doesn’t change anything in this area. Only handling of
“interrupts” property is changed, all other cases are unchanged (I
hope). Also, INTRNG code is currently shared by ARM, ARM64 and MIPS.
But "interrupts" isn't a generic part of OF. This makes it one, incorrectly.
I'm hesitant to ask for reversion on something that landed 6 weeks ago
without me noticing, but this needs a lot more architectural work before
any parts of the kernel should use it.
-Nathan
I think that it’s too late. This patch series consist of r301451
(https://reviews.freebsd.org/D6632),
r301453, r301539 and 301543. And new GPIO interrupts are currently used
(by in tree drivers or in development trees).
Well, then we need in-place rearchitecture.
The root of problem is that standard way of delivering interrupt
resource to consumer driver doesn’t works in OFW world.
So we have some fact:
- the format of interrupt property is dependent of interrupt
controller and only interrupt controller can parse it.
- the interrupt property can have more data than just interrupt
number.
- single interrupt controller must be able to handle multiple
format of interrupt description.
In pre-patchset era, simplebus enumerates children and attempts to set
memory and interrupts to resource list for them. But the interrupt
controllers are not yet populated so nobody can parse interrupt
property. Moreover, in all cases (parsed or not), we cannot store
complete interrupt description into resource list.
We have done this for many years on PowerPC and sparc64 with delayed
configuration of interrupts and a look-up table. This handles
complicated bus configurations better than this code and requires no
changes outside of a few MD files. That is why the (now partially
duplicated) OFW_BUS_MAP_INTR() function exists. That one also has the
benefit of still working when used in conjunction with, e.g., devices
with an interrupt-map-mask property.
The patch simply postpones reading of interrupt property to
bus_alloc_resource() (called by consumer driver) time.
Due to this, we can:
- parse interrupt property. The interrupt driver must exist
at this time.
This only works with some types of interrupt properties, not all, and
breaks if the interrupt driver hasn't attached yet (which it can't be
guaranteed to -- some PPC systems have interrupt drivers that live on
the PCI bus, for example).
- bus_alloc_resource() returns resource, so we can attach parsed
interrupt data to it. By this, the resource itself can be used
for delivering configuration data to subsequent call to
bus_setup_intr() (or to all related bus_<foo>() calls).
The patched code still accepts delivering of interrupts in resource list.
Michal
Given that other code depends on this, fixing it will likely require
some complex work. I wish I had known about it when it went in.
There are three main problems:
1. It doesn't work for interrupts defined by other mechanisms (e.g.
interrupt-map properties)
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)
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). It is much
easier to do this correctly at bus attach time when the resource lists
are made (how PPC does it).
(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.
2. There is no ordering requirement on PIC attachment vs. the attachment
of anything else.
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.
4. It happens at bus enumeration time, when results can be guaranteed
self-consistent.
5. It combines naturally with ofw_bus_lookup_imap() and friends in the
interrupt-map case (e.g. for PCI).
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
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
