Some CoreSight components have programming clocks (pclk) and are enabled
using clk_get() and clk_prepare_enable(). However, in many cases, these
clocks are not disabled when modules exit and only released by clk_put().
To fix the issue, this commit refactors coresight_get_enable_apb_pclk()
by replacing clk_get() and clk_prepare_enable() with
devm_clk_get_enabled() for enabling APB clock. Callers are updated
to reuse the returned error value.
With the change, programming clocks are managed as resources in driver
model layer, allowing clock cleanup to be handled automatically. As a
result, manual cleanup operations are no longer needed and are removed
from the Coresight drivers.
Fixes: 73d779a03a76 ("coresight: etm4x: Change etm4_platform_driver driver for MMIO devices")
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
drivers/hwtracing/coresight/coresight-catu.c | 9 ++-------
drivers/hwtracing/coresight/coresight-cpu-debug.c | 6 +-----
drivers/hwtracing/coresight/coresight-ctcu-core.c | 10 ++--------
drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 ++-------
drivers/hwtracing/coresight/coresight-funnel.c | 6 +-----
drivers/hwtracing/coresight/coresight-replicator.c | 6 +-----
drivers/hwtracing/coresight/coresight-stm.c | 4 +---
drivers/hwtracing/coresight/coresight-tmc-core.c | 4 +---
drivers/hwtracing/coresight/coresight-tpiu.c | 4 +---
include/linux/coresight.h | 16 +++-------------
10 files changed, 15 insertions(+), 59 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index 9fcda5e49253..c0a51ab0312c 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -627,7 +627,7 @@ static int catu_platform_probe(struct platform_device *pdev)
drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
@@ -636,11 +636,8 @@ static int catu_platform_probe(struct platform_device *pdev)
dev_set_drvdata(&pdev->dev, drvdata);
ret = __catu_probe(&pdev->dev, res);
pm_runtime_put(&pdev->dev);
- if (ret) {
+ if (ret)
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
- }
return ret;
}
@@ -654,8 +651,6 @@ static void catu_platform_remove(struct platform_device *pdev)
__catu_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
#ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 342c3aaf414d..744b6f9b065e 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -699,7 +699,7 @@ static int debug_platform_probe(struct platform_device *pdev)
drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
dev_set_drvdata(&pdev->dev, drvdata);
pm_runtime_get_noresume(&pdev->dev);
@@ -710,8 +710,6 @@ static int debug_platform_probe(struct platform_device *pdev)
if (ret) {
pm_runtime_put_noidle(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
return ret;
}
@@ -725,8 +723,6 @@ static void debug_platform_remove(struct platform_device *pdev)
__debug_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
#ifdef CONFIG_ACPI
diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
index c6bafc96db96..de279efe3405 100644
--- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
+++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
@@ -209,7 +209,7 @@ static int ctcu_probe(struct platform_device *pdev)
drvdata->apb_clk = coresight_get_enable_apb_pclk(dev);
if (IS_ERR(drvdata->apb_clk))
- return -ENODEV;
+ return PTR_ERR(drvdata->apb_clk);
cfgs = of_device_get_match_data(dev);
if (cfgs) {
@@ -233,12 +233,8 @@ static int ctcu_probe(struct platform_device *pdev)
desc.access = CSDEV_ACCESS_IOMEM(base);
drvdata->csdev = coresight_register(&desc);
- if (IS_ERR(drvdata->csdev)) {
- if (!IS_ERR_OR_NULL(drvdata->apb_clk))
- clk_put(drvdata->apb_clk);
-
+ if (IS_ERR(drvdata->csdev))
return PTR_ERR(drvdata->csdev);
- }
return 0;
}
@@ -275,8 +271,6 @@ static void ctcu_platform_remove(struct platform_device *pdev)
ctcu_remove(pdev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->apb_clk))
- clk_put(drvdata->apb_clk);
}
#ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 537d57006a25..ff4ac4b686c4 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2237,14 +2237,12 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
if (res) {
drvdata->base = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(drvdata->base)) {
- clk_put(drvdata->pclk);
+ if (IS_ERR(drvdata->base))
return PTR_ERR(drvdata->base);
- }
}
dev_set_drvdata(&pdev->dev, drvdata);
@@ -2351,9 +2349,6 @@ static void etm4_remove_platform_dev(struct platform_device *pdev)
if (drvdata)
etm4_remove_dev(drvdata);
pm_runtime_disable(&pdev->dev);
-
- if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
static const struct amba_id etm4_ids[] = {
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 0541712b2bcb..3fb9d0a37d55 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -240,7 +240,7 @@ static int funnel_probe(struct device *dev, struct resource *res)
drvdata->pclk = coresight_get_enable_apb_pclk(dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
/*
* Map the device base for dynamic-funnel, which has been
@@ -283,8 +283,6 @@ static int funnel_probe(struct device *dev, struct resource *res)
out_disable_clk:
if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
clk_disable_unprepare(drvdata->atclk);
- if (ret && !IS_ERR_OR_NULL(drvdata->pclk))
- clk_disable_unprepare(drvdata->pclk);
return ret;
}
@@ -354,8 +352,6 @@ static void funnel_platform_remove(struct platform_device *pdev)
funnel_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
static const struct of_device_id funnel_match[] = {
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index ee7ee79f6cf7..87346617b852 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -247,7 +247,7 @@ static int replicator_probe(struct device *dev, struct resource *res)
drvdata->pclk = coresight_get_enable_apb_pclk(dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
/*
* Map the device base for dynamic-replicator, which has been
@@ -295,8 +295,6 @@ static int replicator_probe(struct device *dev, struct resource *res)
out_disable_clk:
if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
clk_disable_unprepare(drvdata->atclk);
- if (ret && !IS_ERR_OR_NULL(drvdata->pclk))
- clk_disable_unprepare(drvdata->pclk);
return ret;
}
@@ -334,8 +332,6 @@ static void replicator_platform_remove(struct platform_device *pdev)
replicator_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
#ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 26f9339f38b9..c32d0bd92f30 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -851,7 +851,7 @@ static int __stm_probe(struct device *dev, struct resource *res)
drvdata->pclk = coresight_get_enable_apb_pclk(dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
dev_set_drvdata(dev, drvdata);
base = devm_ioremap_resource(dev, res);
@@ -1033,8 +1033,6 @@ static void stm_platform_remove(struct platform_device *pdev)
__stm_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
#ifdef CONFIG_ACPI
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index ddca5ddf4ed2..517850d39a0e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -990,7 +990,7 @@ static int tmc_platform_probe(struct platform_device *pdev)
drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
dev_set_drvdata(&pdev->dev, drvdata);
pm_runtime_get_noresume(&pdev->dev);
@@ -1014,8 +1014,6 @@ static void tmc_platform_remove(struct platform_device *pdev)
__tmc_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
#ifdef CONFIG_PM
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index 97ef36f03ec2..4b9634941752 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -153,7 +153,7 @@ static int __tpiu_probe(struct device *dev, struct resource *res)
drvdata->pclk = coresight_get_enable_apb_pclk(dev);
if (IS_ERR(drvdata->pclk))
- return -ENODEV;
+ return PTR_ERR(drvdata->pclk);
dev_set_drvdata(dev, drvdata);
/* Validity for the resource is already checked by the AMBA core */
@@ -293,8 +293,6 @@ static void tpiu_platform_remove(struct platform_device *pdev)
__tpiu_remove(&pdev->dev);
pm_runtime_disable(&pdev->dev);
- if (!IS_ERR_OR_NULL(drvdata->pclk))
- clk_put(drvdata->pclk);
}
#ifdef CONFIG_ACPI
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index d79a242b271d..b888f6ed59b2 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -476,26 +476,16 @@ static inline bool is_coresight_device(void __iomem *base)
* Returns:
*
* clk - Clock is found and enabled
- * NULL - clock is not found
* ERROR - Clock is found but failed to enable
*/
static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
{
struct clk *pclk;
- int ret;
- pclk = clk_get(dev, "apb_pclk");
- if (IS_ERR(pclk)) {
- pclk = clk_get(dev, "apb");
- if (IS_ERR(pclk))
- return NULL;
- }
+ pclk = devm_clk_get_enabled(dev, "apb_pclk");
+ if (IS_ERR(pclk))
+ pclk = devm_clk_get_enabled(dev, "apb");
- ret = clk_prepare_enable(pclk);
- if (ret) {
- clk_put(pclk);
- return ERR_PTR(ret);
- }
return pclk;
}
--
2.34.1Even though this might seem to be being bike shedding, the subject
line above could be re-organized something like the following for
better clarity.
coresight: Properly/Appropriately disable programming clocks
On 4/23/25 20:47, Leo Yan wrote:
> Some CoreSight components have programming clocks (pclk) and are enabled
> using clk_get() and clk_prepare_enable(). However, in many cases, these
> clocks are not disabled when modules exit and only released by clk_put().
That is correct, it would be definitely better to disable these clocks
rather than doing a clk_put() that is prevalent for the pclk clocks in
context.
>
> To fix the issue, this commit refactors coresight_get_enable_apb_pclk()
> by replacing clk_get() and clk_prepare_enable() with
> devm_clk_get_enabled() for enabling APB clock. Callers are updated
> to reuse the returned error value.
>
> With the change, programming clocks are managed as resources in driver
> model layer, allowing clock cleanup to be handled automatically. As a
> result, manual cleanup operations are no longer needed and are removed
> from the Coresight drivers.
Looks correct. Although emphasizing the fact that devm_clk_get_enabled()
is the function which gets apb and apb_pclk clocks managed in the driver
model layer there after, might be better.
>
> Fixes: 73d779a03a76 ("coresight: etm4x: Change etm4_platform_driver driver for MMIO devices")
Seems right to classify this patch as a "Fixes: " as the clocks were not
handled properly earlier. The commit ID is also correct as it introduced
coresight_get_enable_apb_pclk() function.
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-catu.c | 9 ++-------
> drivers/hwtracing/coresight/coresight-cpu-debug.c | 6 +-----
> drivers/hwtracing/coresight/coresight-ctcu-core.c | 10 ++--------
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 9 ++-------
> drivers/hwtracing/coresight/coresight-funnel.c | 6 +-----
> drivers/hwtracing/coresight/coresight-replicator.c | 6 +-----
> drivers/hwtracing/coresight/coresight-stm.c | 4 +---
> drivers/hwtracing/coresight/coresight-tmc-core.c | 4 +---
> drivers/hwtracing/coresight/coresight-tpiu.c | 4 +---
All call sites have been changed.
> include/linux/coresight.h | 16 +++-------------
> 10 files changed, 15 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
> index 9fcda5e49253..c0a51ab0312c 100644
> --- a/drivers/hwtracing/coresight/coresight-catu.c
> +++ b/drivers/hwtracing/coresight/coresight-catu.c
> @@ -627,7 +627,7 @@ static int catu_platform_probe(struct platform_device *pdev)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
Returning PTR_ERR() on the clock after detecting a problem via IS_ERR() is correct.
>
> pm_runtime_get_noresume(&pdev->dev);
> pm_runtime_set_active(&pdev->dev);
> @@ -636,11 +636,8 @@ static int catu_platform_probe(struct platform_device *pdev)
> dev_set_drvdata(&pdev->dev, drvdata);
> ret = __catu_probe(&pdev->dev, res);
> pm_runtime_put(&pdev->dev);
> - if (ret) {
> + if (ret)
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> - }
>
> return ret;
> }
> @@ -654,8 +651,6 @@ static void catu_platform_remove(struct platform_device *pdev)
>
> __catu_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
This kind of error handling is not required any more as it would be
handled in the driver model layer here after.
> }
>
> #ifdef CONFIG_PM
> diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> index 342c3aaf414d..744b6f9b065e 100644
> --- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
> +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
> @@ -699,7 +699,7 @@ static int debug_platform_probe(struct platform_device *pdev)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
>
> dev_set_drvdata(&pdev->dev, drvdata);
> pm_runtime_get_noresume(&pdev->dev);
> @@ -710,8 +710,6 @@ static int debug_platform_probe(struct platform_device *pdev)
> if (ret) {
> pm_runtime_put_noidle(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
> return ret;
> }
> @@ -725,8 +723,6 @@ static void debug_platform_remove(struct platform_device *pdev)
>
> __debug_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
Should not these IS_ERR_OR_NULL() here be changed to IS_ERR() ?
Because now there could not be a NULL return value.
drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev)
#ifdef CONFIG_PM
static int debug_runtime_suspend(struct device *dev)
{
struct debug_drvdata *drvdata = dev_get_drvdata(dev);
if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
clk_disable_unprepare(drvdata->pclk);
return 0;
}
static int debug_runtime_resume(struct device *dev)
{
struct debug_drvdata *drvdata = dev_get_drvdata(dev);
if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
clk_prepare_enable(drvdata->pclk);
return 0;
}
#endif
There might more instances like these as well.
git grep IS_ERR_OR_NULL drivers/hwtracing/coresight/ | grep "drvdata->pclk"
drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>
> #ifdef CONFIG_ACPI
> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index c6bafc96db96..de279efe3405 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> @@ -209,7 +209,7 @@ static int ctcu_probe(struct platform_device *pdev)
>
> drvdata->apb_clk = coresight_get_enable_apb_pclk(dev);
> if (IS_ERR(drvdata->apb_clk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->apb_clk);
>
> cfgs = of_device_get_match_data(dev);
> if (cfgs) {
> @@ -233,12 +233,8 @@ static int ctcu_probe(struct platform_device *pdev)
> desc.access = CSDEV_ACCESS_IOMEM(base);
>
> drvdata->csdev = coresight_register(&desc);
> - if (IS_ERR(drvdata->csdev)) {
> - if (!IS_ERR_OR_NULL(drvdata->apb_clk))
> - clk_put(drvdata->apb_clk);
> -
> + if (IS_ERR(drvdata->csdev))
> return PTR_ERR(drvdata->csdev);
> - }
>
> return 0;
> }
> @@ -275,8 +271,6 @@ static void ctcu_platform_remove(struct platform_device *pdev)
>
> ctcu_remove(pdev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->apb_clk))
> - clk_put(drvdata->apb_clk);
> }
>
> #ifdef CONFIG_PM
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 537d57006a25..ff4ac4b686c4 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -2237,14 +2237,12 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
>
> if (res) {
> drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> - if (IS_ERR(drvdata->base)) {
> - clk_put(drvdata->pclk);
> + if (IS_ERR(drvdata->base))
> return PTR_ERR(drvdata->base);
> - }
> }
>
> dev_set_drvdata(&pdev->dev, drvdata);
> @@ -2351,9 +2349,6 @@ static void etm4_remove_platform_dev(struct platform_device *pdev)
> if (drvdata)
> etm4_remove_dev(drvdata);
> pm_runtime_disable(&pdev->dev);
> -
> - if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
>
> static const struct amba_id etm4_ids[] = {
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 0541712b2bcb..3fb9d0a37d55 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -240,7 +240,7 @@ static int funnel_probe(struct device *dev, struct resource *res)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
>
> /*
> * Map the device base for dynamic-funnel, which has been
> @@ -283,8 +283,6 @@ static int funnel_probe(struct device *dev, struct resource *res)
> out_disable_clk:
> if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
> clk_disable_unprepare(drvdata->atclk);
> - if (ret && !IS_ERR_OR_NULL(drvdata->pclk))
> - clk_disable_unprepare(drvdata->pclk);
> return ret;
> }
>
> @@ -354,8 +352,6 @@ static void funnel_platform_remove(struct platform_device *pdev)
>
> funnel_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
>
> static const struct of_device_id funnel_match[] = {
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index ee7ee79f6cf7..87346617b852 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -247,7 +247,7 @@ static int replicator_probe(struct device *dev, struct resource *res)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
>
> /*
> * Map the device base for dynamic-replicator, which has been
> @@ -295,8 +295,6 @@ static int replicator_probe(struct device *dev, struct resource *res)
> out_disable_clk:
> if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
> clk_disable_unprepare(drvdata->atclk);
> - if (ret && !IS_ERR_OR_NULL(drvdata->pclk))
> - clk_disable_unprepare(drvdata->pclk);
> return ret;
> }
>
> @@ -334,8 +332,6 @@ static void replicator_platform_remove(struct platform_device *pdev)
>
> replicator_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
>
> #ifdef CONFIG_PM
> diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
> index 26f9339f38b9..c32d0bd92f30 100644
> --- a/drivers/hwtracing/coresight/coresight-stm.c
> +++ b/drivers/hwtracing/coresight/coresight-stm.c
> @@ -851,7 +851,7 @@ static int __stm_probe(struct device *dev, struct resource *res)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
> dev_set_drvdata(dev, drvdata);
>
> base = devm_ioremap_resource(dev, res);
> @@ -1033,8 +1033,6 @@ static void stm_platform_remove(struct platform_device *pdev)
>
> __stm_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
>
> #ifdef CONFIG_ACPI
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index ddca5ddf4ed2..517850d39a0e 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -990,7 +990,7 @@ static int tmc_platform_probe(struct platform_device *pdev)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
>
> dev_set_drvdata(&pdev->dev, drvdata);
> pm_runtime_get_noresume(&pdev->dev);
> @@ -1014,8 +1014,6 @@ static void tmc_platform_remove(struct platform_device *pdev)
>
> __tmc_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
>
> #ifdef CONFIG_PM
> diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
> index 97ef36f03ec2..4b9634941752 100644
> --- a/drivers/hwtracing/coresight/coresight-tpiu.c
> +++ b/drivers/hwtracing/coresight/coresight-tpiu.c
> @@ -153,7 +153,7 @@ static int __tpiu_probe(struct device *dev, struct resource *res)
>
> drvdata->pclk = coresight_get_enable_apb_pclk(dev);
> if (IS_ERR(drvdata->pclk))
> - return -ENODEV;
> + return PTR_ERR(drvdata->pclk);
> dev_set_drvdata(dev, drvdata);
>
> /* Validity for the resource is already checked by the AMBA core */
> @@ -293,8 +293,6 @@ static void tpiu_platform_remove(struct platform_device *pdev)
>
> __tpiu_remove(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> - if (!IS_ERR_OR_NULL(drvdata->pclk))
> - clk_put(drvdata->pclk);
> }
>
> #ifdef CONFIG_ACPI
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index d79a242b271d..b888f6ed59b2 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -476,26 +476,16 @@ static inline bool is_coresight_device(void __iomem *base)
> * Returns:
> *
> * clk - Clock is found and enabled
> - * NULL - clock is not found
NULL is not a return value any more.
> * ERROR - Clock is found but failed to enable
> */
> static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
> {
> struct clk *pclk;
> - int ret;
>
> - pclk = clk_get(dev, "apb_pclk");
> - if (IS_ERR(pclk)) {
> - pclk = clk_get(dev, "apb");
> - if (IS_ERR(pclk))
> - return NULL;
> - }
> + pclk = devm_clk_get_enabled(dev, "apb_pclk");
> + if (IS_ERR(pclk))
> + pclk = devm_clk_get_enabled(dev, "apb");
>
> - ret = clk_prepare_enable(pclk);
> - if (ret) {
> - clk_put(pclk);
> - return ERR_PTR(ret);
> - }
> return pclk;
> }
>
Updated coresight_get_enable_apb_pclk() LGTM. IS_ERR() on the returned
pclk value can indicate, if there was a problem in finding or enabling
apb/apb_pclk clock.
On Fri, May 02, 2025 at 11:40:31AM +0530, Anshuman Khandual wrote:
> Even though this might seem to be being bike shedding, the subject
> line above could be re-organized something like the following for
> better clarity.
>
> coresight: Properly/Appropriately disable programming clocks
Sure. I will change the subject to this.
[...]
> > @@ -725,8 +723,6 @@ static void debug_platform_remove(struct platform_device *pdev)
> >
> > __debug_remove(&pdev->dev);
> > pm_runtime_disable(&pdev->dev);
> > - if (!IS_ERR_OR_NULL(drvdata->pclk))
> > - clk_put(drvdata->pclk);
> > }
> Should not these IS_ERR_OR_NULL() here be changed to IS_ERR() ?
For the case above, after changed to devm_clk_get_enabled() for the
enabling programming clocks, we don't need any special handling and
leave the clock disabling and releasing to the device model layer.
> Because now there could not be a NULL return value.
>
> drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev)
>
> #ifdef CONFIG_PM
> static int debug_runtime_suspend(struct device *dev)
> {
> struct debug_drvdata *drvdata = dev_get_drvdata(dev);
>
> if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> clk_disable_unprepare(drvdata->pclk);
> return 0;
> }
>
> static int debug_runtime_resume(struct device *dev)
> {
> struct debug_drvdata *drvdata = dev_get_drvdata(dev);
>
> if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> clk_prepare_enable(drvdata->pclk);
> return 0;
> }
> #endif
> There might more instances like these as well.
>
> git grep IS_ERR_OR_NULL drivers/hwtracing/coresight/ | grep "drvdata->pclk"
> drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
> drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
I would like the current patch to focus on the issue of disabling /
releasing the programming clocks.
Though the IS_ERR_OR_NULL() check is redundant, it does not cause
issue or regression. The refactoring is left in patch 09 for removing
IS_ERR_OR_NULL() checks.
Does this make sense?
Thanks
Leo
On 5/6/25 15:24, Leo Yan wrote:
> On Fri, May 02, 2025 at 11:40:31AM +0530, Anshuman Khandual wrote:
>> Even though this might seem to be being bike shedding, the subject
>> line above could be re-organized something like the following for
>> better clarity.
>>
>> coresight: Properly/Appropriately disable programming clocks
>
> Sure. I will change the subject to this.
>
> [...]
>
>>> @@ -725,8 +723,6 @@ static void debug_platform_remove(struct platform_device *pdev)
>>>
>>> __debug_remove(&pdev->dev);
>>> pm_runtime_disable(&pdev->dev);
>>> - if (!IS_ERR_OR_NULL(drvdata->pclk))
>>> - clk_put(drvdata->pclk);
>>> }
>> Should not these IS_ERR_OR_NULL() here be changed to IS_ERR() ?
>
> For the case above, after changed to devm_clk_get_enabled() for the
> enabling programming clocks, we don't need any special handling and
> leave the clock disabling and releasing to the device model layer.
So it can be left unchanged for now and cleaned up later ?
>
>> Because now there could not be a NULL return value.
>>
>> drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev)
>>
>> #ifdef CONFIG_PM
>> static int debug_runtime_suspend(struct device *dev)
>> {
>> struct debug_drvdata *drvdata = dev_get_drvdata(dev);
>>
>> if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> clk_disable_unprepare(drvdata->pclk);
>> return 0;
>> }
>>
>> static int debug_runtime_resume(struct device *dev)
>> {
>> struct debug_drvdata *drvdata = dev_get_drvdata(dev);
>>
>> if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> clk_prepare_enable(drvdata->pclk);
>> return 0;
>> }
>> #endif
>
>> There might more instances like these as well.
>>
>> git grep IS_ERR_OR_NULL drivers/hwtracing/coresight/ | grep "drvdata->pclk"
>> drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-cpu-debug.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-funnel.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-replicator.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-stm.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>> drivers/hwtracing/coresight/coresight-tpiu.c: if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk))
>
> I would like the current patch to focus on the issue of disabling /
> releasing the programming clocks.
>
> Though the IS_ERR_OR_NULL() check is redundant, it does not cause
> issue or regression. The refactoring is left in patch 09 for removing
> IS_ERR_OR_NULL() checks.
>
> Does this make sense?
Yes, it does now.
© 2016 - 2025 Red Hat, Inc.