Re: [PATCH v5 24/27] irqchip/gic-v5: Add GICv5 ITS support

Lorenzo Pieralisi
2025-06-23T09:26:48Z

On Fri, Jun 20, 2025 at 09:18:32PM +0200, Thomas Gleixner wrote:

[...]

> > + * Taken from msi_lib_irq_domain_select(). The only difference is that
> > + * we have to match the fwspec->fwnode parent against the domain->fwnode
> > + * in that in GICv5 the ITS domain is represented by the ITS fwnode but
> > + * the MSI controller (ie the ITS frames) are ITS child nodes.
> > + */
> > +static int gicv5_its_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> > + enum irq_domain_bus_token bus_token)
> > +{
> > + const struct msi_parent_ops *ops = d->msi_parent_ops;
> > + u32 busmask = BIT(bus_token);
> > +
> > + if (!ops)
> > + return 0;
> > +
> > + if (fwnode_get_parent(fwspec->fwnode) != d->fwnode ||
> > + fwspec->param_count != 0)
> > + return 0;
>
> Just add a MSI flag and set it in parent_ops::required_flags and extend

I added that but it does not work (not if we use d->flags as below), it works
if I add it as an

IRQ_DOMAIN_FLAG_*

and set it in irq_domain_info in the msi_create_parent_irq_domain()
call in the GICv5 ITS driver when creating the domain.

> the lib with
>
> struct fwnode_handle *fwh;
>
> fwh = d->flags & MAGIC ? fwnode_get_parent(fwspec->fwnode) : fwspec->fwnode;

Here we are using the domain flags and I think that's what we want.

If I go with parent_ops flag, I believe here we need to use the parent
msi_domain_info::flags - I don't think that's what we want.

It is a property of the IRQ domain so I think that adding an

IRQ_DOMAIN_FLAG_FWNODE_PARENT

is the best option.

Please let me know.

Thanks,
Lorenzo

>
> No?
>
> > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> > index c2e7ba7e38f7..4a0990f46358 100644
> > --- a/drivers/irqchip/irq-gic-v5.c
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -57,12 +57,12 @@ static void release_lpi(u32 lpi)
> > ida_free(&lpi_ida, lpi);
> > }
> >
> > -static int gicv5_alloc_lpi(void)
> > +int gicv5_alloc_lpi(void)
> > {
> > return alloc_lpi();
> > }
> >
> > -static void gicv5_free_lpi(u32 lpi)
> > +void gicv5_free_lpi(u32 lpi)
> > {
> > release_lpi(lpi);
> > }
>
> Just make them global right away when you implement them. No point for
> this kind of churn.
>
> Thanks,
>
> tglx