Re: [PATCH v3 6/9] irqchip: Add the ingenic-tcu-intc driver

[Marc Zyngier]

On Thu, 11 Jan 2018 17:25:45 +0100
Paul Cercueil <paul@xxxxxxxxxxxxxxx> 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.