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

Gupta, Nipun
2023-10-09T04:53:48Z

On 10/7/2023 2:21 PM, Greg KH wrote:
> 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?

Agree, CDX bus can work without MSI. GENERIC_MSI_IRQ can be selected by
a controller if it is relying on MSI. Will update the code accordingly.

>
>>>> +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?

sure.. will update this in next spin.

Thanks,
Nipun

>
> thanks,
>
> greg k-h