Re: [PATCH v6] irqchip: Add support for tango interrupt router

[Mason <slash.tmp@free.fr>]

On 23/08/2017 12:58, Marc Zyngier wrote:

> On 20/08/17 18:22, Mason wrote:
>
>> On 07/08/2017 14:47, Marc Zyngier wrote:
>>
>>> On 01/08/17 17:56, Mason wrote:
>>>
>>>> +static int tango_alloc(struct irq_domain *dom, uint virq, uint n, void *arg)
>>>> +{
>>>> +	int spi;
>>>> +	struct irq_fwspec *fwspec = arg;
>>>> +	struct tango_intc *intc = dom->host_data;
>>>> +	u32 hwirq = fwspec->param[0], trigger = fwspec->param[1];
>>>> +
>>>> +	if (trigger & IRQ_TYPE_EDGE_FALLING || trigger & IRQ_TYPE_LEVEL_LOW)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (trigger & IRQ_TYPE_LEVEL_HIGH)
>>>> +		intc->tango_irq_to_spi[hwirq] = LEVEL_SPI;
>>>> +
>>>> +	if (trigger & IRQ_TYPE_EDGE_RISING) {
>>>> +		for (spi = 1; spi < SPI_MAX; ++spi) {
>>>> +			if (intc->spi_to_tango_irq[spi] == 0) {
>>>> +				intc->tango_irq_to_spi[hwirq] = spi;
>>>> +				intc->spi_to_tango_irq[spi] = hwirq;
>>>> +				break;
>>>> +			}
>>>> +		}
>>>> +		if (spi == SPI_MAX)
>>>> +			return -ENOSPC;
>>>> +	}
>>>
>>> What's wrong with having a bitmap allocation, just like on other drivers?
>>
>> I don't understand what you are suggesting.
>>
>> The mapping is set up at run-time, I need to record it
>> somewhere.
> 
> Again. All the other drivers in the tree are using a bitmap to deal with
> their slot allocation. Why do you have to use a different data structure?

You appear to be objecting to the spi_to_tango_irq array.

The spi-to-tango-irq mapping has to be stored somewhere.

If I use a hierarchy for edge interrupts, as you have
demanded, then it becomes the core's responsibility to
store the mapping. Thus, I can drop the array, and just
use a bitmap to keep track of which output has already
been allocated.


>>> Calling panic? For a secondary interrupt controller? Don't. We call
>>> panic when we know for sure that the system is in such a state that
>>> we're better off killing it altogether than keeping it running (to avoid
>>> corruption, for example). panic is not a substitute for proper error
>>> handling.
>>
>> I handled the setup like irq-tango.c did.
> 
> Doesn't make it less crap.

Just want to clear something up.

If irq-tango.c were submitted today, would you demand this
issue be fixed, or are some submitters given more leeway
than others?


>>> Overall, this edge business feels wrong. If you want to mux a single
>>> output for all level interrupts, fine by me. But edge interrupts that
>>> have a 1:1 mapping with the underlying SPI must be represented as a
>>> hierarchy.
>>
>> I don't understand what you mean by "feels wrong".
>>
>> There are 128 inputs, and only 24 outputs.
>> Therefore, I must map some inputs to the same output.
>> Thomas explained that edge interrupts *cannot* be shared.
>> So edge interrupts must receive a dedicated output line.
>> Did I write anything wrong so far?
> 
> Let me repeat what Thomas already said:
> 
> - you dedicate one line to level interrupts using a multiplexer (chained
> interrupts).

OK.

> - you use the remaining 23 inputs in a hierarchical model, each input
> being mapped to one output, no chained handler.
> 
> That's what I want to see.

OK.

Can you confirm that this means two separate domains?


One last thing: about generic_handle_irq() and virq==0
I understand your point that irq_to_desc() is an expensive
operation, so it is better to check beforehand. But then,
would it not make sense to add the check in generic_handle_irq()
if all drivers are expected to do it? (Code factoring)

Regards.