| Message ID | eea0ba7d-15d2-fb20-7c23-b2d3c4a7cfa7@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/08/17 17:56, Mason wrote: > This controller maps 128 input lines to 24 output lines. > The output lines are routed to GIC SPI 0 to 23. > This driver muxes LEVEL_HIGH IRQs onto output line 0, > and gives every EDGE_RISING IRQ a dedicated output line. > --- > Changes from v5 to v6: > Add suspend/resume support > > This driver is now feature complete AFAICT. > Marc, Thomas, Jason, it would be great if you could tell me > if I did something stupid wrt the irqchip API. > --- > .../interrupt-controller/sigma,smp8759-intc.txt | 18 ++ > drivers/irqchip/Makefile | 2 +- > drivers/irqchip/irq-smp8759.c | 204 +++++++++++++++++++++ > 3 files changed, 223 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt > create mode 100644 drivers/irqchip/irq-smp8759.c > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt > new file mode 100644 > index 000000000000..9ec922f3c0a4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt > @@ -0,0 +1,18 @@ > +Sigma Designs SMP8759 interrupt router > + > +Required properties: > +- compatible: "sigma,smp8759-intc" > +- reg: address/size of register area > +- interrupt-controller > +- #interrupt-cells: <2> (hwirq and trigger_type) > +- interrupt-parent: parent phandle > + > +Example: > + > + interrupt-controller@6f800 { > + compatible = "sigma,smp8759-intc"; > + reg = <0x6f800 0x430>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupt-parent = <&gic>; > + }; > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index e4dbfc85abdb..013104923b71 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -47,7 +47,7 @@ obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o > obj-$(CONFIG_ARCH_NSPIRE) += irq-zevio.o > obj-$(CONFIG_ARCH_VT8500) += irq-vt8500.o > obj-$(CONFIG_ST_IRQCHIP) += irq-st.o > -obj-$(CONFIG_TANGO_IRQ) += irq-tango.o > +obj-$(CONFIG_TANGO_IRQ) += irq-tango.o irq-smp8759.o > obj-$(CONFIG_TB10X_IRQC) += irq-tb10x.o > obj-$(CONFIG_TS4800_IRQ) += irq-ts4800.o > obj-$(CONFIG_XTENSA) += irq-xtensa-pic.o > diff --git a/drivers/irqchip/irq-smp8759.c b/drivers/irqchip/irq-smp8759.c > new file mode 100644 > index 000000000000..a1680a250598 > --- /dev/null > +++ b/drivers/irqchip/irq-smp8759.c > @@ -0,0 +1,204 @@ > +#include <linux/irqchip.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/syscore_ops.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > + > +/* > + * This controller maps IRQ_MAX input lines to SPI_MAX output lines. > + * The output lines are routed to GIC SPI 0 to 23. > + * This driver muxes LEVEL_HIGH IRQs onto output line 0, > + * and gives every EDGE_RISING IRQ a dedicated output line. > + */ > +#define IRQ_MAX 128 > +#define SPI_MAX 24 > +#define LEVEL_SPI 0 > +#define IRQ_ENABLE BIT(31) > +#define STATUS 0x420 > + > +struct tango_intc { > + void __iomem *base; > + struct irq_domain *dom; > + u8 spi_to_tango_irq[SPI_MAX]; > + u8 tango_irq_to_spi[IRQ_MAX]; > + DECLARE_BITMAP(enabled_level, IRQ_MAX); > + DECLARE_BITMAP(enabled_edge, IRQ_MAX); > + spinlock_t lock; > +}; > + > +static struct tango_intc *tango_intc; > + > +static void tango_level_isr(struct irq_desc *desc) > +{ > + uint pos, virq; > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct tango_intc *intc = irq_desc_get_handler_data(desc); > + DECLARE_BITMAP(status, IRQ_MAX); > + > + chained_irq_enter(chip, desc); > + > + spin_lock(&intc->lock); > + memcpy_fromio(status, intc->base + STATUS, IRQ_MAX / BITS_PER_BYTE); No. Please don't. Nothing guarantees that your bus can deal with those. We have readl_relaxed, which is what should be used. Also, you do know which inputs are level, right? So why do you need to read the whole register array all the time? > + bitmap_and(status, status, intc->enabled_level, IRQ_MAX); > + spin_unlock(&intc->lock); > + > + for_each_set_bit(pos, status, IRQ_MAX) { > + virq = irq_find_mapping(intc->dom, pos); > + generic_handle_irq(virq); Please check for virq==0, just in case you get a spurious interrupt. > + } > + > + chained_irq_exit(chip, desc); > +} > + > +static void tango_edge_isr(struct irq_desc *desc) > +{ > + uint virq; > + struct irq_data *data = irq_desc_get_irq_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct tango_intc *intc = irq_desc_get_handler_data(desc); > + int tango_irq = intc->spi_to_tango_irq[data->hwirq - 32]; > + > + chained_irq_enter(chip, desc); > + virq = irq_find_mapping(intc->dom, tango_irq); > + generic_handle_irq(virq); > + chained_irq_exit(chip, desc); If you have a 1:1 mapping between an edge input and its output, why do you with a chained interrupt handler? This should be a hierarchical setup for these 23 interrupts. > +} > + > +static void tango_mask(struct irq_data *data) > +{ > + unsigned long flags; > + struct tango_intc *intc = data->chip_data; > + u32 spi = intc->tango_irq_to_spi[data->hwirq]; > + > + spin_lock_irqsave(&intc->lock, flags); > + writel_relaxed(0, intc->base + data->hwirq * 4); > + if (spi == LEVEL_SPI) > + __clear_bit(data->hwirq, intc->enabled_level); > + else > + __clear_bit(data->hwirq, intc->enabled_edge); > + spin_unlock_irqrestore(&intc->lock, flags); > +} > + > +static void tango_unmask(struct irq_data *data) > +{ > + unsigned long flags; > + struct tango_intc *intc = data->chip_data; > + u32 spi = intc->tango_irq_to_spi[data->hwirq]; > + > + spin_lock_irqsave(&intc->lock, flags); > + writel_relaxed(IRQ_ENABLE | spi, intc->base + data->hwirq * 4); > + if (spi == LEVEL_SPI) > + __set_bit(data->hwirq, intc->enabled_level); > + else > + __set_bit(data->hwirq, intc->enabled_edge); > + spin_unlock_irqrestore(&intc->lock, flags); > +} > + > +static int tango_set_type(struct irq_data *data, uint flow_type) > +{ > + return 0; What does this mean? Either you can do a set-type (and you do it), or you cannot, and you fail. At least you check that what you're asked to do matches the configuration. > +} > + > +static struct irq_chip tango_chip = { > + .name = "tango", > + .irq_mask = tango_mask, > + .irq_unmask = tango_unmask, > + .irq_set_type = tango_set_type, > +}; > + > +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 haing a bitmap allocation, just like on other drivers? > + > + irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &tango_chip, intc); > + irq_set_handler(virq, handle_simple_irq); > + > + return 0; > +} > + > +static struct irq_domain_ops dom_ops = { > + .xlate = irq_domain_xlate_twocell, > + .alloc = tango_alloc, > +}; > + > +static void tango_resume(void) > +{ > + int hwirq; > + DECLARE_BITMAP(enabled, IRQ_MAX); > + struct tango_intc *intc = tango_intc; > + > + bitmap_or(enabled, intc->enabled_level, intc->enabled_edge, IRQ_MAX); > + for (hwirq = 0; hwirq < IRQ_MAX; ++hwirq) { > + u32 val = intc->tango_irq_to_spi[hwirq]; > + if (test_bit(hwirq, enabled)) > + val |= IRQ_ENABLE; > + writel_relaxed(val, intc->base + hwirq * 4); > + } > +} > + > +static struct syscore_ops tango_syscore_ops = { > + .resume = tango_resume, > +}; > + > +static int __init map_irq(struct device_node *gic, int spi, int trigger) > +{ > + struct of_phandle_args irq_data = { gic, 3, { 0, spi, trigger }}; > + return irq_create_of_mapping(&irq_data); > +} > + > +static int __init tango_irq_init(struct device_node *node, struct device_node *parent) > +{ > + int spi, virq; > + struct tango_intc *intc; > + > + intc = kzalloc(sizeof(*intc), GFP_KERNEL); > + if (!intc) > + panic("%s: Failed to kalloc\n", node->name); > + > + virq = map_irq(parent, LEVEL_SPI, IRQ_TYPE_LEVEL_HIGH); > + if (!virq) > + panic("%s: Failed to map IRQ %d\n", node->name, LEVEL_SPI); > + > + irq_set_chained_handler_and_data(virq, tango_level_isr, intc); > + > + for (spi = 1; spi < SPI_MAX; ++spi) { > + virq = map_irq(parent, spi, IRQ_TYPE_EDGE_RISING); > + if (!virq) > + panic("%s: Failed to map IRQ %d\n", node->name, spi); > + > + irq_set_chained_handler_and_data(virq, tango_edge_isr, intc); > + } 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. > + > + tango_intc = intc; > + register_syscore_ops(&tango_syscore_ops); > + > + spin_lock_init(&intc->lock); > + intc->base = of_iomap(node, 0); > + intc->dom = irq_domain_add_linear(node, IRQ_MAX, &dom_ops, intc); > + if (!intc->base || !intc->dom) > + panic("%s: Failed to setup IRQ controller\n", node->name); > + > + return 0; > +} > +IRQCHIP_DECLARE(tango_intc, "sigma,smp8759-intc", tango_irq_init); > Overall, this edge business feels wrong. If you want to mux a single putput 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. M.
On 07/08/2017 14:47, Marc Zyngier wrote: > On 01/08/17 17:56, Mason wrote: > >> +/* >> + * This controller maps IRQ_MAX input lines to SPI_MAX output lines. >> + * The output lines are routed to GIC SPI 0 to 23. >> + * This driver muxes LEVEL_HIGH IRQs onto output line 0, >> + * and gives every EDGE_RISING IRQ a dedicated output line. >> + */ >> +#define IRQ_MAX 128 >> +#define SPI_MAX 24 >> +#define LEVEL_SPI 0 >> +#define IRQ_ENABLE BIT(31) >> +#define STATUS 0x420 >> + >> +struct tango_intc { >> + void __iomem *base; >> + struct irq_domain *dom; >> + u8 spi_to_tango_irq[SPI_MAX]; >> + u8 tango_irq_to_spi[IRQ_MAX]; >> + DECLARE_BITMAP(enabled_level, IRQ_MAX); >> + DECLARE_BITMAP(enabled_edge, IRQ_MAX); >> + spinlock_t lock; >> +}; >> + >> +static struct tango_intc *tango_intc; >> + >> +static void tango_level_isr(struct irq_desc *desc) >> +{ >> + uint pos, virq; >> + struct irq_chip *chip = irq_desc_get_chip(desc); >> + struct tango_intc *intc = irq_desc_get_handler_data(desc); >> + DECLARE_BITMAP(status, IRQ_MAX); >> + >> + chained_irq_enter(chip, desc); >> + >> + spin_lock(&intc->lock); >> + memcpy_fromio(status, intc->base + STATUS, IRQ_MAX / BITS_PER_BYTE); > > No. Please don't. Nothing guarantees that your bus can deal with those. > We have readl_relaxed, which is what should be used. Is that because readl_relaxed() handles endianness, while memcpy_fromio() does not? How do I fill a DECLARE_BITMAP using readl_relaxed() ? > Also, you do know which inputs are level, right? So why do you need to > read the whole register array all the time? AFAIR, interrupts are scattered all over the map, so there's at least one interrupt per word. I'll double-check. >> + bitmap_and(status, status, intc->enabled_level, IRQ_MAX); >> + spin_unlock(&intc->lock); >> + >> + for_each_set_bit(pos, status, IRQ_MAX) { >> + virq = irq_find_mapping(intc->dom, pos); >> + generic_handle_irq(virq); > > Please check for virq==0, just in case you get a spurious interrupt. AFAICT, generic_handle_irq() would handle virq==0 gracefully(?) >> +static void tango_edge_isr(struct irq_desc *desc) >> +{ >> + uint virq; >> + struct irq_data *data = irq_desc_get_irq_data(desc); >> + struct irq_chip *chip = irq_desc_get_chip(desc); >> + struct tango_intc *intc = irq_desc_get_handler_data(desc); >> + int tango_irq = intc->spi_to_tango_irq[data->hwirq - 32]; >> + >> + chained_irq_enter(chip, desc); >> + virq = irq_find_mapping(intc->dom, tango_irq); >> + generic_handle_irq(virq); >> + chained_irq_exit(chip, desc); > > If you have a 1:1 mapping between an edge input and its output, why do > you with a chained interrupt handler? This should be a hierarchical > setup for these 23 interrupts. I don't understand what you are suggesting. I should not call chained_irq_enter/chained_irq_exit in tango_edge_isr()? >> +static int tango_set_type(struct irq_data *data, uint flow_type) >> +{ >> + return 0; > > What does this mean? Either you can do a set-type (and you do it), or > you cannot, and you fail. At least you check that what you're asked to > do matches the configuration. IIRC, __irq_set_trigger() barfed when I did it differently. (FWIW, this HW block only routes interrupt signals, it doesn't latch anything.) >> +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. >> +static int __init tango_irq_init(struct device_node *node, struct device_node *parent) >> +{ >> + int spi, virq; >> + struct tango_intc *intc; >> + >> + intc = kzalloc(sizeof(*intc), GFP_KERNEL); >> + if (!intc) >> + panic("%s: Failed to kalloc\n", node->name); >> + >> + virq = map_irq(parent, LEVEL_SPI, IRQ_TYPE_LEVEL_HIGH); >> + if (!virq) >> + panic("%s: Failed to map IRQ %d\n", node->name, LEVEL_SPI); >> + >> + irq_set_chained_handler_and_data(virq, tango_level_isr, intc); >> + >> + for (spi = 1; spi < SPI_MAX; ++spi) { >> + virq = map_irq(parent, spi, IRQ_TYPE_EDGE_RISING); >> + if (!virq) >> + panic("%s: Failed to map IRQ %d\n", node->name, spi); >> + >> + irq_set_chained_handler_and_data(virq, tango_edge_isr, intc); >> + } > > 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. What is the point in bringing up a system where interrupts do not work? Nothing will work. >> + tango_intc = intc; >> + register_syscore_ops(&tango_syscore_ops); >> + >> + spin_lock_init(&intc->lock); >> + intc->base = of_iomap(node, 0); >> + intc->dom = irq_domain_add_linear(node, IRQ_MAX, &dom_ops, intc); >> + if (!intc->base || !intc->dom) >> + panic("%s: Failed to setup IRQ controller\n", node->name); >> + >> + return 0; >> +} >> +IRQCHIP_DECLARE(tango_intc, "sigma,smp8759-intc", tango_irq_init); > > 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? Therefore, I figured that I must A) map every edge interrupt to a different output B) map at least some level interrupts to the same output Are you saying that I could do things differently? Do you mean I should have defined two separate domains, one for level interrupts, one for edge interrupts? For level interrupts: irq_domain_add_linear(node, IRQ_MAX, &dom_ops, intc); For edge interrupts: irq_domain_add_hierarchy(...) Eventually, irq_domain_create_hierarchy() calls irq_domain_create_linear() anyway. So maybe you are suggesting a single hierarchical domain. I will test tomorrow. What differences will it make? Better performance? Regards.
On 20/08/2017 19:22, Mason wrote: > On 07/08/2017 14:47, Marc Zyngier wrote: > >> On 01/08/17 17:56, Mason wrote: >> >>> +static int tango_set_type(struct irq_data *data, uint flow_type) >>> +{ >>> + return 0; >> >> What does this mean? Either you can do a set-type (and you do it), or >> you cannot, and you fail. At least you check that what you're asked to >> do matches the configuration. > > IIRC, __irq_set_trigger() barfed when I did it differently. The way the problem manifests is that /proc/interrupts cannot determine the trigger types. (I think the issue might be deeper than this cosmetic aspect.) # cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 40: 1832 333 2378 471 GIC-0 29 Edge twd 41: 487 0 0 0 mapper 1 Edge serial 44: 50 0 0 0 mapper 38 Edge eth0 51: 3 0 0 0 mapper 37 Edge phy_interrupt (1 and 38 are actually level interrupts.) ret = sprintf(buf, "%s\n", irqd_is_level_type(&desc->irq_data) ? "level" : "edge"); __irq_set_trigger() is a no-op when irq_set_type is not implemented. But if the callback returns 0, then __irq_set_trigger() eventually calls irqd_clear(&desc->irq_data, IRQD_TRIGGER_MASK); irqd_set(&desc->irq_data, flags); Should I be calling irqd_get_trigger_type() myself earlier, perhaps in the domain alloc function? That's what drivers/irqchip/irq-pic32-evic.c seems to do. The comment seems to state otherwise though: /* * Must only be called inside irq_chip.irq_set_type() functions. */ static inline void irqd_set_trigger_type(struct irq_data *d, u32 type) Regards.
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: >> >>> +/* >>> + * This controller maps IRQ_MAX input lines to SPI_MAX output lines. >>> + * The output lines are routed to GIC SPI 0 to 23. >>> + * This driver muxes LEVEL_HIGH IRQs onto output line 0, >>> + * and gives every EDGE_RISING IRQ a dedicated output line. >>> + */ >>> +#define IRQ_MAX 128 >>> +#define SPI_MAX 24 >>> +#define LEVEL_SPI 0 >>> +#define IRQ_ENABLE BIT(31) >>> +#define STATUS 0x420 >>> + >>> +struct tango_intc { >>> + void __iomem *base; >>> + struct irq_domain *dom; >>> + u8 spi_to_tango_irq[SPI_MAX]; >>> + u8 tango_irq_to_spi[IRQ_MAX]; >>> + DECLARE_BITMAP(enabled_level, IRQ_MAX); >>> + DECLARE_BITMAP(enabled_edge, IRQ_MAX); >>> + spinlock_t lock; >>> +}; >>> + >>> +static struct tango_intc *tango_intc; >>> + >>> +static void tango_level_isr(struct irq_desc *desc) >>> +{ >>> + uint pos, virq; >>> + struct irq_chip *chip = irq_desc_get_chip(desc); >>> + struct tango_intc *intc = irq_desc_get_handler_data(desc); >>> + DECLARE_BITMAP(status, IRQ_MAX); >>> + >>> + chained_irq_enter(chip, desc); >>> + >>> + spin_lock(&intc->lock); >>> + memcpy_fromio(status, intc->base + STATUS, IRQ_MAX / BITS_PER_BYTE); >> >> No. Please don't. Nothing guarantees that your bus can deal with those. >> We have readl_relaxed, which is what should be used. > > Is that because readl_relaxed() handles endianness, while > memcpy_fromio() does not? That's because memcpy_fromio is just that. A memcpy. No guarantees about the size of the access, no indianness, no barriers. > How do I fill a DECLARE_BITMAP using readl_relaxed() ? You don't. >> Also, you do know which inputs are level, right? So why do you need to >> read the whole register array all the time? > > AFAIR, interrupts are scattered all over the map, so there's > at least one interrupt per word. I'll double-check. And even if that's the case. You just read each word that contains a level interrupt, deal with that one, and move on to the next. > > >>> + bitmap_and(status, status, intc->enabled_level, IRQ_MAX); >>> + spin_unlock(&intc->lock); >>> + >>> + for_each_set_bit(pos, status, IRQ_MAX) { >>> + virq = irq_find_mapping(intc->dom, pos); >>> + generic_handle_irq(virq); >> >> Please check for virq==0, just in case you get a spurious interrupt. > > AFAICT, generic_handle_irq() would handle virq==0 > gracefully(?) Yes, by calling irq_to_desc, which results in a radix_tree_lookup. Just check for zero here, there is no reason to propagate the crap if we can avoid it. You can even take that opportunity to dump a splat, because it means that something is pretty fishy if you're getting spurious interrupts. >>> +static void tango_edge_isr(struct irq_desc *desc) >>> +{ >>> + uint virq; >>> + struct irq_data *data = irq_desc_get_irq_data(desc); >>> + struct irq_chip *chip = irq_desc_get_chip(desc); >>> + struct tango_intc *intc = irq_desc_get_handler_data(desc); >>> + int tango_irq = intc->spi_to_tango_irq[data->hwirq - 32]; >>> + >>> + chained_irq_enter(chip, desc); >>> + virq = irq_find_mapping(intc->dom, tango_irq); >>> + generic_handle_irq(virq); >>> + chained_irq_exit(chip, desc); >> >> If you have a 1:1 mapping between an edge input and its output, why do >> you with a chained interrupt handler? This should be a hierarchical >> setup for these 23 interrupts. > > I don't understand what you are suggesting. > > I should not call chained_irq_enter/chained_irq_exit > in tango_edge_isr()? I'm suggesting a hierarchical mapping, and not a multiplexer. I'm sorry, there is no other words to explain this. > >>> +static int tango_set_type(struct irq_data *data, uint flow_type) >>> +{ >>> + return 0; >> >> What does this mean? Either you can do a set-type (and you do it), or >> you cannot, and you fail. At least you check that what you're asked to >> do matches the configuration. > > IIRC, __irq_set_trigger() barfed when I did it differently. > > (FWIW, this HW block only routes interrupt signals, it doesn't > latch anything.) And? You obviously have different ways of dealing with edge and level interrupts. Also, the parent irqchip deals with probably has its own views about interrupt triggering. > > >>> +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? > > >>> +static int __init tango_irq_init(struct device_node *node, struct device_node *parent) >>> +{ >>> + int spi, virq; >>> + struct tango_intc *intc; >>> + >>> + intc = kzalloc(sizeof(*intc), GFP_KERNEL); >>> + if (!intc) >>> + panic("%s: Failed to kalloc\n", node->name); >>> + >>> + virq = map_irq(parent, LEVEL_SPI, IRQ_TYPE_LEVEL_HIGH); >>> + if (!virq) >>> + panic("%s: Failed to map IRQ %d\n", node->name, LEVEL_SPI); >>> + >>> + irq_set_chained_handler_and_data(virq, tango_level_isr, intc); >>> + >>> + for (spi = 1; spi < SPI_MAX; ++spi) { >>> + virq = map_irq(parent, spi, IRQ_TYPE_EDGE_RISING); >>> + if (!virq) >>> + panic("%s: Failed to map IRQ %d\n", node->name, spi); >>> + >>> + irq_set_chained_handler_and_data(virq, tango_edge_isr, intc); >>> + } >> >> 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. > What is the point in bringing up a system where > interrupts do not work? Nothing will work. This is a secondary interrupt controller. For things that are secondary. You already have a primary interrupt controller which can be used to find out about what is going on and diagnostic the platform. And if the platform is really toasted, then there is no need for calling panic, the box won't go anywhere. > > >>> + tango_intc = intc; >>> + register_syscore_ops(&tango_syscore_ops); >>> + >>> + spin_lock_init(&intc->lock); >>> + intc->base = of_iomap(node, 0); >>> + intc->dom = irq_domain_add_linear(node, IRQ_MAX, &dom_ops, intc); >>> + if (!intc->base || !intc->dom) >>> + panic("%s: Failed to setup IRQ controller\n", node->name); >>> + >>> + return 0; >>> +} >>> +IRQCHIP_DECLARE(tango_intc, "sigma,smp8759-intc", tango_irq_init); >> >> 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). - 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. M. > > Therefore, I figured that I must > A) map every edge interrupt to a different output > B) map at least some level interrupts to the same output > > Are you saying that I could do things differently? > > Do you mean I should have defined two separate domains, > one for level interrupts, one for edge interrupts? > > For level interrupts: > irq_domain_add_linear(node, IRQ_MAX, &dom_ops, intc); > > For edge interrupts: > irq_domain_add_hierarchy(...) > > Eventually, irq_domain_create_hierarchy() calls > irq_domain_create_linear() anyway. > > So maybe you are suggesting a single hierarchical > domain. I will test tomorrow. What differences will > it make? Better performance? > > Regards. >
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.
diff --git a/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt new file mode 100644 index 000000000000..9ec922f3c0a4 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/sigma,smp8759-intc.txt @@ -0,0 +1,18 @@ +Sigma Designs SMP8759 interrupt router + +Required properties: +- compatible: "sigma,smp8759-intc" +- reg: address/size of register area +- interrupt-controller +- #interrupt-cells: <2> (hwirq and trigger_type) +- interrupt-parent: parent phandle + +Example: + + interrupt-controller@6f800 { + compatible = "sigma,smp8759-intc"; + reg = <0x6f800 0x430>; + interrupt-controller; + #interrupt-cells = <2>; + interrupt-parent = <&gic>; + }; diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index e4dbfc85abdb..013104923b71 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -47,7 +47,7 @@ obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o obj-$(CONFIG_ARCH_NSPIRE) += irq-zevio.o obj-$(CONFIG_ARCH_VT8500) += irq-vt8500.o obj-$(CONFIG_ST_IRQCHIP) += irq-st.o -obj-$(CONFIG_TANGO_IRQ) += irq-tango.o +obj-$(CONFIG_TANGO_IRQ) += irq-tango.o irq-smp8759.o obj-$(CONFIG_TB10X_IRQC) += irq-tb10x.o obj-$(CONFIG_TS4800_IRQ) += irq-ts4800.o obj-$(CONFIG_XTENSA) += irq-xtensa-pic.o diff --git a/drivers/irqchip/irq-smp8759.c b/drivers/irqchip/irq-smp8759.c new file mode 100644 index 000000000000..a1680a250598 --- /dev/null +++ b/drivers/irqchip/irq-smp8759.c @@ -0,0 +1,204 @@ +#include <linux/irqchip.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/syscore_ops.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> + +/* + * This controller maps IRQ_MAX input lines to SPI_MAX output lines. + * The output lines are routed to GIC SPI 0 to 23. + * This driver muxes LEVEL_HIGH IRQs onto output line 0, + * and gives every EDGE_RISING IRQ a dedicated output line. + */ +#define IRQ_MAX 128 +#define SPI_MAX 24 +#define LEVEL_SPI 0 +#define IRQ_ENABLE BIT(31) +#define STATUS 0x420 + +struct tango_intc { + void __iomem *base; + struct irq_domain *dom; + u8 spi_to_tango_irq[SPI_MAX]; + u8 tango_irq_to_spi[IRQ_MAX]; + DECLARE_BITMAP(enabled_level, IRQ_MAX); + DECLARE_BITMAP(enabled_edge, IRQ_MAX); + spinlock_t lock; +}; + +static struct tango_intc *tango_intc; + +static void tango_level_isr(struct irq_desc *desc) +{ + uint pos, virq; + struct irq_chip *chip = irq_desc_get_chip(desc); + struct tango_intc *intc = irq_desc_get_handler_data(desc); + DECLARE_BITMAP(status, IRQ_MAX); + + chained_irq_enter(chip, desc); + + spin_lock(&intc->lock); + memcpy_fromio(status, intc->base + STATUS, IRQ_MAX / BITS_PER_BYTE); + bitmap_and(status, status, intc->enabled_level, IRQ_MAX); + spin_unlock(&intc->lock); + + for_each_set_bit(pos, status, IRQ_MAX) { + virq = irq_find_mapping(intc->dom, pos); + generic_handle_irq(virq); + } + + chained_irq_exit(chip, desc); +} + +static void tango_edge_isr(struct irq_desc *desc) +{ + uint virq; + struct irq_data *data = irq_desc_get_irq_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + struct tango_intc *intc = irq_desc_get_handler_data(desc); + int tango_irq = intc->spi_to_tango_irq[data->hwirq - 32]; + + chained_irq_enter(chip, desc); + virq = irq_find_mapping(intc->dom, tango_irq); + generic_handle_irq(virq); + chained_irq_exit(chip, desc); +} + +static void tango_mask(struct irq_data *data) +{ + unsigned long flags; + struct tango_intc *intc = data->chip_data; + u32 spi = intc->tango_irq_to_spi[data->hwirq]; + + spin_lock_irqsave(&intc->lock, flags); + writel_relaxed(0, intc->base + data->hwirq * 4); + if (spi == LEVEL_SPI) + __clear_bit(data->hwirq, intc->enabled_level); + else + __clear_bit(data->hwirq, intc->enabled_edge); + spin_unlock_irqrestore(&intc->lock, flags); +} + +static void tango_unmask(struct irq_data *data) +{ + unsigned long flags; + struct tango_intc *intc = data->chip_data; + u32 spi = intc->tango_irq_to_spi[data->hwirq]; + + spin_lock_irqsave(&intc->lock, flags); + writel_relaxed(IRQ_ENABLE | spi, intc->base + data->hwirq * 4); + if (spi == LEVEL_SPI) + __set_bit(data->hwirq, intc->enabled_level); + else + __set_bit(data->hwirq, intc->enabled_edge); + spin_unlock_irqrestore(&intc->lock, flags); +} + +static int tango_set_type(struct irq_data *data, uint flow_type) +{ + return 0; +} + +static struct irq_chip tango_chip = { + .name = "tango", + .irq_mask = tango_mask, + .irq_unmask = tango_unmask, + .irq_set_type = tango_set_type, +}; + +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; + } + + irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &tango_chip, intc); + irq_set_handler(virq, handle_simple_irq); + + return 0; +} + +static struct irq_domain_ops dom_ops = { + .xlate = irq_domain_xlate_twocell, + .alloc = tango_alloc, +}; + +static void tango_resume(void) +{ + int hwirq; + DECLARE_BITMAP(enabled, IRQ_MAX); + struct tango_intc *intc = tango_intc; + + bitmap_or(enabled, intc->enabled_level, intc->enabled_edge, IRQ_MAX); + for (hwirq = 0; hwirq < IRQ_MAX; ++hwirq) { + u32 val = intc->tango_irq_to_spi[hwirq]; + if (test_bit(hwirq, enabled)) + val |= IRQ_ENABLE; + writel_relaxed(val, intc->base + hwirq * 4); + } +} + +static struct syscore_ops tango_syscore_ops = { + .resume = tango_resume, +}; + +static int __init map_irq(struct device_node *gic, int spi, int trigger) +{ + struct of_phandle_args irq_data = { gic, 3, { 0, spi, trigger }}; + return irq_create_of_mapping(&irq_data); +} + +static int __init tango_irq_init(struct device_node *node, struct device_node *parent) +{ + int spi, virq; + struct tango_intc *intc; + + intc = kzalloc(sizeof(*intc), GFP_KERNEL); + if (!intc) + panic("%s: Failed to kalloc\n", node->name); + + virq = map_irq(parent, LEVEL_SPI, IRQ_TYPE_LEVEL_HIGH); + if (!virq) + panic("%s: Failed to map IRQ %d\n", node->name, LEVEL_SPI); + + irq_set_chained_handler_and_data(virq, tango_level_isr, intc); + + for (spi = 1; spi < SPI_MAX; ++spi) { + virq = map_irq(parent, spi, IRQ_TYPE_EDGE_RISING); + if (!virq) + panic("%s: Failed to map IRQ %d\n", node->name, spi); + + irq_set_chained_handler_and_data(virq, tango_edge_isr, intc); + } + + tango_intc = intc; + register_syscore_ops(&tango_syscore_ops); + + spin_lock_init(&intc->lock); + intc->base = of_iomap(node, 0); + intc->dom = irq_domain_add_linear(node, IRQ_MAX, &dom_ops, intc); + if (!intc->base || !intc->dom) + panic("%s: Failed to setup IRQ controller\n", node->name); + + return 0; +} +IRQCHIP_DECLARE(tango_intc, "sigma,smp8759-intc", tango_irq_init);