diff mbox

[v2,1/2] doc/devicetree: Add Aspeed VIC bindings

Message ID 1462802317-27086-2-git-send-email-joel@jms.id.au (mailing list archive)
State New, archived
Headers show

Commit Message

Joel Stanley May 9, 2016, 1:58 p.m. UTC
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

Comments

Rob Herring (Arm) May 11, 2016, 2:33 p.m. UTC | #1
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
Benjamin Herrenschmidt May 12, 2016, 1:01 a.m. UTC | #2
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
Rob Herring (Arm) May 12, 2016, 5:20 p.m. UTC | #3
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
Joel Stanley May 18, 2016, 1:50 p.m. UTC | #4
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
Rob Herring (Arm) May 20, 2016, 8:13 p.m. UTC | #5
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 mbox

Patch

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>;
+ };