Re: [PATCH v4] cdx: add MSI support for CDX bus

[Greg KH]

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@xxxxxxx>
> > > Signed-off-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx>
> > > Co-developed-by: Abhijit Gangurde <abhijit.gangurde@xxxxxxx>
> > > Signed-off-by: Abhijit Gangurde <abhijit.gangurde@xxxxxxx>
> > > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > > Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@xxxxxxx>
> > > Tested-by: Nikhil Agarwal <nikhil.agarwal@xxxxxxx>
> > > ---
> > > 
> > > 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