[PATCH 0/5] Use Device Lifecycle managed functions in TI R5 Remoteproc

[PATCH 0/5] Use Device Lifecycle managed functions in TI R5 Remoteproc

[Beleswar Padhi]

This series uses various devm_ helpers to simplify device removal path in
ti_k3_r5_remoteproc driver. This is the first series in the TI K3
Remoteproc refactoring as long planned since [0].

Testing Done:
1. Tested boot of R5F remoteprocs in MCU and MAIN voltage domain in both
IPC-Only mode and Kernel remoteproc mode in all Jacinto K3 devices.
2. Tested Lockstep, Split and Single-CPU Mode configuration (wherever
applicable) of R5F remoteprocs in all Jacinto K3 devices.
3. Tested shutdown of R5F remoteprocs from Linux userspace and also by
executing `modprobe -r ti_k3_r5_remoteproc`.
4. Tested that each patch in this series generates no new warnings/errors.

[0]: https://lore.kernel.org/all/Zr4w8Vj0mVo5sBsJ@p14s/

Beleswar Padhi (5):
  remoteproc: k3-r5: Add devm action to release reserved memory
  remoteproc: k3-r5: Use devm_kcalloc() helper
  remoteproc: k3-r5: Add devm action to release tsp
  remoteproc: k3-r5: Use devm_ioremap_wc() helper
  remoteproc: k3-r5: Use devm_rproc_add() helper

 drivers/remoteproc/ti_k3_r5_remoteproc.c | 86 ++++++++++--------------
 1 file changed, 34 insertions(+), 52 deletions(-)

-- 
2.34.1

[PATCH 1/5] remoteproc: k3-r5: Add devm action to release reserved memory

[Beleswar Padhi]

Use a device lifecycle managed action to release reserved memory. This
helps prevent mistakes like releasing out of order in cleanup functions
and forgetting to release on error paths.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 6560b7954027..d51f88a5abc1 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -955,6 +955,13 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
 	return ret;
 }
 
+static void k3_r5_mem_release(void *data)
+{
+	struct device *dev = data;
+
+	of_reserved_mem_device_release(dev);
+}
+
 static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 {
 	struct device *dev = kproc->dev;
@@ -985,12 +992,14 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 		return ret;
 	}
 
+	ret = devm_add_action_or_reset(dev, k3_r5_mem_release, dev);
+	if (ret)
+		return ret;
+
 	num_rmems--;
 	kproc->rmem = kcalloc(num_rmems, sizeof(*kproc->rmem), GFP_KERNEL);
-	if (!kproc->rmem) {
-		ret = -ENOMEM;
-		goto release_rmem;
-	}
+	if (!kproc->rmem)
+		return -ENOMEM;
 
 	/* use remaining reserved memory regions for static carveouts */
 	for (i = 0; i < num_rmems; i++) {
@@ -1041,8 +1050,6 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 	for (i--; i >= 0; i--)
 		iounmap(kproc->rmem[i].cpu_addr);
 	kfree(kproc->rmem);
-release_rmem:
-	of_reserved_mem_device_release(dev);
 	return ret;
 }
 
@@ -1053,8 +1060,6 @@ static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
 	for (i = 0; i < kproc->num_rmems; i++)
 		iounmap(kproc->rmem[i].cpu_addr);
 	kfree(kproc->rmem);
-
-	of_reserved_mem_device_release(kproc->dev);
 }
 
 /*
-- 
2.34.1

[PATCH 2/5] remoteproc: k3-r5: Use devm_kcalloc() helper

[Beleswar Padhi]

Use a device lifecycle managed action to free memory. This helps prevent
mistakes like freeing out of order in cleanup functions and forgetting
to free on error paths.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index d51f88a5abc1..0753a5c35c7e 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -997,7 +997,7 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 		return ret;
 
 	num_rmems--;
-	kproc->rmem = kcalloc(num_rmems, sizeof(*kproc->rmem), GFP_KERNEL);
+	kproc->rmem = devm_kcalloc(dev, num_rmems, sizeof(*kproc->rmem), GFP_KERNEL);
 	if (!kproc->rmem)
 		return -ENOMEM;
 
@@ -1049,7 +1049,6 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 unmap_rmem:
 	for (i--; i >= 0; i--)
 		iounmap(kproc->rmem[i].cpu_addr);
-	kfree(kproc->rmem);
 	return ret;
 }
 
@@ -1059,7 +1058,6 @@ static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
 
 	for (i = 0; i < kproc->num_rmems; i++)
 		iounmap(kproc->rmem[i].cpu_addr);
-	kfree(kproc->rmem);
 }
 
 /*
-- 
2.34.1

[PATCH 3/5] remoteproc: k3-r5: Add devm action to release tsp

[Beleswar Padhi]

Use a device lifecycle managed action to release tsp ti_sci_proc handle.
This helps prevent mistakes like releasing out of order in cleanup
functions and forgetting to release on error paths.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 0753a5c35c7e..2966cb210403 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1513,6 +1513,13 @@ static int k3_r5_core_of_get_sram_memories(struct platform_device *pdev,
 	return 0;
 }
 
+static void k3_r5_release_tsp(void *data)
+{
+	struct ti_sci_proc *tsp = data;
+
+	ti_sci_proc_release(tsp);
+}
+
 static int k3_r5_core_of_init(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct platform_device *pdev)
 		goto err;
 	}
 
+	ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, core->tsp);
+	if (ret)
+		goto err;
+
 	platform_set_drvdata(pdev, core);
 	devres_close_group(dev, k3_r5_core_of_init);
 
@@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct platform_device *pdev)
  */
 static void k3_r5_core_of_exit(struct platform_device *pdev)
 {
-	struct k3_r5_core *core = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
-	int ret;
-
-	ret = ti_sci_proc_release(core->tsp);
-	if (ret)
-		dev_err(dev, "failed to release proc, ret = %d\n", ret);
 
 	platform_set_drvdata(pdev, NULL);
 	devres_release_group(dev, k3_r5_core_of_init);
-- 
2.34.1

Re: [PATCH 3/5] remoteproc: k3-r5: Add devm action to release tsp

[Andrew Davis]

On 12/4/24 5:11 AM, Beleswar Padhi wrote:
> Use a device lifecycle managed action to release tsp ti_sci_proc handle.
> This helps prevent mistakes like releasing out of order in cleanup
> functions and forgetting to release on error paths.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 0753a5c35c7e..2966cb210403 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -1513,6 +1513,13 @@ static int k3_r5_core_of_get_sram_memories(struct platform_device *pdev,
>   	return 0;
>   }
>   
> +static void k3_r5_release_tsp(void *data)
> +{
> +	struct ti_sci_proc *tsp = data;
> +
> +	ti_sci_proc_release(tsp);
> +}
> +
>   static int k3_r5_core_of_init(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct platform_device *pdev)
>   		goto err;
>   	}
>   
> +	ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, core->tsp);
> +	if (ret)
> +		goto err;
> +
>   	platform_set_drvdata(pdev, core);
>   	devres_close_group(dev, k3_r5_core_of_init);
>   
> @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct platform_device *pdev)
>    */
>   static void k3_r5_core_of_exit(struct platform_device *pdev)
>   {
> -	struct k3_r5_core *core = platform_get_drvdata(pdev);
>   	struct device *dev = &pdev->dev;
> -	int ret;
> -
> -	ret = ti_sci_proc_release(core->tsp);
> -	if (ret)
> -		dev_err(dev, "failed to release proc, ret = %d\n", ret);
>   

One thing to remember is devm unrolling happens after remove(). So
here you are changing the order things happen. ti_sci_proc_release()
now will get called after the below functions. This most likely
isn't wrong, but to make review easier it helps to start from the
last called function in remove() and work backwards so nothing
is reordered.

Andrew

>   	platform_set_drvdata(pdev, NULL);
>   	devres_release_group(dev, k3_r5_core_of_init);

Re: [PATCH 3/5] remoteproc: k3-r5: Add devm action to release tsp

[Beleswar Prasad Padhi]

Hi Andrew,

On 17/12/24 21:30, Andrew Davis wrote:
> On 12/4/24 5:11 AM, Beleswar Padhi wrote:
>> Use a device lifecycle managed action to release tsp ti_sci_proc handle.
>> This helps prevent mistakes like releasing out of order in cleanup
>> functions and forgetting to release on error paths.
>>
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------
>>   1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index 0753a5c35c7e..2966cb210403 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -1513,6 +1513,13 @@ static int 
>> k3_r5_core_of_get_sram_memories(struct platform_device *pdev,
>>       return 0;
>>   }
>>   +static void k3_r5_release_tsp(void *data)
>> +{
>> +    struct ti_sci_proc *tsp = data;
>> +
>> +    ti_sci_proc_release(tsp);
>> +}
>> +
>>   static int k3_r5_core_of_init(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>> @@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct 
>> platform_device *pdev)
>>           goto err;
>>       }
>>   +    ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, 
>> core->tsp);
>> +    if (ret)
>> +        goto err;
>> +
>>       platform_set_drvdata(pdev, core);
>>       devres_close_group(dev, k3_r5_core_of_init);
>>   @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct 
>> platform_device *pdev)
>>    */
>>   static void k3_r5_core_of_exit(struct platform_device *pdev)
>>   {
>> -    struct k3_r5_core *core = platform_get_drvdata(pdev);
>>       struct device *dev = &pdev->dev;
>> -    int ret;
>> -
>> -    ret = ti_sci_proc_release(core->tsp);
>> -    if (ret)
>> -        dev_err(dev, "failed to release proc, ret = %d\n", ret);
>
> One thing to remember is devm unrolling happens after remove(). So
> here you are changing the order things happen. ti_sci_proc_release()
> now will get called after the below functions. This most likely
> isn't wrong, but to make review easier it helps to start from the
> last called function in remove() and work backwards so nothing
> is reordered.


That's a great insight! Will send out v2 following this order.

Thanks,
Beleswar

>
> Andrew
>
>>       platform_set_drvdata(pdev, NULL);
>>       devres_release_group(dev, k3_r5_core_of_init);

[PATCH 4/5] remoteproc: k3-r5: Use devm_ioremap_wc() helper

[Beleswar Padhi]

Use a device lifecycle managed ioremap helper function. This helps
prevent mistakes like unmapping out of order in cleanup functions and
forgetting to unmap on all error paths.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 +++++-------------------
 1 file changed, 8 insertions(+), 32 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 2966cb210403..1a7681502f62 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1004,17 +1004,13 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 	/* use remaining reserved memory regions for static carveouts */
 	for (i = 0; i < num_rmems; i++) {
 		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
-		if (!rmem_np) {
-			ret = -EINVAL;
-			goto unmap_rmem;
-		}
+		if (!rmem_np)
+			return -EINVAL;
 
 		rmem = of_reserved_mem_lookup(rmem_np);
 		of_node_put(rmem_np);
-		if (!rmem) {
-			ret = -EINVAL;
-			goto unmap_rmem;
-		}
+		if (!rmem)
+			return -EINVAL;
 
 		kproc->rmem[i].bus_addr = rmem->base;
 		/*
@@ -1029,12 +1025,11 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 		 */
 		kproc->rmem[i].dev_addr = (u32)rmem->base;
 		kproc->rmem[i].size = rmem->size;
-		kproc->rmem[i].cpu_addr = ioremap_wc(rmem->base, rmem->size);
+		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
 		if (!kproc->rmem[i].cpu_addr) {
 			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
 				i + 1, &rmem->base, &rmem->size);
-			ret = -ENOMEM;
-			goto unmap_rmem;
+			return -ENOMEM;
 		}
 
 		dev_dbg(dev, "reserved memory%d: bus addr %pa size 0x%zx va %pK da 0x%x\n",
@@ -1045,19 +1040,6 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
 	kproc->num_rmems = num_rmems;
 
 	return 0;
-
-unmap_rmem:
-	for (i--; i >= 0; i--)
-		iounmap(kproc->rmem[i].cpu_addr);
-	return ret;
-}
-
-static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
-{
-	int i;
-
-	for (i = 0; i < kproc->num_rmems; i++)
-		iounmap(kproc->rmem[i].cpu_addr);
 }
 
 /*
@@ -1285,10 +1267,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 		}
 
 		ret = rproc_add(rproc);
-		if (ret) {
-			dev_err(dev, "rproc_add failed, ret = %d\n", ret);
-			goto err_add;
-		}
+		if (ret)
+			dev_err_probe(dev, ret, "rproc_add failed\n");
 
 		/* create only one rproc in lockstep, single-cpu or
 		 * single core mode
@@ -1333,8 +1313,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 
 err_powerup:
 	rproc_del(rproc);
-err_add:
-	k3_r5_reserved_mem_exit(kproc);
 out:
 	/* undo core0 upon any failures on core1 in split-mode */
 	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
@@ -1379,8 +1357,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
 		mbox_free_channel(kproc->mbox);
 
 		rproc_del(rproc);
-
-		k3_r5_reserved_mem_exit(kproc);
 	}
 }
 
-- 
2.34.1

Re: [PATCH 4/5] remoteproc: k3-r5: Use devm_ioremap_wc() helper

[Andrew Davis]

On 12/4/24 5:11 AM, Beleswar Padhi wrote:
> Use a device lifecycle managed ioremap helper function. This helps
> prevent mistakes like unmapping out of order in cleanup functions and
> forgetting to unmap on all error paths.
> 
> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
> ---
>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 +++++-------------------
>   1 file changed, 8 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 2966cb210403..1a7681502f62 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -1004,17 +1004,13 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>   	/* use remaining reserved memory regions for static carveouts */
>   	for (i = 0; i < num_rmems; i++) {
>   		rmem_np = of_parse_phandle(np, "memory-region", i + 1);
> -		if (!rmem_np) {
> -			ret = -EINVAL;
> -			goto unmap_rmem;
> -		}
> +		if (!rmem_np)
> +			return -EINVAL;
>   
>   		rmem = of_reserved_mem_lookup(rmem_np);
>   		of_node_put(rmem_np);
> -		if (!rmem) {
> -			ret = -EINVAL;
> -			goto unmap_rmem;
> -		}
> +		if (!rmem)
> +			return -EINVAL;
>   
>   		kproc->rmem[i].bus_addr = rmem->base;
>   		/*
> @@ -1029,12 +1025,11 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>   		 */
>   		kproc->rmem[i].dev_addr = (u32)rmem->base;
>   		kproc->rmem[i].size = rmem->size;
> -		kproc->rmem[i].cpu_addr = ioremap_wc(rmem->base, rmem->size);
> +		kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, rmem->size);
>   		if (!kproc->rmem[i].cpu_addr) {
>   			dev_err(dev, "failed to map reserved memory#%d at %pa of size %pa\n",
>   				i + 1, &rmem->base, &rmem->size);
> -			ret = -ENOMEM;
> -			goto unmap_rmem;
> +			return -ENOMEM;
>   		}
>   
>   		dev_dbg(dev, "reserved memory%d: bus addr %pa size 0x%zx va %pK da 0x%x\n",
> @@ -1045,19 +1040,6 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc *kproc)
>   	kproc->num_rmems = num_rmems;
>   
>   	return 0;
> -
> -unmap_rmem:
> -	for (i--; i >= 0; i--)
> -		iounmap(kproc->rmem[i].cpu_addr);
> -	return ret;
> -}
> -
> -static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
> -{
> -	int i;
> -
> -	for (i = 0; i < kproc->num_rmems; i++)
> -		iounmap(kproc->rmem[i].cpu_addr);
>   }
>   
>   /*
> @@ -1285,10 +1267,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   		}
>   
>   		ret = rproc_add(rproc);
> -		if (ret) {
> -			dev_err(dev, "rproc_add failed, ret = %d\n", ret);
> -			goto err_add;
> -		}
> +		if (ret)
> +			dev_err_probe(dev, ret, "rproc_add failed\n");

Did you mean to return the result of dev_err_probe() here? Without that
now anything between here and where err_add used to be will get run.
Might be more clear that this is correct by still doing a "goto out;"

Andrew

>   
>   		/* create only one rproc in lockstep, single-cpu or
>   		 * single core mode
> @@ -1333,8 +1313,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>   
>   err_powerup:
>   	rproc_del(rproc);
> -err_add:
> -	k3_r5_reserved_mem_exit(kproc);
>   out:
>   	/* undo core0 upon any failures on core1 in split-mode */
>   	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
> @@ -1379,8 +1357,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
>   		mbox_free_channel(kproc->mbox);
>   
>   		rproc_del(rproc);
> -
> -		k3_r5_reserved_mem_exit(kproc);
>   	}
>   }
>   

Re: [PATCH 4/5] remoteproc: k3-r5: Use devm_ioremap_wc() helper

[Beleswar Prasad Padhi]

On 17/12/24 21:33, Andrew Davis wrote:
> On 12/4/24 5:11 AM, Beleswar Padhi wrote:
>> Use a device lifecycle managed ioremap helper function. This helps
>> prevent mistakes like unmapping out of order in cleanup functions and
>> forgetting to unmap on all error paths.
>>
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 40 +++++-------------------
>>   1 file changed, 8 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
>> b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index 2966cb210403..1a7681502f62 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -1004,17 +1004,13 @@ static int k3_r5_reserved_mem_init(struct 
>> k3_r5_rproc *kproc)
>>       /* use remaining reserved memory regions for static carveouts */
>>       for (i = 0; i < num_rmems; i++) {
>>           rmem_np = of_parse_phandle(np, "memory-region", i + 1);
>> -        if (!rmem_np) {
>> -            ret = -EINVAL;
>> -            goto unmap_rmem;
>> -        }
>> +        if (!rmem_np)
>> +            return -EINVAL;
>>             rmem = of_reserved_mem_lookup(rmem_np);
>>           of_node_put(rmem_np);
>> -        if (!rmem) {
>> -            ret = -EINVAL;
>> -            goto unmap_rmem;
>> -        }
>> +        if (!rmem)
>> +            return -EINVAL;
>>             kproc->rmem[i].bus_addr = rmem->base;
>>           /*
>> @@ -1029,12 +1025,11 @@ static int k3_r5_reserved_mem_init(struct 
>> k3_r5_rproc *kproc)
>>            */
>>           kproc->rmem[i].dev_addr = (u32)rmem->base;
>>           kproc->rmem[i].size = rmem->size;
>> -        kproc->rmem[i].cpu_addr = ioremap_wc(rmem->base, rmem->size);
>> +        kproc->rmem[i].cpu_addr = devm_ioremap_wc(dev, rmem->base, 
>> rmem->size);
>>           if (!kproc->rmem[i].cpu_addr) {
>>               dev_err(dev, "failed to map reserved memory#%d at %pa 
>> of size %pa\n",
>>                   i + 1, &rmem->base, &rmem->size);
>> -            ret = -ENOMEM;
>> -            goto unmap_rmem;
>> +            return -ENOMEM;
>>           }
>>             dev_dbg(dev, "reserved memory%d: bus addr %pa size 0x%zx 
>> va %pK da 0x%x\n",
>> @@ -1045,19 +1040,6 @@ static int k3_r5_reserved_mem_init(struct 
>> k3_r5_rproc *kproc)
>>       kproc->num_rmems = num_rmems;
>>         return 0;
>> -
>> -unmap_rmem:
>> -    for (i--; i >= 0; i--)
>> -        iounmap(kproc->rmem[i].cpu_addr);
>> -    return ret;
>> -}
>> -
>> -static void k3_r5_reserved_mem_exit(struct k3_r5_rproc *kproc)
>> -{
>> -    int i;
>> -
>> -    for (i = 0; i < kproc->num_rmems; i++)
>> -        iounmap(kproc->rmem[i].cpu_addr);
>>   }
>>     /*
>> @@ -1285,10 +1267,8 @@ static int k3_r5_cluster_rproc_init(struct 
>> platform_device *pdev)
>>           }
>>             ret = rproc_add(rproc);
>> -        if (ret) {
>> -            dev_err(dev, "rproc_add failed, ret = %d\n", ret);
>> -            goto err_add;
>> -        }
>> +        if (ret)
>> +            dev_err_probe(dev, ret, "rproc_add failed\n");
>
> Did you mean to return the result of dev_err_probe() here? Without that


Thanks for catching this. Should return here. Will fix in v2.

Thanks,
Beleswar

> now anything between here and where err_add used to be will get run.
> Might be more clear that this is correct by still doing a "goto out;"
>
> Andrew
>
>>             /* create only one rproc in lockstep, single-cpu or
>>            * single core mode
>> @@ -1333,8 +1313,6 @@ static int k3_r5_cluster_rproc_init(struct 
>> platform_device *pdev)
>>     err_powerup:
>>       rproc_del(rproc);
>> -err_add:
>> -    k3_r5_reserved_mem_exit(kproc);
>>   out:
>>       /* undo core0 upon any failures on core1 in split-mode */
>>       if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
>> @@ -1379,8 +1357,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
>>           mbox_free_channel(kproc->mbox);
>>             rproc_del(rproc);
>> -
>> -        k3_r5_reserved_mem_exit(kproc);
>>       }
>>   }

[PATCH 5/5] remoteproc: k3-r5: Use devm_rproc_add() helper

[Beleswar Padhi]

Use device lifecycle managed devm_rproc_add() helper function. This
helps prevent mistakes like deleting out of order in cleanup functions
and forgetting to delete on all error paths.

Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
---
 drivers/remoteproc/ti_k3_r5_remoteproc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 1a7681502f62..1050f4a94a14 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1266,7 +1266,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			goto out;
 		}
 
-		ret = rproc_add(rproc);
+		ret = devm_rproc_add(dev, rproc);
 		if (ret)
 			dev_err_probe(dev, ret, "rproc_add failed\n");
 
@@ -1355,8 +1355,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
 		}
 
 		mbox_free_channel(kproc->mbox);
-
-		rproc_del(rproc);
 	}
 }
 
-- 
2.34.1

Re: [PATCH 0/5] Use Device Lifecycle managed functions in TI R5 Remoteproc

[Mathieu Poirier]

On Wed, 4 Dec 2024 at 04:11, Beleswar Padhi <b-padhi@ti.com> wrote:
>
> This series uses various devm_ helpers to simplify device removal path in
> ti_k3_r5_remoteproc driver. This is the first series in the TI K3
> Remoteproc refactoring as long planned since [0].
>
> Testing Done:
> 1. Tested boot of R5F remoteprocs in MCU and MAIN voltage domain in both
> IPC-Only mode and Kernel remoteproc mode in all Jacinto K3 devices.
> 2. Tested Lockstep, Split and Single-CPU Mode configuration (wherever
> applicable) of R5F remoteprocs in all Jacinto K3 devices.
> 3. Tested shutdown of R5F remoteprocs from Linux userspace and also by
> executing `modprobe -r ti_k3_r5_remoteproc`.
> 4. Tested that each patch in this series generates no new warnings/errors.
>
> [0]: https://lore.kernel.org/all/Zr4w8Vj0mVo5sBsJ@p14s/
>
> Beleswar Padhi (5):
>   remoteproc: k3-r5: Add devm action to release reserved memory
>   remoteproc: k3-r5: Use devm_kcalloc() helper
>   remoteproc: k3-r5: Add devm action to release tsp
>   remoteproc: k3-r5: Use devm_ioremap_wc() helper
>   remoteproc: k3-r5: Use devm_rproc_add() helper
>
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 86 ++++++++++--------------
>  1 file changed, 34 insertions(+), 52 deletions(-)
>

Please note that I am looking for an R-B and T-B tag from two
different people (for now one of each will suffice) before looking at
patches.

Thanks,
Mathieu

> --
> 2.34.1
>