| Message ID | 20220817150542.483291-3-nipun.gupta@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for CDX bus controller | expand |
On Wed, Aug 17, 2022 at 08:35:38PM +0530, Nipun Gupta wrote: > CDX bus driver manages the scanning and populating FPGA > based devices present on the CDX bus. > > The bus driver sets up the basic infrastructure and fetches > the device related information from the firmware. These > devices are registered as platform devices. Ick, why? These aren't platform devices, they are CDX devices. Make them real devices here, don't abuse the platform device interface for things that are not actually on the platform bus. > CDX bus is capable of scanning devices dynamically, > supporting rescanning of dynamically added, removed or > updated devices. Wonderful, that's a real bus, so be a real bus please. thanks, greg k-h
On Wed, 17 Aug 2022 16:32:45 +0100, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Aug 17, 2022 at 08:35:38PM +0530, Nipun Gupta wrote: > > CDX bus driver manages the scanning and populating FPGA > > based devices present on the CDX bus. > > > > The bus driver sets up the basic infrastructure and fetches > > the device related information from the firmware. These > > devices are registered as platform devices. > > Ick, why? These aren't platform devices, they are CDX devices. Make > them real devices here, don't abuse the platform device interface for > things that are not actually on the platform bus. > > > CDX bus is capable of scanning devices dynamically, > > supporting rescanning of dynamically added, removed or > > updated devices. > > Wonderful, that's a real bus, so be a real bus please. +1. This should follow something like PCI, which has semi-sane semantics. Thanks, M.
[AMD Official Use Only - General] > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Wednesday, August 17, 2022 9:03 PM > To: Gupta, Nipun <Nipun.Gupta@amd.com> > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; maz@kernel.org; > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; saravanak@google.com; > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > jgg@ziepe.ca; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > <harpreet.anand@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>; > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > [CAUTION: External Email] > > On Wed, Aug 17, 2022 at 08:35:38PM +0530, Nipun Gupta wrote: > > CDX bus driver manages the scanning and populating FPGA > > based devices present on the CDX bus. > > > > The bus driver sets up the basic infrastructure and fetches > > the device related information from the firmware. These > > devices are registered as platform devices. > > Ick, why? These aren't platform devices, they are CDX devices. Make > them real devices here, don't abuse the platform device interface for > things that are not actually on the platform bus. CDX is a virtual bus (FW based) which discovers FPGA based platform devices based on communication with FW. These devices are essentially platform devices as these are memory mapped on system bus, but having a property that they are dynamically discovered via FW and are rescannable. I think your point is correct in the sense that CDX bus is not an actual bus, but a FW based mechanism to discover FPGA based platform devices. Can you kindly suggest us if we should have the CDX platform device scanning code as a CDX bus in "drivers/bus/" folder OR have it in "drivers/fpga/" or "drivers/platform/" or which other suitable location? Thanks, Nipun > > > CDX bus is capable of scanning devices dynamically, > > supporting rescanning of dynamically added, removed or > > updated devices. > > Wonderful, that's a real bus, so be a real bus please. > > thanks, > > greg k-h
On Mon, Aug 22, 2022 at 01:21:47PM +0000, Gupta, Nipun wrote: > [AMD Official Use Only - General] > > > > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Wednesday, August 17, 2022 9:03 PM > > To: Gupta, Nipun <Nipun.Gupta@amd.com> > > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; maz@kernel.org; > > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; saravanak@google.com; > > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > > jgg@ziepe.ca; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > > <harpreet.anand@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>; > > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com> > > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > > > [CAUTION: External Email] > > > > On Wed, Aug 17, 2022 at 08:35:38PM +0530, Nipun Gupta wrote: > > > CDX bus driver manages the scanning and populating FPGA > > > based devices present on the CDX bus. > > > > > > The bus driver sets up the basic infrastructure and fetches > > > the device related information from the firmware. These > > > devices are registered as platform devices. > > > > Ick, why? These aren't platform devices, they are CDX devices. Make > > them real devices here, don't abuse the platform device interface for > > things that are not actually on the platform bus. > > CDX is a virtual bus (FW based) which discovers FPGA based platform > devices based on communication with FW. virtual busses are fine to have as a real bus in the kernel, no problem there. > These devices are essentially platform devices as these are memory mapped > on system bus, but having a property that they are dynamically discovered > via FW and are rescannable. If they are dynamically discoverable and rescannable, then great, it's a bus in the kernel and NOT a platform device. > I think your point is correct in the sense that CDX bus is not an actual bus, > but a FW based mechanism to discover FPGA based platform devices. > > Can you kindly suggest us if we should have the CDX platform device scanning > code as a CDX bus in "drivers/bus/" folder OR have it in "drivers/fpga/" or > "drivers/platform/" or which other suitable location? drivers/cdx/ ? thanks, greg k-h
[AMD Official Use Only - General] > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, August 22, 2022 7:00 PM > To: Gupta, Nipun <Nipun.Gupta@amd.com> > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > rafael@kernel.org; eric.auger@redhat.com; alex.williamson@redhat.com; > cohuck@redhat.com; Gupta, Puneet (DCG-ENG) > <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; > mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; > jeffrey.l.hugo@gmail.com; saravanak@google.com; > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > jgg@ziepe.ca; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > <harpreet.anand@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>; > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; > jgg@nvidia.com; Robin Murphy <robin.murphy@arm.com> > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > [CAUTION: External Email] > > On Mon, Aug 22, 2022 at 01:21:47PM +0000, Gupta, Nipun wrote: > > [AMD Official Use Only - General] > > > > > > > > > -----Original Message----- > > > From: Greg KH <gregkh@linuxfoundation.org> > > > Sent: Wednesday, August 17, 2022 9:03 PM > > > To: Gupta, Nipun <Nipun.Gupta@amd.com> > > > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > rafael@kernel.org; > > > eric.auger@redhat.com; alex.williamson@redhat.com; > cohuck@redhat.com; > > > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > > > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; > maz@kernel.org; > > > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; saravanak@google.com; > > > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > > > jgg@ziepe.ca; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; > > > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > > > <harpreet.anand@amd.com>; Agarwal, Nikhil > <nikhil.agarwal@amd.com>; > > > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) > <git@amd.com> > > > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > > > > > [CAUTION: External Email] > > > > > > On Wed, Aug 17, 2022 at 08:35:38PM +0530, Nipun Gupta wrote: > > > > CDX bus driver manages the scanning and populating FPGA > > > > based devices present on the CDX bus. > > > > > > > > The bus driver sets up the basic infrastructure and fetches > > > > the device related information from the firmware. These > > > > devices are registered as platform devices. > > > > > > Ick, why? These aren't platform devices, they are CDX devices. Make > > > them real devices here, don't abuse the platform device interface for > > > things that are not actually on the platform bus. > > > > CDX is a virtual bus (FW based) which discovers FPGA based platform > > devices based on communication with FW. > > virtual busses are fine to have as a real bus in the kernel, no problem > there. > > > These devices are essentially platform devices as these are memory > mapped > > on system bus, but having a property that they are dynamically discovered > > via FW and are rescannable. > > If they are dynamically discoverable and rescannable, then great, it's a > bus in the kernel and NOT a platform device. > > > I think your point is correct in the sense that CDX bus is not an actual bus, > > but a FW based mechanism to discover FPGA based platform devices. > > > > Can you kindly suggest us if we should have the CDX platform device > scanning > > code as a CDX bus in "drivers/bus/" folder OR have it in "drivers/fpga/" or > > "drivers/platform/" or which other suitable location? > > drivers/cdx/ ? I agree that the approach, which is correct should be used, just wanted to reconfirm as adding a new bus would lead to change in other areas like SMMU, MSI and VFIO too and we will need vfio-cdx interface for CDX bus, similar to vfio-platform. On another mail Robin and Jason have suggested to use OF_DYNAMIC. Can you please also let us know in case that is a suited option where we use OF_DYNAMIC and have our code as part of "drivers/fpga" instead of using the bus. (something like pseries CPU hotplug is using to add new CPU platform devices on runtime: https://elixir.bootlin.com/linux/v5.19.3/source/arch/powerpc/platforms/pseries/hotplug-cpu.c#L534). We can share the RFC in case you are interested in looking at code flow using the of_dynamic approach. The reason we were inclined towards the platform bus is due to existing SMMU. MSI and VFIO support available for platform, though would work on the bus if adding to the bus is correct thing to move ahead. Robin/Jason, Your comments are also kindly welcomed regarding the suitable approach. Thanks, Nipun > > thanks, > > greg k-h
On Wed, Aug 24, 2022 at 08:50:19AM +0000, Gupta, Nipun wrote: > [AMD Official Use Only - General] > > > > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Monday, August 22, 2022 7:00 PM > > To: Gupta, Nipun <Nipun.Gupta@amd.com> > > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > rafael@kernel.org; eric.auger@redhat.com; alex.williamson@redhat.com; > > cohuck@redhat.com; Gupta, Puneet (DCG-ENG) > > <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; > > mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; > > jeffrey.l.hugo@gmail.com; saravanak@google.com; > > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > > jgg@ziepe.ca; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > > <harpreet.anand@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>; > > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com>; > > jgg@nvidia.com; Robin Murphy <robin.murphy@arm.com> > > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > > > [CAUTION: External Email] > > > > On Mon, Aug 22, 2022 at 01:21:47PM +0000, Gupta, Nipun wrote: > > > [AMD Official Use Only - General] > > > > > > > > > > > > > -----Original Message----- > > > > From: Greg KH <gregkh@linuxfoundation.org> > > > > Sent: Wednesday, August 17, 2022 9:03 PM > > > > To: Gupta, Nipun <Nipun.Gupta@amd.com> > > > > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > > rafael@kernel.org; > > > > eric.auger@redhat.com; alex.williamson@redhat.com; > > cohuck@redhat.com; > > > > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > > > > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; > > maz@kernel.org; > > > > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; saravanak@google.com; > > > > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > > > > jgg@ziepe.ca; linux-kernel@vger.kernel.org; > > devicetree@vger.kernel.org; > > > > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > > > > <harpreet.anand@amd.com>; Agarwal, Nikhil > > <nikhil.agarwal@amd.com>; > > > > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) > > <git@amd.com> > > > > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > > > > > > > [CAUTION: External Email] > > > > > > > > On Wed, Aug 17, 2022 at 08:35:38PM +0530, Nipun Gupta wrote: > > > > > CDX bus driver manages the scanning and populating FPGA > > > > > based devices present on the CDX bus. > > > > > > > > > > The bus driver sets up the basic infrastructure and fetches > > > > > the device related information from the firmware. These > > > > > devices are registered as platform devices. > > > > > > > > Ick, why? These aren't platform devices, they are CDX devices. Make > > > > them real devices here, don't abuse the platform device interface for > > > > things that are not actually on the platform bus. > > > > > > CDX is a virtual bus (FW based) which discovers FPGA based platform > > > devices based on communication with FW. > > > > virtual busses are fine to have as a real bus in the kernel, no problem > > there. > > > > > These devices are essentially platform devices as these are memory > > mapped > > > on system bus, but having a property that they are dynamically discovered > > > via FW and are rescannable. > > > > If they are dynamically discoverable and rescannable, then great, it's a > > bus in the kernel and NOT a platform device. > > > > > I think your point is correct in the sense that CDX bus is not an actual bus, > > > but a FW based mechanism to discover FPGA based platform devices. > > > > > > Can you kindly suggest us if we should have the CDX platform device > > scanning > > > code as a CDX bus in "drivers/bus/" folder OR have it in "drivers/fpga/" or > > > "drivers/platform/" or which other suitable location? > > > > drivers/cdx/ ? > > I agree that the approach, which is correct should be used, just wanted > to reconfirm as adding a new bus would lead to change in other areas > like SMMU, MSI and VFIO too and we will need vfio-cdx interface for CDX > bus, similar to vfio-platform. > > On another mail Robin and Jason have suggested to use OF_DYNAMIC. > Can you please also let us know in case that is a suited option where we > use OF_DYNAMIC and have our code as part of "drivers/fpga" instead of > using the bus. (something like pseries CPU hotplug is using to add new > CPU platform devices on runtime: > https://elixir.bootlin.com/linux/v5.19.3/source/arch/powerpc/platforms/pseries/hotplug-cpu.c#L534). > We can share the RFC in case you are interested in looking at code flow > using the of_dynamic approach. Please no more abuse of the platform device. If your device can be discovered by scanning a bus, it is not a platform device. greg k-h
On Wed, Aug 24, 2022 at 02:11:48PM +0200, Greg KH wrote: > > We can share the RFC in case you are interested in looking at code flow > > using the of_dynamic approach. > > Please no more abuse of the platform device. Last time this came up there was some disagreement from the ARM folks, they were not keen on having xx_drivers added all over the place to support the same OF/DT devices just discovered in a different way. It is why ACPI is mapped to platform_device even in some cases. I think if you push them down this path they will get resistance to get the needed additional xx_drivers into the needed places. > If your device can be discovered by scanning a bus, it is not a platform > device. A DT fragment loaded during boot binds a driver using a platform_driver, why should a DT fragment loaded post-boot bind using an XX_driver and further why should the CDX way of getting the DT raise to such importantance that it gets its own cdx_driver ? In the end the driver does not care about how the DT was loaded. None of these things are on a discoverable bus in any sense like PCI or otherwise. They are devices described by a DT fragement and they take all their parameters from that chunk of DT. How the DT was loaded into the system is not a useful distinction that raises the level of needing an entire new set of xx_driver structs all over the tree, IMHO. Jason
On Wed, Aug 24, 2022 at 4:31 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Aug 24, 2022 at 02:11:48PM +0200, Greg KH wrote: > > > We can share the RFC in case you are interested in looking at code flow > > > using the of_dynamic approach. > > > > Please no more abuse of the platform device. > > Last time this came up there was some disagreement from the ARM folks, > they were not keen on having xx_drivers added all over the place to > support the same OF/DT devices just discovered in a different way. It is > why ACPI is mapped to platform_device even in some cases. > > I think if you push them down this path they will get resistance to > get the needed additional xx_drivers into the needed places. > > > If your device can be discovered by scanning a bus, it is not a platform > > device. > > A DT fragment loaded during boot binds a driver using a > platform_driver, why should a DT fragment loaded post-boot bind using > an XX_driver and further why should the CDX way of getting the DT > raise to such importantance that it gets its own cdx_driver ? > > In the end the driver does not care about how the DT was loaded. > None of these things are on a discoverable bus in any sense like PCI > or otherwise. They are devices described by a DT fragement and they > take all their parameters from that chunk of DT. > > How the DT was loaded into the system is not a useful distinction that > raises the level of needing an entire new set of xx_driver structs all > over the tree, IMHO. Jason, I see your point or rather the point the ARM folks might have made. But in this case, why not use DT overlays to add these devices? IIRC there's an in kernel API to add DT overlays. If so, should this be more of a FPGA driver that reads FPGA stuff and adds DT overlays? That'd at least make a stronger case for why this isn't a separate bus. -Saravana
On 2022-08-25 19:38, Saravana Kannan wrote: > On Wed, Aug 24, 2022 at 4:31 PM Jason Gunthorpe <jgg@nvidia.com> wrote: >> >> On Wed, Aug 24, 2022 at 02:11:48PM +0200, Greg KH wrote: >>>> We can share the RFC in case you are interested in looking at code flow >>>> using the of_dynamic approach. >>> >>> Please no more abuse of the platform device. >> >> Last time this came up there was some disagreement from the ARM folks, >> they were not keen on having xx_drivers added all over the place to >> support the same OF/DT devices just discovered in a different way. It is >> why ACPI is mapped to platform_device even in some cases. >> >> I think if you push them down this path they will get resistance to >> get the needed additional xx_drivers into the needed places. >> >>> If your device can be discovered by scanning a bus, it is not a platform >>> device. >> >> A DT fragment loaded during boot binds a driver using a >> platform_driver, why should a DT fragment loaded post-boot bind using >> an XX_driver and further why should the CDX way of getting the DT >> raise to such importantance that it gets its own cdx_driver ? >> >> In the end the driver does not care about how the DT was loaded. >> None of these things are on a discoverable bus in any sense like PCI >> or otherwise. They are devices described by a DT fragement and they >> take all their parameters from that chunk of DT. >> >> How the DT was loaded into the system is not a useful distinction that >> raises the level of needing an entire new set of xx_driver structs all >> over the tree, IMHO. > > Jason, I see your point or rather the point the ARM folks might have > made. But in this case, why not use DT overlays to add these devices? > IIRC there's an in kernel API to add DT overlays. If so, should this > be more of a FPGA driver that reads FPGA stuff and adds DT overlays? > That'd at least make a stronger case for why this isn't a separate > bus. Right, that's exactly where this discussion started. To my mind, it would definitely help to understand if this is a *real* discoverable bus in hardware, i.e. does one have to configure one's device with some sort of CDX wrapper at FPGA synthesis time, that then physically communicates with some sort of CDX controller to identify itself once loaded; or is it "discoverable" in the sense that there's some firmware on an MCU controlling what gets loaded into the FPGA, and software can query that and get back whatever precompiled DTB fragment came bundled with the bitstream, i.e. it's really more like fpga-mgr in a fancy hat? It's pretty much impossible to judge from all the empty placeholder code here how much is real and constrained by hardware and how much is firmware abstraction, which makes it particularly hard to review whether any proposal heading in the right direction. Even if it *is* entirely firmware smoke-and-mirrors, if that firmware can provide a standardised discovery and configuration interface for common resources, it can be a bus. But then it should *be* a bus, with its own bus_type and its own device type to model those standard interfaces and IDs and resources. Or if it is really just a very clever dynamic DT overlay manager for platform devices, then it can be that instead. But what it should clearly not be is some in-between mess making the worst of both worlds, which is what the code here inescapably smells of. Thanks, Robin.
On Thu, Aug 25, 2022 at 08:57:49PM +0100, Robin Murphy wrote: > To my mind, it would definitely help to understand if this is a *real* > discoverable bus in hardware, i.e. does one have to configure one's device > with some sort of CDX wrapper at FPGA synthesis time, that then physically > communicates with some sort of CDX controller to identify itself once > loaded; or is it "discoverable" in the sense that there's some firmware on > an MCU controlling what gets loaded into the FPGA, and software can query > that and get back whatever precompiled DTB fragment came bundled with the > bitstream, i.e. it's really more like fpga-mgr in a fancy hat? So much of the IP that you might want to put in a FPGA needs DT, I don't thing a simplistic AMBA like discoverable thing would be that interesting. Think about things like FPGA GPIOs being configured as SPI/I2C, then describing the board config of SPI/I2C busses, setting up PCI bridges, flash storage controllers and all sorts of other typically embedded stuff that really relies on DT these days. It would be nice if Xilinx could explain more about what environment this is targetting. Is it Zynq-like stuff? Jason
[AMD Official Use Only - General] > -----Original Message----- > From: Robin Murphy <robin.murphy@arm.com> > Sent: Friday, August 26, 2022 1:28 AM > To: Saravana Kannan <saravanak@google.com>; Jason Gunthorpe > <jgg@nvidia.com> > Cc: Greg KH <gregkh@linuxfoundation.org>; Gupta, Nipun > <Nipun.Gupta@amd.com>; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; eric.auger@redhat.com; > alex.williamson@redhat.com; cohuck@redhat.com; Gupta, Puneet (DCG-ENG) > <puneet.gupta@amd.com>; song.bao.hua@hisilicon.com; > mchehab+huawei@kernel.org; maz@kernel.org; f.fainelli@gmail.com; > jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; mani@kernel.org; > yishaih@nvidia.com; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; > kvm@vger.kernel.org; okaya@kernel.org; Anand, Harpreet > <harpreet.anand@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>; > Simek, Michal <michal.simek@amd.com>; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > [CAUTION: External Email] > > On 2022-08-25 19:38, Saravana Kannan wrote: > > On Wed, Aug 24, 2022 at 4:31 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > >> > >> On Wed, Aug 24, 2022 at 02:11:48PM +0200, Greg KH wrote: > >>>> We can share the RFC in case you are interested in looking at code flow > >>>> using the of_dynamic approach. > >>> > >>> Please no more abuse of the platform device. > >> > >> Last time this came up there was some disagreement from the ARM folks, > >> they were not keen on having xx_drivers added all over the place to > >> support the same OF/DT devices just discovered in a different way. It is > >> why ACPI is mapped to platform_device even in some cases. > >> > >> I think if you push them down this path they will get resistance to > >> get the needed additional xx_drivers into the needed places. > >> > >>> If your device can be discovered by scanning a bus, it is not a platform > >>> device. > >> > >> A DT fragment loaded during boot binds a driver using a > >> platform_driver, why should a DT fragment loaded post-boot bind using > >> an XX_driver and further why should the CDX way of getting the DT > >> raise to such importantance that it gets its own cdx_driver ? > >> > >> In the end the driver does not care about how the DT was loaded. > >> None of these things are on a discoverable bus in any sense like PCI > >> or otherwise. They are devices described by a DT fragement and they > >> take all their parameters from that chunk of DT. > >> > >> How the DT was loaded into the system is not a useful distinction that > >> raises the level of needing an entire new set of xx_driver structs all > >> over the tree, IMHO. > > > > Jason, I see your point or rather the point the ARM folks might have > > made. But in this case, why not use DT overlays to add these devices? > > IIRC there's an in kernel API to add DT overlays. If so, should this > > be more of a FPGA driver that reads FPGA stuff and adds DT overlays? > > That'd at least make a stronger case for why this isn't a separate > > bus. > > Right, that's exactly where this discussion started. > > To my mind, it would definitely help to understand if this is a *real* > discoverable bus in hardware, i.e. does one have to configure one's > device with some sort of CDX wrapper at FPGA synthesis time, that then > physically communicates with some sort of CDX controller to identify > itself once loaded; or is it "discoverable" in the sense that there's > some firmware on an MCU controlling what gets loaded into the FPGA, and > software can query that and get back whatever precompiled DTB fragment > came bundled with the bitstream, i.e. it's really more like fpga-mgr in > a fancy hat? Devices are created in FPFGA with a CDX wrapper, and CDX controller(firmware) reads that CDX wrapper to find out new devices. Host driver then interacts with firmware to find newly discovered devices. This bus aligns with PCI infrastructure. It happens to be an embedded interface as opposed to off-chip connection. We are trying to do an RFC which proposes CDX as a new bus. It seems to be a cleaner interface than what was added in RFC v2. > > It's pretty much impossible to judge from all the empty placeholder code > here how much is real and constrained by hardware and how much is > firmware abstraction, which makes it particularly hard to review whether > any proposal heading in the right direction. You can consider the placeholders for now as API calls which would eventually communicate with FW, and fetch required info like number of FPGA devices, device related parameters (vendor_id, device_id etc), and command the firmware to reset the device. In next rev, we would add new API stubs instead of empty placeholders (as FW interaction code is under development), which could give more clear view. > > Even if it *is* entirely firmware smoke-and-mirrors, if that firmware > can provide a standardised discovery and configuration interface for > common resources, it can be a bus. But then it should *be* a bus, with > its own bus_type and its own device type to model those standard > interfaces and IDs and resources. Or if it is really just a very clever > dynamic DT overlay manager for platform devices, then it can be that > instead. But what it should clearly not be is some in-between mess > making the worst of both worlds, which is what the code here inescapably > smells of. > > Thanks, > Robin.
[AMD Official Use Only - General] > -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, August 26, 2022 5:38 AM > To: Robin Murphy <robin.murphy@arm.com> > Cc: Saravana Kannan <saravanak@google.com>; Greg KH > <gregkh@linuxfoundation.org>; Gupta, Nipun <Nipun.Gupta@amd.com>; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; maz@kernel.org; > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; > mani@kernel.org; yishaih@nvidia.com; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, > Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil > <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; git > (AMD-Xilinx) <git@amd.com> > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > [CAUTION: External Email] > > On Thu, Aug 25, 2022 at 08:57:49PM +0100, Robin Murphy wrote: > > > To my mind, it would definitely help to understand if this is a *real* > > discoverable bus in hardware, i.e. does one have to configure one's device > > with some sort of CDX wrapper at FPGA synthesis time, that then physically > > communicates with some sort of CDX controller to identify itself once > > loaded; or is it "discoverable" in the sense that there's some firmware on > > an MCU controlling what gets loaded into the FPGA, and software can query > > that and get back whatever precompiled DTB fragment came bundled with the > > bitstream, i.e. it's really more like fpga-mgr in a fancy hat? > > So much of the IP that you might want to put in a FPGA needs DT, I > don't thing a simplistic AMBA like discoverable thing would be that > interesting. > > Think about things like FPGA GPIOs being configured as SPI/I2C, then > describing the board config of SPI/I2C busses, setting up PCI bridges, > flash storage controllers and all sorts of other typically embedded > stuff that really relies on DT these days. > > It would be nice if Xilinx could explain more about what environment > this is targetting. Is it Zynq-like stuff? This solution is not targeted for GPIO/SPI or I2C like devices which rely on DT, but would have PCI like network/storage devices. It is not targeted for Zynq platform. I have added more details on other mail. Please refer to that mail. Thanks, Nipun > > Jason
On Mon, Aug 29, 2022 at 04:49:02AM +0000, Gupta, Nipun wrote: > Devices are created in FPFGA with a CDX wrapper, and CDX controller(firmware) > reads that CDX wrapper to find out new devices. Host driver then interacts with > firmware to find newly discovered devices. This bus aligns with PCI infrastructure. > It happens to be an embedded interface as opposed to off-chip connection. Why do you need an FW in all of this? And why do you need DT at all? It is still not clear Jason
[AMD Official Use Only - General] > -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Monday, August 29, 2022 9:02 PM > To: Gupta, Nipun <Nipun.Gupta@amd.com> > Cc: Robin Murphy <robin.murphy@arm.com>; Saravana Kannan > <saravanak@google.com>; Greg KH <gregkh@linuxfoundation.org>; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; > maz@kernel.org; f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; kvm@vger.kernel.org; > okaya@kernel.org; Anand, Harpreet <harpreet.anand@amd.com>; Agarwal, > Nikhil <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; > git (AMD-Xilinx) <git@amd.com> > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > [CAUTION: External Email] > > On Mon, Aug 29, 2022 at 04:49:02AM +0000, Gupta, Nipun wrote: > > > Devices are created in FPFGA with a CDX wrapper, and CDX > controller(firmware) > > reads that CDX wrapper to find out new devices. Host driver then interacts > with > > firmware to find newly discovered devices. This bus aligns with PCI > infrastructure. > > It happens to be an embedded interface as opposed to off-chip > connection. > > Why do you need an FW in all of this? > > And why do you need DT at all? We need DT to describe the CDX controller only, similar to how PCI controller is described in DT. PCI devices are never enumerated in DT. All children are to be dynamically discovered. Children devices do not require DT as they will be discovered by the bus driver. Like PCI controller talks to PCI device over PCI spec defined channel, we need CDX controller to talk to CDX device over a custom defined (FW managed) channel. > > It is still not clear > > Jason
On 2022-08-30 08:06, Gupta, Nipun wrote: > [AMD Official Use Only - General] > > > >> -----Original Message----- >> From: Jason Gunthorpe <jgg@nvidia.com> >> Sent: Monday, August 29, 2022 9:02 PM >> To: Gupta, Nipun <Nipun.Gupta@amd.com> >> Cc: Robin Murphy <robin.murphy@arm.com>; Saravana Kannan >> <saravanak@google.com>; Greg KH <gregkh@linuxfoundation.org>; >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; >> eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; >> Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; >> song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; >> maz@kernel.org; f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; >> Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; linux- >> kernel@vger.kernel.org; devicetree@vger.kernel.org; kvm@vger.kernel.org; >> okaya@kernel.org; Anand, Harpreet <harpreet.anand@amd.com>; Agarwal, >> Nikhil <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; >> git (AMD-Xilinx) <git@amd.com> >> Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver >> >> [CAUTION: External Email] >> >> On Mon, Aug 29, 2022 at 04:49:02AM +0000, Gupta, Nipun wrote: >> >>> Devices are created in FPFGA with a CDX wrapper, and CDX >> controller(firmware) >>> reads that CDX wrapper to find out new devices. Host driver then interacts >> with >>> firmware to find newly discovered devices. This bus aligns with PCI >> infrastructure. >>> It happens to be an embedded interface as opposed to off-chip >> connection. >> >> Why do you need an FW in all of this? >> >> And why do you need DT at all? > > We need DT to describe the CDX controller only, similar to > how PCI controller is described in DT. PCI devices are > never enumerated in DT. All children are to be dynamically > discovered. > > Children devices do not require DT as they will be discovered > by the bus driver. > > Like PCI controller talks to PCI device over PCI spec defined channel, > we need CDX controller to talk to CDX device over a custom > defined (FW managed) channel. OK, thanks for clarifying - it actually sounds quite cool :) I think it's clear now that this should be a a full-fledged bus implementation. Note that if the CDX interface provides a way to query arbitrary properties beyond standard resources then you may well also want your own fwnode type to hook into the device_property APIs too. Yes, it then means a bit more work adapting individual drivers too, but that should be far cleaner in the long run, and there's already plenty of precedent for IPs which exist with multiple standard interfaces for PCI/USB/SDIO/platform MMIO/etc. Plus it means that if CDX ever makes its way into PCIe-attached FPGA cards which can be used on non-OF systems, you've not painted yourself into a corner. Thanks, Robin.
On Tue, Aug 30, 2022 at 07:06:12AM +0000, Gupta, Nipun wrote: > [AMD Official Use Only - General] > > > > > -----Original Message----- > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Monday, August 29, 2022 9:02 PM > > To: Gupta, Nipun <Nipun.Gupta@amd.com> > > Cc: Robin Murphy <robin.murphy@arm.com>; Saravana Kannan > > <saravanak@google.com>; Greg KH <gregkh@linuxfoundation.org>; > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; > > maz@kernel.org; f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; > > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; linux- > > kernel@vger.kernel.org; devicetree@vger.kernel.org; kvm@vger.kernel.org; > > okaya@kernel.org; Anand, Harpreet <harpreet.anand@amd.com>; Agarwal, > > Nikhil <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; > > git (AMD-Xilinx) <git@amd.com> > > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > > > [CAUTION: External Email] > > > > On Mon, Aug 29, 2022 at 04:49:02AM +0000, Gupta, Nipun wrote: > > > > > Devices are created in FPFGA with a CDX wrapper, and CDX > > controller(firmware) > > > reads that CDX wrapper to find out new devices. Host driver then interacts > > with > > > firmware to find newly discovered devices. This bus aligns with PCI > > infrastructure. > > > It happens to be an embedded interface as opposed to off-chip > > connection. > > > > Why do you need an FW in all of this? > > > > And why do you need DT at all? > > We need DT to describe the CDX controller only, similar to > how PCI controller is described in DT. PCI devices are > never enumerated in DT. All children are to be dynamically > discovered. > > Children devices do not require DT as they will be discovered > by the bus driver. > > Like PCI controller talks to PCI device over PCI spec defined channel, > we need CDX controller to talk to CDX device over a custom > defined (FW managed) channel. It would be alot clearer to see a rfc cdx driver that doesn't have all the dt,fwnode,of stuff in it and works like PCI does, with a custom matcher and custom properies instead of trying to co-opt the DT things: Eg stuff like this make it look like you are building DT nodes: + struct property_entry port_props[] = { + PROPERTY_ENTRY_STRING("compatible", + dev_types[dev_params->dev_type_idx].compat), + { } + ret = of_map_id(np, req_id, "iommu-map", "iommu-map-mask", + NULL, &dev_params.stream_id); I still don't understand why FW would be involved, we usually don't involve FW for PCI.. Jason
[AMD Official Use Only - General] > -----Original Message----- > From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, August 30, 2022 6:31 PM > To: Gupta, Nipun <Nipun.Gupta@amd.com> > Cc: Robin Murphy <robin.murphy@arm.com>; Saravana Kannan > <saravanak@google.com>; Greg KH <gregkh@linuxfoundation.org>; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; maz@kernel.org; > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; Michael.Srba@seznam.cz; > mani@kernel.org; yishaih@nvidia.com; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; kvm@vger.kernel.org; okaya@kernel.org; Anand, > Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil > <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; git > (AMD-Xilinx) <git@amd.com> > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > [CAUTION: External Email] > > On Tue, Aug 30, 2022 at 07:06:12AM +0000, Gupta, Nipun wrote: > > [AMD Official Use Only - General] > > > > > > > > > -----Original Message----- > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Monday, August 29, 2022 9:02 PM > > > To: Gupta, Nipun <Nipun.Gupta@amd.com> > > > Cc: Robin Murphy <robin.murphy@arm.com>; Saravana Kannan > > > <saravanak@google.com>; Greg KH <gregkh@linuxfoundation.org>; > > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > > > eric.auger@redhat.com; alex.williamson@redhat.com; > cohuck@redhat.com; > > > Gupta, Puneet (DCG-ENG) <puneet.gupta@amd.com>; > > > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; > > > maz@kernel.org; f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; > > > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; linux- > > > kernel@vger.kernel.org; devicetree@vger.kernel.org; kvm@vger.kernel.org; > > > okaya@kernel.org; Anand, Harpreet <harpreet.anand@amd.com>; Agarwal, > > > Nikhil <nikhil.agarwal@amd.com>; Simek, Michal > <michal.simek@amd.com>; > > > git (AMD-Xilinx) <git@amd.com> > > > Subject: Re: [RFC PATCH v2 2/6] bus/cdx: add the cdx bus driver > > > > > > [CAUTION: External Email] > > > > > > On Mon, Aug 29, 2022 at 04:49:02AM +0000, Gupta, Nipun wrote: > > > > > > > Devices are created in FPFGA with a CDX wrapper, and CDX > > > controller(firmware) > > > > reads that CDX wrapper to find out new devices. Host driver then interacts > > > with > > > > firmware to find newly discovered devices. This bus aligns with PCI > > > infrastructure. > > > > It happens to be an embedded interface as opposed to off-chip > > > connection. > > > > > > Why do you need an FW in all of this? > > > > > > And why do you need DT at all? > > > > We need DT to describe the CDX controller only, similar to > > how PCI controller is described in DT. PCI devices are > > never enumerated in DT. All children are to be dynamically > > discovered. > > > > Children devices do not require DT as they will be discovered > > by the bus driver. > > > > Like PCI controller talks to PCI device over PCI spec defined channel, > > we need CDX controller to talk to CDX device over a custom > > defined (FW managed) channel. > > It would be alot clearer to see a rfc cdx driver that doesn't have all > the dt,fwnode,of stuff in it and works like PCI does, with a custom > matcher and custom properies instead of trying to co-opt the DT things: > > Eg stuff like this make it look like you are building DT nodes: > > + struct property_entry port_props[] = { > + PROPERTY_ENTRY_STRING("compatible", > + dev_types[dev_params->dev_type_idx].compat), > + { } > > + ret = of_map_id(np, req_id, "iommu-map", "iommu-map-mask", > + NULL, &dev_params.stream_id); This would be removed with the CDX bus model. It is currently here because we were deliberately trying to use platform bus. We will be sending out v3 with CDX bus next week. Thanks, Nipun > > I still don't understand why FW would be involved, we usually don't > involve FW for PCI.. > > Jason
diff --git a/MAINTAINERS b/MAINTAINERS index 32c5be3d6a53..b0eea32dbb39 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22301,6 +22301,7 @@ M: Nipun Gupta <nipun.gupta@amd.com> M: Nikhil Agarwal <nikhil.agarwal@amd.com> S: Maintained F: Documentation/devicetree/bindings/bus/xlnx,cdx.yaml +F: drivers/bus/cdx/* XILINX GPIO DRIVER M: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig index 7bfe998f3514..b0324efb9a6a 100644 --- a/drivers/bus/Kconfig +++ b/drivers/bus/Kconfig @@ -251,5 +251,6 @@ config DA8XX_MSTPRI source "drivers/bus/fsl-mc/Kconfig" source "drivers/bus/mhi/Kconfig" +source "drivers/bus/cdx/Kconfig" endmenu diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile index d90eed189a65..88649111c395 100644 --- a/drivers/bus/Makefile +++ b/drivers/bus/Makefile @@ -20,6 +20,9 @@ obj-$(CONFIG_INTEL_IXP4XX_EB) += intel-ixp4xx-eb.o obj-$(CONFIG_MIPS_CDMM) += mips_cdmm.o obj-$(CONFIG_MVEBU_MBUS) += mvebu-mbus.o +#CDX bus +obj-$(CONFIG_CDX_BUS) += cdx/ + # Interconnect bus driver for OMAP SoCs. obj-$(CONFIG_OMAP_INTERCONNECT) += omap_l3_smx.o omap_l3_noc.o diff --git a/drivers/bus/cdx/Kconfig b/drivers/bus/cdx/Kconfig new file mode 100644 index 000000000000..ae3f2ee5a768 --- /dev/null +++ b/drivers/bus/cdx/Kconfig @@ -0,0 +1,7 @@ +config CDX_BUS + bool "CDX Bus platform driver" + help + Driver to enable CDX Bus infrastructure. CDX bus is + capable of scanning devices dynamically, supporting + rescanning of dynamically added, removed or updated + devices. diff --git a/drivers/bus/cdx/Makefile b/drivers/bus/cdx/Makefile new file mode 100644 index 000000000000..c9cee5b6fa8a --- /dev/null +++ b/drivers/bus/cdx/Makefile @@ -0,0 +1,3 @@ +obj-$(CONFIG_CDX_BUS) += cdx-bus-device-driver.o + +cdx-bus-device-driver-objs := cdx.o diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c new file mode 100644 index 000000000000..f28329770af8 --- /dev/null +++ b/drivers/bus/cdx/cdx.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Platform driver for CDX bus. + * + * Copyright(C) 2022 Xilinx Inc. + */ + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/of.h> +#include <linux/property.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/dma-mapping.h> +#include <linux/property.h> +#include <linux/cdx/cdx_bus.h> + +#include "cdx.h" + +static struct cdx_device_types_t dev_types[MAX_CDX_DEVICE_TYPES] = { + {"cdx-cdma-1.0", "xlnx,cdx-cdma-1.0"} +}; + +static int cdx_populate_one(struct platform_device *pdev_parent, + struct cdx_dev_params_t *dev_params) +{ + struct platform_device *new_pdev; + struct fwnode_handle *swnode; + struct platform_device_info pdevinfo; + struct cdx_device_data dev_data; + int ret = 0; + struct property_entry port_props[] = { + PROPERTY_ENTRY_STRING("compatible", + dev_types[dev_params->dev_type_idx].compat), + { } + }; + + swnode = fwnode_create_software_node(port_props, NULL); + if (IS_ERR(swnode)) { + ret = PTR_ERR(swnode); + dev_err(&pdev_parent->dev, + "fwnode_create_software_node() failed: %d\n", ret); + goto out; + } + + dev_data.bus_id = dev_params->bus_id; + dev_data.func_id = dev_params->func_id; + + memset(&pdevinfo, 0, sizeof(pdevinfo)); + pdevinfo.fwnode = swnode; + pdevinfo.parent = &pdev_parent->dev; + pdevinfo.name = dev_params->name; + pdevinfo.id = (dev_params->bus_id << 16) | (dev_params->func_id); + pdevinfo.res = dev_params->res; + pdevinfo.num_res = dev_params->res_cnt; + pdevinfo.data = &dev_data; + pdevinfo.size_data = sizeof(struct cdx_device_data); + pdevinfo.dma_mask = DMA_BIT_MASK(64); + new_pdev = platform_device_register_full(&pdevinfo); + if (IS_ERR(new_pdev)) { + ret = PTR_ERR(new_pdev); + dev_err(&pdev_parent->dev, + "platform_device_register_full() failed: %d\n", ret); + goto out; + } + + /* Configure the IOMMU */ + ret = of_dma_configure_id(&new_pdev->dev, pdev_parent->dev.of_node, + 1, &dev_params->stream_id); + if (ret) { + if (ret != -EPROBE_DEFER) + dev_err(&pdev_parent->dev, + "of_dma_configure_id() failed: %d\n", ret); + goto out; + } + + return 0; + +out: + if (new_pdev != NULL && !IS_ERR(new_pdev)) + platform_device_unregister(new_pdev); + + if (swnode != NULL && IS_ERR(swnode)) + fwnode_remove_software_node(swnode); + + return ret; +} + +static int cdx_unregister_device(struct device *dev, + void * __always_unused data) +{ + struct platform_device *pdev = to_platform_device(dev); + + platform_device_unregister(pdev); + fwnode_remove_software_node(pdev->dev.fwnode); + + return 0; +} + +void cdx_unregister_devices(struct device *parent_dev) +{ + device_for_each_child(parent_dev, NULL, cdx_unregister_device); +} + +static int cdx_bus_device_discovery(struct platform_device *pdev) +{ + int num_cdx_bus = 0, num_cdx_func = 0; + int bus_id = 0, func_id = 0; + struct device_node *np = pdev->dev.of_node; + int ret; + + /* TODO: Get number of busses from firmware */ + num_cdx_bus = 1; + + for (bus_id = 0; bus_id < num_cdx_bus; bus_id++) { + /* TODO: Get number of functions/devices on the bus + * from firmware + */ + num_cdx_func = 1; + + for (func_id = 0; func_id < num_cdx_func; func_id++) { + struct cdx_dev_params_t dev_params; + u64 mmio_size; /* MMIO size */ + u64 mmio_addr; /* MMIO address */ + u32 req_id; /* requester ID */ + + /* TODO: Read device configuration from the firmware + * and remove the hardcoded configuration parameters. + */ + mmio_addr = 0xe4020000; + mmio_size = 0x1000; + req_id = 0x250; + + memset(&dev_params, 0, sizeof(dev_params)); + + /* Populate device parameters */ + ret = of_map_id(np, req_id, "iommu-map", "iommu-map-mask", + NULL, &dev_params.stream_id); + if (ret != 0) { + dev_err(&pdev->dev, + "of_map_id failed for IOMMU: %d\n", + ret); + goto fail; + } + + dev_params.dev_type_idx = 0; + dev_params.res_cnt = 1; + + /* Populate dev_type_idx */ + dev_params.dev_type_idx = 0; + + /* Populate resource */ + dev_params.res->start = (u64)mmio_addr; + dev_params.res->end = (u64)(mmio_addr + mmio_size - 1); + dev_params.res->flags = IORESOURCE_MEM; + + dev_params.bus_id = bus_id; + dev_params.func_id = func_id; + + strncpy(dev_params.name, dev_types[dev_params.dev_type_idx].name, + sizeof(dev_params.name)); + + ret = cdx_populate_one(pdev, &dev_params); + if (ret == -EPROBE_DEFER) { + goto fail; + } else if (ret) { + dev_err(&pdev->dev, + "registering cdx dev: %d failed: %d\n", + func_id, ret); + goto fail; + } else { + dev_info(&pdev->dev, + "CDX dev: %d on cdx bus: %d created\n", + func_id, bus_id); + } + } + } + + return 0; +fail: + cdx_unregister_devices(&pdev->dev); + return ret; +} + +static int cdx_probe(struct platform_device *pdev) +{ + int ret; + + /* TODO: Firmware path initialization */ + + ret = cdx_bus_device_discovery(pdev); + if (ret) + return ret; + + return 0; +} + +static void cdx_shutdown(struct platform_device *pdev) +{ + /* TODO: add shutdown for CDX bus*/ +} + +static int cdx_remove(struct platform_device *pdev) +{ + /* TODO: add remove of CDX bus */ + return 0; +} + +static const struct of_device_id cdx_match_table[] = { + {.compatible = "xlnx,cdxbus-controller-1.0",}, + { }, +}; + +MODULE_DEVICE_TABLE(of, cdx_match_table); + +static struct platform_driver cdx_driver = { + .driver = { + .name = "cdx-bus", + .pm = NULL, + .of_match_table = cdx_match_table, + }, + .probe = cdx_probe, + .remove = cdx_remove, + .shutdown = cdx_shutdown, +}; + +static int __init cdx_driver_init(void) +{ + int error; + + error = platform_driver_register(&cdx_driver); + if (error < 0) { + pr_err("platform_driver_register() failed: %d\n", error); + return error; + } + + return 0; +} +postcore_initcall(cdx_driver_init); diff --git a/drivers/bus/cdx/cdx.h b/drivers/bus/cdx/cdx.h new file mode 100644 index 000000000000..7db8b06de9cd --- /dev/null +++ b/drivers/bus/cdx/cdx.h @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Header file for the CDX Bus + * + * Copyright(c) 2022 Xilinx Inc. + */ + +#ifndef _CDX_H_ +#define _CDX_H_ + +#define CDX_DEV_NUM_RESOURCES 4 +#define CDX_NAME_LEN 64 + +struct cdx_dev_params_t { + char name[CDX_NAME_LEN]; + u32 bus_id; + u32 func_id; + u32 dev_type_idx; + struct resource res[CDX_DEV_NUM_RESOURCES]; + int res_cnt; + u32 stream_id; +}; + +/** + * struct cdx_device_data_t - private data associated with the + * CDX device. + * @bus_id: Bus ID for reset + * @func_id: Function ID for reset + */ +struct cdx_device_data { + u32 bus_id; + u32 func_id; +}; + +#endif /* _CDX_H_ */ diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h new file mode 100644 index 000000000000..7c6ad7dfe97a --- /dev/null +++ b/include/linux/cdx/cdx_bus.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * CDX bus public interface + * + * Copyright(C) 2022 Xilinx Inc. + * + */ +#ifndef _CDX_BUS_H_ +#define _CDX_BUS_H_ + +#define MAX_CDX_DEVICE_TYPES 16 +#define MAX_CDX_COMPAT_LEN 64 +#define MAX_CDX_NAME_LEN 64 + +/** + * struct cdx_device_type_t - info on CDX devices type. + * @compatible: Describes the specific binding, to which + * the devices of a particular type complies. It is used + * for driver binding. + */ +struct cdx_device_types_t { + char name[MAX_CDX_NAME_LEN]; + char compat[MAX_CDX_COMPAT_LEN]; +}; + +#endif /* _CDX_H_ */
CDX bus driver manages the scanning and populating FPGA based devices present on the CDX bus. The bus driver sets up the basic infrastructure and fetches the device related information from the firmware. These devices are registered as platform devices. CDX bus is capable of scanning devices dynamically, supporting rescanning of dynamically added, removed or updated devices. Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> --- Please NOTE: This is a RFC change which does not yet support the CDX bus firmware interface as it is under development, and this series aims to get an early feedback from the community. There are TODO items mentioned in this patch which needs to be updated once firmware support is complete. MAINTAINERS | 1 + drivers/bus/Kconfig | 1 + drivers/bus/Makefile | 3 + drivers/bus/cdx/Kconfig | 7 ++ drivers/bus/cdx/Makefile | 3 + drivers/bus/cdx/cdx.c | 241 ++++++++++++++++++++++++++++++++++++ drivers/bus/cdx/cdx.h | 35 ++++++ include/linux/cdx/cdx_bus.h | 26 ++++ 8 files changed, 317 insertions(+) create mode 100644 drivers/bus/cdx/Kconfig create mode 100644 drivers/bus/cdx/Makefile create mode 100644 drivers/bus/cdx/cdx.c create mode 100644 drivers/bus/cdx/cdx.h create mode 100644 include/linux/cdx/cdx_bus.h