Messages in this thread |  | | Subject | Re: [PATCH v6] irqchip: Add support for tango interrupt router | From | Mason <> | Date | Wed, 23 Aug 2017 17:45:49 +0200 |
| |
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.
|  |