Messages in this thread | | | Subject | Re: [RFC PATCH v2 4/6] remoteproc: virtio: Create platform device for the remoteproc_virtio | From | Arnaud POULIQUEN <> | Date | Fri, 7 Jan 2022 10:30:45 +0100 |
| |
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 <arnaud.pouliquen@foss.st.com> >> --- >> 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 <ohad@wizery.com> >> * Brian Swetland <swetland@google.com> >> @@ -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 >>
| |