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
_______________________________________________
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"
