Message-ID: <575EDE48.6090006@arm.com>
Date:	Mon, 13 Jun 2016 17:24:40 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Sebastian Frias <sf84@...oste.net>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Grygorii Strashko <grygorii.strashko@...com>,
	Sricharan R <r.sricharan@...com>, Mason <slash.tmp@...e.fr>,
	Måns Rullgård <mans@...sr.com>
Subject: Re: Using irq-crossbar.c

Sebastian,

On 13/06/16 16:46, Sebastian Frias wrote:
> Hi Marc,
> 
> Thanks for your reply, please find my comments below.
> 
> On 06/10/2016 06:05 PM, Marc Zyngier wrote:
>> On 10/06/16 16:37, Sebastian Frias wrote:
>>> Hi,
>>>
>>> We are trying to write a driver for an interrupt controller (actually
>>> more of a crossbar) for an ARM-based SoC. This IRQ crossbar has 128
>>> inputs and 24 outputs, the outputs are connected directly to the
>>> GIC. The idea is that the GIC handles everything, and just request a
>>> mapping from an IRQ number (0...127, from a device's DT entry) into
>>> one of its 24 input lines.
>>
>> "Just request a mapping...".
>>
>>> By looking at current code (4.7-rc1) there seems to be a driver
>>> (drivers/irqchip/irq-crossbar.c) that provides similar
>>> functionality. The driver uses hierarchical irq domains (since commit
>>> 783d31863fb8 "irqchip: crossbar: Convert dra7 crossbar to stacked
>>> domains") which we believe we don't need because the only controller
>>> is the GIC.
>>
>> So you need it, but you don't need it? The GIC may be the only interrupt
>> controller with which software interacts when the interrupt occurs, but
>> the crossbar does play a major role in *routing* the interrupt to the
>> right GIC pin.
> 
> My understanding of "hierarchical irq domains" is that it is useful
> when there are multiple stacked interrupt controllers. Also, the
> documentation says "the majority of drivers should use the linear
> map" (as opposed to the hierarchical one).

The "linear map" to be opposed to the "tree", not to the hierarchy.
Hierarchies themselves can be built out most domain type (only the
underlying data structure changes).

> Maybe the definition of "interrupt controller" could benefit from
> some clarification, but my understanding is that, in our case, the
> GIC is the only interrupt controller (that's where the interrupt type
> must be set active_high/active_low/edge, etc.), in front of it, it
> happens to be a crossbar, that happens to be programmable, and that
> can be used to map any 128 line into any of 24 lines of the GIC
> (actually it is a many-to-many router, without any latching nor edge
> detection functionality)

An interrupt controller is absolutely *anything* that is on the
interrupt path, unless it is absolutely transparent, invisible to
software, and doesn't require any form of configuration. Your own
definition of an interrupt controller is way too restrictive.

> Obviously, when the DT says that ethernet driver uses IRQ=120 (for
> example), the crossbar must be setup to route line 120 to one of the
> 24 lines going to the GIC. So a minimum of interaction between the
> GIC driver (and/or the Linux IRQ framework) and the driver
> programming the crossbar is required, and that's what we are trying
> to achieve.
>
> Does that makes sense?

Maybe you and Mason should get together and decide what you want to
support. Because you seem to have diverging requirements (Mason
suggesting the exact opposite over the weekend).

> 
>>
>>> However the API used previously, register_routable_domain_ops(), was
>>> removed with commit a5561c3e845c "irqchip: gic: Get rid of routable
>>> domain".
>>
>> And every day, I thank $DEITY for having delivered us from this evil.
>> Really. And it wasn't much of an API. It was the son of a hack, bolted
>> on the side of another hack. Unmaintainable, getting in the way. I had
>> much fun slaughtering it! ;-)
>>
>>> Trying to use the driver with hierarchical domains (after
>>> modifications for our SoC), results on the kernel being blocked at
>>> some point:
>>>
>>> [    0.041524] ThumbEE CPU extension supported.
>>> [    0.041589] Registering SWP/SWPB emulation handler
>>> [    0.052022] Freeing unused kernel memory: 12364K (c029b000 - c0eae000)
>>> [    0.074084] random: dbus-uuidgen urandom read with 0 bits of entropy available
>>
>> Sorry, but that's not much of a log. Anything related to interrupts, maybe?
> 
> That's the last log (it's stuck there) and I was asking how/where to enable more logs to be able to debug this.
> 
> Or there are no standardised logs and every person has to come up
> with its own debug logs?

We have basic tracing in the irqdomain layer, and you can then
instrument your own driver.

>>
>>> We've put logs on the different domain_ops calls (alloc, free,
>>> translate) but they are not called, even if the DT is supposed to
>>> tell devices to take interrupts from this controller (*).
>>
>> At all? Nobody is talking to the GIC?
>>
>>> Do you have suggestions on what APIs should be used, further
>>> reading/examples and/or pointers on how debug this (logs to enable,
>>> things to look for, etc.)?
>>
>> You could start with posting actual logs of an interrupt being
>> requested, as well as perform some basic tracing of the various
>> callbacks into the irqdomain and irqchip layers, all the way down to the
>> interrupt controllers (note the plural). 
> 
> Ok, thanks.
> 
> In the meanwhile, and in case irq-crossbar is not the good example for our case, would it be possible to get some guidance, examples, tips, on how to write a driver like the one described? ie:
> 
> [0..127] IRQ inputs => BIG_MUX => [0..23] outputs => [0..23] GIC inputs
> 
> BIG_MUX is a many-to-many router:
> - 128x32bit registers to setup a route between any of the 128 input
> (IRQ_dev) and any of the 24 outputs (IRQ_gic)
> -   4x32bit registers that read the RAW status of each of the 128
> lines (no latching nor edge detection) not sure how useful is to read
> such RAW status, because (naive hypothesis follows) Linux's IRQ
> framework could remember which IRQ_dev lines are routed to which
> IRQ_gic, and thus when handling IRQ_gic(x), Linux could just ask the
> drivers tied to it to check for interruptions.
> 

OK, so this is definitely a pure router, and the lack of latch makes it
completely unsuitable for a a cascaded interrupt controller. At least,
we've managed to establish that this thing will never be able to handle
more than 24 devices in a sane way. So let's forget about Mason's idea
of cascading everything to a single output line, and let's focus on your
initial idea of having something similar to TI's crossbar, which is a
much saner approach.

> 
>> Also, without seeing the code,
>> it is pretty difficult to make any meaningful comment...
> 
> Base code is either 4.7rc1 or 4.4.
> The irq-crossbar code is not much different from TI, but you can find it attached.

Please post it separately (and inline), the email client I have here
makes it hard to review attached patches.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Powered by blists - more mailing lists