Configurable interrupt sources, and DT bindings

Configurable interrupt sources, and DT bindings

[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.

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.

-- 
nvpublic

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Mon, Nov 28, 2011 at 03:29:51PM -0700, Stephen Warren wrote:

> 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.

> 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.

Re: Configurable interrupt sources, and DT bindings

[Rob Herring]

On 11/28/2011 04:29 PM, Stephen Warren wrote:
> 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.
> 

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

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Mon, Nov 28, 2011 at 03:29:51PM -0700, Stephen Warren wrote:
> 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

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).

> 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?

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.

> 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.
> 

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

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Tue, Nov 29, 2011 at 12:30:55PM +1100, David Gibson wrote:
> On Mon, Nov 28, 2011 at 03:29:51PM -0700, Stephen Warren wrote:

> > One possibility is to describe this directly in the binding for each
> > interrupt source. I originally proposed the following for the WM8903:

...

> > 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.

> 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.

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Mon, Nov 28, 2011 at 05:23:30PM -0600, Rob Herring wrote:

> 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:

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.

RE: Configurable interrupt sources, and DT bindings

[Stephen Warren]

Rob Herring wrote at Monday, November 28, 2011 4:24 PM:
> On 11/28/2011 04:29 PM, Stephen Warren wrote:
> > 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....
...
> > 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.
> 
> 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.

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.

-- 
nvpublic

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Tue, Nov 29, 2011 at 10:55:38AM +0000, Mark Brown wrote:
> On Tue, Nov 29, 2011 at 12:30:55PM +1100, David Gibson wrote:
> > On Mon, Nov 28, 2011 at 03:29:51PM -0700, Stephen Warren wrote:
> 
> > > One possibility is to describe this directly in the binding for each
> > > interrupt source. I originally proposed the following for the WM8903:
> 
> ...
> 
> > > 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.

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.

> 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.

> > 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.

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.

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

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Tue, Nov 29, 2011 at 06:24:06PM -0800, Stephen Warren wrote:
> Rob Herring wrote at Monday, November 28, 2011 4:24 PM:
> > On 11/28/2011 04:29 PM, Stephen Warren wrote:
> > > 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....
> ...
> > > 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.
> > 
> > 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.
> 
> 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.

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().

> * 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.

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.

> * 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.

I don't see how there is anything DT related here at all.  It's just
about communicating between different parts of the kernel.

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

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Wed, Nov 30, 2011 at 04:13:49PM +1100, David Gibson wrote:
> On Tue, Nov 29, 2011 at 10:55:38AM +0000, Mark Brown wrote:
> > On Tue, Nov 29, 2011 at 12:30:55PM +1100, David Gibson wrote:

> > > 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.

> 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.

> > 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?

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Wed, Nov 30, 2011 at 09:33:06AM +0000, Mark Brown wrote:
> On Wed, Nov 30, 2011 at 04:13:49PM +1100, David Gibson wrote:
> > On Tue, Nov 29, 2011 at 10:55:38AM +0000, Mark Brown wrote:
> > > On Tue, Nov 29, 2011 at 12:30:55PM +1100, David Gibson wrote:
> 
> > > > 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.
> 
> > 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,

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.

> 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.

>  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.

> 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.

> (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.

> but this all seems so far off base that I fear there must be some
> fundamental misunderstaniding here.

I tend to agree.

> > > 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?

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.

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

Re: Configurable interrupt sources, and DT bindings

[Rob Herring]

On 11/29/2011 08:24 PM, Stephen Warren wrote:
> Rob Herring wrote at Monday, November 28, 2011 4:24 PM:
>> On 11/28/2011 04:29 PM, Stephen Warren wrote:
>>> 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....
> ...
>>> 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.
>>
>> 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.
> 
> 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.

Sounds good to me.

Rob

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Thu, Dec 01, 2011 at 12:31:40AM +1100, David Gibson wrote:
> On Wed, Nov 30, 2011 at 09:33:06AM +0000, Mark Brown wrote:

> > 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.

> > 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.

> > (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.  

> > 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.

> 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.

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Thu, Dec 01, 2011 at 11:07:39AM +0000, Mark Brown wrote:
> On Thu, Dec 01, 2011 at 12:31:40AM +1100, David Gibson wrote:
> > On Wed, Nov 30, 2011 at 09:33:06AM +0000, Mark Brown wrote:
> 
> > > 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.

Roughly, yes.  In fact even the existence and layout of a "type" field
is up to the controller.

> > > 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.

> > > (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.  

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.

> > > 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).

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

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Fri, Dec 02, 2011 at 04:38:14PM +1100, David Gibson wrote:
> On Thu, Dec 01, 2011 at 11:07:39AM +0000, Mark Brown wrote:

> > 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.

> 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.

> > 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.

> > 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.

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Fri, Dec 02, 2011 at 11:27:22AM +0000, Mark Brown wrote:
> On Fri, Dec 02, 2011 at 04:38:14PM +1100, David Gibson wrote:
> > On Thu, Dec 01, 2011 at 11:07:39AM +0000, Mark Brown wrote:
> 
> > > 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.

No, you suggested it be a global property of the interrupt source.
Same problem, just less so.

> > 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.
> 
> > > 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.
> 
> > > 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.

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.

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

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Fri, Dec 02, 2011 at 11:24:18PM +1100, David Gibson wrote:

> No, you suggested it be a global property of the interrupt source.
> Same problem, just less so.

Cite?

> > 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.

Re: Configurable interrupt sources, and DT bindings

[David Gibson]

On Fri, Dec 02, 2011 at 01:34:13PM +0000, Mark Brown wrote:
> On Fri, Dec 02, 2011 at 11:24:18PM +1100, David Gibson wrote:
> 
> > 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.

> > > 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.

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.

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

Re: Configurable interrupt sources, and DT bindings

[Mark Brown]

On Sat, Dec 03, 2011 at 01:31:07AM +1100, David Gibson wrote:

> 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.

> 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.

> > *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.

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.