[PATCH 1/8] SH: intc: Add support OF for INTC

Arnd Bergmann arnd at arndb.de
Wed Jan 9 14:11:04 EST 2013

On Wednesday 09 January 2013, Mark Rutland wrote:
> Hi,
> Thanks for updating the text, this is far easier to read than previously.
> However, I'm still concerned by how complex the binding seems. As I don't have
> any familiarity with the device, I don't know whether that's just an artifact
> of the hardware or something that can be cleared up.
> I think the approach used by the binding needs some serious review before this
> should be merged. It seems far more complex than any existing interrupt
> controller binding. Without a dts example for a complete board (complete with
> devices wired up to the interrupt controller), it's difficult to judge how this
> will work in practice.
> I've added Arnd to Cc in case he has any thoughts on the matter.

Sorry for having been absent from this discussion for so long. I didn't
realize there were 9 versions of this patch set.

I tend to agree with your interpretation above, but I may be missing
important facts from the previous review rounds.

For all I can tell, the binding is an attempt to describe the
entire drivers/sh/intc capabilities, which is probably not the
best way to approach things. The sh intc driver is not just an
irqchip driver, but rather a framework to describe arbitrary
irqchips, which is what makes this so hard.

When I first looked at the situation last year, I suggested doing
a new irqchip driver with a much simpler binding that can only
handle the irq chips from shmobile, rather than the whole thing.

I am not sure if the binding in the current form is already the
"simplified" version, or if it actually implements all the
capabilities of the intc driver.

> > +       intca: interrupt-controller at 0 {
> > +               compatible = "renesas,sh_intc";
> > +               interrupt-controller;
> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               #interrupt-cells = <1>;
> > +               ranges;
> > +
> > +               reg = <0xe6940000 0x200>, <0xe6950000 0x200>;
> > +               group_size = <19>;
> > +
> > +               DIRC: intsrc1 { vector = <0x0560>; };
> > +               ATAPI: intsrc2 { vector = <0x05E0>; };
> This looks suspiciously like a way of encoding a device's interrupt information
> into the interrupt controller's device node. That strikes me as being the wrong
> way round.

Agreed, it would be simpler to have e.g. #interrupt-cells = <4>, to describe
the various offsets when needed (I forgot how many are actually required
in practice, rather than being computable from the other numbers), and
possibly a global interrupt-map/interrupt-map-mask property pair to map
this into a flat number space.

I need to take some more time to understand the actual requirements again,
but IIRC it would be possible to do something much simpler than the
proposed binding.


More information about the linux-arm-kernel mailing list