[RFC PATCH 0/5] vfio: platform: reset: Introduce tegra234 mgbe reset module

[RFC PATCH 0/5] vfio: platform: reset: Introduce tegra234 mgbe reset module

[Eric Auger]

We introduce a new vfio platform reset module for the tegra234
Multi-Gigabit Ethernet (MGBE).

This reset driver is more complex than previous ones because some
resources need to be prepared and released (clocks and reset). The
existing infrastructure was too simplistic to do that. So this series
extends the original single reset function to an ops struct enhanced
with optional open/close callbacks.

There the reset and clocks are handled. the functions are
called on open_device/close_device callback. That's questionable
whether this should be called in init/release instead but memory
regions cannot be ioremapped at that time since
vfio_platform_regions_init is called on .opendevice.

The actual reset toggles the mac reset, disable mac interrupts,
stop DMA requests and do a SW reset.

The reset code is inspired of the native driver:
net/ethernet/stmicro/stmmac/dwxgmac2_dma.c and
net/ethernet/stmicro/stmmac/dwmac-tegra.c

This series can be found at:
https://github.com/eauger/linux/tree/tegra234-mgbe-reset-module-rfc

The qemu series to test with can be found at
https://github.com/eauger/qemu/tree/tegra234-mgbe-rfc

Best Regards

Eric


Eric Auger (5):
  vfio_platform: Introduce vfio_platform_get_region helper
  vfio_platform: reset: Prepare for additional reset ops
  vfio_platform: reset: Introduce new open and close callbacks
  vfio-platform: Add a new handle to store reset data
  vfio/platform: Add tegra234-mgbe vfio platform reset module

 drivers/vfio/platform/reset/Kconfig           |   7 +
 drivers/vfio/platform/reset/Makefile          |   2 +
 .../platform/reset/vfio_platform_amdxgbe.c    |   7 +-
 .../reset/vfio_platform_calxedaxgmac.c        |   7 +-
 .../reset/vfio_platform_tegra234_mgbe.c       | 245 ++++++++++++++++++
 drivers/vfio/platform/vfio_platform_common.c  |  78 ++++--
 drivers/vfio/platform/vfio_platform_private.h |  43 ++-
 7 files changed, 358 insertions(+), 31 deletions(-)
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_tegra234_mgbe.c

-- 
2.41.0

[RFC PATCH 1/5] vfio_platform: Introduce vfio_platform_get_region helper

[Eric Auger]

Reset modules need to access some specific regions. It may
easier and safer to refer to those regions using their
name instead of relying on their index.

So let's introduce a helper that looks for the
struct vfio_platform_region with a given name.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

I don't know if reg names described in binding yaml are guaranteed to
appear in the listed order. I guess no, hence the reg-names.
In my case, for the host tegra234 dt node, regs I even observe regs that
are not documented in the yaml:
mac, xpcs, macsec-base, hypervisor whereas yaml only describes
hypervisor, mac, xpcs
---
 drivers/vfio/platform/vfio_platform_common.c  | 14 ++++++++++++++
 drivers/vfio/platform/vfio_platform_private.h |  5 +++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index e53757d1d095..6861f977fd5b 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -153,6 +153,7 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 		vdev->regions[i].addr = res->start;
 		vdev->regions[i].size = resource_size(res);
 		vdev->regions[i].flags = 0;
+		vdev->regions[i].name = res->name;
 
 		switch (resource_type(res)) {
 		case IORESOURCE_MEM:
@@ -188,6 +189,19 @@ static int vfio_platform_regions_init(struct vfio_platform_device *vdev)
 	return -EINVAL;
 }
 
+struct vfio_platform_region*
+vfio_platform_get_region(struct vfio_platform_device *vdev, const char *name)
+{
+	int i;
+
+	for (i = 0; i < vdev->num_regions; i++) {
+		if (!strcmp(vdev->regions[i].name, name))
+			return &vdev->regions[i];
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(vfio_platform_get_region);
+
 static void vfio_platform_regions_cleanup(struct vfio_platform_device *vdev)
 {
 	int i;
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 8d8fab516849..20d67634bc41 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -37,6 +37,7 @@ struct vfio_platform_region {
 	resource_size_t		size;
 	u32			flags;
 	u32			type;
+	const char		*name;
 #define VFIO_PLATFORM_REGION_TYPE_MMIO	1
 #define VFIO_PLATFORM_REGION_TYPE_PIO	2
 	void __iomem		*ioaddr;
@@ -104,6 +105,10 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 void __vfio_platform_register_reset(struct vfio_platform_reset_node *n);
 void vfio_platform_unregister_reset(const char *compat,
 				    vfio_platform_reset_fn_t fn);
+
+struct vfio_platform_region *
+vfio_platform_get_region(struct vfio_platform_device *vdev, const char *name);
+
 #define vfio_platform_register_reset(__compat, __reset)		\
 static struct vfio_platform_reset_node __reset ## _node = {	\
 	.owner = THIS_MODULE,					\
-- 
2.41.0

[RFC PATCH 2/5] vfio_platform: reset: Prepare for additional reset ops

[Eric Auger]

Reset modules currently offer a single reset function that gets
called on open, close and VFIO_DEVICE_RESET ioctl.

For more complex devices this infrastructure looks too simplistic.
Indeed some resources may be needed from the open() until the close(),
like clocks, resets. A single function does not allow that setup.

So let's encapsulate the current reset function into an ops struct
that will be soon extended with new open and close callbacks.

Existing reset modules are adapted.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 .../platform/reset/vfio_platform_amdxgbe.c    |  7 +++-
 .../reset/vfio_platform_calxedaxgmac.c        |  7 +++-
 drivers/vfio/platform/vfio_platform_common.c  | 36 ++++++++++---------
 drivers/vfio/platform/vfio_platform_private.h | 30 ++++++++++------
 4 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
index abdca900802d..6d1b641480f4 100644
--- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
+++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
@@ -109,7 +109,12 @@ static int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset);
+static const struct vfio_platform_reset_ops
+vfio_platform_amdxgbe_reset_ops = {
+	.reset = vfio_platform_amdxgbe_reset,
+};
+
+module_vfio_reset_handler("amd,xgbe-seattle-v1a", vfio_platform_amdxgbe_reset_ops);
 
 MODULE_VERSION("0.1");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
index 63cc7f0b2e4a..392a8579f491 100644
--- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
+++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
@@ -66,7 +66,12 @@ static int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
 	return 0;
 }
 
-module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset);
+static const struct vfio_platform_reset_ops
+vfio_platform_calxedaxgmac_reset_ops = {
+	.reset = vfio_platform_calxedaxgmac_reset,
+};
+
+module_vfio_reset_handler("calxeda,hb-xgmac", vfio_platform_calxedaxgmac_reset_ops);
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6861f977fd5b..3be08e58365b 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -28,23 +28,23 @@
 static LIST_HEAD(reset_list);
 static DEFINE_MUTEX(driver_lock);
 
-static vfio_platform_reset_fn_t vfio_platform_lookup_reset(const char *compat,
-					struct module **module)
+static const struct vfio_platform_reset_ops *
+vfio_platform_lookup_reset(const char *compat, struct module **module)
 {
+	const struct vfio_platform_reset_ops *ops = NULL;
 	struct vfio_platform_reset_node *iter;
-	vfio_platform_reset_fn_t reset_fn = NULL;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry(iter, &reset_list, link) {
 		if (!strcmp(iter->compat, compat) &&
 			try_module_get(iter->owner)) {
 			*module = iter->owner;
-			reset_fn = iter->of_reset;
+			ops = &iter->ops;
 			break;
 		}
 	}
 	mutex_unlock(&driver_lock);
-	return reset_fn;
+	return ops;
 }
 
 static int vfio_platform_acpi_probe(struct vfio_platform_device *vdev,
@@ -106,7 +106,7 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev)
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return vfio_platform_acpi_has_reset(vdev);
 
-	return vdev->of_reset ? true : false;
+	return vdev->reset_ops ? true : false;
 }
 
 static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
@@ -114,15 +114,15 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
-	vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
-						    &vdev->reset_module);
-	if (!vdev->of_reset) {
+	vdev->reset_ops = vfio_platform_lookup_reset(vdev->compat,
+						     &vdev->reset_module);
+	if (!vdev->reset_ops) {
 		request_module("vfio-reset:%s", vdev->compat);
-		vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
-							&vdev->reset_module);
+		vdev->reset_ops = vfio_platform_lookup_reset(vdev->compat,
+							     &vdev->reset_module);
 	}
 
-	return vdev->of_reset ? 0 : -ENOENT;
+	return vdev->reset_ops ? 0 : -ENOENT;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -130,7 +130,7 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
 	if (VFIO_PLATFORM_IS_ACPI(vdev))
 		return;
 
-	if (vdev->of_reset)
+	if (vdev->reset_ops)
 		module_put(vdev->reset_module);
 }
 
@@ -219,9 +219,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	if (VFIO_PLATFORM_IS_ACPI(vdev)) {
 		dev_info(vdev->device, "reset\n");
 		return vfio_platform_acpi_call_reset(vdev, extra_dbg);
-	} else if (vdev->of_reset) {
+	} else if (vdev->reset_ops && vdev->reset_ops->reset) {
 		dev_info(vdev->device, "reset\n");
-		return vdev->of_reset(vdev);
+		return vdev->reset_ops->reset(vdev);
 	}
 
 	dev_warn(vdev->device, "no reset function found!\n");
@@ -686,13 +686,15 @@ void __vfio_platform_register_reset(struct vfio_platform_reset_node *node)
 EXPORT_SYMBOL_GPL(__vfio_platform_register_reset);
 
 void vfio_platform_unregister_reset(const char *compat,
-				    vfio_platform_reset_fn_t fn)
+				    struct vfio_platform_reset_ops ops)
 {
 	struct vfio_platform_reset_node *iter, *temp;
 
 	mutex_lock(&driver_lock);
 	list_for_each_entry_safe(iter, temp, &reset_list, link) {
-		if (!strcmp(iter->compat, compat) && (iter->of_reset == fn)) {
+		if (!strcmp(iter->compat, compat) &&
+		    !memcmp(&iter->ops, &ops,
+			    sizeof(struct vfio_platform_reset_ops))) {
 			list_del(&iter->link);
 			break;
 		}
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 20d67634bc41..90c99d2e70f4 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -65,18 +65,26 @@ struct vfio_platform_device {
 	struct resource*
 		(*get_resource)(struct vfio_platform_device *vdev, int i);
 	int	(*get_irq)(struct vfio_platform_device *vdev, int i);
-	int	(*of_reset)(struct vfio_platform_device *vdev);
 
+	const struct vfio_platform_reset_ops *reset_ops;
 	bool				reset_required;
 };
 
-typedef int (*vfio_platform_reset_fn_t)(struct vfio_platform_device *vdev);
+/**
+ * struct vfio_platform_reset_ops - reset ops
+ *
+ * @reset:	reset function (required)
+ */
+struct vfio_platform_reset_ops {
+	int (*reset)(struct vfio_platform_device *vdev);
+};
+
 
 struct vfio_platform_reset_node {
 	struct list_head link;
 	char *compat;
 	struct module *owner;
-	vfio_platform_reset_fn_t of_reset;
+	struct vfio_platform_reset_ops ops;
 };
 
 int vfio_platform_init_common(struct vfio_platform_device *vdev);
@@ -104,29 +112,29 @@ int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
 
 void __vfio_platform_register_reset(struct vfio_platform_reset_node *n);
 void vfio_platform_unregister_reset(const char *compat,
-				    vfio_platform_reset_fn_t fn);
+				    struct vfio_platform_reset_ops ops);
 
 struct vfio_platform_region *
 vfio_platform_get_region(struct vfio_platform_device *vdev, const char *name);
 
-#define vfio_platform_register_reset(__compat, __reset)		\
-static struct vfio_platform_reset_node __reset ## _node = {	\
+#define vfio_platform_register_reset(__compat, __ops)		\
+static struct vfio_platform_reset_node __ops ## _node = {	\
 	.owner = THIS_MODULE,					\
 	.compat = __compat,					\
-	.of_reset = __reset,					\
+	.ops = __ops,						\
 };								\
-__vfio_platform_register_reset(&__reset ## _node)
+__vfio_platform_register_reset(&__ops ## _node)
 
-#define module_vfio_reset_handler(compat, reset)		\
+#define module_vfio_reset_handler(compat, ops)			\
 MODULE_ALIAS("vfio-reset:" compat);				\
 static int __init reset ## _module_init(void)			\
 {								\
-	vfio_platform_register_reset(compat, reset);		\
+	vfio_platform_register_reset(compat, ops);		\
 	return 0;						\
 };								\
 static void __exit reset ## _module_exit(void)			\
 {								\
-	vfio_platform_unregister_reset(compat, reset);		\
+	vfio_platform_unregister_reset(compat, ops);		\
 };								\
 module_init(reset ## _module_init);				\
 module_exit(reset ## _module_exit)
-- 
2.41.0

[RFC PATCH 3/5] vfio_platform: reset: Introduce new open and close callbacks

[Eric Auger]

Some devices may require resources such as clocks and resets
which cannot be handled in the vfio_platform agnostic code. Let's
add 2 new callbacks to handle those resources. Those new callbacks
are optional, as opposed to the reset callback. In case they are
implemented, both need to be.

They are not implemented by the existing reset modules.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/vfio_platform_common.c  | 28 ++++++++++++++++++-
 drivers/vfio/platform/vfio_platform_private.h |  6 ++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 3be08e58365b..2174e402dc70 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -228,6 +228,23 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
 	return -EINVAL;
 }
 
+static void vfio_platform_reset_module_close(struct vfio_platform_device *vpdev)
+{
+	if (VFIO_PLATFORM_IS_ACPI(vpdev))
+		return;
+	if (vpdev->reset_ops && vpdev->reset_ops->close)
+		vpdev->reset_ops->close(vpdev);
+}
+
+static int vfio_platform_reset_module_open(struct vfio_platform_device *vpdev)
+{
+	if (VFIO_PLATFORM_IS_ACPI(vpdev))
+		return 0;
+	if (vpdev->reset_ops && vpdev->reset_ops->open)
+		return vpdev->reset_ops->open(vpdev);
+	return 0;
+}
+
 void vfio_platform_close_device(struct vfio_device *core_vdev)
 {
 	struct vfio_platform_device *vdev =
@@ -242,6 +259,7 @@ void vfio_platform_close_device(struct vfio_device *core_vdev)
 			"reset driver is required and reset call failed in release (%d) %s\n",
 			ret, extra_dbg ? extra_dbg : "");
 	}
+	vfio_platform_reset_module_close(vdev);
 	pm_runtime_put(vdev->device);
 	vfio_platform_regions_cleanup(vdev);
 	vfio_platform_irq_cleanup(vdev);
@@ -265,7 +283,13 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
 
 	ret = pm_runtime_get_sync(vdev->device);
 	if (ret < 0)
-		goto err_rst;
+		goto err_rst_open;
+
+	ret = vfio_platform_reset_module_open(vdev);
+	if (ret) {
+		dev_info(vdev->device, "reset module load failed (%d)\n", ret);
+		goto err_rst_open;
+	}
 
 	ret = vfio_platform_call_reset(vdev, &extra_dbg);
 	if (ret && vdev->reset_required) {
@@ -278,6 +302,8 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
 	return 0;
 
 err_rst:
+	vfio_platform_reset_module_close(vdev);
+err_rst_open:
 	pm_runtime_put(vdev->device);
 	vfio_platform_irq_cleanup(vdev);
 err_irq:
diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 90c99d2e70f4..528b01c56de6 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -74,9 +74,13 @@ struct vfio_platform_device {
  * struct vfio_platform_reset_ops - reset ops
  *
  * @reset:	reset function (required)
+ * @open:	Called when the first fd is opened for this device (optional)
+ * @close:	Called when the last fd is closed for this device (optional)
  */
 struct vfio_platform_reset_ops {
 	int (*reset)(struct vfio_platform_device *vdev);
+	int (*open)(struct vfio_platform_device *vdev);
+	void (*close)(struct vfio_platform_device *vdev);
 };
 
 
@@ -129,6 +133,8 @@ __vfio_platform_register_reset(&__ops ## _node)
 MODULE_ALIAS("vfio-reset:" compat);				\
 static int __init reset ## _module_init(void)			\
 {								\
+	if (!!ops.open ^ !!ops.close)				\
+		return -EINVAL;					\
 	vfio_platform_register_reset(compat, ops);		\
 	return 0;						\
 };								\
-- 
2.41.0

[RFC PATCH 4/5] vfio-platform: Add a new handle to store reset data

[Eric Auger]

Add a new field to store data used by the reset modules.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/vfio_platform_private.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
index 528b01c56de6..0d689241be23 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -68,6 +68,8 @@ struct vfio_platform_device {
 
 	const struct vfio_platform_reset_ops *reset_ops;
 	bool				reset_required;
+	/* This field can be used by reset driver to store some data */
+	void		*reset_opaque;
 };
 
 /**
-- 
2.41.0

[RFC PATCH 5/5] vfio/platform: Add tegra234-mgbe vfio platform reset module

[Eric Auger]

open and close callbacks take care of resources requested by
the reset code, ie. clocks and reset.

The actual reset function toggles the mac reset, disable mac
ihterrupts, stop DMA requests and do a SW reset.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/vfio/platform/reset/Kconfig           |   7 +
 drivers/vfio/platform/reset/Makefile          |   2 +
 .../reset/vfio_platform_tegra234_mgbe.c       | 245 ++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 drivers/vfio/platform/reset/vfio_platform_tegra234_mgbe.c

diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
index dcc08dc145a5..3113fae21ebf 100644
--- a/drivers/vfio/platform/reset/Kconfig
+++ b/drivers/vfio/platform/reset/Kconfig
@@ -14,6 +14,13 @@ config VFIO_PLATFORM_AMDXGBE_RESET
 
 	  If you don't know what to do here, say N.
 
+config VFIO_PLATFORM_TEGRA234_MGBE_RESET
+	tristate "VFIO support for NVidia tegra234 MGBE reset"
+	help
+	  Enables the VFIO platform driver to handle reset for NVidia tegra234 mgbe
+
+	  If you don't know what to do here, say N.
+
 config VFIO_PLATFORM_BCMFLEXRM_RESET
 	tristate "VFIO support for Broadcom FlexRM reset"
 	depends on ARCH_BCM_IPROC || COMPILE_TEST
diff --git a/drivers/vfio/platform/reset/Makefile b/drivers/vfio/platform/reset/Makefile
index 7294c5ea122e..5ebef71f61a0 100644
--- a/drivers/vfio/platform/reset/Makefile
+++ b/drivers/vfio/platform/reset/Makefile
@@ -1,7 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 vfio-platform-calxedaxgmac-y := vfio_platform_calxedaxgmac.o
 vfio-platform-amdxgbe-y := vfio_platform_amdxgbe.o
+vfio-platform-tegra234-mgbe-y := vfio_platform_tegra234_mgbe.o
 
 obj-$(CONFIG_VFIO_PLATFORM_CALXEDAXGMAC_RESET) += vfio-platform-calxedaxgmac.o
 obj-$(CONFIG_VFIO_PLATFORM_AMDXGBE_RESET) += vfio-platform-amdxgbe.o
+obj-$(CONFIG_VFIO_PLATFORM_TEGRA234_MGBE_RESET) += vfio-platform-tegra234-mgbe.o
 obj-$(CONFIG_VFIO_PLATFORM_BCMFLEXRM_RESET) += vfio_platform_bcmflexrm.o
diff --git a/drivers/vfio/platform/reset/vfio_platform_tegra234_mgbe.c b/drivers/vfio/platform/reset/vfio_platform_tegra234_mgbe.c
new file mode 100644
index 000000000000..09b9e10be3ff
--- /dev/null
+++ b/drivers/vfio/platform/reset/vfio_platform_tegra234_mgbe.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VFIO platform driver specialized for NVidia tegra234-mgbe reset
+ * Code is inspired from dwxgmac2_dma.c code
+ *
+ * Copyright (c) 2024 Red Hat, Inc.  All rights reserved.
+ *     Author: Eric Auger <eric.auger@redhat.com>
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/reset.h>
+#include <linux/iopoll.h>
+#include <linux/clk.h>
+
+#include "../vfio_platform_private.h"
+
+static const char *const mgbe_clks[] = {
+	"rx-pcs", "tx", "tx-pcs", "mac-divider", "mac", "mgbe", "ptp-ref", "mac"
+};
+
+struct tegra_mgbe {
+	struct clk_bulk_data *clks;
+	struct reset_control *mac_rst;
+	void __iomem *mac;
+};
+
+#define XGMAC_TX_CONFIG                 0x00000000
+#define XGMAC_CONFIG_TE                 BIT(0)
+#define XGMAC_RX_CONFIG                 0x00000004
+#define XGMAC_CONFIG_RE                 BIT(0)
+#define XGMAC_DMA_MODE			0x00003000
+#define XGMAC_SWR			BIT(0)
+
+#define XGMAC_DMA_CH_INT_EN(x)		(0x00003138 + (0x80 * (x)))
+#define XGMAC_TIE			BIT(0)
+#define XGMAC_RIE			BIT(6)
+#define XGMAC_RBUE			BIT(7)
+#define XGMAC_DMA_INT_DEFAULT_RX	(XGMAC_RBUE | XGMAC_RIE)
+#define XGMAC_DMA_INT_DEFAULT_TX	(XGMAC_TIE)
+
+#define XGMAC_DMA_CH_STATUS(x)		(0x00003160 + (0x80 * (x)))
+#define XGMAC_DMA_CH_RX_CONTROL(x)      (0x00003108 + (0x80 * (x)))
+#define XGMAC_RXST                      BIT(0)
+#define XGMAC_DMA_CH_TX_CONTROL(x)      (0x00003104 + (0x80 * (x)))
+#define XGMAC_TXST                      BIT(0)
+
+#define XGMAC_INT_STATUS                0x000000b0
+#define XGMAC_INT_EN                    0x000000b4
+
+#define MGBE_WRAP_COMMON_INTR_ENABLE 0x8704
+
+static int
+toggle_reset(struct device *dev, const char *rst_str, struct reset_control *rst)
+{
+	int ret;
+
+	ret = reset_control_assert(rst);
+	if (ret < 0)
+		dev_err(dev, "Failed to assert %s reset %d\n",
+			rst_str, ret);
+	usleep_range(2000, 4000);
+
+	ret = reset_control_deassert(rst);
+	if (ret < 0)
+		dev_err(dev, "Failed to deassert %s reset %d\n", rst_str, ret);
+	usleep_range(2000, 4000);
+	return ret;
+}
+
+static void stop_dma(void __iomem *mac, uint channel)
+{
+	u32 value;
+
+	/* DMA Stop RX */
+	value = readl(mac + XGMAC_DMA_CH_RX_CONTROL(channel));
+	value &= ~XGMAC_RXST;
+	writel(value, mac + XGMAC_DMA_CH_RX_CONTROL(channel));
+
+	value = readl(mac + XGMAC_RX_CONFIG);
+	value &= ~XGMAC_CONFIG_RE;
+	writel(value, mac + XGMAC_RX_CONFIG);
+
+	usleep_range(10, 15);
+
+	/* DMA Stop TX */
+	value = readl(mac + XGMAC_DMA_CH_TX_CONTROL(channel));
+	value &= ~XGMAC_RXST;
+	writel(value, mac + XGMAC_DMA_CH_TX_CONTROL(channel));
+
+	value = readl(mac + XGMAC_TX_CONFIG);
+	value &= ~XGMAC_CONFIG_TE;
+	writel(value, mac + XGMAC_TX_CONFIG);
+
+	usleep_range(10, 15);
+}
+static int dma_sw_reset(void __iomem *mac)
+{
+	u32 value;
+
+	value = readl(mac + XGMAC_DMA_MODE);
+	writel(value | XGMAC_SWR, mac + XGMAC_DMA_MODE);
+	return readl_poll_timeout(mac + XGMAC_DMA_MODE, value,
+				  !(value & XGMAC_SWR), 0, 100000);
+}
+
+static void disable_dma_irq(void __iomem *mac, u32 channel)
+{
+	u32 intr_en, intr_status;
+
+	intr_en = readl(mac + XGMAC_DMA_CH_INT_EN(channel));
+
+	intr_en &= ~XGMAC_DMA_INT_DEFAULT_RX;
+	intr_en &= ~XGMAC_DMA_INT_DEFAULT_TX;
+	writel(intr_en, mac + XGMAC_DMA_CH_INT_EN(channel));
+	usleep_range(10, 15);
+
+	intr_status = readl(mac + XGMAC_DMA_CH_STATUS(channel));
+	writel(0, mac + XGMAC_DMA_CH_STATUS(channel));
+}
+
+static int prepare_enable_clocks(struct device *dev, struct clk_bulk_data **clocks)
+{
+	struct clk_bulk_data *clks;
+	int ret;
+
+	clks = kcalloc(ARRAY_SIZE(mgbe_clks), sizeof(*clks), GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	for (int i = 0; i <  ARRAY_SIZE(mgbe_clks); i++)
+		clks[i].id = mgbe_clks[i];
+
+	ret = clk_bulk_get(dev, ARRAY_SIZE(mgbe_clks), clks);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get clocks %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(mgbe_clks), clks);
+	if (ret < 0) {
+		dev_err(dev, "Failed to prepare_enable clocks %d\n", ret);
+		clk_bulk_put(ARRAY_SIZE(mgbe_clks), clks);
+		return ret;
+	}
+	*clocks = clks;
+	return ret;
+}
+
+static int vfio_platform_tegra234_mgbe_open(struct vfio_platform_device *vpdev)
+{
+	struct tegra_mgbe *mgbe;
+	struct vfio_platform_region *mac_regs;
+	struct vfio_device *vdev = &vpdev->vdev;
+	struct device *dev = vdev->dev;
+	int ret;
+
+	mac_regs = vfio_platform_get_region(vpdev, "mac");
+	if (!mac_regs)
+		return -EINVAL;
+
+	mac_regs->ioaddr = ioremap(mac_regs->addr, mac_regs->size);
+	if (!mac_regs->ioaddr)
+		return -ENOMEM;
+
+	mgbe = kmalloc(sizeof(struct tegra_mgbe), GFP_KERNEL);
+	if (!mgbe) {
+		ret = -ENOMEM;
+		goto iounmap;
+	}
+
+	mgbe->mac = mac_regs->ioaddr;
+
+	ret = prepare_enable_clocks(dev, &mgbe->clks);
+	if (ret)
+		goto res_err;
+
+	mgbe->mac_rst = reset_control_get_exclusive(dev, "mac");
+	if (IS_ERR(mgbe->mac_rst)) {
+		dev_err(dev, "Failed to get mac reset %ld\n", PTR_ERR(mgbe->mac_rst));
+		ret = PTR_ERR(mgbe->mac_rst);
+		goto res_err;
+	}
+	vpdev->reset_opaque = mgbe;
+	return 0;
+res_err:
+	kfree(mgbe);
+iounmap:
+	iounmap(mac_regs->ioaddr);
+
+	return ret;
+}
+
+static void vfio_platform_tegra234_mgbe_close(struct vfio_platform_device *vpdev)
+{
+	struct tegra_mgbe *mgbe = vpdev->reset_opaque;
+
+	/* iounmap is done in vfio_platform_common */
+	reset_control_put(mgbe->mac_rst);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(mgbe_clks), mgbe->clks);
+	clk_bulk_put(ARRAY_SIZE(mgbe_clks), mgbe->clks);
+	kfree(mgbe->clks);
+	vpdev->reset_opaque = NULL;
+}
+
+static int vfio_platform_tegra234_mgbe_reset(struct vfio_platform_device *vpdev)
+{
+	struct tegra_mgbe *mgbe = vpdev->reset_opaque;
+	struct vfio_device *vdev = &vpdev->vdev;
+	struct device *dev = vdev->dev;
+	int ret;
+
+	if (!mgbe)
+		return -ENODEV;
+
+	toggle_reset(dev, "mac", mgbe->mac_rst);
+
+	for (int i = 0; i < 10; i++)
+		disable_dma_irq(mgbe->mac, i);
+
+	writel(0, mgbe->mac + MGBE_WRAP_COMMON_INTR_ENABLE);
+
+	for (int i = 0; i < 10; i++)
+		stop_dma(mgbe->mac, i);
+
+	ret = dma_sw_reset(mgbe->mac);
+	if (ret)
+		dev_err(dev, "Failed to reset the DMA %d\n", ret);
+
+	return ret;
+}
+
+static const struct vfio_platform_reset_ops
+vfio_platform_tegra234_mgbe_reset_ops = {
+	.reset = vfio_platform_tegra234_mgbe_reset,
+	.open = vfio_platform_tegra234_mgbe_open,
+	.close = vfio_platform_tegra234_mgbe_close,
+};
+
+module_vfio_reset_handler("nvidia,tegra234-mgbe", vfio_platform_tegra234_mgbe_reset_ops);
+
+MODULE_VERSION("0.1");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Eric Auger <eric.auger@redhat.com>");
+MODULE_DESCRIPTION("Reset support for NVidia tegra234 mgbe vfio platform device");
-- 
2.41.0

Re: [RFC PATCH 3/5] vfio_platform: reset: Introduce new open and close callbacks

[Alex Williamson]

On Thu, 29 Aug 2024 18:11:07 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Some devices may require resources such as clocks and resets
> which cannot be handled in the vfio_platform agnostic code. Let's
> add 2 new callbacks to handle those resources. Those new callbacks
> are optional, as opposed to the reset callback. In case they are
> implemented, both need to be.
> 
> They are not implemented by the existing reset modules.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  drivers/vfio/platform/vfio_platform_common.c  | 28 ++++++++++++++++++-
>  drivers/vfio/platform/vfio_platform_private.h |  6 ++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index 3be08e58365b..2174e402dc70 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -228,6 +228,23 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>  	return -EINVAL;
>  }
>  
> +static void vfio_platform_reset_module_close(struct vfio_platform_device *vpdev)
> +{
> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
> +		return;
> +	if (vpdev->reset_ops && vpdev->reset_ops->close)
> +		vpdev->reset_ops->close(vpdev);
> +}
> +
> +static int vfio_platform_reset_module_open(struct vfio_platform_device *vpdev)
> +{
> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
> +		return 0;
> +	if (vpdev->reset_ops && vpdev->reset_ops->open)
> +		return vpdev->reset_ops->open(vpdev);
> +	return 0;
> +}

Hi Eric,

I didn't get why these are no-op'd on an ACPI platform.  Shouldn't it
be up to the reset ops to decide whether to implement something based
on the system firmware rather than vfio-platform-common?

> +
>  void vfio_platform_close_device(struct vfio_device *core_vdev)
>  {
>  	struct vfio_platform_device *vdev =
> @@ -242,6 +259,7 @@ void vfio_platform_close_device(struct vfio_device *core_vdev)
>  			"reset driver is required and reset call failed in release (%d) %s\n",
>  			ret, extra_dbg ? extra_dbg : "");
>  	}
> +	vfio_platform_reset_module_close(vdev);
>  	pm_runtime_put(vdev->device);
>  	vfio_platform_regions_cleanup(vdev);
>  	vfio_platform_irq_cleanup(vdev);
> @@ -265,7 +283,13 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>  
>  	ret = pm_runtime_get_sync(vdev->device);
>  	if (ret < 0)
> -		goto err_rst;
> +		goto err_rst_open;
> +
> +	ret = vfio_platform_reset_module_open(vdev);
> +	if (ret) {
> +		dev_info(vdev->device, "reset module load failed (%d)\n", ret);
> +		goto err_rst_open;
> +	}
>  
>  	ret = vfio_platform_call_reset(vdev, &extra_dbg);
>  	if (ret && vdev->reset_required) {
> @@ -278,6 +302,8 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>  	return 0;
>  
>  err_rst:
> +	vfio_platform_reset_module_close(vdev);
> +err_rst_open:
>  	pm_runtime_put(vdev->device);
>  	vfio_platform_irq_cleanup(vdev);
>  err_irq:
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index 90c99d2e70f4..528b01c56de6 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -74,9 +74,13 @@ struct vfio_platform_device {
>   * struct vfio_platform_reset_ops - reset ops
>   *
>   * @reset:	reset function (required)
> + * @open:	Called when the first fd is opened for this device (optional)
> + * @close:	Called when the last fd is closed for this device (optional)

This doesn't note any platform firmware dependency.  We should probably
also note here the XOR requirement enforced below here.  Thanks,

Alex

>   */
>  struct vfio_platform_reset_ops {
>  	int (*reset)(struct vfio_platform_device *vdev);
> +	int (*open)(struct vfio_platform_device *vdev);
> +	void (*close)(struct vfio_platform_device *vdev);
>  };
>  
>  
> @@ -129,6 +133,8 @@ __vfio_platform_register_reset(&__ops ## _node)
>  MODULE_ALIAS("vfio-reset:" compat);				\
>  static int __init reset ## _module_init(void)			\
>  {								\
> +	if (!!ops.open ^ !!ops.close)				\
> +		return -EINVAL;					\
>  	vfio_platform_register_reset(compat, ops);		\
>  	return 0;						\
>  };								\

Re: [RFC PATCH 3/5] vfio_platform: reset: Introduce new open and close callbacks

[Eric Auger]

Hi Alex,

On 8/30/24 01:21, Alex Williamson wrote:
> On Thu, 29 Aug 2024 18:11:07 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Some devices may require resources such as clocks and resets
>> which cannot be handled in the vfio_platform agnostic code. Let's
>> add 2 new callbacks to handle those resources. Those new callbacks
>> are optional, as opposed to the reset callback. In case they are
>> implemented, both need to be.
>>
>> They are not implemented by the existing reset modules.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c  | 28 ++++++++++++++++++-
>>  drivers/vfio/platform/vfio_platform_private.h |  6 ++++
>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index 3be08e58365b..2174e402dc70 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -228,6 +228,23 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>>  	return -EINVAL;
>>  }
>>  
>> +static void vfio_platform_reset_module_close(struct vfio_platform_device *vpdev)
>> +{
>> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
>> +		return;
>> +	if (vpdev->reset_ops && vpdev->reset_ops->close)
>> +		vpdev->reset_ops->close(vpdev);
>> +}
>> +
>> +static int vfio_platform_reset_module_open(struct vfio_platform_device *vpdev)
>> +{
>> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
>> +		return 0;
>> +	if (vpdev->reset_ops && vpdev->reset_ops->open)
>> +		return vpdev->reset_ops->open(vpdev);
>> +	return 0;
>> +}
> Hi Eric,
>
> I didn't get why these are no-op'd on an ACPI platform.  Shouldn't it
> be up to the reset ops to decide whether to implement something based
> on the system firmware rather than vfio-platform-common?

In case of ACPI boot, ie. VFIO_PLATFORM_IS_ACPI(vpdev) is set, I
understand we don't use the vfio platform reset module but the ACPI _RST
method. see vfio_platform_acpi_call_reset() and
vfio_platform_acpi_has_reset() introduced by d30daa33ec1d ("vfio:
platform: call _RST method when using ACPI"). I have never had the
opportunity to test acpi boot reset though.
>
>> +
>>  void vfio_platform_close_device(struct vfio_device *core_vdev)
>>  {
>>  	struct vfio_platform_device *vdev =
>> @@ -242,6 +259,7 @@ void vfio_platform_close_device(struct vfio_device *core_vdev)
>>  			"reset driver is required and reset call failed in release (%d) %s\n",
>>  			ret, extra_dbg ? extra_dbg : "");
>>  	}
>> +	vfio_platform_reset_module_close(vdev);
>>  	pm_runtime_put(vdev->device);
>>  	vfio_platform_regions_cleanup(vdev);
>>  	vfio_platform_irq_cleanup(vdev);
>> @@ -265,7 +283,13 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>>  
>>  	ret = pm_runtime_get_sync(vdev->device);
>>  	if (ret < 0)
>> -		goto err_rst;
>> +		goto err_rst_open;
>> +
>> +	ret = vfio_platform_reset_module_open(vdev);
>> +	if (ret) {
>> +		dev_info(vdev->device, "reset module load failed (%d)\n", ret);
>> +		goto err_rst_open;
>> +	}
>>  
>>  	ret = vfio_platform_call_reset(vdev, &extra_dbg);
>>  	if (ret && vdev->reset_required) {
>> @@ -278,6 +302,8 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>>  	return 0;
>>  
>>  err_rst:
>> +	vfio_platform_reset_module_close(vdev);
>> +err_rst_open:
>>  	pm_runtime_put(vdev->device);
>>  	vfio_platform_irq_cleanup(vdev);
>>  err_irq:
>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>> index 90c99d2e70f4..528b01c56de6 100644
>> --- a/drivers/vfio/platform/vfio_platform_private.h
>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>> @@ -74,9 +74,13 @@ struct vfio_platform_device {
>>   * struct vfio_platform_reset_ops - reset ops
>>   *
>>   * @reset:	reset function (required)
>> + * @open:	Called when the first fd is opened for this device (optional)
>> + * @close:	Called when the last fd is closed for this device (optional)
> This doesn't note any platform firmware dependency.  We should probably
> also note here the XOR requirement enforced below here.  Thanks,
To me this is just used along with dt boot, hence the lack of check.

Thanks

Eric
>
> Alex
>
>>   */
>>  struct vfio_platform_reset_ops {
>>  	int (*reset)(struct vfio_platform_device *vdev);
>> +	int (*open)(struct vfio_platform_device *vdev);
>> +	void (*close)(struct vfio_platform_device *vdev);
>>  };
>>  
>>  
>> @@ -129,6 +133,8 @@ __vfio_platform_register_reset(&__ops ## _node)
>>  MODULE_ALIAS("vfio-reset:" compat);				\
>>  static int __init reset ## _module_init(void)			\
>>  {								\
>> +	if (!!ops.open ^ !!ops.close)				\
>> +		return -EINVAL;					\
>>  	vfio_platform_register_reset(compat, ops);		\
>>  	return 0;						\
>>  };								\

Re: [RFC PATCH 3/5] vfio_platform: reset: Introduce new open and close callbacks

[Alex Williamson]

On Mon, 2 Sep 2024 18:03:23 +0200
Eric Auger <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 8/30/24 01:21, Alex Williamson wrote:
> > On Thu, 29 Aug 2024 18:11:07 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >  
> >> Some devices may require resources such as clocks and resets
> >> which cannot be handled in the vfio_platform agnostic code. Let's
> >> add 2 new callbacks to handle those resources. Those new callbacks
> >> are optional, as opposed to the reset callback. In case they are
> >> implemented, both need to be.
> >>
> >> They are not implemented by the existing reset modules.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  drivers/vfio/platform/vfio_platform_common.c  | 28 ++++++++++++++++++-
> >>  drivers/vfio/platform/vfio_platform_private.h |  6 ++++
> >>  2 files changed, 33 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> >> index 3be08e58365b..2174e402dc70 100644
> >> --- a/drivers/vfio/platform/vfio_platform_common.c
> >> +++ b/drivers/vfio/platform/vfio_platform_common.c
> >> @@ -228,6 +228,23 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
> >>  	return -EINVAL;
> >>  }
> >>  
> >> +static void vfio_platform_reset_module_close(struct vfio_platform_device *vpdev)
> >> +{
> >> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
> >> +		return;
> >> +	if (vpdev->reset_ops && vpdev->reset_ops->close)
> >> +		vpdev->reset_ops->close(vpdev);
> >> +}
> >> +
> >> +static int vfio_platform_reset_module_open(struct vfio_platform_device *vpdev)
> >> +{
> >> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
> >> +		return 0;
> >> +	if (vpdev->reset_ops && vpdev->reset_ops->open)
> >> +		return vpdev->reset_ops->open(vpdev);
> >> +	return 0;
> >> +}  
> > Hi Eric,
> >
> > I didn't get why these are no-op'd on an ACPI platform.  Shouldn't it
> > be up to the reset ops to decide whether to implement something based
> > on the system firmware rather than vfio-platform-common?  
> 
> In case of ACPI boot, ie. VFIO_PLATFORM_IS_ACPI(vpdev) is set, I
> understand we don't use the vfio platform reset module but the ACPI _RST
> method. see vfio_platform_acpi_call_reset() and
> vfio_platform_acpi_has_reset() introduced by d30daa33ec1d ("vfio:
> platform: call _RST method when using ACPI"). I have never had the
> opportunity to test acpi boot reset though.

Aha, I was expecting that VFIO_PLATFORM_IS_ACPI() wouldn't exclusively
require _RST support, but indeed in various places we only look for the
acpihid for the device without also checking for a _RST method.  In
fact commit 7aef80cf3187 ("vfio: platform: rename reset function")
prefixed the reset function pointer with "of_" to try to make that
exclusion more clear, but the previous patch of this series introducing
the ops structure chose a more generic name.  Should we instead use
"of_reset_ops" to maintain that we have two distinct paths, ACPI vs DT?

TBH I'm not sure why we couldn't check that an acpihid also supports a
_RST method and continue to look for reset module support otherwise,
but that's not the way it's coded and there's apparently no demand for
it.

> >> +
> >>  void vfio_platform_close_device(struct vfio_device *core_vdev)
> >>  {
> >>  	struct vfio_platform_device *vdev =
> >> @@ -242,6 +259,7 @@ void vfio_platform_close_device(struct vfio_device *core_vdev)
> >>  			"reset driver is required and reset call failed in release (%d) %s\n",
> >>  			ret, extra_dbg ? extra_dbg : "");
> >>  	}
> >> +	vfio_platform_reset_module_close(vdev);
> >>  	pm_runtime_put(vdev->device);
> >>  	vfio_platform_regions_cleanup(vdev);
> >>  	vfio_platform_irq_cleanup(vdev);
> >> @@ -265,7 +283,13 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
> >>  
> >>  	ret = pm_runtime_get_sync(vdev->device);
> >>  	if (ret < 0)
> >> -		goto err_rst;
> >> +		goto err_rst_open;
> >> +
> >> +	ret = vfio_platform_reset_module_open(vdev);
> >> +	if (ret) {
> >> +		dev_info(vdev->device, "reset module load failed (%d)\n", ret);
> >> +		goto err_rst_open;
> >> +	}
> >>  
> >>  	ret = vfio_platform_call_reset(vdev, &extra_dbg);
> >>  	if (ret && vdev->reset_required) {
> >> @@ -278,6 +302,8 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
> >>  	return 0;
> >>  
> >>  err_rst:
> >> +	vfio_platform_reset_module_close(vdev);
> >> +err_rst_open:
> >>  	pm_runtime_put(vdev->device);
> >>  	vfio_platform_irq_cleanup(vdev);
> >>  err_irq:
> >> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> >> index 90c99d2e70f4..528b01c56de6 100644
> >> --- a/drivers/vfio/platform/vfio_platform_private.h
> >> +++ b/drivers/vfio/platform/vfio_platform_private.h
> >> @@ -74,9 +74,13 @@ struct vfio_platform_device {
> >>   * struct vfio_platform_reset_ops - reset ops
> >>   *
> >>   * @reset:	reset function (required)
> >> + * @open:	Called when the first fd is opened for this device (optional)
> >> + * @close:	Called when the last fd is closed for this device (optional)  
> > This doesn't note any platform firmware dependency.  We should probably
> > also note here the XOR requirement enforced below here.  Thanks,  
> To me this is just used along with dt boot, hence the lack of check.

Per the above comment, I'd just specify the whole struct as a DT reset
ops interface and sprinkle "_of_" into the name to make that more
obvious.  Thanks,

Alex

> >>   */
> >>  struct vfio_platform_reset_ops {
> >>  	int (*reset)(struct vfio_platform_device *vdev);
> >> +	int (*open)(struct vfio_platform_device *vdev);
> >> +	void (*close)(struct vfio_platform_device *vdev);
> >>  };
> >>  
> >>  
> >> @@ -129,6 +133,8 @@ __vfio_platform_register_reset(&__ops ## _node)
> >>  MODULE_ALIAS("vfio-reset:" compat);				\
> >>  static int __init reset ## _module_init(void)			\
> >>  {								\
> >> +	if (!!ops.open ^ !!ops.close)				\
> >> +		return -EINVAL;					\
> >>  	vfio_platform_register_reset(compat, ops);		\
> >>  	return 0;						\
> >>  };								\  
> 

Re: [RFC PATCH 3/5] vfio_platform: reset: Introduce new open and close callbacks

[Eric Auger]

Hi Alex,

On 9/4/24 21:40, Alex Williamson wrote:
> On Mon, 2 Sep 2024 18:03:23 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Hi Alex,
>>
>> On 8/30/24 01:21, Alex Williamson wrote:
>>> On Thu, 29 Aug 2024 18:11:07 +0200
>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>  
>>>> Some devices may require resources such as clocks and resets
>>>> which cannot be handled in the vfio_platform agnostic code. Let's
>>>> add 2 new callbacks to handle those resources. Those new callbacks
>>>> are optional, as opposed to the reset callback. In case they are
>>>> implemented, both need to be.
>>>>
>>>> They are not implemented by the existing reset modules.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  drivers/vfio/platform/vfio_platform_common.c  | 28 ++++++++++++++++++-
>>>>  drivers/vfio/platform/vfio_platform_private.h |  6 ++++
>>>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>>>> index 3be08e58365b..2174e402dc70 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>>> @@ -228,6 +228,23 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev,
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> +static void vfio_platform_reset_module_close(struct vfio_platform_device *vpdev)
>>>> +{
>>>> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
>>>> +		return;
>>>> +	if (vpdev->reset_ops && vpdev->reset_ops->close)
>>>> +		vpdev->reset_ops->close(vpdev);
>>>> +}
>>>> +
>>>> +static int vfio_platform_reset_module_open(struct vfio_platform_device *vpdev)
>>>> +{
>>>> +	if (VFIO_PLATFORM_IS_ACPI(vpdev))
>>>> +		return 0;
>>>> +	if (vpdev->reset_ops && vpdev->reset_ops->open)
>>>> +		return vpdev->reset_ops->open(vpdev);
>>>> +	return 0;
>>>> +}  
>>> Hi Eric,
>>>
>>> I didn't get why these are no-op'd on an ACPI platform.  Shouldn't it
>>> be up to the reset ops to decide whether to implement something based
>>> on the system firmware rather than vfio-platform-common?  
>> In case of ACPI boot, ie. VFIO_PLATFORM_IS_ACPI(vpdev) is set, I
>> understand we don't use the vfio platform reset module but the ACPI _RST
>> method. see vfio_platform_acpi_call_reset() and
>> vfio_platform_acpi_has_reset() introduced by d30daa33ec1d ("vfio:
>> platform: call _RST method when using ACPI"). I have never had the
>> opportunity to test acpi boot reset though.
> Aha, I was expecting that VFIO_PLATFORM_IS_ACPI() wouldn't exclusively
> require _RST support, but indeed in various places we only look for the
> acpihid for the device without also checking for a _RST method.  In
> fact commit 7aef80cf3187 ("vfio: platform: rename reset function")
> prefixed the reset function pointer with "of_" to try to make that
> exclusion more clear, but the previous patch of this series introducing
> the ops structure chose a more generic name.  Should we instead use
> "of_reset_ops" to maintain that we have two distinct paths, ACPI vs DT?
Yes I will rename with of_ prefix.
>
> TBH I'm not sure why we couldn't check that an acpihid also supports a
> _RST method and continue to look for reset module support otherwise,
> but that's not the way it's coded and there's apparently no demand for
> it.
I agree. Without explicit request I am reluctant to change that because
I can't test atm
>
>>>> +
>>>>  void vfio_platform_close_device(struct vfio_device *core_vdev)
>>>>  {
>>>>  	struct vfio_platform_device *vdev =
>>>> @@ -242,6 +259,7 @@ void vfio_platform_close_device(struct vfio_device *core_vdev)
>>>>  			"reset driver is required and reset call failed in release (%d) %s\n",
>>>>  			ret, extra_dbg ? extra_dbg : "");
>>>>  	}
>>>> +	vfio_platform_reset_module_close(vdev);
>>>>  	pm_runtime_put(vdev->device);
>>>>  	vfio_platform_regions_cleanup(vdev);
>>>>  	vfio_platform_irq_cleanup(vdev);
>>>> @@ -265,7 +283,13 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>>>>  
>>>>  	ret = pm_runtime_get_sync(vdev->device);
>>>>  	if (ret < 0)
>>>> -		goto err_rst;
>>>> +		goto err_rst_open;
>>>> +
>>>> +	ret = vfio_platform_reset_module_open(vdev);
>>>> +	if (ret) {
>>>> +		dev_info(vdev->device, "reset module load failed (%d)\n", ret);
>>>> +		goto err_rst_open;
>>>> +	}
>>>>  
>>>>  	ret = vfio_platform_call_reset(vdev, &extra_dbg);
>>>>  	if (ret && vdev->reset_required) {
>>>> @@ -278,6 +302,8 @@ int vfio_platform_open_device(struct vfio_device *core_vdev)
>>>>  	return 0;
>>>>  
>>>>  err_rst:
>>>> +	vfio_platform_reset_module_close(vdev);
>>>> +err_rst_open:
>>>>  	pm_runtime_put(vdev->device);
>>>>  	vfio_platform_irq_cleanup(vdev);
>>>>  err_irq:
>>>> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
>>>> index 90c99d2e70f4..528b01c56de6 100644
>>>> --- a/drivers/vfio/platform/vfio_platform_private.h
>>>> +++ b/drivers/vfio/platform/vfio_platform_private.h
>>>> @@ -74,9 +74,13 @@ struct vfio_platform_device {
>>>>   * struct vfio_platform_reset_ops - reset ops
>>>>   *
>>>>   * @reset:	reset function (required)
>>>> + * @open:	Called when the first fd is opened for this device (optional)
>>>> + * @close:	Called when the last fd is closed for this device (optional)  
>>> This doesn't note any platform firmware dependency.  We should probably
>>> also note here the XOR requirement enforced below here.  Thanks,  
>> To me this is just used along with dt boot, hence the lack of check.
> Per the above comment, I'd just specify the whole struct as a DT reset
> ops interface and sprinkle "_of_" into the name to make that more
> obvious.  Thanks,

agreed

Thanks

Eric
>
> Alex
>
>>>>   */
>>>>  struct vfio_platform_reset_ops {
>>>>  	int (*reset)(struct vfio_platform_device *vdev);
>>>> +	int (*open)(struct vfio_platform_device *vdev);
>>>> +	void (*close)(struct vfio_platform_device *vdev);
>>>>  };
>>>>  
>>>>  
>>>> @@ -129,6 +133,8 @@ __vfio_platform_register_reset(&__ops ## _node)
>>>>  MODULE_ALIAS("vfio-reset:" compat);				\
>>>>  static int __init reset ## _module_init(void)			\
>>>>  {								\
>>>> +	if (!!ops.open ^ !!ops.close)				\
>>>> +		return -EINVAL;					\
>>>>  	vfio_platform_register_reset(compat, ops);		\
>>>>  	return 0;						\
>>>>  };								\