Messages in this thread | | | Date | Sat, 7 Oct 2023 10:51:07 +0200 | From | Greg KH <> | Subject | Re: [PATCH v4] cdx: add MSI support for CDX bus |
| |
On Sat, Oct 07, 2023 at 02:13:15PM +0530, Gupta, Nipun wrote: > > > On 10/5/2023 3:54 PM, Greg KH wrote: > > On Mon, Sep 11, 2023 at 07:22:59PM +0530, Nipun Gupta wrote: > > > Add CDX-MSI domain per CDX controller with gic-its domain as > > > a parent, to support MSI for CDX devices. CDX devices allocate > > > MSIs from the CDX domain. Also, introduce APIs to alloc and free > > > IRQs for CDX domain. > > > > > > In CDX subsystem firmware is a controller for all devices and > > > their configuration. CDX bus controller sends all the write_msi_msg > > > commands to firmware running on RPU and the firmware interfaces with > > > actual devices to pass this information to devices > > > > > > Since, CDX controller is the only way to communicate with the Firmware > > > for MSI write info, CDX domain per controller required in contrast to > > > having a CDX domain per device. > > > > > > Co-developed-by: Nikhil Agarwal <nikhil.agarwal@amd.com> > > > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@amd.com> > > > Co-developed-by: Abhijit Gangurde <abhijit.gangurde@amd.com> > > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@amd.com> > > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com> > > > Tested-by: Nikhil Agarwal <nikhil.agarwal@amd.com> > > > --- > > > > > > Changes v3->v4: > > > - Rebased on Linux 6.6-rc1 > > > > > > Changes v2->v3: > > > - Rebased on Linux 6.5-rc1 > > > - Used FW provided 'msi_dev_id' as device ID for GIC instead of 'req_id'. > > > > > > Changes v1->v2: > > > - fixed scenario where msi write was called asyncronously in > > > an atomic context, by using irq_chip_(un)lock, and using sync > > > MCDI API for write MSI message. > > > - fixed broken Signed-off-by chain. > > > > > > drivers/cdx/Kconfig | 1 + > > > drivers/cdx/Makefile | 2 +- > > > drivers/cdx/cdx.c | 9 ++ > > > drivers/cdx/cdx.h | 12 ++ > > > drivers/cdx/cdx_msi.c | 183 ++++++++++++++++++++++++ > > > drivers/cdx/controller/cdx_controller.c | 23 +++ > > > drivers/cdx/controller/mc_cdx_pcol.h | 64 +++++++++ > > > drivers/cdx/controller/mcdi_functions.c | 26 +++- > > > drivers/cdx/controller/mcdi_functions.h | 20 +++ > > > include/linux/cdx/cdx_bus.h | 32 +++++ > > > kernel/irq/msi.c | 1 + > > > 11 files changed, 370 insertions(+), 3 deletions(-) > > > create mode 100644 drivers/cdx/cdx_msi.c > > > > > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig > > > index a08958485e31..86df7ccb76bb 100644 > > > --- a/drivers/cdx/Kconfig > > > +++ b/drivers/cdx/Kconfig > > > @@ -8,6 +8,7 @@ > > > config CDX_BUS > > > bool "CDX Bus driver" > > > depends on OF && ARM64 > > > + select GENERIC_MSI_IRQ_DOMAIN > > > > This config option isn't in my tree anywhere, where did it come from? > > What is it supposed to do? > > > > > help > > > Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus > > > exposes Fabric devices which uses composable DMA IP to the > > > diff --git a/drivers/cdx/Makefile b/drivers/cdx/Makefile > > > index 0324e4914f6e..4bad79d1d188 100644 > > > --- a/drivers/cdx/Makefile > > > +++ b/drivers/cdx/Makefile > > > @@ -5,4 +5,4 @@ > > > # Copyright (C) 2022-2023, Advanced Micro Devices, Inc. > > > # > > > -obj-$(CONFIG_CDX_BUS) += cdx.o controller/ > > > +obj-$(CONFIG_CDX_BUS) += cdx.o cdx_msi.o controller/ > > > > So you are always building this in even if the build doesn't support > > MSI? Why will that not break the build? > > CDX bus will select GENERIC_MSI_IRQ, so I think we can have this only with > CONFIG_CDX_BUS?
As CDX works today without MSI, why are you adding this requirement to the codebase forcing everyone to have it?
> > > +struct cdx_msi_config { > > > + u16 msi_index; > > > + u32 data; > > > + u64 addr; > > > +}; > > > > Are you ok with the "hole" in this structure? > > This is only a software placeholder for information to be passed to hardware > in a different message format (using MCDI).
Great, then how about reording things so there isn't a hole?
thanks,
greg k-h
| |