Re: [RFC PATCH v2 4/6] remoteproc: virtio: Create platform device for the remoteproc_virtio

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

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
>>