Re: [RFC PATCH 0/2] add support for CDX bus MSI domain

From: Jason Gunthorpe <jgg@nvidia.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: Nipun Gupta <nipun.gupta@amd.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
	rafael@kernel.org, maz@kernel.org, tglx@linutronix.de,
	okaya@kernel.org, harpreet.anand@amd.com, michal.simek@amd.com,
	nikhil.agarwal@amd.com
Subject: Re: [RFC PATCH 0/2] add support for CDX bus MSI domain
Date: Wed, 17 Aug 2022 13:00:28 -0300	[thread overview]
Message-ID: <Yv0QnAMWxL7p5W2g@nvidia.com> (raw)
In-Reply-To: <846c663a-5999-977b-8a69-89c8ca522cea@arm.com>

On Wed, Aug 03, 2022 at 03:16:11PM +0100, Robin Murphy wrote:
> On 2022-08-03 13:26, Nipun Gupta wrote:
> > Devices in FPGA can be added/modified dynamically on run-time.
> > These devices are exposed on system bus to embedded CPUs.
> > 
> > CDX is an upcoming bus, that caters to the requirement for
> > dynamically discovered FPGA devices. These devices are added
> > as platform devices where fwnode is created using 'software
> > nodes' in Linux framework.
> > 
> > This RFC targets to solves 2 issues when adding devices
> > dynamically using platform_device_register API.
> > 
> > 1. It creates a MSI domain for CDX bus devices, which can
> >     discover device ID used by GIC ITS without depending
> >     on of_node.
> > 2. Since these devices are not present in device tree, it
> >     creates a sysfs entry to expose the compatible string.
> 
> Isn't this pretty much what CONFIG_OF_DYNAMIC is for? From the look of these
> patches this thing is still completely tied to devicetree, so why reinvent
> that wheel?

+1

Since the v2 was posted, I strongly agree with this.

The idea that "FW" should somehow provide the FPGA DT components seems
completely backwards. The DT components of a FPGA should come along
with the bitfile that is loaded into the FPGA, and not be part of FW
at all - unless FW is loading the FPGA, then FW can install it in the
main DT and we don't need this.

OF_DYNAMIC or some other version of it like this CDX, should be used
to patch in an entire DT for the FPGA loaded from the filesystem.

This is important because of a lot of useful use cases on FPGAs are
things like I2C, SPI, PCI and flash controllers that rely on DT sub
nodes to be functional.

And this explains why this is creating a platform device - because if
you have a DT node and want to instantiate an OF driver for it, you
pretty create a platform device since platform device has been turned
into the "universal device for OF nodes"

This CDX thing isn't even a bus!

Look at what cdx_bus_device_discovery() does, look at its YAML. There
is no halfway sane bus here like PCI, it is all driven by a DT. The
code to actually get the DT is all "FIXME'D" out, but that seems to be
the design.

The only thing the CDX does is provide some fairly hacky support for
mapping the stream IDs and devices IDs in the FPGA fabric so IOMMU and
MSI can work because the DT fragment it is loading can't be properly
connected back to the main DT due to how it was loaded.

Jason

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