| Message ID | 1462802317-27086-2-git-send-email-joel@jms.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 09, 2016 at 11:28:36PM +0930, Joel Stanley wrote: > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > v2: > Fix device tree binding example, thanks Baruch and Arnd for pointing this out > > .../interrupt-controller/aspeed,ast2400-vic.txt | 24 ++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt > new file mode 100644 > index 000000000000..86dede1755a2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt > @@ -0,0 +1,24 @@ > +Aspeed Vectored Interrupt Controller > + > +These bindings are for the Aspeed AST2400 interrupt controller register layout. > +The SoC has an legacy register layout, but this driver does not support that > +mode of operation. > + > +Required properties: > + > +- compatible : should be "aspeed,ast2400-vic". > + > +- interrupt-controller : Identifies the node as an interrupt controller > +- #interrupt-cells : Specifies the number of cells needed to encode an > + interrupt source. The value shall be 1. No need for level vs. edge flags? > +- valid-sources : bitmask of valid irq sources Drop this. Either all interrupt controllers need this or none of them do. The valid sources are the ones described in the DT. > + > +Example: > + > + vic: interrupt-controller { Needs a unit address. > + compatible = "aspeed,ast2400-vic"; > + interrupt-controller; > + #interrupt-cells = <1>; > + valid-sources = < 0xffffffff 0x0007ffff>; > + reg = <0x1e6c0080 0x80>; > + }; > -- > 2.8.1 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-05-11 at 09:33 -0500, Rob Herring wrote: > > +- interrupt-controller : Identifies the node as an interrupt controller > > +- #interrupt-cells : Specifies the number of cells needed to encode an > > + interrupt source. The value shall be 1. > > No need for level vs. edge flags? That's an open question. Most interrupts are fixed. A handful of GPIOs can be configured either way. For now I am relying on uboot setting up the right config for them and I read it back at boot time, but we could make it part of the binding I suppose. > > +- valid-sources : bitmask of valid irq sources > > Drop this. Either all interrupt controllers need this or none of > them do. The valid sources are the ones described in the DT. Looking at the code (I wrote that ages ago), this is only used for: - Failing map on an unsupported source, so we could drop it - Counting the number of sources in order to optimize the revmap size allocation. We could unconditionally allocate 64, I don't see a big deal here. So yes Joel, feel free to just ditch this. > > + > > +Example: > > + > > + vic: interrupt-controller { > > Needs a unit address. > > > + compatible = "aspeed,ast2400-vic"; > > + interrupt-controller; > > + #interrupt-cells = <1>; > > + valid-sources = < 0xffffffff 0x0007ffff>; > > + reg = <0x1e6c0080 0x80>; > > + }; > > -- > > 2.8.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe devicetree" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 11, 2016 at 8:01 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Wed, 2016-05-11 at 09:33 -0500, Rob Herring wrote: >> > +- interrupt-controller : Identifies the node as an interrupt controller >> > +- #interrupt-cells : Specifies the number of cells needed to encode an >> > + interrupt source. The value shall be 1. >> >> No need for level vs. edge flags? > > That's an open question. Most interrupts are fixed. A handful of GPIOs > can be configured either way. For now I am relying on uboot setting up > the right config for them and I read it back at boot time, but we could > make it part of the binding I suppose. It is almost standard to just use 2 cells here even if reserved for future use. Especially since the IP block seems to have registers to control that. > >> > +- valid-sources : bitmask of valid irq sources >> >> Drop this. Either all interrupt controllers need this or none of >> them do. The valid sources are the ones described in the DT. > > Looking at the code (I wrote that ages ago), this is only used for: I'm guessing it came from the VIC binding. I never really liked the property, but not enough to worry about it. Rob
On Fri, May 13, 2016 at 2:50 AM, Rob Herring <robh@kernel.org> wrote: > On Wed, May 11, 2016 at 8:01 PM, Benjamin Herrenschmidt > <benh@kernel.crashing.org> wrote: >> On Wed, 2016-05-11 at 09:33 -0500, Rob Herring wrote: >>> > +- interrupt-controller : Identifies the node as an interrupt controller >>> > +- #interrupt-cells : Specifies the number of cells needed to encode an >>> > + interrupt source. The value shall be 1. >>> >>> No need for level vs. edge flags? >> >> That's an open question. Most interrupts are fixed. A handful of GPIOs >> can be configured either way. For now I am relying on uboot setting up >> the right config for them and I read it back at boot time, but we could >> make it part of the binding I suppose. > > It is almost standard to just use 2 cells here even if reserved for > future use. Especially since the IP block seems to have registers to > control that. Yep, makes sense. I've added this to the bindings document. I was trying to use the second cell in our driver: static struct irq_domain_ops avic_dom_ops = { .map = avic_map, .xlate = irq_domain_xlate_twocell, }; So we have irq_domain_xlate_twocell to parse the device tree and grabs the type property from the second cell. In avic_map we set the irq handler: if (vic->edge_sources[sidx] & sbit) { /* TODO: Check aginst type from dt and warn if not edge */ irq_set_chip_and_handler(irq, &avic_chip, handle_edge_irq); } else { /* TODO: Check aginst type from dt and warn if not level */ irq_set_chip_and_handler(irq, &avic_chip, handle_level_irq); } However, we don't have the type here, so we can't use it to check that we're setting the correct edge/level callback (or use it to set the register in the future if we want to use the device tree to override the bootloader settings). I had a look at some other drivers and they don't seem to use the dt property when mapping the irq. What am I missing here? Cheers, Joel
On Wed, May 18, 2016 at 8:50 AM, Joel Stanley <joel@jms.id.au> wrote: > On Fri, May 13, 2016 at 2:50 AM, Rob Herring <robh@kernel.org> wrote: >> On Wed, May 11, 2016 at 8:01 PM, Benjamin Herrenschmidt >> <benh@kernel.crashing.org> wrote: >>> On Wed, 2016-05-11 at 09:33 -0500, Rob Herring wrote: >>>> > +- interrupt-controller : Identifies the node as an interrupt controller >>>> > +- #interrupt-cells : Specifies the number of cells needed to encode an >>>> > + interrupt source. The value shall be 1. >>>> >>>> No need for level vs. edge flags? >>> >>> That's an open question. Most interrupts are fixed. A handful of GPIOs >>> can be configured either way. For now I am relying on uboot setting up >>> the right config for them and I read it back at boot time, but we could >>> make it part of the binding I suppose. >> >> It is almost standard to just use 2 cells here even if reserved for >> future use. Especially since the IP block seems to have registers to >> control that. > > Yep, makes sense. I've added this to the bindings document. > > I was trying to use the second cell in our driver: > > static struct irq_domain_ops avic_dom_ops = { > .map = avic_map, > .xlate = irq_domain_xlate_twocell, > }; > > So we have irq_domain_xlate_twocell to parse the device tree and > grabs the type property from the second cell. > > In avic_map we set the irq handler: > > if (vic->edge_sources[sidx] & sbit) { > /* TODO: Check aginst type from dt and warn if not edge */ > irq_set_chip_and_handler(irq, &avic_chip, handle_edge_irq); > } else { > /* TODO: Check aginst type from dt and warn if not level */ > irq_set_chip_and_handler(irq, &avic_chip, handle_level_irq); > } > > However, we don't have the type here, so we can't use it to check that > we're setting the correct edge/level callback (or use it to set the > register in the future if we want to use the device tree to override > the bootloader settings). > > I had a look at some other drivers and they don't seem to use the dt > property when mapping the irq. What am I missing here? The type will be set by the irqdomain core when the mapping is created and this should result in irq_set_type() being called on your irqchip. The map function doesn't need to deal with type. Rob
diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt new file mode 100644 index 000000000000..86dede1755a2 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt @@ -0,0 +1,24 @@ +Aspeed Vectored Interrupt Controller + +These bindings are for the Aspeed AST2400 interrupt controller register layout. +The SoC has an legacy register layout, but this driver does not support that +mode of operation. + +Required properties: + +- compatible : should be "aspeed,ast2400-vic". + +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. The value shall be 1. +- valid-sources : bitmask of valid irq sources + +Example: + + vic: interrupt-controller { + compatible = "aspeed,ast2400-vic"; + interrupt-controller; + #interrupt-cells = <1>; + valid-sources = < 0xffffffff 0x0007ffff>; + reg = <0x1e6c0080 0x80>; + };
Signed-off-by: Joel Stanley <joel@jms.id.au> --- v2: Fix device tree binding example, thanks Baruch and Arnd for pointing this out .../interrupt-controller/aspeed,ast2400-vic.txt | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-vic.txt