Re: Potential issue with (or misunderstanding of) of_irq_get()

[Conor Dooley]

Hey Marc,

On Mon, May 22, 2023 at 12:56:30PM +0100, Conor Dooley wrote:
> On Sun, May 21, 2023 at 01:38:11PM +0100, Marc Zyngier wrote:
> > Yup. Now that you have disassociated yourself from the firmware-based
> > naming, you cannot use it to drive the mapping and sh*t happens. The
> > thing is, named fwnode are only there as a band-aid to be able to
> > designate objects that have no fwnode representation.
> > 
> > And it goes downhill from there. My gut felling for this is that you
> > should try and build something that looks like this:
> > 
> > - the mux exposes a single hierarchical domain that is directly
> >   connected to the PLIC.
> > 
> > - the first 40 interrupt allocations are serviced by simply allocating
> >   a corresponding PLIC interrupt and configuring the mux to do its
> >   job.

> > - all the 28 other interrupts must be muxed onto a single PLIC. For
> >   these interrupts, you must make sure that the domain hierarchy gets
> >   truncated at the MUX level (see irq_domain_disconnect_hierarchy()
> >   for the gory details). They all get to be placed behind a chained
> >   interrupt handler, with their own irqchip ops.
> 
> Yeah, so this I cannot do per se since there is one of these mux
> interrupts per GPIO controller. But it doesn't sound too difficult to
> extend that idea to 3 different interrupts. Famous last words.
> 
> > That way, no repainting of fwnodes, no select/match complexity, and
> > must of the interrupts get to benefit from the hierarchical setup
> > (such as being able to set their affinity).
> > 
> > Of course, all of this is assuming that the HW is able to deal with a
> > large number of interrupts muxed to a single one. If not, you may have
> > to use more that one of these, but the idea is the same.
> > 
> > Thoughts?
> 
> Well, I'll have to go poking at this hierarchy truncation function that
> you have pointed out & see how it works - but the idea all sounds doable
> and less cumbersome than what I have right now.
> 
> Thanks for the pointers, I'll resurface with either a patchset or having
> designed myself into another corner.

Sounds like it may be the latter... The hierarchical stuff for the
direct interrupts is working well, so progress at least. I seem to
have gotten stuck with the non-direct/muxxed interrupts though.

My alloc() now looks like, for the direct interrupts:
static int mpfs_irq_mux_alloc(struct irq_domain *d, unsigned int virq,
				     unsigned int nr_irqs, void *arg)
{
	struct mpfs_irq_mux *priv = d->host_data;
	struct irq_fwspec *fwspec = arg;
	struct irq_fwspec parent_fwspec;
	int ret;

	pr_info("attempting to allocate\n");
	ret = mpfs_irq_mux_is_direct_get_mapping(priv, fwspec);
	if (ret == -EINVAL) {
		irq_domain_disconnect_hierarchy(d, virq);
	}

	parent_fwspec.fwnode = d->parent->fwnode;
	parent_fwspec.param[0] = priv->parent_irqs[ret];
	parent_fwspec.param_count = 1;

	ret = irq_domain_alloc_irqs_parent(d, virq, 1, &parent_fwspec);
	if (ret)
		return ret;

	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0],
				      &mpfs_irq_mux_irq_chip, priv);

	return 0;
}

and I am disconnecting the hierarchy as (I think) you suggested. (I'm
not entirely sure whether you suggested that I use it, or employ the
same principle.)

Where I am confused, after quite a lot of trial & error, is how to
actually configure the non-direct interrupts so that they are are using
another irqchip & interrupt handler. Since
irq_domain_disconnect_hierarchy()'s doc comment says that it should be
called in the alloc() callback, what functions I can use there are quite
limited by the lock that has been taken, and I've not been able to
figure out how to get it working. And anyway, fiddling with other
domains inside alloc() feels completely wrong to me.

Would you mind pointing out at which point of the driver or interrupt
registration process you think that the placing of the muxxed interrupts
"behind a chained interrupt handler, with their own irqchip ops" should
be done?

Thanks again,
Conor.

Attachment: signature.asc
Description: PGP signature