On Thu, 11 Jan 2018 17:25:45 +0100
Paul Cercueil <p...@crapouillou.net> wrote:
> Hi Marc,
>
> >> +static int __init ingenic_tcu_intc_of_init(struct device_node
> >> *node,
> >> + struct device_node *parent)
> >> +{
> >> + struct irq_domain *domain;
> >> + struct irq_chip_generic *gc;
> >> + struct irq_chip_type *ct;
> >> + int err, i, num_parent_irqs;
> >> + unsigned int parent_irqs[3];
> >
> > 3 parent interrupts? Really? How do you pick one? Also, given the
> > useage
> > model below, "int" is the wrong type. Probably should be u32.
>
> See below.
>
> >> + struct regmap *map;
> >> +
> >> + num_parent_irqs = of_property_count_elems_of_size(
> >> + node, "interrupts", 4);
> >
> > Nit: on a single line, as here is nothing that hurts my eyes more than
> > reading something like(
> > this). Also, 4 is better expressed as sizeof(u32).
>
> That will make checkpatch.pl unhappy :(
And I don't care about checkpatch. I maintain the irqchip stuff, while
checkpatch doesn't. Hence, I win.
>
> >> + if (num_parent_irqs < 0 || num_parent_irqs >
> >> ARRAY_SIZE(parent_irqs))
> >> + return -EINVAL;
> >> +
> >> + map = syscon_node_to_regmap(node->parent);
> >> + if (IS_ERR(map))
> >> + return PTR_ERR(map);
> >> +
> >> + domain = irq_domain_add_linear(node, 32, &irq_generic_chip_ops,
> >> NULL);
> >> + if (!domain)
> >> + return -ENOMEM;
> >> +
> >> + err = irq_alloc_domain_generic_chips(domain, 32, 1, "TCU",
> >> + handle_level_irq, 0, IRQ_NOPROBE | IRQ_LEVEL, 0);
> >> + if (err)
> >> + goto out_domain_remove;
> >> +
> >> + gc = irq_get_domain_generic_chip(domain, 0);
> >> + ct = gc->chip_types;
> >> +
> >> + gc->wake_enabled = IRQ_MSK(32);
> >> + gc->private = map;
> >> +
> >> + ct->regs.disable = TCU_REG_TMSR;
> >> + ct->regs.enable = TCU_REG_TMCR;
> >> + ct->regs.ack = TCU_REG_TFCR;
> >> + ct->chip.irq_unmask = ingenic_tcu_gc_unmask_enable_reg;
> >> + ct->chip.irq_mask = ingenic_tcu_gc_mask_disable_reg;
> >> + ct->chip.irq_mask_ack = ingenic_tcu_gc_mask_disable_reg_and_ack;
> >> + ct->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
> >> +
> >> + /* Mask all IRQs by default */
> >> + regmap_write(map, TCU_REG_TMSR, IRQ_MSK(32));
> >> +
> >> + for (i = 0; i < num_parent_irqs; i++) {
> >> + parent_irqs[i] = irq_of_parse_and_map(node, i);
> >> + if (!parent_irqs[i]) {
> >> + err = -EINVAL;
> >> + goto out_unmap_irqs;
> >> + }
> >> +
> >> + irq_set_chained_handler_and_data(parent_irqs[i],
> >> + ingenic_tcu_intc_cascade, domain);
> >> + }
> >
> > I don't get the multiple parent irq at all. How are the source
> > interrupt routed to the various cascades?
>
> On JZ4740, channels 0 and 1 have their own interrupt, and channels
> 2-7 share one interrupt line.
> On JZ4770 and JZ4780, the OST (OS timer) channel has its own
> interrupt, channel 5 has its own interrupt, and channels 0-4 and 6-7
> share one interrupt line.
What a mess. I suppose you get a shared status register to find out
which channel has fired?
> To keep things simple, we register the same interrupt handler for the
> three parent interrupts.
Please add a comment to that effect, documenting the above HW blunder.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
