Re: [RFC v3 2/2] vfio/platform: msi: add Broadcom platform devices

Hi Eric,

On Tue, Jan 12, 2021 at 2:52 PM Auger Eric [off-list ref] wrote:

Hi Vikas,

On 12/14/20 6:45 PM, Vikas Gupta wrote:

quoted

Add msi support for Broadcom platform devices

Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
---
 drivers/vfio/platform/Kconfig                 |  1 +
 drivers/vfio/platform/Makefile                |  1 +
 drivers/vfio/platform/msi/Kconfig             |  9 ++++
 drivers/vfio/platform/msi/Makefile            |  2 +
 .../vfio/platform/msi/vfio_platform_bcmplt.c  | 49 +++++++++++++++++++
 5 files changed, 62 insertions(+)
 create mode 100644 drivers/vfio/platform/msi/Kconfig
 create mode 100644 drivers/vfio/platform/msi/Makefile
 create mode 100644 drivers/vfio/platform/msi/vfio_platform_bcmplt.c

what does plt mean?

This(plt) is a generic name for Broadcom platform devices, which we`ll
 plan to add in this file. Currently we have only one in this file.
Do you think this name does not sound good here?

quoted

diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
index dc1a3c44f2c6..7b8696febe61 100644
--- a/drivers/vfio/platform/Kconfig
+++ b/drivers/vfio/platform/Kconfig

@@ -21,3 +21,4 @@ config VFIO_AMBA
        If you don't know what to do here, say N.

 source "drivers/vfio/platform/reset/Kconfig"
+source "drivers/vfio/platform/msi/Kconfig"

diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
index 3f3a24e7c4ef..9ccdcdbf0e7e 100644
--- a/drivers/vfio/platform/Makefile
+++ b/drivers/vfio/platform/Makefile

@@ -5,6 +5,7 @@ vfio-platform-y := vfio_platform.o
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
 obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform-base.o
 obj-$(CONFIG_VFIO_PLATFORM) += reset/
+obj-$(CONFIG_VFIO_PLATFORM) += msi/

 vfio-amba-y := vfio_amba.o

diff --git a/drivers/vfio/platform/msi/Kconfig b/drivers/vfio/platform/msi/Kconfig
new file mode 100644
index 000000000000..54d6b70e1e32
--- /dev/null
+++ b/drivers/vfio/platform/msi/Kconfig

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config VFIO_PLATFORM_BCMPLT_MSI
+     tristate "MSI support for Broadcom platform devices"
+     depends on VFIO_PLATFORM && (ARCH_BCM_IPROC || COMPILE_TEST)
+     default ARCH_BCM_IPROC
+     help
+       Enables the VFIO platform driver to handle msi for Broadcom devices
+
+       If you don't know what to do here, say N.

diff --git a/drivers/vfio/platform/msi/Makefile b/drivers/vfio/platform/msi/Makefile
new file mode 100644
index 000000000000..27422d45cecb
--- /dev/null
+++ b/drivers/vfio/platform/msi/Makefile

@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_VFIO_PLATFORM_BCMPLT_MSI) += vfio_platform_bcmplt.o

diff --git a/drivers/vfio/platform/msi/vfio_platform_bcmplt.c b/drivers/vfio/platform/msi/vfio_platform_bcmplt.c
new file mode 100644
index 000000000000..a074b5e92d77
--- /dev/null
+++ b/drivers/vfio/platform/msi/vfio_platform_bcmplt.c

@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Broadcom.
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/msi.h>
+#include <linux/vfio.h>
+
+#include "../vfio_platform_private.h"
+
+#define RING_SIZE            (64 << 10)
+
+#define RING_MSI_ADDR_LS     0x03c
+#define RING_MSI_ADDR_MS     0x040
+#define RING_MSI_DATA_VALUE  0x064

Those 3 defines would not be needed anymore with that implementation option.

quoted

+
+static u32 bcm_num_msi(struct vfio_platform_device *vdev)
+{
+     struct vfio_platform_region *reg = &vdev->regions[0];
+
+     return (reg->size / RING_SIZE);
+}
+
+static struct vfio_platform_msi_node vfio_platform_bcmflexrm_msi_node = {
+     .owner = THIS_MODULE,
+     .compat = "brcm,iproc-flexrm-mbox",
+     .of_get_msi = bcm_num_msi,
+};
+
+static int __init vfio_platform_bcmflexrm_msi_module_init(void)
+{
+     __vfio_platform_register_msi(&vfio_platform_bcmflexrm_msi_node);
+
+     return 0;
+}
+
+static void __exit vfio_platform_bcmflexrm_msi_module_exit(void)
+{
+     vfio_platform_unregister_msi("brcm,iproc-flexrm-mbox");
+}
+
+module_init(vfio_platform_bcmflexrm_msi_module_init);
+module_exit(vfio_platform_bcmflexrm_msi_module_exit);

One thing I would like to discuss with Alex.

As the reset module is mandated (except if reset_required is forced to
0), I am wondering if we shouldn't try to turn the reset module into a
"specialization" module and put the msi hooks there. I am afraid we may
end up having modules for each and every vfio platform feature
specialization. At the moment that's fully bearable but I can't predict
what's next.

As the mandated feature is the reset capability maybe we could just keep
the config/module name terminology, tune the kconfig help message to
mention the msi support in case of flex-rm?

As I understand, your proposal is that we should not have a separate
module for MSI, rather we add in the existing reset module for
flex-rm. Thus, this way reset modules do not seem to be specialized
just for reset functionality only but for MSI as well. Apart from this
we need not to load the proposed msi module in this patch series. Is
my understanding correct?
For me it looks OK to consolidate MSI in the existing 'reset' module.
Let me know your views so that I can work for the next patch set accordingly.

Thanks,
Vikas

What do you think?

Thanks

Eric

quoted

+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Broadcom");