Configurable interrupt sources, and DT bindings
Many interrupt sinks support various of active high/low, rising/falling edge. This can already be configured by setting #interrupt-cells appropriately, and defining an interrupt-controller-specific binding for the additional cells.
At least some interrupt sources have similar configurability in HW. An example is the WM8903 audio codec, which can generate both active high and active low interrupt signals.
One possibility is to describe this directly in the binding for each interrupt source. I originally proposed the following for the WM8903:
* Interrupt output is active high by default.
* Presence of an "irq-active-low" property selects an active low output instead.
Mark Brown pointed out that we probably want to standardize the binding, so that every interrupt source doesn't do something different.
Perhaps one of:
irq-active-low;
irq-active-high;
irq-edge-falling;
irq-edge-rising;
or:
interrupts-config = <"active-low"> // or "active-high", etc. (perhaps with indices in that list matching the interrupts property)
... and a helper function in the kernel to parse those
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
What are people's thoughts here; should we go down this auto-configuration route, or just explicitly configure interrupt sources using a binding other than the interrupts property?
Related, having this negotiation followed by a request_irq() passing the flags back to the sink seems a little redundant, but I suppose if the sink is configurable and unconstrained, this is necessary.
Signature · 1 line
nvpublic
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · Stephen Warren
Perhaps one of:
irq-active-low;
irq-active-high;
irq-edge-falling;
irq-edge-rising;or:
interrupts-config = <"active-low"> // or "active-high", etc. (perhaps with indices in that list matching the interrupts property)
We do need to support at least the combination of the two edge triggered modes simultaneously also. The first binding handles that nicely but it's a bit more cumbersome with the second.
Quoted 8 lines
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
I think we will also need to restrict this with platform/DT data even if most things can figure it out for themselves. For example, if there's a needless pull on the line then it would be preferable to have the idle state be one where the pull doesn't burn power.
Quoted 42 lines · Stephen Warren
Many interrupt sinks support various of active high/low, rising/falling edge. This can already be configured by setting #interrupt-cells appropriately, and defining an interrupt-controller-specific binding for the additional cells.
At least some interrupt sources have similar configurability in HW. An example is the WM8903 audio codec, which can generate both active high and active low interrupt signals.
One possibility is to describe this directly in the binding for each interrupt source. I originally proposed the following for the WM8903:
* Interrupt output is active high by default.
* Presence of an "irq-active-low" property selects an active low output instead.Mark Brown pointed out that we probably want to standardize the binding, so that every interrupt source doesn't do something different.
Perhaps one of:
irq-active-low;
irq-active-high;
irq-edge-falling;
irq-edge-rising;or:
interrupts-config = <"active-low"> // or "active-high", etc. (perhaps with indices in that list matching the interrupts property)
... and a helper function in the kernel to parse those
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
Quoted 8 lines
What are people's thoughts here; should we go down this auto-configuration route, or just explicitly configure interrupt sources using a binding other than the interrupts property?
Related, having this negotiation followed by a
request_irq()passing the flags back to the sink seems a little redundant, but I suppose if the sink is configurable and unconstrained, this is necessary.
I think adding another property is the wrong approach. The information is already there in the interrupt binding. irq_create_of_mapping almost does what you need. Perhaps it could be extended to return the type as part of the irq resource. There are already defined resource bits for this:
/* PnP IRQ specific bits (IORESOURCE_BITS) */
#define IORESOURCE_IRQ_HIGHEDGE (1<<0)
#define IORESOURCE_IRQ_LOWEDGE (1<<1)
#define IORESOURCE_IRQ_HIGHLEVEL (1<<2)
#define IORESOURCE_IRQ_LOWLEVEL (1<<3)
It may be a bit problematic to change irq_create_of_mapping as Sparc has a different version.
Rob
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 4 lines · Rob Herring
I think adding another property is the wrong approach. The information is already there in the interrupt binding.
irq_create_of_mappingalmost does what you need. Perhaps it could be extended to return the type as part of the irq resource. There are already defined resource bits for this:
That looks like a sensible place to put the translation but it (or the dt_translate() op it calls) is still going to need to figure out the value to return from somewhere and if the interrupt binding defines that information it's not terribly obvious.
Rob Herring wrote at Monday, November 28, 2011 4:24 PM:
Quoted 12 lines · Stephen Warren
Quoted 11 lines
Many interrupt sinks support various of active high/low, rising/falling edge. This can already be configured by setting #interrupt-cells appropriately, and defining an interrupt-controller-specific binding for the additional cells.
At least some interrupt sources have similar configurability in HW. An example is the WM8903 audio codec, which can generate both active high and active low interrupt signals.
One possibility is to describe this directly in the binding for each interrupt source....
...
Quoted 22 lines
Quoted 9 lines
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
Quoted 7 lines
What are people's thoughts here; should we go down this auto-configuration route, or just explicitly configure interrupt sources using a binding other than the interrupts property?
Related, having this negotiation followed by a
request_irq()passing the flags back to the sink seems a little redundant, but I suppose if the sink is configurable and unconstrained, this is necessary.I think adding another property is the wrong approach. The information is already there in the interrupt binding.
irq_create_of_mappingalmost does what you need. Perhaps it could be extended to return the type as part of the irq resource. There are already defined resource bits for this:
Quoted 8 lines
/* PnP IRQ specific bits (
IORESOURCE_BITS) */
#defineIORESOURCE_IRQ_HIGHEDGE(1<<0)
#defineIORESOURCE_IRQ_LOWEDGE(1<<1)
#defineIORESOURCE_IRQ_HIGHLEVEL(1<<2)
#defineIORESOURCE_IRQ_LOWLEVEL(1<<3)It may be a bit problematic to change
irq_create_of_mappingas Sparc has a different version.
Taking a look at the code, I think it can work like this:
* Except for SPARC, irq_of_parse_and_map() calls irq_create_of_mapping(), which calls the IRQ chip's dt_translate() function which can return the type from the interrupt specifier in DT, and if it did, the type is passed to irq_set_irq_type().
* SPARC's custom irq_of_parse_and_map() doesn't call irq_set_irq_type() as far as I can tell. I think we can just ignore this for now; if SPARC wants to participate in this scheme, its custom irq_of_parse_and_map() could be enhanced appropriately.
* I assert that all IRQ chips that want to participate must define an interrupt specifier DT binding that defines the trigger type; e.g. that implemented by using irq_domain_add_simple() and #interrupt-cells greater than 1.
* I assert that all IRQ chips that want to participate must call irqd_set_trigger_type() from their irq_set_type op. This will cache the type for later access. This call is missing from Tegra's GPIO interrupt controller, but can easily be added.
* Create a new function that drivers can call on their Linux interrupt ID, which internally calls irqd_get_trigger_type(). This will return the configured type, and they can adjust their interrupt output pin appropriately, or fail to probe if they can't support the selected type.
* We can probably remove any platform data fields that configure a device's interrupt output type (e.g. WM8903 has an irq_active_low field in pdata) and replace it with this scheme. If that won't work, we can have drivers call the new API only when instantiated from DT, so they continue to work unchanged in the non-DT case.
Does that sound like a reasonable solution? If so, I can start hooking this together.
Signature · 1 line
nvpublic
A message that is not in the archive (sent off-list or not yet received): 74CDBE0F657A3D45AFBB94109FB122FF174FDAFD60-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org
Quoted 57 lines · Stephen Warren
Rob Herring wrote at Monday, November 28, 2011 4:24 PM:
Quoted 12 lines · Stephen Warren
Quoted 11 lines
Many interrupt sinks support various of active high/low, rising/falling edge. This can already be configured by setting #interrupt-cells appropriately, and defining an interrupt-controller-specific binding for the additional cells.
At least some interrupt sources have similar configurability in HW. An example is the WM8903 audio codec, which can generate both active high and active low interrupt signals.
One possibility is to describe this directly in the binding for each interrupt source....
...
Quoted 22 lines
Quoted 9 lines
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
Quoted 7 lines
What are people's thoughts here; should we go down this auto-configuration route, or just explicitly configure interrupt sources using a binding other than the interrupts property?
Related, having this negotiation followed by a
request_irq()passing the flags back to the sink seems a little redundant, but I suppose if the sink is configurable and unconstrained, this is necessary.I think adding another property is the wrong approach. The information is already there in the interrupt binding.
irq_create_of_mappingalmost does what you need. Perhaps it could be extended to return the type as part of the irq resource. There are already defined resource bits for this:Quoted 8 lines
/* PnP IRQ specific bits (
IORESOURCE_BITS) */
#defineIORESOURCE_IRQ_HIGHEDGE(1<<0)
#defineIORESOURCE_IRQ_LOWEDGE(1<<1)
#defineIORESOURCE_IRQ_HIGHLEVEL(1<<2)
#defineIORESOURCE_IRQ_LOWLEVEL(1<<3)It may be a bit problematic to change
irq_create_of_mappingas Sparc has a different version.Taking a look at the code, I think it can work like this:
* Except for SPARC,
irq_of_parse_and_map()callsirq_create_of_mapping(), which calls the IRQ chip'sdt_translate()function which can return the type from the interrupt specifier in DT, and if it did, the type is passed toirq_set_irq_type().* SPARC's custom
irq_of_parse_and_map()doesn't callirq_set_irq_type()as far as I can tell. I think we can just ignore this for now; if SPARC wants to participate in this scheme, its customirq_of_parse_and_map()could be enhanced appropriately.
Quoted 4 lines
* I assert that all IRQ chips that want to participate must define an interrupt specifier DT binding that defines the trigger type; e.g. that implemented by using
irq_domain_add_simple()and #interrupt-cells greater than 1.
This changes nothing w.r.t. the DT binding. The irq chip driver already needs to determine the irq type from the dt irq specifier. But that doesn't imply that #interrupt-cells must be >1, nor even that the irq specifier encodes the type information. e.g. if an irq chip _only_ supports active-low level interrupts, then the type info need no appear in its irq specifiers, but it can still correctly pass the type info to irq_set_irq_type().
Quoted 9 lines
* I assert that all IRQ chips that want to participate must call
irqd_set_trigger_type()from theirirq_set_typeop. This will cache the type for later access. This call is missing from Tegra's GPIO interrupt controller, but can easily be added.* Create a new function that drivers can call on their Linux interrupt ID, which internally calls
irqd_get_trigger_type(). This will return the configured type, and they can adjust their interrupt output pin appropriately, or fail to probe if they can't support the selected type.
AFAICT this is essentially a means for the irq chip driver to communicate the type info back to the driver for the irq source. It sounds reasonable at first glance.
Quoted 5 lines
* We can probably remove any platform data fields that configure a device's interrupt output type (e.g. WM8903 has an
irq_active_lowfield in pdata) and replace it with this scheme. If that won't work, we can have drivers call the new API only when instantiated from DT, so they continue to work unchanged in the non-DT case.
I don't see how there is anything DT related here at all. It's just about communicating between different parts of the kernel.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Quoted 72 lines · Stephen Warren
Rob Herring wrote at Monday, November 28, 2011 4:24 PM:
Quoted 12 lines · Stephen Warren
Quoted 11 lines
Many interrupt sinks support various of active high/low, rising/falling edge. This can already be configured by setting #interrupt-cells appropriately, and defining an interrupt-controller-specific binding for the additional cells.
At least some interrupt sources have similar configurability in HW. An example is the WM8903 audio codec, which can generate both active high and active low interrupt signals.
One possibility is to describe this directly in the binding for each interrupt source....
...
Quoted 22 lines
Quoted 9 lines
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
Quoted 7 lines
What are people's thoughts here; should we go down this auto-configuration route, or just explicitly configure interrupt sources using a binding other than the interrupts property?
Related, having this negotiation followed by a
request_irq()passing the flags back to the sink seems a little redundant, but I suppose if the sink is configurable and unconstrained, this is necessary.I think adding another property is the wrong approach. The information is already there in the interrupt binding.
irq_create_of_mappingalmost does what you need. Perhaps it could be extended to return the type as part of the irq resource. There are already defined resource bits for this:Quoted 8 lines
/* PnP IRQ specific bits (
IORESOURCE_BITS) */
#defineIORESOURCE_IRQ_HIGHEDGE(1<<0)
#defineIORESOURCE_IRQ_LOWEDGE(1<<1)
#defineIORESOURCE_IRQ_HIGHLEVEL(1<<2)
#defineIORESOURCE_IRQ_LOWLEVEL(1<<3)It may be a bit problematic to change
irq_create_of_mappingas Sparc has a different version.Taking a look at the code, I think it can work like this:
* Except for SPARC,
irq_of_parse_and_map()callsirq_create_of_mapping(), which calls the IRQ chip'sdt_translate()function which can return the type from the interrupt specifier in DT, and if it did, the type is passed toirq_set_irq_type().* SPARC's custom
irq_of_parse_and_map()doesn't callirq_set_irq_type()as far as I can tell. I think we can just ignore this for now; if SPARC wants to participate in this scheme, its customirq_of_parse_and_map()could be enhanced appropriately.* I assert that all IRQ chips that want to participate must define an interrupt specifier DT binding that defines the trigger type; e.g. that implemented by using
irq_domain_add_simple()and #interrupt-cells greater than 1.* I assert that all IRQ chips that want to participate must call
irqd_set_trigger_type()from theirirq_set_typeop. This will cache the type for later access. This call is missing from Tegra's GPIO interrupt controller, but can easily be added.* Create a new function that drivers can call on their Linux interrupt ID, which internally calls
irqd_get_trigger_type(). This will return the configured type, and they can adjust their interrupt output pin appropriately, or fail to probe if they can't support the selected type.
Quoted 8 lines
* We can probably remove any platform data fields that configure a device's interrupt output type (e.g. WM8903 has an
irq_active_lowfield in pdata) and replace it with this scheme. If that won't work, we can have drivers call the new API only when instantiated from DT, so they continue to work unchanged in the non-DT case.Does that sound like a reasonable solution? If so, I can start hooking this together.
Sounds good to me.
Rob
Quoted 26 lines · Stephen Warren
Many interrupt sinks support various of active high/low, rising/falling edge. This can already be configured by setting #interrupt-cells appropriately, and defining an interrupt-controller-specific binding for the additional cells.
At least some interrupt sources have similar configurability in HW. An example is the WM8903 audio codec, which can generate both active high and active low interrupt signals.
One possibility is to describe this directly in the binding for each interrupt source. I originally proposed the following for the WM8903:
* Interrupt output is active high by default.
* Presence of an "irq-active-low" property selects an active low output instead.Mark Brown pointed out that we probably want to standardize the binding, so that every interrupt source doesn't do something different.
Perhaps one of:
irq-active-low;
irq-active-high;
irq-edge-falling;
irq-edge-rising;
Quoted 6 lines
or:
interrupts-config = <"active-low"> // or "active-high", etc. (perhaps with indices in that list matching the interrupts property)
... and a helper function in the kernel to parse those
Um.. why? These new properties don't appear to give any information that isn't already in the interrupts property (albeit in irq controller dependent form).
Quoted 9 lines
However, given that interrupt sinks already know which signaling methods they support, and may be configured to accept a specific method per interrupt input using extra interrupt cells, perhaps the solution isn't to create a binding that the interrupt sink parses in isolation, but rather to create a new API within the kernel where the interrupt source can query the interrupt sink for a list of supported signaling methods, and pick one that it also supports, and use it. This list could be restricted based on the interrupts property's extra cells.
Quoted 3 lines
What are people's thoughts here; should we go down this auto-configuration route, or just explicitly configure interrupt sources using a binding other than the interrupts property?
Auto-configuration is the only one that actually seems to do something new. It is a potentially interesting problem, one example of a fairly common class with modern device tree bindings, where things traditionally assumed to be fixed properties of the hardware are often runtime configurable in modern setups.
The first question is, is there actually any point to runtime configuring this, rather than just picking a reasonable option at device tree creation time.
Quoted 4 lines
Related, having this negotiation followed by a
request_irq()passing the flags back to the sink seems a little redundant, but I suppose if the sink is configurable and unconstrained, this is necessary.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
A message that is not in the archive (sent off-list or not yet received): 20111129013055.GH3508-MK4v0fQdeXQXU02nzanrWNbf9cGiqdzd@public.gmane.org
Quoted 4 lines · David Gibson
Quoted 2 lines · Stephen Warren
One possibility is to describe this directly in the binding for each interrupt source. I originally proposed the following for the WM8903:
...
Quoted 6 lines
Quoted 2 lines
Mark Brown pointed out that we probably want to standardize the binding, so that every interrupt source doesn't do something different.
Um.. why? These new properties don't appear to give any information that isn't already in the interrupts property (albeit in irq controller dependent form).
If nothing else because doing a custom binding for every single interrupt controller out there is depressingly redundant; even if we've got a bunch of legacy devices with odd bindings it's going to be better all round if we have something new devices can just pick up. Pretty much every single interrupt controller in the embedded space is going to need this information.
There's also the issue of communicating the setting to the interrupt consumer, though this isn't really directly relevant to the device tree bindings.
Quoted 5 lines
Auto-configuration is the only one that actually seems to do something new. It is a potentially interesting problem, one example of a fairly common class with modern device tree bindings, where things traditionally assumed to be fixed properties of the hardware are often runtime configurable in modern setups.
Actually there's a large element of things being fixed by the board but the hardware being able to run with a wide range of board configurations depending on what amuses the electrical engineers and what restrictions the other devices in the system have. For example, if there are pulls on the board we would want the idle configuration to be whatever the pull value is.
A message that is not in the archive (sent off-list or not yet received): 20111129105538.GC2851-yzvPICuk2AATkU/[email protected]
Quoted 15 lines · Mark Brown
Quoted 4 lines · David Gibson
Quoted 2 lines · Stephen Warren
One possibility is to describe this directly in the binding for each interrupt source. I originally proposed the following for the WM8903:
...
Quoted 6 lines
Quoted 2 lines
Mark Brown pointed out that we probably want to standardize the binding, so that every interrupt source doesn't do something different.
Um.. why? These new properties don't appear to give any information that isn't already in the interrupts property (albeit in irq controller dependent form).
Quoted 6 lines
If nothing else because doing a custom binding for every single interrupt controller out there is depressingly redundant; even if we've got a bunch of legacy devices with odd bindings it's going to be better all round if we have something new devices can just pick up. Pretty much every single interrupt controller in the embedded space is going to need this information.
Ok, so feel free to define a semi-standard way of encoding this information in interrupt specifiers, for use by all new PIC bindings. But these new properties still duplicate information that already has to be in the interrupts property. Worse, it provides no way of specifying different information for each irq, if the device has several.
Quoted 3 lines
There's also the issue of communicating the setting to the interrupt consumer, though this isn't really directly relevant to the device tree bindings.
Right. I can see a use for some in-kernel interface to let the device driver retrieve the level/sense info from the PIC driver (which already needs to interpret this from the interrupts property). But that has no effect on the DT bindings.
Quoted 6 lines
Quoted 5 lines
Auto-configuration is the only one that actually seems to do something new. It is a potentially interesting problem, one example of a fairly common class with modern device tree bindings, where things traditionally assumed to be fixed properties of the hardware are often runtime configurable in modern setups.
Quoted 6 lines
Actually there's a large element of things being fixed by the board but the hardware being able to run with a wide range of board configurations depending on what amuses the electrical engineers and what restrictions the other devices in the system have. For example, if there are pulls on the board we would want the idle configuration to be whatever the pull value is.
Sure, but that's nothing new. Board constraints are part of the fixed hardware information that can already be specified by fixed interrupt specifiers in the DT.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
A message that is not in the archive (sent off-list or not yet received): 20111130051349.GG5435-MK4v0fQdeXQXU02nzanrWNbf9cGiqdzd@public.gmane.org
Quoted 14 lines · David Gibson
Quoted 6 lines · Mark Brown
Quoted 3 lines · David Gibson
Um.. why? These new properties don't appear to give any information that isn't already in the interrupts property (albeit in irq controller dependent form).
Quoted 6 lines
If nothing else because doing a custom binding for every single interrupt controller out there is depressingly redundant; even if we've got a bunch of legacy devices with odd bindings it's going to be better all round if we have something new devices can just pick up. Pretty much every single interrupt controller in the embedded space is going to need this information.
Quoted 6 lines
Ok, so feel free to define a semi-standard way of encoding this information in interrupt specifiers, for use by all new PIC bindings. But these new properties still duplicate information that already has to be in the interrupts property. Worse, it provides no way of specifying different information for each irq, if the device has several.
I'm sorry, I'm not following you. In what way is this duplicating information that's already there, and for what reason is it not possible to specify different information per IRQ? If the binding is custom to each device then presumably it's not possible make any general comment about the bindings...
Clearly if people are defining per-chip bindings that can't cope with configuring different interrupts differently that's insane (and all the more reason to come up with a standard binding so they don't have to think) but this all seems so far off base that I fear there must be some fundamental misunderstaniding here.
Quoted 10 lines
Quoted 6 lines
Actually there's a large element of things being fixed by the board but the hardware being able to run with a wide range of board configurations depending on what amuses the electrical engineers and what restrictions the other devices in the system have. For example, if there are pulls on the board we would want the idle configuration to be whatever the pull value is.
Sure, but that's nothing new. Board constraints are part of the fixed hardware information that can already be specified by fixed interrupt specifiers in the DT.
But according to what you say above each interrupt controller is on its own when it comes to coming up with a mechanism for specifying this?
A message that is not in the archive (sent off-list or not yet received): 20111130093305.GB2791-yzvPICuk2AATkU/[email protected]
Quoted 22 lines · Mark Brown
Quoted 14 lines · David Gibson
Quoted 6 lines · Mark Brown
Quoted 3 lines · David Gibson
Um.. why? These new properties don't appear to give any information that isn't already in the interrupts property (albeit in irq controller dependent form).
Quoted 6 lines
If nothing else because doing a custom binding for every single interrupt controller out there is depressingly redundant; even if we've got a bunch of legacy devices with odd bindings it's going to be better all round if we have something new devices can just pick up. Pretty much every single interrupt controller in the embedded space is going to need this information.
Quoted 6 lines
Ok, so feel free to define a semi-standard way of encoding this information in interrupt specifiers, for use by all new PIC bindings. But these new properties still duplicate information that already has to be in the interrupts property. Worse, it provides no way of specifying different information for each irq, if the device has several.
Quoted 2 lines
I'm sorry, I'm not following you. In what way is this duplicating information that's already there,
Each interrupt source device has an 'interrupts' property which is an array of irq specifiers. Each irq specifier contains enough information for the irq controller driver to handle it - that is, both the irq number and the type (if applicable for this irq controller). The irq controller driver must already know how to interpret these irq specifiers, or nothing would work.
Quoted 2 lines
and for what reason is it not possible
to specify different information per IRQ?
IIUC, the binding you proposed had individual boolean properties for the trigger types / polarities. Those properties would have to be on or off for the whole node, but that node might have multiple irqs.
Quoted 3 lines
If the binding is custom to
each device then presumably it's not possible make any general comment about the bindings...
They're only custom within certain constraints.
Quoted 2 lines
Clearly if people are defining per-chip bindings that can't cope with configuring different interrupts differently that's insane
Not necessarily. If the irq controller only handles one irq type in hardware, there is no need for its irq specifiers in the DT to be able to specify other types.
Quoted 3 lines
(and all the
more reason to come up with a standard binding so they don't have to think)
Well, we kind of have one already. I believe the binding for several recently defined PICs is source # in the first cell, trigger/polarity in the second, encoded using the same values as the corresponding Linux constants.
Quoted 2 lines
but this all seems so far off base that I fear there must be some fundamental misunderstaniding here.
I tend to agree.
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Actually there's a large element of things being fixed by the board but the hardware being able to run with a wide range of board configurations depending on what amuses the electrical engineers and what restrictions the other devices in the system have. For example, if there are pulls on the board we would want the idle configuration to be whatever the pull value is.
Quoted 6 lines
Quoted 3 lines
Sure, but that's nothing new. Board constraints are part of the fixed hardware information that can already be specified by fixed interrupt specifiers in the DT.
But according to what you say above each interrupt controller is on its own when it comes to coming up with a mechanism for specifying this?
Well, they could just take the irq specifier format from a suitably similar existing PIC binding. e.g. UIC or IPIC (which use the same format IIRC).
I really don't see what problem you're trying to solve.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
A message that is not in the archive (sent off-list or not yet received): 20111130133140.GL5435-MK4v0fQdeXQXU02nzanrWNbf9cGiqdzd@public.gmane.org
Quoted 11 lines · David Gibson
Quoted 2 lines · Mark Brown
I'm sorry, I'm not following you. In what way is this duplicating information that's already there,
Each interrupt source device has an 'interrupts' property which is an array of irq specifiers. Each irq specifier contains enough information for the irq controller driver to handle it - that is, both the irq number and the type (if applicable for this irq controller). The irq controller driver must already know how to interpret these irq specifiers, or nothing would work.
So when you say the information is already there you mean that controllers can define something in the type field but there's no defined meaning for that.
Quoted 6 lines
Quoted 2 lines
and for what reason is it not possible
to specify different information per IRQ?IIUC, the binding you proposed had individual boolean properties for the trigger types / polarities. Those properties would have to be on or off for the whole node, but that node might have multiple irqs.
Right, but you could stuff those in an enormous array or whatever - the point was to describe the per interrupt information.
Quoted 8 lines
Quoted 3 lines
(and all the
more reason to come up with a standard binding so they don't have to think)Well, we kind of have one already. I believe the binding for several recently defined PICs is source # in the first cell, trigger/polarity in the second, encoded using the same values as the corresponding Linux constants.
Well, then it'd be good if we could get that written up as a spec and factor out the parsing code then start pushing back very strongly on any new bindings that don't follow the spec.
Quoted 4 lines
Quoted 2 lines
but this all seems so far off base that I fear there must be some fundamental misunderstaniding here.
I tend to agree.
I don't think so really - it seems like what you're saying is "there's a place where people could put this information in a device specific fashion" and I'm looking for something that's not device specific as this is a general need and we shouldn't have to be doing device specific things here.
Quoted 1 line
I really don't see what problem you're trying to solve.
There's two issues. One was putting this information in the device tree at all (which is probably going to be OK already). The other is that we've got a lot of devices with interrupt controllers that are going to be converting to device tree and it'd be bad for usability if they all had different bindings for what should be pretty basic functionality.
For system integration it's a bit of a step backwards to have to write these things as numeric constants (though that should be something that can be dealt with in the device tree compiler), it'd be a further step backwards if those constants were to change depending on which chip is being configured. It'd be much better if we can do basic interrupt configuration without having to look up the individual chip bindings.
A message that is not in the archive (sent off-list or not yet received): 20111201110738.GA2915-yzvPICuk2AATkU/[email protected]
Quoted 13 lines · Mark Brown
Quoted 11 lines · David Gibson
Quoted 2 lines · Mark Brown
I'm sorry, I'm not following you. In what way is this duplicating information that's already there,
Each interrupt source device has an 'interrupts' property which is an array of irq specifiers. Each irq specifier contains enough information for the irq controller driver to handle it - that is, both the irq number and the type (if applicable for this irq controller). The irq controller driver must already know how to interpret these irq specifiers, or nothing would work.
Quoted 3 lines
So when you say the information is already there you mean that controllers can define something in the type field but there's no defined meaning for that.
Roughly, yes. In fact even the existence and layout of a "type" field is up to the controller.
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
and for what reason is it not possible
to specify different information per IRQ?IIUC, the binding you proposed had individual boolean properties for the trigger types / polarities. Those properties would have to be on or off for the whole node, but that node might have multiple irqs.
Right, but you could stuff those in an enormous array or whatever - the point was to describe the per interrupt information.
Well, ok, but that's not what you proposed. In any case putting important irq type information anywhere othrt than the irq specifers listed in the 'interrupts' property is not a good idea. It would be breakin to overall model of the DT interrupt tree with poor justification.
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
(and all the
more reason to come up with a standard binding so they don't have to think)
Quoted 8 lines
Quoted 4 lines
Well, we kind of have one already. I believe the binding for several recently defined PICs is source # in the first cell, trigger/polarity in the second, encoded using the same values as the corresponding Linux constants.
Well, then it'd be good if we could get that written up as a spec and factor out the parsing code then start pushing back very strongly on any new bindings that don't follow the spec.
Well, write up a best practice document and get it up on devicetree.org. I think you overestimate the problem though. The specific case you've described can be handled with an in-kernel mechanism for the irq controller driver to advertise the irq type back to the irq source's driver - no DT change necessary, and it even handles all the legacy cases.
Quoted 10 lines
Quoted 4 lines
Quoted 2 lines
but this all seems so far off base that I fear there must be some fundamental misunderstaniding here.
I tend to agree.
I don't think so really - it seems like what you're saying is "there's a place where people could put this information in a device specific fashion" and I'm looking for something that's not device specific as this is a general need and we shouldn't have to be doing device specific things here.
It'd be nice, but it's just not worth breaking away from existing practice to achieve it. Remember that more or less by definition, we *already have* code to interpret the device specific tokens (in the irq controller driver).
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
A message that is not in the archive (sent off-list or not yet received): 20111202053814.GH5427-MK4v0fQdeXQXU02nzanrWNbf9cGiqdzd@public.gmane.org
Quoted 6 lines · David Gibson
Quoted 2 lines · Mark Brown
Right, but you could stuff those in an enormous array or whatever - the point was to describe the per interrupt information.
Well, ok, but that's not what you proposed. In any case putting
Please stop claiming this, it's incredibly irritating. At *no* point have I suggested making the interrupt type a global property of the interrupt controller, that would be silly. What I did do was mistakenly assume that the binding would be something more device tree like than a table of magic numbers which you then projected into a desire to make this a global property of the interrupt controller.
Quoted 4 lines
important irq type information anywhere othrt than the irq specifers listed in the 'interrupts' property is not a good idea. It would be breakin to overall model of the DT interrupt tree with poor justification.
I do think there's a reasonable case for just creatinng a new version of the binding which is more readable but perhaps that's just me.
Quoted 10 lines
Quoted 3 lines
Well, then it'd be good if we could get that written up as a spec and factor out the parsing code then start pushing back very strongly on any new bindings that don't follow the spec.
Well, write up a best practice document and get it up on devicetree.org. I think you overestimate the problem though. The specific case you've described can be handled with an in-kernel mechanism for the irq controller driver to advertise the irq type back to the irq source's driver - no DT change necessary, and it even handles all the legacy cases.
The in kernel bit of this is already fine, the problem is on the device tree side.
Quoted 10 lines
Quoted 5 lines
I don't think so really - it seems like what you're saying is "there's a place where people could put this information in a device specific fashion" and I'm looking for something that's not device specific as this is a general need and we shouldn't have to be doing device specific things here.
It'd be nice, but it's just not worth breaking away from existing practice to achieve it. Remember that more or less by definition, we *already have* code to interpret the device specific tokens (in the irq controller driver).
Not in the real world, in the real world we don't have device tree bindings for most of the interrupt controllers out there and therefore no code exists to parse them. There's a fairly small set of current systems using device tree and a very large set of systems which have never used it before and are currently working on implementing it.
A message that is not in the archive (sent off-list or not yet received): 20111202112722.GG8245-yzvPICuk2AATkU/[email protected]
Quoted 6 lines · Mark Brown
Quoted 4 lines · David Gibson
Quoted 2 lines · Mark Brown
Right, but you could stuff those in an enormous array or whatever - the point was to describe the per interrupt information.
Quoted 8 lines
Quoted 1 line
Well, ok, but that's not what you proposed. In any case putting
Please stop claiming this, it's incredibly irritating. At *no* point have I suggested making the interrupt type a global property of the interrupt controller, that would be silly. What I did do was mistakenly assume that the binding would be something more device tree like than a table of magic numbers which you then projected into a desire to make this a global property of the interrupt controller.
No, you suggested it be a global property of the interrupt source. Same problem, just less so.
Quoted 33 lines
Quoted 4 lines
important irq type information anywhere othrt than the irq specifers listed in the 'interrupts' property is not a good idea. It would be breakin to overall model of the DT interrupt tree with poor justification.
I do think there's a reasonable case for just creatinng a new version of the binding which is more readable but perhaps that's just me.
Quoted 10 lines
Quoted 3 lines
Well, then it'd be good if we could get that written up as a spec and factor out the parsing code then start pushing back very strongly on any new bindings that don't follow the spec.
Well, write up a best practice document and get it up on devicetree.org. I think you overestimate the problem though. The specific case you've described can be handled with an in-kernel mechanism for the irq controller driver to advertise the irq type back to the irq source's driver - no DT change necessary, and it even handles all the legacy cases.
The in kernel bit of this is already fine, the problem is on the device tree side.
Quoted 10 lines
Quoted 5 lines
I don't think so really - it seems like what you're saying is "there's a place where people could put this information in a device specific fashion" and I'm looking for something that's not device specific as this is a general need and we shouldn't have to be doing device specific things here.
It'd be nice, but it's just not worth breaking away from existing practice to achieve it. Remember that more or less by definition, we *already have* code to interpret the device specific tokens (in the irq controller driver).
Quoted 5 lines
Not in the real world, in the real world we don't have device tree bindings for most of the interrupt controllers out there and therefore no code exists to parse them. There's a fairly small set of current systems using device tree and a very large set of systems which have never used it before and are currently working on implementing it.
If we support the PIC at all, it must know how to determine the interrupt types. When DT support is added to the PIC, that will include determining the type from the interrupt specifiers. If we don't support the PIC, we don't care.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
A message that is not in the archive (sent off-list or not yet received): 20111202122418.GI5427-MK4v0fQdeXQXU02nzanrWNbf9cGiqdzd@public.gmane.org
Quoted 2 lines · David Gibson
No, you suggested it be a global property of the interrupt source. Same problem, just less so.
Cite?
Quoted 10 lines
Quoted 5 lines
Not in the real world, in the real world we don't have device tree bindings for most of the interrupt controllers out there and therefore no code exists to parse them. There's a fairly small set of current systems using device tree and a very large set of systems which have never used it before and are currently working on implementing it.
If we support the PIC at all, it must know how to determine the interrupt types. When DT support is added to the PIC, that will include determining the type from the interrupt specifiers. If we don't support the PIC, we don't care.
*sigh* We do care because as I say in the paragraph you quote above people are trying to roll device tree out onto new platforms. The coverage on PowerPC and SPARC is near 100% but the coverage on ARM is very far away from that so we're going to have to create a large number of new bindings.
A message that is not in the archive (sent off-list or not yet received): 20111202133413.GR8245-yzvPICuk2AATkU/[email protected]
Quoted 6 lines · Mark Brown
Quoted 2 lines · David Gibson
No, you suggested it be a global property of the interrupt source. Same problem, just less so.
Cite?
My apologies. I was mixing up who said what. It was Stephen who made this proposal, kicking off the thread.
SW> One possibility is to describe this directly in the binding for each SW> interrupt source. I originally proposed the following for the WM8903:
SW> * Interrupt output is active high by default.
SW> * Presence of an "irq-active-low" property selects an active low SW> * output
SW> instead.
SW> Mark Brown pointed out that we probably want to standardize the SW> binding,
SW> so that every interrupt source doesn't do something different.
SW> Perhaps one of:
SW> irq-active-low;
SW> irq-active-high;
SW> irq-edge-falling;
SW> irq-edge-rising;
And he does go on to suggest another alternative that does allow multiple interrupts to be described.
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
Not in the real world, in the real world we don't have device tree bindings for most of the interrupt controllers out there and therefore no code exists to parse them. There's a fairly small set of current systems using device tree and a very large set of systems which have never used it before and are currently working on implementing it.
If we support the PIC at all, it must know how to determine the interrupt types. When DT support is added to the PIC, that will include determining the type from the interrupt specifiers. If we don't support the PIC, we don't care.
Quoted 5 lines
*sigh* We do care because as I say in the paragraph you quote above people are trying to roll device tree out onto new platforms. The coverage on PowerPC and SPARC is near 100% but the coverage on ARM is very far away from that so we're going to have to create a large number of new bindings.
I guess, but irq spec bindings are so trivial, it's just not a big deal. As I say, by all means write a best-current-practice document with a suggested binding. But if people go their own way it really doesn't hurt much. And there are situations where people would reasonably want different irq specifiers. e.g. including polarity information wouldn't make much sense for a PIC which only handled message signalled interrupts.
Signature · 4 lines
David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
A message that is not in the archive (sent off-list or not yet received): 20111202143107.GJ5427-MK4v0fQdeXQXU02nzanrWNbf9cGiqdzd@public.gmane.org
Quoted 2 lines · David Gibson
My apologies. I was mixing up who said what. It was Stephen who made this proposal, kicking off the thread.
Ah, OK - I think there's been some misreading here.
Quoted 2 lines
SW> One possibility is to describe this directly in the binding for each SW> interrupt source. I originally proposed the following for the WM8903:
I think the above (talking as it does about an interrupt source) is intended to refer to a single interrupt line, the confusion being due to the fact that the current binding doesn't have any way to add per interrupt properties.
Quoted 6 lines
Quoted 5 lines
*sigh* We do care because as I say in the paragraph you quote above people are trying to roll device tree out onto new platforms. The coverage on PowerPC and SPARC is near 100% but the coverage on ARM is very far away from that so we're going to have to create a large number of new bindings.
Quoted 7 lines
I guess, but irq spec bindings are so trivial, it's just not a big deal. As I say, by all means write a best-current-practice document with a suggested binding. But if people go their own way it really doesn't hurt much. And there are situations where people would reasonably want different irq specifiers. e.g. including polarity information wouldn't make much sense for a PIC which only handled message signalled interrupts.
It's not that the bindings are complex per se, it's that people working over many different interrupt controllers (which is fairly common if you do system integration) are going to be faced with a range of different magic number sets to learn. If we can at least keep the range of different magic number sets as small as possible that improves things a bit.