Update from V1 [1]:
- miscellaneous fixes based on Bjorn Andersson'and Mathieu Poirier's comments
- add "arm64: defconfig: Config that had RPMSG_CHAR now gets RPMSG_CTRL"
for 5.16 compatibility
- remove the REMOTEPROC_VIRTIO config.
- kept rproc_register_rvdev() and rproc_unregister_rvdev() but in a separate patch
- miscellaneous typo fixes.
[1] https://lkml.org/lkml/2021/10/1/243
Patchset description:
This series is a part of the work initiated a long time ago in
the series "remoteproc: Decorelate virtio from core"[2]
Objective of the work:
- Update the remoteproc VirtIO device creation (use platform device)
- Allow to declare remoteproc VirtIO device in DT
- declare resources associated to a remote proc VirtIO
- declare a list of VirtIO supported by the platform.
- Prepare the enhancement to more VirtIO devices (e.g I2C, audio, video, ...).
For instance be able to declare a I2C device in a virtio-i2C node.
- Keep the legacy working!
- Try to improve the picture about concerns reported by Christoph Hellwing [3][4]
[2] https://lkml.org/lkml/2020/4/16/1817
[3] https://lkml.org/lkml/2021/6/23/607
[4] https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
In term of device tree this would result in such hiearchy (stm32mp1 example with 2 virtio RPMSG):
m4_rproc: m4@10000000 {
compatible = "st,stm32mp1-m4";
reg = <0x10000000 0x40000>,
<0x30000000 0x40000>,
<0x38000000 0x10000>;
memory-region = <&retram>, <&mcuram>,<&mcuram2>;
mboxes = <&ipcc 2>, <&ipcc 3>;
mbox-names = "shutdown", "detach";
status = "okay";
#address-cells = <1>;
#size-cells = <0>;
vdev@0 {
compatible = "rproc-virtio";
reg = <0>;
virtio,id = <7>; /* RPMSG */
memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdev0buffer>;
mboxes = <&ipcc 0>, <&ipcc 1>;
mbox-names = "vq0", "vq1";
status = "okay";
};
vdev@1 {
compatible = "rproc-virtio";
reg = <1>;
virtio,id = <7>; /*RPMSG */
memory-region = <&vdev1vring0>, <&vdev1vring1>, <&vdev1buffer>;
mboxes = <&ipcc 4>, <&ipcc 5>;
mbox-names = "vq0", "vq1";
status = "okay";
};
};
I have divided the work in 4 steps to simplify the review, This series implements only
the step 1:
step 1: redefine the remoteproc VirtIO device as a platform device
- migrate rvdev management in remoteproc virtio.c,
- create a remotproc virtio config ( can be disabled for platform that not use VirtIO IPC.
step 2: add possibility to declare and prob a VirtIO sub node
- VirtIO bindings declaration,
- multi DT VirtIO devices support,
- introduction of a remote proc virtio bind device mechanism ,
=> https://github.com/arnopo/linux/commits/step2-virtio-in-DT
step 3: Add memory declaration in VirtIO subnode
=> https://github.com/arnopo/linux/commits/step3-virtio-memories
step 4: Add mailbox declaration in VirtIO subnode
=> https://github.com/arnopo/linux/commits/step4-virtio-mailboxes
Arnaud Pouliquen (6):
remoteproc: core: Introduce virtio device add/remove functions
remoteproc: core: Introduce rproc_register_rvdev function
remoteproc: Move rproc_vdev management to remoteproc_virtio.c
remoteproc: virtio: Create platform device for the remoteproc_virtio
remoteproc: virtio: Add helper to create platform device
remoteproc: Instantiate the new remoteproc virtio platform device
drivers/remoteproc/remoteproc_core.c | 145 ++++-------------
drivers/remoteproc/remoteproc_internal.h | 26 ++-
drivers/remoteproc/remoteproc_virtio.c | 195 +++++++++++++++++++++--
include/linux/remoteproc.h | 6 +-
4 files changed, 233 insertions(+), 139 deletions(-)
--
2.17.1
Define a platform driver to prepare for the management of
remoteproc virtio devices as platform devices.
The platform device allows to pass rproc_vdev_data platform data to
specify properties that are stored in the rproc_vdev structure.
Such approach will allow to preserve legacy remoteproc virtio device
creation but also to probe the device using device tree mechanism.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
Update vs previous revision:
- Fix commit and rename rproc_vdev_data to rproc_vdev_pdata
---
drivers/remoteproc/remoteproc_internal.h | 6 +++
drivers/remoteproc/remoteproc_virtio.c | 65 ++++++++++++++++++++++++
include/linux/remoteproc.h | 2 +
3 files changed, 73 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index e9e9a551a8c2..6f511c50a15d 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,12 @@ struct rproc_debug_trace {
struct rproc_mem_entry trace_mem;
};
+struct rproc_vdev_pdata {
+ u32 rsc_offset;
+ unsigned int id;
+ unsigned int index;
+};
+
/* from remoteproc_core.c */
void rproc_release(struct kref *kref);
int rproc_of_parse_firmware(struct device *dev, int index,
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 51d415744fc6..5f8005caeb6e 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -4,6 +4,7 @@
*
* Copyright (C) 2011 Texas Instruments, Inc.
* Copyright (C) 2011 Google, Inc.
+ * Copyright (C) 2021 STMicroelectronics
*
* Ohad Ben-Cohen <[email protected]>
* Brian Swetland <[email protected]>
@@ -13,6 +14,7 @@
#include <linux/dma-map-ops.h>
#include <linux/dma-mapping.h>
#include <linux/export.h>
+#include <linux/of_platform.h>
#include <linux/of_reserved_mem.h>
#include <linux/remoteproc.h>
#include <linux/virtio.h>
@@ -575,3 +577,66 @@ void rproc_vdev_release(struct kref *ref)
rproc_rvdev_remove_device(rvdev);
}
+
+static int rproc_virtio_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rproc_vdev_pdata *vdev_data = dev->platform_data;
+ struct rproc_vdev *rvdev;
+ struct rproc *rproc;
+
+ if (!vdev_data)
+ return -EINVAL;
+
+ rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
+ if (!rvdev)
+ return -ENOMEM;
+
+ rproc = container_of(dev->parent, struct rproc, dev);
+
+ rvdev->rsc_offset = vdev_data->rsc_offset;
+ rvdev->id = vdev_data->id;
+ rvdev->index = vdev_data->index;
+
+ rvdev->pdev = pdev;
+ rvdev->rproc = rproc;
+
+ platform_set_drvdata(pdev, rvdev);
+
+ return rproc_rvdev_add_device(rvdev);
+}
+
+static int rproc_virtio_remove(struct platform_device *pdev)
+{
+ struct rproc_vdev *rvdev = dev_get_drvdata(&pdev->dev);
+ struct rproc *rproc = rvdev->rproc;
+ struct rproc_vring *rvring;
+ int id;
+
+ for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
+ rvring = &rvdev->vring[id];
+ rproc_free_vring(rvring);
+ }
+
+ rproc_remove_subdev(rproc, &rvdev->subdev);
+ rproc_unregister_rvdev(rvdev);
+ dev_dbg(&pdev->dev, "virtio dev %d removed\n", rvdev->index);
+
+ return 0;
+}
+
+/* Platform driver */
+static const struct of_device_id rproc_virtio_match[] = {
+ { .compatible = "rproc-virtio", },
+ {},
+};
+
+static struct platform_driver rproc_virtio_driver = {
+ .probe = rproc_virtio_probe,
+ .remove = rproc_virtio_remove,
+ .driver = {
+ .name = "rproc-virtio",
+ .of_match_table = rproc_virtio_match,
+ },
+};
+builtin_platform_driver(rproc_virtio_driver);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e0600e1e5c17..542a3d4664f2 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -616,6 +616,7 @@ struct rproc_vring {
* struct rproc_vdev - remoteproc state for a supported virtio device
* @refcount: reference counter for the vdev and vring allocations
* @subdev: handle for registering the vdev as a rproc subdevice
+ * @pdev: remoteproc virtio platform device
* @dev: device struct used for reference count semantics
* @id: virtio device id (as in virtio_ids.h)
* @node: list node
@@ -628,6 +629,7 @@ struct rproc_vdev {
struct kref refcount;
struct rproc_subdev subdev;
+ struct platform_device *pdev;
struct device dev;
unsigned int id;
--
2.17.1
Add capability to create platform device for the rproc virtio.
This is a step to move forward the management of the rproc virtio
as an independent device.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/remoteproc/remoteproc_internal.h | 3 ++
drivers/remoteproc/remoteproc_virtio.c | 36 ++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 6f511c50a15d..3007d29a26e1 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -37,6 +37,9 @@ int rproc_of_parse_firmware(struct device *dev, int index,
/* from remoteproc_virtio.c */
int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
+struct platform_device *
+rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_data);
+void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
void rproc_vdev_release(struct kref *ref);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 5f8005caeb6e..5eef679cc520 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -578,6 +578,42 @@ void rproc_vdev_release(struct kref *ref)
rproc_rvdev_remove_device(rvdev);
}
+/**
+ * rproc_virtio_register_device() - register a remoteproc virtio device
+ * @rproc: rproc handle to add the remoteproc virtio device to
+ * @vdev_data: platform device data
+ *
+ * Return: 0 on success, and an appropriate error value otherwise
+ */
+struct platform_device *
+rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_data)
+{
+ struct device *dev = &rproc->dev;
+ struct platform_device *pdev;
+
+ pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
+ sizeof(*vdev_data));
+ if (PTR_ERR_OR_ZERO(pdev)) {
+ dev_err(rproc->dev.parent,
+ "failed to create rproc-virtio device\n");
+ }
+
+ return pdev;
+}
+EXPORT_SYMBOL(rproc_virtio_register_device);
+
+/**
+ * rproc_virtio_unregister_device() - unregister a remoteproc virtio device
+ * @rvdev: remote proc virtio handle to unregister
+ *
+ */
+void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
+{
+ if (rvdev->pdev)
+ platform_device_unregister(rvdev->pdev);
+}
+EXPORT_SYMBOL(rproc_virtio_unregister_device);
+
static int rproc_virtio_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
--
2.17.1
Migrate from the rvdev device to the rvdev platform device.
From this patch, when a vdev resource is found in the resource table
the remoteproc core register a platform device.
All reference to the rvdev->dev has been updated to rvdev-pdev->dev
The use of kref counter is replaced by get/put_device on the remoteproc
virtio device.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
Update vs previous revision:
- rework comments around virtio platform device creation
---
drivers/remoteproc/remoteproc_core.c | 48 +++++++-----
drivers/remoteproc/remoteproc_internal.h | 2 -
drivers/remoteproc/remoteproc_virtio.c | 99 ++++++------------------
include/linux/remoteproc.h | 4 -
4 files changed, 52 insertions(+), 101 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ca2fe8d9cac8..4f332ec0fdd0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -465,6 +465,8 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
{
struct fw_rsc_vdev *rsc = ptr;
struct device *dev = &rproc->dev;
+ struct rproc_vdev_pdata vdev_data;
+ struct platform_device *pdev;
struct rproc_vdev *rvdev;
int i, ret;
@@ -484,25 +486,36 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
dev_dbg(dev, "vdev rsc: id %d, dfeatures 0x%x, cfg len %d, %d vrings\n",
rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
- /* we currently support only two vrings per rvdev */
- if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
- dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
- return -EINVAL;
- }
+ /* Register a remoteproc virtio platform device in charge of the virtio device */
+ vdev_data.rsc_offset = offset;
+ vdev_data.id = rsc->id;
+ vdev_data.index = rproc->nb_vdev;
- rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
- if (!rvdev)
- return -ENOMEM;
+ pdev = rproc_virtio_register_device(rproc, &vdev_data);
+ if (PTR_ERR_OR_ZERO(pdev)) {
+ dev_err(rproc->dev.parent,
+ "failed to create rproc-virtio device\n");
+ return PTR_ERR_OR_ZERO(pdev);
+ }
- kref_init(&rvdev->refcount);
+ /*
+ * At this point the registered remoteproc virtio platform device should has been probed.
+ * Get the associated rproc_vdev struct to assign the vrings
+ */
+ rvdev = platform_get_drvdata(pdev);
+ if (WARN_ON(!rvdev)) {
+ ret = -EINVAL;
+ goto free_rvdev;
+ }
- rvdev->id = rsc->id;
- rvdev->rproc = rproc;
- rvdev->index = rproc->nb_vdev++;
+ rproc->nb_vdev++;
- ret = rproc_rvdev_add_device(rvdev);
- if (ret)
+ /* we currently support only two vrings per rvdev */
+ if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
+ dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
+ ret = -EINVAL;
goto free_rvdev;
+ }
/* parse the vrings */
for (i = 0; i < rsc->num_of_vrings; i++) {
@@ -511,9 +524,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
goto free_rvdev;
}
- /* remember the resource offset*/
- rvdev->rsc_offset = offset;
-
/* allocate the vring resources */
for (i = 0; i < rsc->num_of_vrings; i++) {
ret = rproc_alloc_vring(rvdev, i);
@@ -527,7 +537,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
for (i--; i >= 0; i--)
rproc_free_vring(&rvdev->vring[i]);
free_rvdev:
- device_unregister(&rvdev->dev);
+ rproc_virtio_unregister_device(rvdev);
return ret;
}
@@ -1266,7 +1276,7 @@ void rproc_resource_cleanup(struct rproc *rproc)
/* clean up remote vdev entries */
list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
- kref_put(&rvdev->refcount, rproc_vdev_release);
+ rproc_virtio_unregister_device(rvdev);
rproc_coredump_cleanup(rproc);
}
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 3007d29a26e1..1479c5e33e2e 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -36,12 +36,10 @@ int rproc_of_parse_firmware(struct device *dev, int index,
const char **fw_name);
/* from remoteproc_virtio.c */
-int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
struct platform_device *
rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_data);
void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
-void rproc_vdev_release(struct kref *ref);
/* from remoteproc_debugfs.c */
void rproc_remove_trace_file(struct dentry *tfile);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 5eef679cc520..55e797095bfe 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -48,7 +48,11 @@ static int copy_dma_range_map(struct device *to, struct device *from)
static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
{
- return container_of(vdev->dev.parent, struct rproc_vdev, dev);
+ struct platform_device *pdev;
+
+ pdev = container_of(vdev->dev.parent, struct platform_device, dev);
+
+ return platform_get_drvdata(pdev);
}
static struct rproc *vdev_to_rproc(struct virtio_device *vdev)
@@ -343,13 +347,10 @@ static void rproc_virtio_dev_release(struct device *dev)
{
struct virtio_device *vdev = dev_to_virtio(dev);
struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
- struct rproc *rproc = vdev_to_rproc(vdev);
kfree(vdev);
- kref_put(&rvdev->refcount, rproc_vdev_release);
-
- put_device(&rproc->dev);
+ put_device(&rvdev->pdev->dev);
}
/**
@@ -365,7 +366,7 @@ static void rproc_virtio_dev_release(struct device *dev)
static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
{
struct rproc *rproc = rvdev->rproc;
- struct device *dev = &rvdev->dev;
+ struct device *dev = &rvdev->pdev->dev;
struct virtio_device *vdev;
struct rproc_mem_entry *mem;
int ret;
@@ -436,17 +437,15 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
vdev->dev.release = rproc_virtio_dev_release;
/*
- * We're indirectly making a non-temporary copy of the rproc pointer
+ * We're indirectly making a non-temporary copy of the rvdev pointer
* here, because drivers probed with this vdev will indirectly
* access the wrapping rproc.
*
- * Therefore we must increment the rproc refcount here, and decrement
+ * Therefore we must increment the rvdev refcount here, and decrement
* it _only_ when the vdev is released.
*/
- get_device(&rproc->dev);
+ get_device(dev);
- /* Reference the vdev and vring allocations */
- kref_get(&rvdev->refcount);
ret = register_virtio_device(vdev);
if (ret) {
@@ -488,56 +487,30 @@ static int rproc_vdev_do_start(struct rproc_subdev *subdev)
static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
{
struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
+ struct device *dev = &rvdev->pdev->dev;
int ret;
- ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
+ ret = device_for_each_child(dev, NULL, rproc_remove_virtio_dev);
if (ret)
- dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
-}
-
-/**
- * rproc_rvdev_release() - release the existence of a rvdev
- *
- * @dev: the subdevice's dev
- */
-static void rproc_rvdev_release(struct device *dev)
-{
- struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
-
- of_reserved_mem_device_release(dev);
-
- kfree(rvdev);
+ dev_warn(dev, "can't remove vdev child device: %d\n", ret);
}
-int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
+static int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
{
struct rproc *rproc = rvdev->rproc;
- char name[16];
+ struct device *dev = &rvdev->pdev->dev;
int ret;
- snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
- rvdev->dev.parent = &rproc->dev;
- rvdev->dev.release = rproc_rvdev_release;
- dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
- dev_set_drvdata(&rvdev->dev, rvdev);
-
- ret = device_register(&rvdev->dev);
- if (ret) {
- put_device(&rvdev->dev);
- return ret;
- }
- ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent);
+ ret = copy_dma_range_map(dev, rproc->dev.parent);
if (ret)
- goto free_rvdev;
+ return ret;
/* Make device dma capable by inheriting from parent's capabilities */
- set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent));
+ set_dma_ops(dev, get_dma_ops(rproc->dev.parent));
- ret = dma_coerce_mask_and_coherent(&rvdev->dev,
- dma_get_mask(rproc->dev.parent));
+ ret = dma_coerce_mask_and_coherent(dev, dma_get_mask(rproc->dev.parent));
if (ret) {
- dev_warn(&rvdev->dev,
- "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
+ dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
dma_get_mask(rproc->dev.parent), ERR_PTR(ret));
}
@@ -548,34 +521,9 @@ int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
rproc_add_subdev(rproc, &rvdev->subdev);
- return 0;
+ dev_dbg(dev, "virtio dev %d added\n", rvdev->index);
-free_rvdev:
- device_unregister(&rvdev->dev);
- return ret;
-}
-
-static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev)
-{
- struct rproc *rproc = rvdev->rproc;
-
- rproc_remove_subdev(rproc, &rvdev->subdev);
- rproc_unregister_rvdev(rvdev);
- device_unregister(&rvdev->dev);
-}
-
-void rproc_vdev_release(struct kref *ref)
-{
- struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
- struct rproc_vring *rvring;
- int id;
-
- for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
- rvring = &rvdev->vring[id];
- rproc_free_vring(rvring);
- }
-
- rproc_rvdev_remove_device(rvdev);
+ return 0;
}
/**
@@ -594,8 +542,7 @@ rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_
pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
sizeof(*vdev_data));
if (PTR_ERR_OR_ZERO(pdev)) {
- dev_err(rproc->dev.parent,
- "failed to create rproc-virtio device\n");
+ dev_err(rproc->dev.parent, "failed to create rproc-virtio device\n");
}
return pdev;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 542a3d4664f2..7951a3e2b62a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -614,10 +614,8 @@ struct rproc_vring {
/**
* struct rproc_vdev - remoteproc state for a supported virtio device
- * @refcount: reference counter for the vdev and vring allocations
* @subdev: handle for registering the vdev as a rproc subdevice
* @pdev: remoteproc virtio platform device
- * @dev: device struct used for reference count semantics
* @id: virtio device id (as in virtio_ids.h)
* @node: list node
* @rproc: the rproc handle
@@ -626,11 +624,9 @@ struct rproc_vring {
* @index: vdev position versus other vdev declared in resource table
*/
struct rproc_vdev {
- struct kref refcount;
struct rproc_subdev subdev;
struct platform_device *pdev;
- struct device dev;
unsigned int id;
struct list_head node;
--
2.17.1
Hi Arnaud,
On Wed, Dec 22, 2021 at 09:23:48AM +0100, Arnaud Pouliquen wrote:
> Add capability to create platform device for the rproc virtio.
> This is a step to move forward the management of the rproc virtio
> as an independent device.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/remoteproc/remoteproc_internal.h | 3 ++
> drivers/remoteproc/remoteproc_virtio.c | 36 ++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 6f511c50a15d..3007d29a26e1 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -37,6 +37,9 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>
> /* from remoteproc_virtio.c */
> int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
> +struct platform_device *
> +rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_data);
> +void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> void rproc_vdev_release(struct kref *ref);
>
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 5f8005caeb6e..5eef679cc520 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -578,6 +578,42 @@ void rproc_vdev_release(struct kref *ref)
> rproc_rvdev_remove_device(rvdev);
> }
>
> +/**
> + * rproc_virtio_register_device() - register a remoteproc virtio device
> + * @rproc: rproc handle to add the remoteproc virtio device to
> + * @vdev_data: platform device data
> + *
> + * Return: 0 on success, and an appropriate error value otherwise
> + */
> +struct platform_device *
> +rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_data)
> +{
> + struct device *dev = &rproc->dev;
> + struct platform_device *pdev;
> +
> + pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
> + sizeof(*vdev_data));
> + if (PTR_ERR_OR_ZERO(pdev)) {
Can you expand on the reason to use PTR_ERR_OR_ZERO() rather than IS_ERR()?
Looking at the documentation for platform_device_register_data(), it should not
return 0...
> + dev_err(rproc->dev.parent,
> + "failed to create rproc-virtio device\n");
> + }
> +
> + return pdev;
> +}
> +EXPORT_SYMBOL(rproc_virtio_register_device);
> +
> +/**
> + * rproc_virtio_unregister_device() - unregister a remoteproc virtio device
> + * @rvdev: remote proc virtio handle to unregister
> + *
> + */
> +void rproc_virtio_unregister_device(struct rproc_vdev *rvdev)
> +{
> + if (rvdev->pdev)
> + platform_device_unregister(rvdev->pdev);
> +}
> +EXPORT_SYMBOL(rproc_virtio_unregister_device);
> +
> static int rproc_virtio_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> --
> 2.17.1
>
On Wed, Dec 22, 2021 at 09:23:49AM +0100, Arnaud Pouliquen wrote:
> Migrate from the rvdev device to the rvdev platform device.
> From this patch, when a vdev resource is found in the resource table
> the remoteproc core register a platform device.
>
> All reference to the rvdev->dev has been updated to rvdev-pdev->dev
>
> The use of kref counter is replaced by get/put_device on the remoteproc
> virtio device.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
>
> ---
> Update vs previous revision:
> - rework comments around virtio platform device creation
> ---
> drivers/remoteproc/remoteproc_core.c | 48 +++++++-----
> drivers/remoteproc/remoteproc_internal.h | 2 -
> drivers/remoteproc/remoteproc_virtio.c | 99 ++++++------------------
> include/linux/remoteproc.h | 4 -
> 4 files changed, 52 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index ca2fe8d9cac8..4f332ec0fdd0 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -465,6 +465,8 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> {
> struct fw_rsc_vdev *rsc = ptr;
> struct device *dev = &rproc->dev;
> + struct rproc_vdev_pdata vdev_data;
> + struct platform_device *pdev;
> struct rproc_vdev *rvdev;
> int i, ret;
>
> @@ -484,25 +486,36 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> dev_dbg(dev, "vdev rsc: id %d, dfeatures 0x%x, cfg len %d, %d vrings\n",
> rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
>
> - /* we currently support only two vrings per rvdev */
> - if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
> - dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
> - return -EINVAL;
> - }
> + /* Register a remoteproc virtio platform device in charge of the virtio device */
> + vdev_data.rsc_offset = offset;
> + vdev_data.id = rsc->id;
> + vdev_data.index = rproc->nb_vdev;
>
> - rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
> - if (!rvdev)
> - return -ENOMEM;
> + pdev = rproc_virtio_register_device(rproc, &vdev_data);
> + if (PTR_ERR_OR_ZERO(pdev)) {
> + dev_err(rproc->dev.parent,
> + "failed to create rproc-virtio device\n");
> + return PTR_ERR_OR_ZERO(pdev);
The error check and dev_err() message is exactly the same as what is found in
rproc_virtio_register_device(). As such two error messages will be printed out
for every single failure. I would call platform_device_register_data() directly
rather than keeping rproc_virtio_register_device() around.
> + }
>
> - kref_init(&rvdev->refcount);
> + /*
> + * At this point the registered remoteproc virtio platform device should has been probed.
s/has been/have been
> + * Get the associated rproc_vdev struct to assign the vrings
> + */
> + rvdev = platform_get_drvdata(pdev);
> + if (WARN_ON(!rvdev)) {
> + ret = -EINVAL;
> + goto free_rvdev;
> + }
>
> - rvdev->id = rsc->id;
> - rvdev->rproc = rproc;
> - rvdev->index = rproc->nb_vdev++;
> + rproc->nb_vdev++;
>
> - ret = rproc_rvdev_add_device(rvdev);
> - if (ret)
> + /* we currently support only two vrings per rvdev */
> + if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
> + dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
> + ret = -EINVAL;
> goto free_rvdev;
> + }
>
> /* parse the vrings */
> for (i = 0; i < rsc->num_of_vrings; i++) {
> @@ -511,9 +524,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> goto free_rvdev;
> }
>
> - /* remember the resource offset*/
> - rvdev->rsc_offset = offset;
> -
> /* allocate the vring resources */
> for (i = 0; i < rsc->num_of_vrings; i++) {
> ret = rproc_alloc_vring(rvdev, i);
> @@ -527,7 +537,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> for (i--; i >= 0; i--)
> rproc_free_vring(&rvdev->vring[i]);
> free_rvdev:
> - device_unregister(&rvdev->dev);
> + rproc_virtio_unregister_device(rvdev);
Here too I wouldn't bother with a new function - I would simply call
platform_device_unregister().
> return ret;
> }
>
> @@ -1266,7 +1276,7 @@ void rproc_resource_cleanup(struct rproc *rproc)
>
> /* clean up remote vdev entries */
> list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> - kref_put(&rvdev->refcount, rproc_vdev_release);
> + rproc_virtio_unregister_device(rvdev);
Same as above.
>
> rproc_coredump_cleanup(rproc);
> }
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 3007d29a26e1..1479c5e33e2e 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -36,12 +36,10 @@ int rproc_of_parse_firmware(struct device *dev, int index,
> const char **fw_name);
>
> /* from remoteproc_virtio.c */
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
> struct platform_device *
> rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_data);
> void rproc_virtio_unregister_device(struct rproc_vdev *rvdev);
> irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> -void rproc_vdev_release(struct kref *ref);
>
> /* from remoteproc_debugfs.c */
> void rproc_remove_trace_file(struct dentry *tfile);
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 5eef679cc520..55e797095bfe 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -48,7 +48,11 @@ static int copy_dma_range_map(struct device *to, struct device *from)
>
> static struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
> {
> - return container_of(vdev->dev.parent, struct rproc_vdev, dev);
> + struct platform_device *pdev;
> +
> + pdev = container_of(vdev->dev.parent, struct platform_device, dev);
> +
> + return platform_get_drvdata(pdev);
> }
>
> static struct rproc *vdev_to_rproc(struct virtio_device *vdev)
> @@ -343,13 +347,10 @@ static void rproc_virtio_dev_release(struct device *dev)
> {
> struct virtio_device *vdev = dev_to_virtio(dev);
> struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
> - struct rproc *rproc = vdev_to_rproc(vdev);
>
> kfree(vdev);
>
> - kref_put(&rvdev->refcount, rproc_vdev_release);
> -
> - put_device(&rproc->dev);
> + put_device(&rvdev->pdev->dev);
> }
>
> /**
> @@ -365,7 +366,7 @@ static void rproc_virtio_dev_release(struct device *dev)
> static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> {
> struct rproc *rproc = rvdev->rproc;
> - struct device *dev = &rvdev->dev;
> + struct device *dev = &rvdev->pdev->dev;
> struct virtio_device *vdev;
> struct rproc_mem_entry *mem;
> int ret;
> @@ -436,17 +437,15 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> vdev->dev.release = rproc_virtio_dev_release;
>
> /*
> - * We're indirectly making a non-temporary copy of the rproc pointer
> + * We're indirectly making a non-temporary copy of the rvdev pointer
> * here, because drivers probed with this vdev will indirectly
> * access the wrapping rproc.
> *
> - * Therefore we must increment the rproc refcount here, and decrement
> + * Therefore we must increment the rvdev refcount here, and decrement
> * it _only_ when the vdev is released.
> */
> - get_device(&rproc->dev);
> + get_device(dev);
>
> - /* Reference the vdev and vring allocations */
> - kref_get(&rvdev->refcount);
>
> ret = register_virtio_device(vdev);
> if (ret) {
> @@ -488,56 +487,30 @@ static int rproc_vdev_do_start(struct rproc_subdev *subdev)
> static void rproc_vdev_do_stop(struct rproc_subdev *subdev, bool crashed)
> {
> struct rproc_vdev *rvdev = container_of(subdev, struct rproc_vdev, subdev);
> + struct device *dev = &rvdev->pdev->dev;
> int ret;
>
> - ret = device_for_each_child(&rvdev->dev, NULL, rproc_remove_virtio_dev);
> + ret = device_for_each_child(dev, NULL, rproc_remove_virtio_dev);
> if (ret)
> - dev_warn(&rvdev->dev, "can't remove vdev child device: %d\n", ret);
> -}
> -
> -/**
> - * rproc_rvdev_release() - release the existence of a rvdev
> - *
> - * @dev: the subdevice's dev
> - */
> -static void rproc_rvdev_release(struct device *dev)
> -{
> - struct rproc_vdev *rvdev = container_of(dev, struct rproc_vdev, dev);
> -
> - of_reserved_mem_device_release(dev);
> -
> - kfree(rvdev);
> + dev_warn(dev, "can't remove vdev child device: %d\n", ret);
> }
>
> -int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> +static int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> {
> struct rproc *rproc = rvdev->rproc;
> - char name[16];
> + struct device *dev = &rvdev->pdev->dev;
> int ret;
>
> - snprintf(name, sizeof(name), "vdev%dbuffer", rvdev->index);
> - rvdev->dev.parent = &rproc->dev;
> - rvdev->dev.release = rproc_rvdev_release;
> - dev_set_name(&rvdev->dev, "%s#%s", dev_name(rvdev->dev.parent), name);
> - dev_set_drvdata(&rvdev->dev, rvdev);
> -
> - ret = device_register(&rvdev->dev);
> - if (ret) {
> - put_device(&rvdev->dev);
> - return ret;
> - }
> - ret = copy_dma_range_map(&rvdev->dev, rproc->dev.parent);
> + ret = copy_dma_range_map(dev, rproc->dev.parent);
> if (ret)
> - goto free_rvdev;
> + return ret;
>
> /* Make device dma capable by inheriting from parent's capabilities */
> - set_dma_ops(&rvdev->dev, get_dma_ops(rproc->dev.parent));
> + set_dma_ops(dev, get_dma_ops(rproc->dev.parent));
>
> - ret = dma_coerce_mask_and_coherent(&rvdev->dev,
> - dma_get_mask(rproc->dev.parent));
> + ret = dma_coerce_mask_and_coherent(dev, dma_get_mask(rproc->dev.parent));
> if (ret) {
> - dev_warn(&rvdev->dev,
> - "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
> + dev_warn(dev, "Failed to set DMA mask %llx. Trying to continue... (%pe)\n",
> dma_get_mask(rproc->dev.parent), ERR_PTR(ret));
> }
>
> @@ -548,34 +521,9 @@ int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
>
> rproc_add_subdev(rproc, &rvdev->subdev);
>
> - return 0;
> + dev_dbg(dev, "virtio dev %d added\n", rvdev->index);
>
> -free_rvdev:
> - device_unregister(&rvdev->dev);
> - return ret;
> -}
> -
> -static void rproc_rvdev_remove_device(struct rproc_vdev *rvdev)
> -{
> - struct rproc *rproc = rvdev->rproc;
> -
> - rproc_remove_subdev(rproc, &rvdev->subdev);
> - rproc_unregister_rvdev(rvdev);
> - device_unregister(&rvdev->dev);
> -}
> -
> -void rproc_vdev_release(struct kref *ref)
> -{
> - struct rproc_vdev *rvdev = container_of(ref, struct rproc_vdev, refcount);
> - struct rproc_vring *rvring;
> - int id;
> -
> - for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> - rvring = &rvdev->vring[id];
> - rproc_free_vring(rvring);
> - }
> -
> - rproc_rvdev_remove_device(rvdev);
> + return 0;
> }
>
> /**
> @@ -594,8 +542,7 @@ rproc_virtio_register_device(struct rproc *rproc, struct rproc_vdev_pdata *vdev_
> pdev = platform_device_register_data(dev, "rproc-virtio", vdev_data->index, vdev_data,
> sizeof(*vdev_data));
> if (PTR_ERR_OR_ZERO(pdev)) {
> - dev_err(rproc->dev.parent,
> - "failed to create rproc-virtio device\n");
> + dev_err(rproc->dev.parent, "failed to create rproc-virtio device\n");
> }
>
> return pdev;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 542a3d4664f2..7951a3e2b62a 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -614,10 +614,8 @@ struct rproc_vring {
>
> /**
> * struct rproc_vdev - remoteproc state for a supported virtio device
> - * @refcount: reference counter for the vdev and vring allocations
> * @subdev: handle for registering the vdev as a rproc subdevice
> * @pdev: remoteproc virtio platform device
> - * @dev: device struct used for reference count semantics
> * @id: virtio device id (as in virtio_ids.h)
> * @node: list node
> * @rproc: the rproc handle
> @@ -626,11 +624,9 @@ struct rproc_vring {
> * @index: vdev position versus other vdev declared in resource table
> */
> struct rproc_vdev {
> - struct kref refcount;
>
> struct rproc_subdev subdev;
> struct platform_device *pdev;
> - struct device dev;
>
> unsigned int id;
> struct list_head node;
> --
> 2.17.1
>
On Wed, Dec 22, 2021 at 09:23:47AM +0100, Arnaud Pouliquen wrote:
> Define a platform driver to prepare for the management of
> remoteproc virtio devices as platform devices.
>
> The platform device allows to pass rproc_vdev_data platform data to
> specify properties that are stored in the rproc_vdev structure.
>
> Such approach will allow to preserve legacy remoteproc virtio device
> creation but also to probe the device using device tree mechanism.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> Update vs previous revision:
> - Fix commit and rename rproc_vdev_data to rproc_vdev_pdata
> ---
> drivers/remoteproc/remoteproc_internal.h | 6 +++
> drivers/remoteproc/remoteproc_virtio.c | 65 ++++++++++++++++++++++++
> include/linux/remoteproc.h | 2 +
> 3 files changed, 73 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index e9e9a551a8c2..6f511c50a15d 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,12 @@ struct rproc_debug_trace {
> struct rproc_mem_entry trace_mem;
> };
>
> +struct rproc_vdev_pdata {
> + u32 rsc_offset;
> + unsigned int id;
> + unsigned int index;
> +};
> +
> /* from remoteproc_core.c */
> void rproc_release(struct kref *kref);
> int rproc_of_parse_firmware(struct device *dev, int index,
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 51d415744fc6..5f8005caeb6e 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -4,6 +4,7 @@
> *
> * Copyright (C) 2011 Texas Instruments, Inc.
> * Copyright (C) 2011 Google, Inc.
> + * Copyright (C) 2021 STMicroelectronics
> *
> * Ohad Ben-Cohen <[email protected]>
> * Brian Swetland <[email protected]>
> @@ -13,6 +14,7 @@
> #include <linux/dma-map-ops.h>
> #include <linux/dma-mapping.h>
> #include <linux/export.h>
> +#include <linux/of_platform.h>
> #include <linux/of_reserved_mem.h>
> #include <linux/remoteproc.h>
> #include <linux/virtio.h>
> @@ -575,3 +577,66 @@ void rproc_vdev_release(struct kref *ref)
>
> rproc_rvdev_remove_device(rvdev);
> }
> +
> +static int rproc_virtio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rproc_vdev_pdata *vdev_data = dev->platform_data;
> + struct rproc_vdev *rvdev;
> + struct rproc *rproc;
> +
> + if (!vdev_data)
> + return -EINVAL;
> +
> + rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
> + if (!rvdev)
> + return -ENOMEM;
> +
> + rproc = container_of(dev->parent, struct rproc, dev);
> +
> + rvdev->rsc_offset = vdev_data->rsc_offset;
> + rvdev->id = vdev_data->id;
> + rvdev->index = vdev_data->index;
> +
> + rvdev->pdev = pdev;
> + rvdev->rproc = rproc;
> +
> + platform_set_drvdata(pdev, rvdev);
> +
> + return rproc_rvdev_add_device(rvdev);
> +}
> +
> +static int rproc_virtio_remove(struct platform_device *pdev)
> +{
> + struct rproc_vdev *rvdev = dev_get_drvdata(&pdev->dev);
> + struct rproc *rproc = rvdev->rproc;
> + struct rproc_vring *rvring;
> + int id;
> +
> + for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> + rvring = &rvdev->vring[id];
> + rproc_free_vring(rvring);
> + }
> +
> + rproc_remove_subdev(rproc, &rvdev->subdev);
> + rproc_unregister_rvdev(rvdev);
> + dev_dbg(&pdev->dev, "virtio dev %d removed\n", rvdev->index);
> +
Function rproc_virtio_remove() doesn't do the opposite of rproc_virtio_probe(),
making it hard for people to wrap their head around what is happening. This may
get cleaned up as part of the error path problem we already talked about... If not
this is something to improve one.
I am done reviewing this set.
Thanks,
Mathieu
> + return 0;
> +}
> +
> +/* Platform driver */
> +static const struct of_device_id rproc_virtio_match[] = {
> + { .compatible = "rproc-virtio", },
> + {},
> +};
> +
> +static struct platform_driver rproc_virtio_driver = {
> + .probe = rproc_virtio_probe,
> + .remove = rproc_virtio_remove,
> + .driver = {
> + .name = "rproc-virtio",
> + .of_match_table = rproc_virtio_match,
> + },
> +};
> +builtin_platform_driver(rproc_virtio_driver);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..542a3d4664f2 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -616,6 +616,7 @@ struct rproc_vring {
> * struct rproc_vdev - remoteproc state for a supported virtio device
> * @refcount: reference counter for the vdev and vring allocations
> * @subdev: handle for registering the vdev as a rproc subdevice
> + * @pdev: remoteproc virtio platform device
> * @dev: device struct used for reference count semantics
> * @id: virtio device id (as in virtio_ids.h)
> * @node: list node
> @@ -628,6 +629,7 @@ struct rproc_vdev {
> struct kref refcount;
>
> struct rproc_subdev subdev;
> + struct platform_device *pdev;
> struct device dev;
>
> unsigned int id;
> --
> 2.17.1
>
Hello Mathieu,
On 1/6/22 7:53 PM, Mathieu Poirier wrote:
> On Wed, Dec 22, 2021 at 09:23:47AM +0100, Arnaud Pouliquen wrote:
>> Define a platform driver to prepare for the management of
>> remoteproc virtio devices as platform devices.
>>
>> The platform device allows to pass rproc_vdev_data platform data to
>> specify properties that are stored in the rproc_vdev structure.
>>
>> Such approach will allow to preserve legacy remoteproc virtio device
>> creation but also to probe the device using device tree mechanism.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> Update vs previous revision:
>> - Fix commit and rename rproc_vdev_data to rproc_vdev_pdata
>> ---
>> drivers/remoteproc/remoteproc_internal.h | 6 +++
>> drivers/remoteproc/remoteproc_virtio.c | 65 ++++++++++++++++++++++++
>> include/linux/remoteproc.h | 2 +
>> 3 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>> index e9e9a551a8c2..6f511c50a15d 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -24,6 +24,12 @@ struct rproc_debug_trace {
>> struct rproc_mem_entry trace_mem;
>> };
>>
>> +struct rproc_vdev_pdata {
>> + u32 rsc_offset;
>> + unsigned int id;
>> + unsigned int index;
>> +};
>> +
>> /* from remoteproc_core.c */
>> void rproc_release(struct kref *kref);
>> int rproc_of_parse_firmware(struct device *dev, int index,
>> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
>> index 51d415744fc6..5f8005caeb6e 100644
>> --- a/drivers/remoteproc/remoteproc_virtio.c
>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>> @@ -4,6 +4,7 @@
>> *
>> * Copyright (C) 2011 Texas Instruments, Inc.
>> * Copyright (C) 2011 Google, Inc.
>> + * Copyright (C) 2021 STMicroelectronics
>> *
>> * Ohad Ben-Cohen <[email protected]>
>> * Brian Swetland <[email protected]>
>> @@ -13,6 +14,7 @@
>> #include <linux/dma-map-ops.h>
>> #include <linux/dma-mapping.h>
>> #include <linux/export.h>
>> +#include <linux/of_platform.h>
>> #include <linux/of_reserved_mem.h>
>> #include <linux/remoteproc.h>
>> #include <linux/virtio.h>
>> @@ -575,3 +577,66 @@ void rproc_vdev_release(struct kref *ref)
>>
>> rproc_rvdev_remove_device(rvdev);
>> }
>> +
>> +static int rproc_virtio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct rproc_vdev_pdata *vdev_data = dev->platform_data;
>> + struct rproc_vdev *rvdev;
>> + struct rproc *rproc;
>> +
>> + if (!vdev_data)
>> + return -EINVAL;
>> +
>> + rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
>> + if (!rvdev)
>> + return -ENOMEM;
>> +
>> + rproc = container_of(dev->parent, struct rproc, dev);
>> +
>> + rvdev->rsc_offset = vdev_data->rsc_offset;
>> + rvdev->id = vdev_data->id;
>> + rvdev->index = vdev_data->index;
>> +
>> + rvdev->pdev = pdev;
>> + rvdev->rproc = rproc;
>> +
>> + platform_set_drvdata(pdev, rvdev);
>> +
>> + return rproc_rvdev_add_device(rvdev);
>> +}
>> +
>> +static int rproc_virtio_remove(struct platform_device *pdev)
>> +{
>> + struct rproc_vdev *rvdev = dev_get_drvdata(&pdev->dev);
>> + struct rproc *rproc = rvdev->rproc;
>> + struct rproc_vring *rvring;
>> + int id;
>> +
>> + for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
>> + rvring = &rvdev->vring[id];
>> + rproc_free_vring(rvring);
>> + }
>> +
>> + rproc_remove_subdev(rproc, &rvdev->subdev);
>> + rproc_unregister_rvdev(rvdev);
>> + dev_dbg(&pdev->dev, "virtio dev %d removed\n", rvdev->index);
>> +
>
> Function rproc_virtio_remove() doesn't do the opposite of rproc_virtio_probe(),
> making it hard for people to wrap their head around what is happening. This may
> get cleaned up as part of the error path problem we already talked about... If not
> this is something to improve one.
>
> I am done reviewing this set.
Thanks for the review. I will address all your points in next version.
Regards,
Arnaud
>
> Thanks,
> Mathieu
>
>> + return 0;
>> +}
>> +
>> +/* Platform driver */
>> +static const struct of_device_id rproc_virtio_match[] = {
>> + { .compatible = "rproc-virtio", },
>> + {},
>> +};
>> +
>> +static struct platform_driver rproc_virtio_driver = {
>> + .probe = rproc_virtio_probe,
>> + .remove = rproc_virtio_remove,
>> + .driver = {
>> + .name = "rproc-virtio",
>> + .of_match_table = rproc_virtio_match,
>> + },
>> +};
>> +builtin_platform_driver(rproc_virtio_driver);
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index e0600e1e5c17..542a3d4664f2 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -616,6 +616,7 @@ struct rproc_vring {
>> * struct rproc_vdev - remoteproc state for a supported virtio device
>> * @refcount: reference counter for the vdev and vring allocations
>> * @subdev: handle for registering the vdev as a rproc subdevice
>> + * @pdev: remoteproc virtio platform device
>> * @dev: device struct used for reference count semantics
>> * @id: virtio device id (as in virtio_ids.h)
>> * @node: list node
>> @@ -628,6 +629,7 @@ struct rproc_vdev {
>> struct kref refcount;
>>
>> struct rproc_subdev subdev;
>> + struct platform_device *pdev;
>> struct device dev;
>>
>> unsigned int id;
>> --
>> 2.17.1
>>