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.
