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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

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