Re: [RFC PATCH 1/2] irqchip: cdx-bus: add cdx-MSI domain with gic-its domain as parent

From: Marc Zyngier <maz@kernel.org>
To: "Gupta, Nipun" <Nipun.Gupta@amd.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"rafael@kernel.org" <rafael@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"okaya@kernel.org" <okaya@kernel.org>,
	"Anand, Harpreet" <harpreet.anand@amd.com>,
	"Simek,\ Michal" <michal.simek@amd.com>,
	"Agarwal, Nikhil" <nikhil.agarwal@amd.com>
Subject: Re: [RFC PATCH 1/2] irqchip: cdx-bus: add cdx-MSI domain with gic-its domain as parent
Date: Thu, 04 Aug 2022 11:38:41 +0100	[thread overview]
Message-ID: <87mtckwaxa.wl-maz@kernel.org> (raw)
In-Reply-To: <DM6PR12MB3082B8F57D02A359FC61F449E89F9@DM6PR12MB3082.namprd12.prod.outlook.com>

On Thu, 04 Aug 2022 10:18:19 +0100,
"Gupta, Nipun" <Nipun.Gupta@amd.com> wrote:

> > > +static const struct of_device_id cdx_device_id[] = {
> > > +     {.compatible = "xlnx,cdx-controller-1.0", },
> > 
> > What is this? If this is supposed to represent am ITS, it really
> > should say so.
> 
> This is a CDX bus controller, not an ITS. This will be added as a part of
> device-tree documentation when we add formal CDX bus patches.
> CDX is an upcoming AMD bus, supporting dynamically discovered
> FPGA devices.

Right. So that's yet another reason to not have this glue code at all.

> 
> > 
> > > +     {},
> > > +};
> > > +
> > > +struct irq_domain *get_parent(struct fwnode_handle *handle)
> > > +{
> > > +     return irq_find_matching_fwnode(handle, DOMAIN_BUS_NEXUS);
> > > +}
> > > +
> > > +static void __init its_cdx_msi_init_one(struct device_node *np,
> > > +                                 const char *name)
> > > +{
> > > +     struct irq_domain *parent;
> > > +     struct irq_domain *cdx_msi_domain;
> > > +     struct fwnode_handle *fwnode_handle;
> > > +     struct device_node *parent_node;
> > > +
> > > +     parent_node = of_parse_phandle(np, "msi-parent", 0);
> > > +
> > > +     parent = get_parent(of_node_to_fwnode(parent_node));
> > > +     if (!parent || !msi_get_domain_info(parent)) {
> > > +             pr_err("%s: unable to locate ITS domain\n", name);
> > > +             return;
> > > +     }
> > > +
> > > +     fwnode_handle = of_node_to_fwnode(np);
> > > +     cdx_msi_domain = platform_msi_create_irq_domain(fwnode_handle,
> > > +                                             &its_cdx_msi_domain_info,
> > > +                                             parent);
> > > +     if (!cdx_msi_domain) {
> > > +             pr_err("%s: unable to create cdx bus domain\n", name);
> > > +             return;
> > > +     }
> > > +
> > > +     pr_info("cdx bus MSI: %s domain created\n", name);
> > > +}
> > > +
> > > +static void __init its_cdx_of_msi_init(void)
> > > +{
> > > +     struct device_node *np;
> > > +
> > > +     for (np = of_find_matching_node(NULL, cdx_device_id); np;
> > > +          np = of_find_matching_node(np, cdx_device_id)) {
> > > +             if (!of_device_is_available(np))
> > > +                     continue;
> > > +
> > > +             its_cdx_msi_init_one(np, np->full_name);
> > > +     }
> > > +}
> > > +
> > > +static int __init its_cdx_msi_init(void)
> > > +{
> > > +     its_cdx_of_msi_init();
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +early_initcall(its_cdx_msi_init);
> > 
> > I really don't think we should have any more of this muck. Yes, the
> > other busses are also doing that, but here's our chance to do
> > something better.
> > 
> > Why can't the *bus* driver (I assume there is one) perform these
> > tasks? It would really help if this patch was shown in context,
> > because I have no idea how this fits in the grand scheme of things.
> 
> Agree, bus driver could do this stuff, and I was considering it too,
> but as this was the norm so added these changes in this IRQ chip :).

Well, let's not miss this opportunity to improve things! :-)

> Bus driver is not out yet, and we have pushed these as RFC changes
> for early feedback (mentioned in the cover letter), but yes, these
> patches would be clubbed as a part of overall changes.
> 
> And we can move it as a part of bus driver itself. I hope that suits?

It certainly sounds better than the above looks! Please Cc me on the
submission of this infrastructure.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel