Message-ID: <20230522-niece-vanish-a1ee36494ef0@wendy>
Date:   Mon, 22 May 2023 12:56:30 +0100
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Marc Zyngier <maz@...nel.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <conor@...nel.org>, <daire.mcnamara@...rochip.com>
Subject: Re: Potential issue with (or misunderstanding of) of_irq_get()

Hey Marc,

Thanks for taking a look.

On Sun, May 21, 2023 at 01:38:11PM +0100, Marc Zyngier wrote:
> On Fri, 19 May 2023 12:02:47 +0100, Conor Dooley <conor.dooley@...rochip.com> wrote:
> > I've run into an issue with of_irq_get() while writing an irqchip driver
> > and I was hoping that by posting about it I might get some guidance as
> > to whether I just doing something fundamentally wrong in my code, or
> > if the specific case was just an oversight.
> > 
> > I've been trying to solve the issue that I pointed out here:
> > https://lore.kernel.org/linux-gpio/23a69be6-96d3-1c28-f1aa-555e38ff991e@microchip.com/
> > 
> > To spare reading that, the TL;DR is that the SoC has 3 GPIO controllers,
> > with 14, 24 and 32 GPIOs each. All 68 can be used for interrupts.
> > The PLIC only has 41 interrupts for GPIOs, so there's a bit of extra RTL
> > sitting between the GPIO controllers and the PLIC, that is runtime
> > configurable, deciding whether an GPIO gets a PLIC interrupt of its
> > own or shares an interrupt with other GPIOs from the same GPIO controller.
> > 
> > Since the interrupt router/mux is not part of the GPIO controller blocks,
> > I have written a driver for the it & changed the representation in the DT
> > to the below. For each of the 41 interrupts "consumed" by the driver
> > bound to the irqmux node, I have created a domain.
> 
> In general, this feels a wee bit wrong.
> 
> From what I understand of the HW, it is expected that most of the GPIO
> interrupt will be directly associated with a PLIC interrupt in an 1:1
> fashion (only 68 - 41 + 1 = 28 interrupts will be muxed). So 40 GPIOs
> could have a chance of being directly assigned to a PLIC input without
> any muxing.

Not quite.
Firstly, I made a silly maths mistake, and 14 + 24 + 32 is not 68.
There are 70 GPIOs on this SoC.

Probably more important though is that there are 3 mux interrupts
though, not only 1, so the "40 GPIOs" becomes 38. It's also the source
of some of the complexity in what I wrote, since each of the GPIO
controller's has a different interrupt that the non direct connections
go to.

There's a bit that "scared" me further on down, that I think came out of
this assumption.

> If you start allocating a domain per interrupt, you end-up actively
> preventing the use of hierarchical domains, and you don't really
> benefit from what the mux HW can do for you.

Honestly, I'm happy with being able to use more than 32 of the GPIOs for
interrupts. Setting affinity etc is a nice bonus on top of working in
the first place.

> > In my irqchip's select callback I check the struct irq_fwspec's param[0]
> > to determine which domain is actually responsible for it.
> 
> Huh. In general, if you want to resort to 'select', you're doing
> something that is a bit iffy.

Aye, given the lack of other select callbacks, I figured I was going
down a potentially dodgy path.

> > That's all working nicely & I was doing some cleanup before submitting,
> > when I noticed that debugfs complained about the fact that I had several
> > domains hanging off the same of device_node:
> > debugfs: File ':soc:interrupt-controller@...02054' in directory 'domains' already present!
> > debugfs: File ':soc:interrupt-controller@...02054' in directory 'domains' already present!
> 
> Of course. You get 41 domains with all the same node...

Aye, it is obvious in hindsight ;)

> > To get around that, I tried to switch to creating fwnodes instead,
> > one for each domain:
> > 
> > 	for (; i < MPFS_MUX_NUM_IRQS; i++) {
> > 		priv->irqchip_data[i].output_hwirq = i;
> > 
> > 		priv->irqchip_data[i].irq = irq_of_parse_and_map(node, i);
> > 
> > 		fwnode = irq_domain_alloc_named_id_fwnode("mpfs-irq-mux", i);
> > 
> > 		domain = irq_domain_create_linear(fwnode, MPFS_MAX_IRQS_PER_GPIO,
> > 						  &mpfs_irq_mux_nondirect_domain_ops,
> > 						  &priv->irqchip_data[i]);
> > 
> > 		irq_set_chained_handler_and_data(priv->irqchip_data[i].irq,
> > 						 mpfs_irq_mux_nondirect_handler,
> > 						 &priv->irqchip_data[i]);
> > 	}
> > 
> > That's grand for debugfs, but I then ran into a problem that made me feel
> > I had designed myself into an incorrect corner.
> 
> 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.

So you say "first" here and "configuring the mux to do its job" - this
scares me a little as the mux cannot be used to map arbitrary GPIOs to
arbitrary PLIC interrupts.
All it can do is pick which one of either GPIO2 or GPIO0/1 gets to use
the corresponding direct connection. For example, if bit 0 in the
control register is cleared, GPIO0's 0th GPIO gets the direct connection
& GPIO2's 0th GPIO gets muxed into the GPIO2 non-direct interrupt.
Ditto for bit 1, 2 etc. From bit 14 onwards, s/GPIO0/GPIO1/.

I'm not sure if your "first" here was temporal, IOW "for the first 40
interrupts you allocate, use a direct connection". If it was, then
that's not a possibility.

I'd also rather not edit the mux based on what Linux wants, since there
could well be some other program running in AMP on other cores that
would not appreciate it changing underneath its feet. I don't think that
makes things any more/less easy though, since temporal is already not
possible.

> - 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.

Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists