Update from V2 [1]:
In order to better handle error cases and to have something more symmetrical between
the functions in charge of rvdev initialization/deletion, the patchset has been reworked.
- Introduction in the first patch, of rproc_vdev_data structure which allows to better
decorrelate the rproc from the management of the rvdev structure. This structure is reused
in the last patch of the series for the creation of the remoteproc virtio platform device.
- In addition to the previous version, the management of the vring lifecycle has been fully
migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring, rproc_free_vring)
[1] https://lkml.org/lkml/2021/12/22/111
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 (4):
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
drivers/remoteproc/remoteproc_core.c | 159 +++----------------
drivers/remoteproc/remoteproc_internal.h | 33 +++-
drivers/remoteproc/remoteproc_virtio.c | 193 ++++++++++++++++++++---
include/linux/remoteproc.h | 6 +-
4 files changed, 227 insertions(+), 164 deletions(-)
--
2.25.1
Define a platform driver to manage the remoteproc virtio device as
a 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.
remoteproc_virtio.c update:
- Add rproc_virtio_driver platform driver. The probe/remove ops replace
the rproc_rvdev_add_device/rproc_rvdev_remove_device functions.
- All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
- rproc_rvdev_release is removed as associated to the rvdev device.
- The use of rvdev->kref counter is replaced by get/put_device on the
remoteproc virtio platform device.
- The vdev device no longer increments rproc device counter.
increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
function in charge of the vrings allocation/free.
remoteproc_core.c update:
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.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
Update vs previous revision:
- squash following two patches
- [4/6] remoteproc: virtio: Create platform device for the remoteproc_virtio
https://lkml.org/lkml/2021/12/22/112
- [6/6] remoteproc: Instantiate the new remoteproc virtio platform device
https://lkml.org/lkml/2021/12/22/114
---
drivers/remoteproc/remoteproc_core.c | 23 +++-
drivers/remoteproc/remoteproc_internal.h | 3 -
drivers/remoteproc/remoteproc_virtio.c | 151 +++++++++++------------
include/linux/remoteproc.h | 6 +-
4 files changed, 93 insertions(+), 90 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eb6b43b71c2b..5b864c9c6244 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -467,6 +467,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
struct device *dev = &rproc->dev;
struct rproc_vdev *rvdev;
struct rproc_vdev_data rvdev_data;
+ struct platform_device *pdev;
/* make sure resource isn't truncated */
if (struct_size(rsc, vring, rsc->num_of_vrings) + rsc->config_len >
@@ -495,9 +496,23 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
rvdev_data.rsc_offset = offset;
rvdev_data.rsc = rsc;
- rvdev = rproc_rvdev_add_device(rproc, &rvdev_data);
- if (IS_ERR(rvdev))
- return PTR_ERR(rvdev);
+ pdev = platform_device_register_data(dev, "rproc-virtio", rvdev_data.index, &rvdev_data,
+ sizeof(rvdev_data));
+ if (IS_ERR(pdev)) {
+ dev_err(rproc->dev.parent,
+ "failed to create rproc-virtio device\n");
+ return PTR_ERR(pdev);
+ }
+
+ /*
+ * At this point the registered remoteproc virtio platform device should have been probed.
+ * Get the associated rproc_vdev struct to assign the vrings.
+ */
+ rvdev = platform_get_drvdata(pdev);
+ if (!rvdev) {
+ platform_device_unregister(pdev);
+ return -EINVAL;
+ }
return 0;
}
@@ -1237,7 +1252,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);
+ platform_device_unregister(rvdev->pdev);
rproc_coredump_cleanup(rproc);
}
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 7725b404afc6..175e64a1f3a1 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -45,10 +45,7 @@ int rproc_of_parse_firmware(struct device *dev, int index,
const char **fw_name);
/* from remoteproc_virtio.c */
-struct rproc_vdev *rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data);
-void rproc_rvdev_remove_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 1c4fd79ac1c5..de9f12fcd044 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -13,6 +13,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>
@@ -46,7 +47,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)
@@ -341,13 +346,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);
}
/**
@@ -363,7 +365,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;
@@ -433,18 +435,8 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
vdev->dev.parent = dev;
vdev->dev.release = rproc_virtio_dev_release;
- /*
- * We're indirectly making a non-temporary copy of the rproc pointer
- * here, because drivers probed with this vdev will indirectly
- * access the wrapping rproc.
- *
- * Therefore we must increment the rproc refcount here, and decrement
- * it _only_ when the vdev is released.
- */
- get_device(&rproc->dev);
-
/* Reference the vdev and vring allocations */
- kref_get(&rvdev->refcount);
+ get_device(dev);
ret = register_virtio_device(vdev);
if (ret) {
@@ -486,78 +478,57 @@ 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);
}
-struct rproc_vdev *
-rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
+static int rproc_virtio_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
+ struct rproc_vdev_data *rvdev_data = dev->platform_data;
struct rproc_vdev *rvdev;
- struct fw_rsc_vdev *rsc = rvdev_data->rsc;
- char name[16];
+ struct rproc *rproc = container_of(dev->parent, struct rproc, dev);
+ struct fw_rsc_vdev *rsc;
int i, ret;
- rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
- if (!rvdev)
- return ERR_PTR(-ENOMEM);
+ if (!rvdev_data)
+ return -EINVAL;
- kref_init(&rvdev->refcount);
+ rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
+ if (!rvdev)
+ return -ENOMEM;
rvdev->id = rvdev_data->id;
rvdev->rproc = rproc;
rvdev->index = rvdev_data->index;
- /* Initialise vdev subdevice */
- 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 ERR_PTR(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));
}
+ platform_set_drvdata(pdev, rvdev);
+ rvdev->pdev = pdev;
+
+ rsc = rvdev_data->rsc;
+
/* parse the vrings */
for (i = 0; i < rsc->num_of_vrings; i++) {
ret = rproc_parse_vring(rvdev, rsc, i);
if (ret)
- goto free_rvdev;
+ return ret;
}
/* remember the resource offset*/
@@ -577,18 +548,30 @@ rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
rproc_add_subdev(rproc, &rvdev->subdev);
- return rvdev;
+ dev_dbg(dev, "virtio dev %d added\n", rvdev->index);
+
+ /*
+ * We're indirectly making a non-temporary copy of the rproc pointer
+ * here, because the platform devicer or the vdev device will indirectly
+ * access the wrapping rproc.
+ *
+ * Therefore we must increment the rproc refcount here, and decrement
+ * it _only_ on platform remove.
+ */
+ get_device(&rproc->dev);
+
+ return 0;
unwind_vring_allocations:
for (i--; i >= 0; i--)
rproc_free_vring(&rvdev->vring[i]);
-free_rvdev:
- device_unregister(&rvdev->dev);
- return ERR_PTR(ret);
+
+ return ret;
}
-void rproc_rvdev_remove_device(struct rproc_vdev *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;
@@ -600,19 +583,29 @@ void rproc_rvdev_remove_device(struct rproc_vdev *rvdev)
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;
+ of_reserved_mem_device_release(&pdev->dev);
- for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
- rvring = &rvdev->vring[id];
- rproc_free_vring(rvring);
- }
+ dev_dbg(&pdev->dev, "virtio dev %d removed\n", rvdev->index);
- rproc_rvdev_remove_device(rvdev);
+ /* The remote proc device can be removed */
+ put_device(&rproc->dev);
+
+ 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..7951a3e2b62a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -614,9 +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
- * @dev: device struct used for reference count semantics
+ * @pdev: remoteproc virtio platform device
* @id: virtio device id (as in virtio_ids.h)
* @node: list node
* @rproc: the rproc handle
@@ -625,10 +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 device dev;
+ struct platform_device *pdev;
unsigned int id;
struct list_head node;
--
2.25.1
> Subject: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc VirtIO
> device
>
> Update from V2 [1]:
> In order to better handle error cases and to have something more
> symmetrical between the functions in charge of rvdev initialization/deletion,
> the patchset has been reworked.
> - Introduction in the first patch, of rproc_vdev_data structure which allows
> to better
> decorrelate the rproc from the management of the rvdev structure. This
> structure is reused
> in the last patch of the series for the creation of the remoteproc virtio
> platform device.
> - In addition to the previous version, the management of the vring lifecycle
> has been fully
> migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring,
> rproc_free_vring)
>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
> g%2Flkml%2F2021%2F12%2F22%2F111&data=04%7C01%7Cpeng.fan%4
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000&sdata=bFfSxpPMpPRGYcMBcwxaQ152mRzf3c
> fwoFPjiJ0SIgw%3D&reserved=0
>
> 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
This means not using resource table anymore with new approach?
If yes, would that introduce a problem that different M-core images
requires different dtb?
> - 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.
As my understanding virtio-i2c is a i2c bus, you could declare a i2c device
in the virtual bus without your patchset, would you please share more?
Thanks,
Peng.
> - Keep the legacy working!
> - Try to improve the picture about concerns reported by Christoph Hellwing
> [3][4]
>
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
> g%2Flkml%2F2020%2F4%2F16%2F1817&data=04%7C01%7Cpeng.fan%4
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> JXVCI6Mn0%3D%7C3000&sdata=O2BZw5PCY19eD5xMGxrGUKC%2Fty1
> Sdc3LE6rhK4cSXvs%3D&reserved=0
> [3]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
> g%2Flkml%2F2021%2F6%2F23%2F607&data=04%7C01%7Cpeng.fan%40
> nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CTW
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6Mn0%3D%7C3000&sdata=xqX50iDeL%2BtFBOgyADnEUE5HH4gogK
> C0MwyqZSxVqNo%3D&reserved=0
> [4]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2FAOKowLclCbO
> CKxyiJ71WeNyuAAj2q8EUtxrXbyky5E%40cp7-web-042.plabs.ch%2F&da
> ta=04%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e85
> 5e2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> 757786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=mvSm3wM
> LgQ%2BDFhqjXIkG8de58zFjwPSURzw55JhGNaA%3D&reserved=0
>
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-DT&data=04%7
> C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757786
> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=X%2B462681gcxe6
> 2GP%2BV7ji2nef%2FuTbQVvIlddcMQwtmg%3D&reserved=0
> step 3: Add memory declaration in VirtIO subnode =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-memories&data=0
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=eMlXgCgrV6l46
> h3Ywv1%2BCoX3gLBabdTZs9ybsm4t4ys%3D&reserved=0
> step 4: Add mailbox declaration in VirtIO subnode =>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-mailboxes&data=0
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=75hApOwihqMZ
> UUKz1VcitY2VPDc6KAIwAvH8enEZOPY%3D&reserved=0
>
> Arnaud Pouliquen (4):
> 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
>
> drivers/remoteproc/remoteproc_core.c | 159 +++----------------
> drivers/remoteproc/remoteproc_internal.h | 33 +++-
> drivers/remoteproc/remoteproc_virtio.c | 193
> ++++++++++++++++++++---
> include/linux/remoteproc.h | 6 +-
> 4 files changed, 227 insertions(+), 164 deletions(-)
>
> --
> 2.25.1
On 2/15/22 09:34, Peng Fan wrote:
>> Subject: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc VirtIO
>> device
>>
>> Update from V2 [1]:
>> In order to better handle error cases and to have something more
>> symmetrical between the functions in charge of rvdev initialization/deletion,
>> the patchset has been reworked.
>> - Introduction in the first patch, of rproc_vdev_data structure which allows
>> to better
>> decorrelate the rproc from the management of the rvdev structure. This
>> structure is reused
>> in the last patch of the series for the creation of the remoteproc virtio
>> platform device.
>> - In addition to the previous version, the management of the vring lifecycle
>> has been fully
>> migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring,
>> rproc_free_vring)
>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
>> g%2Flkml%2F2021%2F12%2F22%2F111&data=04%7C01%7Cpeng.fan%4
>> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
>> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
>> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
>> JXVCI6Mn0%3D%7C3000&sdata=bFfSxpPMpPRGYcMBcwxaQ152mRzf3c
>> fwoFPjiJ0SIgw%3D&reserved=0
>>
>> 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
>
> This means not using resource table anymore with new approach?
> If yes, would that introduce a problem that different M-core images
> requires different dtb?
The resource table still exists. The main difference is that the virtio devices
would be predefined in the DT with their own resources ( memories , mailboxes,...)
No need to inherit from the rproc device.
On resource table parsing, the remoteproc looks first for pre registered
rproc_virtio devices. If found then it uses it. Else it instantiates a new
one (legacy method).
>
>> - 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.
>
> As my understanding virtio-i2c is a i2c bus, you could declare a i2c device
> in the virtual bus without your patchset, would you please share more?
Yes virtio-i2c is a bus, There is different methods to declare I2C device on
a bus[1].
In ST we rely on DT to statically declare an I2C device,as child of the I2C
adapter node.
I haven't implemented the virtio-I2C part yet, but it would make sense to have
such an implementation.
Which alternative have you in mind?
[1] https://www.kernel.org/doc/html/latest/i2c/instantiating-devices.html
Thanks,
Arnaud
>
> Thanks,
> Peng.
>
>> - Keep the legacy working!
>> - Try to improve the picture about concerns reported by Christoph Hellwing
>> [3][4]
>>
>> [2]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
>> g%2Flkml%2F2020%2F4%2F16%2F1817&data=04%7C01%7Cpeng.fan%4
>> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
>> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
>> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
>> JXVCI6Mn0%3D%7C3000&sdata=O2BZw5PCY19eD5xMGxrGUKC%2Fty1
>> Sdc3LE6rhK4cSXvs%3D&reserved=0
>> [3]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.or
>> g%2Flkml%2F2021%2F6%2F23%2F607&data=04%7C01%7Cpeng.fan%40
>> nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa9
>> 2cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CTW
>> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
>> VCI6Mn0%3D%7C3000&sdata=xqX50iDeL%2BtFBOgyADnEUE5HH4gogK
>> C0MwyqZSxVqNo%3D&reserved=0
>> [4]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
>> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2FAOKowLclCbO
>> CKxyiJ71WeNyuAAj2q8EUtxrXbyky5E%40cp7-web-042.plabs.ch%2F&da
>> ta=04%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e85
>> 5e2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
>> 757786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
>> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=mvSm3wM
>> LgQ%2BDFhqjXIkG8de58zFjwPSURzw55JhGNaA%3D&reserved=0
>>
>> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>> com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-DT&data=04%7
>> C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C
>> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757786
>> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiL
>> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=X%2B462681gcxe6
>> 2GP%2BV7ji2nef%2FuTbQVvIlddcMQwtmg%3D&reserved=0
>> step 3: Add memory declaration in VirtIO subnode =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>> com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-memories&data=0
>> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
>> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=eMlXgCgrV6l46
>> h3Ywv1%2BCoX3gLBabdTZs9ybsm4t4ys%3D&reserved=0
>> step 4: Add mailbox declaration in VirtIO subnode =>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
>> com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-mailboxes&data=0
>> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757
>> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
>> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=75hApOwihqMZ
>> UUKz1VcitY2VPDc6KAIwAvH8enEZOPY%3D&reserved=0
>>
>> Arnaud Pouliquen (4):
>> 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
>>
>> drivers/remoteproc/remoteproc_core.c | 159 +++----------------
>> drivers/remoteproc/remoteproc_internal.h | 33 +++-
>> drivers/remoteproc/remoteproc_virtio.c | 193
>> ++++++++++++++++++++---
>> include/linux/remoteproc.h | 6 +-
>> 4 files changed, 227 insertions(+), 164 deletions(-)
>>
>> --
>> 2.25.1
>
> Subject: Re: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc
> VirtIO device
>
>
>
> On 2/15/22 09:34, Peng Fan wrote:
> >> Subject: [RFC PATCH v3 0/4] remoteproc: restructure the remoteproc
> >> VirtIO device
> >>
> >> Update from V2 [1]:
> >> In order to better handle error cases and to have something more
> >> symmetrical between the functions in charge of rvdev
> >> initialization/deletion, the patchset has been reworked.
> >> - Introduction in the first patch, of rproc_vdev_data structure
> >> which allows to better
> >> decorrelate the rproc from the management of the rvdev structure.
> >> This structure is reused
> >> in the last patch of the series for the creation of the remoteproc
> >> virtio platform device.
> >> - In addition to the previous version, the management of the vring
> >> lifecycle has been fully
> >> migrated to the remoteproc_virtio.c (rproc_parse_vring,
> >> rproc_alloc_vring,
> >> rproc_free_vring)
> >>
> >> [1]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >>
> l.or%2F&data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a
> 845cbf0
> >>
> 8d9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63780
> 5203140
> >>
> 739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLC
> >>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=MDamNuBkyFebgG
> BuP5shcU9
> >> aw%2FdMuM9GBTEEzffQQkA%3D&reserved=0
> >>
> g%2Flkml%2F2021%2F12%2F22%2F111&data=04%7C01%7Cpeng.fan%4
> >>
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> >>
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> >>
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> >>
> JXVCI6Mn0%3D%7C3000&sdata=bFfSxpPMpPRGYcMBcwxaQ152mRzf3c
> >> fwoFPjiJ0SIgw%3D&reserved=0
> >>
> >> 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
> >
> > This means not using resource table anymore with new approach?
> > If yes, would that introduce a problem that different M-core images
> > requires different dtb?
>
> The resource table still exists. The main difference is that the virtio devices
> would be predefined in the DT with their own resources ( memories ,
> mailboxes,...) No need to inherit from the rproc device.
>
>
> On resource table parsing, the remoteproc looks first for pre registered
> rproc_virtio devices. If found then it uses it. Else it instantiates a new one
> (legacy method).
>
>
> >
> >> - 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.
> >
> > As my understanding virtio-i2c is a i2c bus, you could declare a i2c
> > device in the virtual bus without your patchset, would you please share
> more?
>
> Yes virtio-i2c is a bus, There is different methods to declare I2C device on a
> bus[1].
>
> In ST we rely on DT to statically declare an I2C device,as child of the I2C
> adapter node.
> I haven't implemented the virtio-I2C part yet, but it would make sense to
> have such an implementation.
I misunderstood. Virtio-i2c bus with I2C device in DT is preferred.
>
> Which alternative have you in mind?
No. NXP use same method, we have a rpmsg i2c driver, rpmsg i2c bus node
and i2c device in DT.
Regards,
Peng.
>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> kernel.org%2Fdoc%2Fhtml%2Flatest%2Fi2c%2Finstantiating-devices.html&a
> mp;data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a845cbf08d
> 9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378052
> 03140739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=C1S
> BPEtDhp7Y9XLB8wHgTLaQ%2BBE6T%2BD8eUr34SFRJYQ%3D&reserved
> =0
>
> Thanks,
> Arnaud
>
> >
> > Thanks,
> > Peng.
> >
> >> - Keep the legacy working!
> >> - Try to improve the picture about concerns reported by Christoph
> >> Hellwing [3][4]
> >>
> >> [2]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >>
> l.or%2F&data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a
> 845cbf0
> >>
> 8d9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63780
> 5203140
> >>
> 739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLC
> >>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=MDamNuBkyFebgG
> BuP5shcU9
> >> aw%2FdMuM9GBTEEzffQQkA%3D&reserved=0
> >>
> g%2Flkml%2F2020%2F4%2F16%2F1817&data=04%7C01%7Cpeng.fan%4
> >>
> 0nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa
> >>
> 92cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CT
> >>
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLC
> >>
> JXVCI6Mn0%3D%7C3000&sdata=O2BZw5PCY19eD5xMGxrGUKC%2Fty1
> >> Sdc3LE6rhK4cSXvs%3D&reserved=0
> >> [3]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkm
> >>
> l.or%2F&data=04%7C01%7Cpeng.fan%40nxp.com%7C31ba612e9d444a
> 845cbf0
> >>
> 8d9f073f744%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63780
> 5203140
> >>
> 739333%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLC
> >>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=MDamNuBkyFebgG
> BuP5shcU9
> >> aw%2FdMuM9GBTEEzffQQkA%3D&reserved=0
> >>
> g%2Flkml%2F2021%2F6%2F23%2F607&data=04%7C01%7Cpeng.fan%40
> >>
> nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C686ea1d3bc2b4c6fa9
> >>
> 2cd99c5c301635%7C0%7C0%7C637788110748757786%7CUnknown%7CTW
> >>
> FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> >>
> VCI6Mn0%3D%7C3000&sdata=xqX50iDeL%2BtFBOgyADnEUE5HH4gogK
> >> C0MwyqZSxVqNo%3D&reserved=0
> >> [4]
> >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >> ch
> work.kernel.org%2Fproject%2Flinux-remoteproc%2Fpatch%2FAOKowLclCbO
> >>
> CKxyiJ71WeNyuAAj2q8EUtxrXbyky5E%40cp7-web-042.plabs.ch%2F&da
> >>
> ta=04%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e85
> >>
> 5e2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> >>
> 757786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> >>
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=mvSm3wM
> >> LgQ%2BDFhqjXIkG8de58zFjwPSURzw55JhGNaA%3D&reserved=0
> >>
> >> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> >>
> com%2Farnopo%2Flinux%2Fcommits%2Fstep2-virtio-in-DT&data=04%7
> >>
> C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2%7C
> >>
> 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748757786
> >> %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiL
> >>
> CJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=X%2B462681gcxe6
> >> 2GP%2BV7ji2nef%2FuTbQVvIlddcMQwtmg%3D&reserved=0
> >> step 3: Add memory declaration in VirtIO subnode =>
> >>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> >>
> com%2Farnopo%2Flinux%2Fcommits%2Fstep3-virtio-memories&data=0
> >>
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> >> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> 757
> >>
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> >>
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=eMlXgCgrV6l46
> >> h3Ywv1%2BCoX3gLBabdTZs9ybsm4t4ys%3D&reserved=0
> >> step 4: Add mailbox declaration in VirtIO subnode =>
> >>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> >>
> com%2Farnopo%2Flinux%2Fcommits%2Fstep4-virtio-mailboxes&data=0
> >>
> 4%7C01%7Cpeng.fan%40nxp.com%7C9e663eefc30a4fbb1fdb08d9e0e855e2
> >> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637788110748
> 757
> >>
> 786%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> >>
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=75hApOwihqMZ
> >> UUKz1VcitY2VPDc6KAIwAvH8enEZOPY%3D&reserved=0
> >>
> >> Arnaud Pouliquen (4):
> >> 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
> >>
> >> drivers/remoteproc/remoteproc_core.c | 159 +++----------------
> >> drivers/remoteproc/remoteproc_internal.h | 33 +++-
> >> drivers/remoteproc/remoteproc_virtio.c | 193
> >> ++++++++++++++++++++---
> >> include/linux/remoteproc.h | 6 +-
> >> 4 files changed, 227 insertions(+), 164 deletions(-)
> >>
> >> --
> >> 2.25.1
> >
Good day,
On Wed, 26 Jan 2022 at 09:24, Arnaud Pouliquen
<[email protected]> wrote:
>
> Update from V2 [1]:
> In order to better handle error cases and to have something more symmetrical between
> the functions in charge of rvdev initialization/deletion, the patchset has been reworked.
> - Introduction in the first patch, of rproc_vdev_data structure which allows to better
> decorrelate the rproc from the management of the rvdev structure. This structure is reused
> in the last patch of the series for the creation of the remoteproc virtio platform device.
> - In addition to the previous version, the management of the vring lifecycle has been fully
> migrated to the remoteproc_virtio.c (rproc_parse_vring, rproc_alloc_vring, rproc_free_vring)
>
> [1] https://lkml.org/lkml/2021/12/22/111
>
> 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]
>
I started working on this set - comments to follow throughout the week.
Thanks,
Mathieu
> [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 (4):
> 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
>
> drivers/remoteproc/remoteproc_core.c | 159 +++----------------
> drivers/remoteproc/remoteproc_internal.h | 33 +++-
> drivers/remoteproc/remoteproc_virtio.c | 193 ++++++++++++++++++++---
> include/linux/remoteproc.h | 6 +-
> 4 files changed, 227 insertions(+), 164 deletions(-)
>
> --
> 2.25.1
>
On Wed, Jan 26, 2022 at 05:24:05PM +0100, Arnaud Pouliquen wrote:
> Define a platform driver to manage the remoteproc virtio device as
> a 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.
>
> remoteproc_virtio.c update:
> - Add rproc_virtio_driver platform driver. The probe/remove ops replace
> the rproc_rvdev_add_device/rproc_rvdev_remove_device functions.
> - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
> - rproc_rvdev_release is removed as associated to the rvdev device.
> - The use of rvdev->kref counter is replaced by get/put_device on the
> remoteproc virtio platform device.
> - The vdev device no longer increments rproc device counter.
> increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
> function in charge of the vrings allocation/free.
>
> remoteproc_core.c update:
> 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.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> Update vs previous revision:
> - squash following two patches
> - [4/6] remoteproc: virtio: Create platform device for the remoteproc_virtio
> https://lkml.org/lkml/2021/12/22/112
> - [6/6] remoteproc: Instantiate the new remoteproc virtio platform device
> https://lkml.org/lkml/2021/12/22/114
> ---
> drivers/remoteproc/remoteproc_core.c | 23 +++-
> drivers/remoteproc/remoteproc_internal.h | 3 -
> drivers/remoteproc/remoteproc_virtio.c | 151 +++++++++++------------
> include/linux/remoteproc.h | 6 +-
> 4 files changed, 93 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index eb6b43b71c2b..5b864c9c6244 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -467,6 +467,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> struct device *dev = &rproc->dev;
> struct rproc_vdev *rvdev;
> struct rproc_vdev_data rvdev_data;
> + struct platform_device *pdev;
>
> /* make sure resource isn't truncated */
> if (struct_size(rsc, vring, rsc->num_of_vrings) + rsc->config_len >
> @@ -495,9 +496,23 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
> rvdev_data.rsc_offset = offset;
> rvdev_data.rsc = rsc;
>
> - rvdev = rproc_rvdev_add_device(rproc, &rvdev_data);
> - if (IS_ERR(rvdev))
> - return PTR_ERR(rvdev);
> + pdev = platform_device_register_data(dev, "rproc-virtio", rvdev_data.index, &rvdev_data,
> + sizeof(rvdev_data));
> + if (IS_ERR(pdev)) {
> + dev_err(rproc->dev.parent,
> + "failed to create rproc-virtio device\n");
> + return PTR_ERR(pdev);
> + }
> +
> + /*
> + * At this point the registered remoteproc virtio platform device should have been probed.
> + * Get the associated rproc_vdev struct to assign the vrings.
> + */
> + rvdev = platform_get_drvdata(pdev);
> + if (!rvdev) {
> + platform_device_unregister(pdev);
> + return -EINVAL;
> + }
I can't find a reason to justify this check... Any error condition should be
handled in rproc_virtio_probe() and reported by IS_ERR(pdev) above.
Otherwise this patchset is holding together.
Thanks,
Mathieu
>
> return 0;
> }
> @@ -1237,7 +1252,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);
> + platform_device_unregister(rvdev->pdev);
>
> rproc_coredump_cleanup(rproc);
> }
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 7725b404afc6..175e64a1f3a1 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -45,10 +45,7 @@ int rproc_of_parse_firmware(struct device *dev, int index,
> const char **fw_name);
>
> /* from remoteproc_virtio.c */
> -struct rproc_vdev *rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data);
> -void rproc_rvdev_remove_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 1c4fd79ac1c5..de9f12fcd044 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -13,6 +13,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>
> @@ -46,7 +47,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)
> @@ -341,13 +346,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);
> }
>
> /**
> @@ -363,7 +365,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;
> @@ -433,18 +435,8 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
> vdev->dev.parent = dev;
> vdev->dev.release = rproc_virtio_dev_release;
>
> - /*
> - * We're indirectly making a non-temporary copy of the rproc pointer
> - * here, because drivers probed with this vdev will indirectly
> - * access the wrapping rproc.
> - *
> - * Therefore we must increment the rproc refcount here, and decrement
> - * it _only_ when the vdev is released.
> - */
> - get_device(&rproc->dev);
> -
> /* Reference the vdev and vring allocations */
> - kref_get(&rvdev->refcount);
> + get_device(dev);
>
> ret = register_virtio_device(vdev);
> if (ret) {
> @@ -486,78 +478,57 @@ 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);
> }
>
> -struct rproc_vdev *
> -rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
> +static int rproc_virtio_probe(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> + struct rproc_vdev_data *rvdev_data = dev->platform_data;
> struct rproc_vdev *rvdev;
> - struct fw_rsc_vdev *rsc = rvdev_data->rsc;
> - char name[16];
> + struct rproc *rproc = container_of(dev->parent, struct rproc, dev);
> + struct fw_rsc_vdev *rsc;
> int i, ret;
>
> - rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
> - if (!rvdev)
> - return ERR_PTR(-ENOMEM);
> + if (!rvdev_data)
> + return -EINVAL;
>
> - kref_init(&rvdev->refcount);
> + rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
> + if (!rvdev)
> + return -ENOMEM;
>
> rvdev->id = rvdev_data->id;
> rvdev->rproc = rproc;
> rvdev->index = rvdev_data->index;
>
> - /* Initialise vdev subdevice */
> - 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 ERR_PTR(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));
> }
>
> + platform_set_drvdata(pdev, rvdev);
> + rvdev->pdev = pdev;
> +
> + rsc = rvdev_data->rsc;
> +
> /* parse the vrings */
> for (i = 0; i < rsc->num_of_vrings; i++) {
> ret = rproc_parse_vring(rvdev, rsc, i);
> if (ret)
> - goto free_rvdev;
> + return ret;
> }
>
> /* remember the resource offset*/
> @@ -577,18 +548,30 @@ rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
>
> rproc_add_subdev(rproc, &rvdev->subdev);
>
> - return rvdev;
> + dev_dbg(dev, "virtio dev %d added\n", rvdev->index);
> +
> + /*
> + * We're indirectly making a non-temporary copy of the rproc pointer
> + * here, because the platform devicer or the vdev device will indirectly
> + * access the wrapping rproc.
> + *
> + * Therefore we must increment the rproc refcount here, and decrement
> + * it _only_ on platform remove.
> + */
> + get_device(&rproc->dev);
> +
> + return 0;
>
> unwind_vring_allocations:
> for (i--; i >= 0; i--)
> rproc_free_vring(&rvdev->vring[i]);
> -free_rvdev:
> - device_unregister(&rvdev->dev);
> - return ERR_PTR(ret);
> +
> + return ret;
> }
>
> -void rproc_rvdev_remove_device(struct rproc_vdev *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;
> @@ -600,19 +583,29 @@ void rproc_rvdev_remove_device(struct rproc_vdev *rvdev)
>
> 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;
> + of_reserved_mem_device_release(&pdev->dev);
>
> - for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
> - rvring = &rvdev->vring[id];
> - rproc_free_vring(rvring);
> - }
> + dev_dbg(&pdev->dev, "virtio dev %d removed\n", rvdev->index);
>
> - rproc_rvdev_remove_device(rvdev);
> + /* The remote proc device can be removed */
> + put_device(&rproc->dev);
> +
> + 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..7951a3e2b62a 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -614,9 +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
> - * @dev: device struct used for reference count semantics
> + * @pdev: remoteproc virtio platform device
> * @id: virtio device id (as in virtio_ids.h)
> * @node: list node
> * @rproc: the rproc handle
> @@ -625,10 +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 device dev;
> + struct platform_device *pdev;
>
> unsigned int id;
> struct list_head node;
> --
> 2.25.1
>
Hello Mathieu,
On 3/4/22 19:53, Mathieu Poirier wrote:
> On Wed, Jan 26, 2022 at 05:24:05PM +0100, Arnaud Pouliquen wrote:
>> Define a platform driver to manage the remoteproc virtio device as
>> a 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.
>>
>> remoteproc_virtio.c update:
>> - Add rproc_virtio_driver platform driver. The probe/remove ops replace
>> the rproc_rvdev_add_device/rproc_rvdev_remove_device functions.
>> - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
>> - rproc_rvdev_release is removed as associated to the rvdev device.
>> - The use of rvdev->kref counter is replaced by get/put_device on the
>> remoteproc virtio platform device.
>> - The vdev device no longer increments rproc device counter.
>> increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
>> function in charge of the vrings allocation/free.
>>
>> remoteproc_core.c update:
>> 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.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> Update vs previous revision:
>> - squash following two patches
>> - [4/6] remoteproc: virtio: Create platform device for the remoteproc_virtio
>> https://lkml.org/lkml/2021/12/22/112
>> - [6/6] remoteproc: Instantiate the new remoteproc virtio platform device
>> https://lkml.org/lkml/2021/12/22/114
>> ---
>> drivers/remoteproc/remoteproc_core.c | 23 +++-
>> drivers/remoteproc/remoteproc_internal.h | 3 -
>> drivers/remoteproc/remoteproc_virtio.c | 151 +++++++++++------------
>> include/linux/remoteproc.h | 6 +-
>> 4 files changed, 93 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index eb6b43b71c2b..5b864c9c6244 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -467,6 +467,7 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>> struct device *dev = &rproc->dev;
>> struct rproc_vdev *rvdev;
>> struct rproc_vdev_data rvdev_data;
>> + struct platform_device *pdev;
>>
>> /* make sure resource isn't truncated */
>> if (struct_size(rsc, vring, rsc->num_of_vrings) + rsc->config_len >
>> @@ -495,9 +496,23 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>> rvdev_data.rsc_offset = offset;
>> rvdev_data.rsc = rsc;
>>
>> - rvdev = rproc_rvdev_add_device(rproc, &rvdev_data);
>> - if (IS_ERR(rvdev))
>> - return PTR_ERR(rvdev);
>> + pdev = platform_device_register_data(dev, "rproc-virtio", rvdev_data.index, &rvdev_data,
>> + sizeof(rvdev_data));
>> + if (IS_ERR(pdev)) {
>> + dev_err(rproc->dev.parent,
>> + "failed to create rproc-virtio device\n");
>> + return PTR_ERR(pdev);
>> + }
>> +
>> + /*
>> + * At this point the registered remoteproc virtio platform device should have been probed.
>> + * Get the associated rproc_vdev struct to assign the vrings.
>> + */
>> + rvdev = platform_get_drvdata(pdev);
>> + if (!rvdev) {
>> + platform_device_unregister(pdev);
>> + return -EINVAL;
>> + }
>
> I can't find a reason to justify this check... Any error condition should be
> handled in rproc_virtio_probe() and reported by IS_ERR(pdev) above.
>
> Otherwise this patchset is holding together.
Thanks for the review!
I plan to send this week or next week a new version integrating all your comments.
Regards,
Arnaud
> Thanks,
> Mathieu
>
>>
>> return 0;
>> }
>> @@ -1237,7 +1252,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);
>> + platform_device_unregister(rvdev->pdev);
>>
>> rproc_coredump_cleanup(rproc);
>> }
>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>> index 7725b404afc6..175e64a1f3a1 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -45,10 +45,7 @@ int rproc_of_parse_firmware(struct device *dev, int index,
>> const char **fw_name);
>>
>> /* from remoteproc_virtio.c */
>> -struct rproc_vdev *rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data);
>> -void rproc_rvdev_remove_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 1c4fd79ac1c5..de9f12fcd044 100644
>> --- a/drivers/remoteproc/remoteproc_virtio.c
>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>> @@ -13,6 +13,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>
>> @@ -46,7 +47,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)
>> @@ -341,13 +346,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);
>> }
>>
>> /**
>> @@ -363,7 +365,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;
>> @@ -433,18 +435,8 @@ static int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>> vdev->dev.parent = dev;
>> vdev->dev.release = rproc_virtio_dev_release;
>>
>> - /*
>> - * We're indirectly making a non-temporary copy of the rproc pointer
>> - * here, because drivers probed with this vdev will indirectly
>> - * access the wrapping rproc.
>> - *
>> - * Therefore we must increment the rproc refcount here, and decrement
>> - * it _only_ when the vdev is released.
>> - */
>> - get_device(&rproc->dev);
>> -
>> /* Reference the vdev and vring allocations */
>> - kref_get(&rvdev->refcount);
>> + get_device(dev);
>>
>> ret = register_virtio_device(vdev);
>> if (ret) {
>> @@ -486,78 +478,57 @@ 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);
>> }
>>
>> -struct rproc_vdev *
>> -rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
>> +static int rproc_virtio_probe(struct platform_device *pdev)
>> {
>> + struct device *dev = &pdev->dev;
>> + struct rproc_vdev_data *rvdev_data = dev->platform_data;
>> struct rproc_vdev *rvdev;
>> - struct fw_rsc_vdev *rsc = rvdev_data->rsc;
>> - char name[16];
>> + struct rproc *rproc = container_of(dev->parent, struct rproc, dev);
>> + struct fw_rsc_vdev *rsc;
>> int i, ret;
>>
>> - rvdev = kzalloc(sizeof(*rvdev), GFP_KERNEL);
>> - if (!rvdev)
>> - return ERR_PTR(-ENOMEM);
>> + if (!rvdev_data)
>> + return -EINVAL;
>>
>> - kref_init(&rvdev->refcount);
>> + rvdev = devm_kzalloc(dev, sizeof(*rvdev), GFP_KERNEL);
>> + if (!rvdev)
>> + return -ENOMEM;
>>
>> rvdev->id = rvdev_data->id;
>> rvdev->rproc = rproc;
>> rvdev->index = rvdev_data->index;
>>
>> - /* Initialise vdev subdevice */
>> - 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 ERR_PTR(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));
>> }
>>
>> + platform_set_drvdata(pdev, rvdev);
>> + rvdev->pdev = pdev;
>> +
>> + rsc = rvdev_data->rsc;
>> +
>> /* parse the vrings */
>> for (i = 0; i < rsc->num_of_vrings; i++) {
>> ret = rproc_parse_vring(rvdev, rsc, i);
>> if (ret)
>> - goto free_rvdev;
>> + return ret;
>> }
>>
>> /* remember the resource offset*/
>> @@ -577,18 +548,30 @@ rproc_rvdev_add_device(struct rproc *rproc, struct rproc_vdev_data *rvdev_data)
>>
>> rproc_add_subdev(rproc, &rvdev->subdev);
>>
>> - return rvdev;
>> + dev_dbg(dev, "virtio dev %d added\n", rvdev->index);
>> +
>> + /*
>> + * We're indirectly making a non-temporary copy of the rproc pointer
>> + * here, because the platform devicer or the vdev device will indirectly
>> + * access the wrapping rproc.
>> + *
>> + * Therefore we must increment the rproc refcount here, and decrement
>> + * it _only_ on platform remove.
>> + */
>> + get_device(&rproc->dev);
>> +
>> + return 0;
>>
>> unwind_vring_allocations:
>> for (i--; i >= 0; i--)
>> rproc_free_vring(&rvdev->vring[i]);
>> -free_rvdev:
>> - device_unregister(&rvdev->dev);
>> - return ERR_PTR(ret);
>> +
>> + return ret;
>> }
>>
>> -void rproc_rvdev_remove_device(struct rproc_vdev *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;
>> @@ -600,19 +583,29 @@ void rproc_rvdev_remove_device(struct rproc_vdev *rvdev)
>>
>> 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;
>> + of_reserved_mem_device_release(&pdev->dev);
>>
>> - for (id = 0; id < ARRAY_SIZE(rvdev->vring); id++) {
>> - rvring = &rvdev->vring[id];
>> - rproc_free_vring(rvring);
>> - }
>> + dev_dbg(&pdev->dev, "virtio dev %d removed\n", rvdev->index);
>>
>> - rproc_rvdev_remove_device(rvdev);
>> + /* The remote proc device can be removed */
>> + put_device(&rproc->dev);
>> +
>> + 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..7951a3e2b62a 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -614,9 +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
>> - * @dev: device struct used for reference count semantics
>> + * @pdev: remoteproc virtio platform device
>> * @id: virtio device id (as in virtio_ids.h)
>> * @node: list node
>> * @rproc: the rproc handle
>> @@ -625,10 +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 device dev;
>> + struct platform_device *pdev;
>>
>> unsigned int id;
>> struct list_head node;
>> --
>> 2.25.1
>>