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

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

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

v2: Changelog:
1. Re-ordered patches in the series to use devm functions starting from
the last called function in remove(), to ease review. [Andrew]
2. Fixed a missing return after dev_err_probe() call in [PATCH v2 3/5]
("remoteproc: k3-r5: Use devm_ioremap_wc() helper"). [Andrew]
3. Removed redundant rproc_del() call in [PATCH v2 4/5] ("remoteproc:
k3-r5: Use devm_rproc_add() helper").

Link to v1:
https://lore.kernel.org/all/20241204111130.2218497-1-b-padhi@ti.com/

[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: Use devm_ioremap_wc() helper
  remoteproc: k3-r5: Use devm_rproc_add() helper
  remoteproc: k3-r5: Add devm action to release tsp

 drivers/remoteproc/ti_k3_r5_remoteproc.c | 88 ++++++++++--------------
 1 file changed, 35 insertions(+), 53 deletions(-)

-- 
2.34.1

[PATCH v2 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>
---
v2: Changelog:
1. None to this patch
 
Link to v1:
https://lore.kernel.org/all/20241204111130.2218497-2-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 v2 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>
---
v2: Changelog:
1. None to this patch

Link to v1:
https://lore.kernel.org/all/20241204111130.2218497-3-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 v2 3/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>
---
v2: Changelog:
1. Re-ordered [PATCH 4/5] from v1 to [PATCH v2 3/5] in v2. [Andrew]
2. Returned after dev_err_probe() call by jumping to 'out' label in
k3_r5_cluster_rproc_init(). [Andrew]
 
Link to v1:
https://lore.kernel.org/all/20241204111130.2218497-5-b-padhi@ti.com/

 drivers/remoteproc/ti_k3_r5_remoteproc.c | 38 +++++-------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 0753a5c35c7e..3493bae95fdf 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);
 }
 
 /*
@@ -1286,8 +1268,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;
+			dev_err_probe(dev, ret, "rproc_add failed\n");
+			goto out;
 		}
 
 		/* create only one rproc in lockstep, single-cpu or
@@ -1333,8 +1315,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 +1359,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

[PATCH v2 4/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>
---
v2: Changelog:
1. Re-ordered [PATCH 5/5] from v1 to [PATCH v2 4/5] in v2. [Andrew]
2. Removed redundant rproc_del() call in k3_r5_cluster_rproc_init() as
devm_rproc_add is used.

Link to v1:
https://lore.kernel.org/all/20241204111130.2218497-6-b-padhi@ti.com/

 drivers/remoteproc/ti_k3_r5_remoteproc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 3493bae95fdf..30b72bcb4d68 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");
 			goto out;
@@ -1297,7 +1297,7 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 			dev_err(dev,
 				"Timed out waiting for %s core to power up!\n",
 				rproc->name);
-			goto err_powerup;
+			goto out;
 		}
 	}
 
@@ -1313,8 +1313,6 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
 		}
 	}
 
-err_powerup:
-	rproc_del(rproc);
 out:
 	/* undo core0 upon any failures on core1 in split-mode */
 	if (cluster->mode == CLUSTER_MODE_SPLIT && core == core1) {
@@ -1357,8 +1355,6 @@ static void k3_r5_cluster_rproc_exit(void *data)
 		}
 
 		mbox_free_channel(kproc->mbox);
-
-		rproc_del(rproc);
 	}
 }
 
-- 
2.34.1

[PATCH v2 5/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>
---
v2: Changelog:
1. Re-ordered [PATCH 3/5] from v1 to [PATCH v2 5/5] in v2. [Andrew]

Link to v1:
https://lore.kernel.org/all/20241204111130.2218497-4-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 30b72bcb4d68..dbc513c5569c 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1487,6 +1487,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;
@@ -1580,6 +1587,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);
 
@@ -1596,13 +1607,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 v2 0/5] Use Device Lifecycle managed functions in TI R5 Remoteproc driver

[Andrew Davis]

On 12/19/24 5:05 AM, Beleswar Padhi 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`.

Did you also test that you could then start the cores back up?

I think that might need some firmware fixes we are working on, so
might not work even before these patches, but just wanted to check
if we have tried it yet.

> 4. Tested that each patch in this series generates no new warnings/errors.

Was that with `make W=1 C=1`? Sparse checks will be done during -next
testing so good to check for those too.

Otherwise patches all look good to me, for the series:

Reviewed-by: Andrew Davis <afd@ti.com>

> 
> v2: Changelog:
> 1. Re-ordered patches in the series to use devm functions starting from
> the last called function in remove(), to ease review. [Andrew]
> 2. Fixed a missing return after dev_err_probe() call in [PATCH v2 3/5]
> ("remoteproc: k3-r5: Use devm_ioremap_wc() helper"). [Andrew]
> 3. Removed redundant rproc_del() call in [PATCH v2 4/5] ("remoteproc:
> k3-r5: Use devm_rproc_add() helper").
> 
> Link to v1:
> https://lore.kernel.org/all/20241204111130.2218497-1-b-padhi@ti.com/
> 
> [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: Use devm_ioremap_wc() helper
>    remoteproc: k3-r5: Use devm_rproc_add() helper
>    remoteproc: k3-r5: Add devm action to release tsp
> 
>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 88 ++++++++++--------------
>   1 file changed, 35 insertions(+), 53 deletions(-)
> 

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

[Beleswar Prasad Padhi]

On 12/19/2024 8:22 PM, Andrew Davis wrote:
> On 12/19/24 5:05 AM, Beleswar Padhi 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`.
>
> Did you also test that you could then start the cores back up?
>
> I think that might need some firmware fixes we are working on, so
> might not work even before these patches, but just wanted to check
> if we have tried it yet.


Yes, the above graceful shutdown feature is part of firmware fixes. 
Also, some support needs to be added in the Linux driver, which sends a 
special "SHUTDOWN" mailbox message to the remotecore which signals the 
firmware to relinquish all resources and shutdown gracefully. So, 
support for turning the remoteprocs back on is not yet added, and not 
tested.

>
>> 4. Tested that each patch in this series generates no new 
>> warnings/errors.
>
> Was that with `make W=1 C=1`? Sparse checks will be done during -next
> testing so good to check for those too.


Yes, did that testing.

>
> Otherwise patches all look good to me, for the series:
>
> Reviewed-by: Andrew Davis <afd@ti.com>


Thanks!

>
>>
>> v2: Changelog:
>> 1. Re-ordered patches in the series to use devm functions starting from
>> the last called function in remove(), to ease review. [Andrew]
>> 2. Fixed a missing return after dev_err_probe() call in [PATCH v2 3/5]
>> ("remoteproc: k3-r5: Use devm_ioremap_wc() helper"). [Andrew]
>> 3. Removed redundant rproc_del() call in [PATCH v2 4/5] ("remoteproc:
>> k3-r5: Use devm_rproc_add() helper").
>>
>> Link to v1:
>> https://lore.kernel.org/all/20241204111130.2218497-1-b-padhi@ti.com/
>>
>> [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: Use devm_ioremap_wc() helper
>>    remoteproc: k3-r5: Use devm_rproc_add() helper
>>    remoteproc: k3-r5: Add devm action to release tsp
>>
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c | 88 ++++++++++--------------
>>   1 file changed, 35 insertions(+), 53 deletions(-)
>>

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

[Mathieu Poirier]

On Thu, Dec 19, 2024 at 04:35:40PM +0530, Beleswar Padhi 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.
> 
> v2: Changelog:
> 1. Re-ordered patches in the series to use devm functions starting from
> the last called function in remove(), to ease review. [Andrew]
> 2. Fixed a missing return after dev_err_probe() call in [PATCH v2 3/5]
> ("remoteproc: k3-r5: Use devm_ioremap_wc() helper"). [Andrew]
> 3. Removed redundant rproc_del() call in [PATCH v2 4/5] ("remoteproc:
> k3-r5: Use devm_rproc_add() helper").
> 
> Link to v1:
> https://lore.kernel.org/all/20241204111130.2218497-1-b-padhi@ti.com/
> 
> [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: Use devm_ioremap_wc() helper
>   remoteproc: k3-r5: Use devm_rproc_add() helper
>   remoteproc: k3-r5: Add devm action to release tsp
> 
>  drivers/remoteproc/ti_k3_r5_remoteproc.c | 88 ++++++++++--------------
>  1 file changed, 35 insertions(+), 53 deletions(-

I have applied this set.

Thanks,
Mathieu

> 
> -- 
> 2.34.1
>