Messages in this thread | | | Date | Thu, 15 Feb 2024 11:25:48 +0000 | Subject | Re: [PATCH V4 05/11] coresight: replicator: Move ACPI support from AMBA driver to platform driver | From | Suzuki K Poulose <> |
| |
On 15/02/2024 11:23, Suzuki K Poulose wrote: > Hi Anshuman > > On 23/01/2024 05:46, Anshuman Khandual wrote: >> Add support for the dynamic replicator device in the platform driver, >> which >> can then be used on ACPI based platforms. This change would now allow >> runtime power management for repliacator devices on ACPI based systems. >> >> The driver would try to enable the APB clock if available. Also, >> rename the >> code to reflect the fact that it now handles both static and dynamic >> replicators. >> >> Cc: Lorenzo Pieralisi <lpieralisi@kernel.org> >> Cc: Sudeep Holla <sudeep.holla@arm.com> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mike Leach <mike.leach@linaro.org> >> Cc: James Clark <james.clark@arm.com> >> Cc: linux-acpi@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Cc: coresight@lists.linaro.org >> Tested-by: Sudeep Holla <sudeep.holla@arm.com> # Boot and driver probe >> only >> Acked-by: Sudeep Holla <sudeep.holla@arm.com> # For ACPI related changes >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > I think the patch is doing three different things: > > 1) Use new helper to register/remove AMBA/Platform drivers > 2) Refactor replicator_probe() to make sure it can be reused for > platform/amba driver, by moving the pm_runtime_put() to the callers. > 3) Actually moving the ACPI driver to Platform driver > > While (1) and (3) are obvious, (2) gave me hard time to review this > patch, without proper description. If you don't mind, are you able to > split the patch and add proper description of the 3 changes mentioned > above. >
You could even move (1) for all the existing drivers into a single patch or even fold it with the patch that introduces the helpers. That way it is cleaner and easier to review. And (2) & (3) could be in the same patch for each driver, but please add something in the description for (2).
Suzuki
> > Kind regards > Suzuki > >> --- >> Changes in V4: >> >> - Added pm_runtime_disable() in replicator_platform_probe() >> - Changed replicator_platform_remove() for platform_driver->remove_new() >> >> drivers/acpi/arm64/amba.c | 1 - >> .../coresight/coresight-replicator.c | 81 ++++++++++--------- >> 2 files changed, 43 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/acpi/arm64/amba.c b/drivers/acpi/arm64/amba.c >> index 171b5c2c7edd..270f4e3819a2 100644 >> --- a/drivers/acpi/arm64/amba.c >> +++ b/drivers/acpi/arm64/amba.c >> @@ -27,7 +27,6 @@ static const struct acpi_device_id amba_id_list[] = { >> {"ARMHC503", 0}, /* ARM CoreSight Debug */ >> {"ARMHC979", 0}, /* ARM CoreSight TPIU */ >> {"ARMHC97C", 0}, /* ARM CoreSight SoC-400 TMC, SoC-600 ETF/ETB */ >> - {"ARMHC98D", 0}, /* ARM CoreSight Dynamic Replicator */ >> {"ARMHC9CA", 0}, /* ARM CoreSight CATU */ >> {"ARMHC9FF", 0}, /* ARM CoreSight Dynamic Funnel */ >> {"", 0}, >> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c >> b/drivers/hwtracing/coresight/coresight-replicator.c >> index 91d93060dda5..9b5f52725f43 100644 >> --- a/drivers/hwtracing/coresight/coresight-replicator.c >> +++ b/drivers/hwtracing/coresight/coresight-replicator.c >> @@ -31,6 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(replicator_devs, >> "replicator"); >> * @base: memory mapped base address for this component. Also >> indicates >> * whether this one is programmable or not. >> * @atclk: optional clock for the core parts of the replicator. >> + * @pclk: APB clock if present, otherwise NULL >> * @csdev: component vitals needed by the framework >> * @spinlock: serialize enable/disable operations. >> * @check_idfilter_val: check if the context is lost upon clock >> removal. >> @@ -38,6 +39,7 @@ DEFINE_CORESIGHT_DEVLIST(replicator_devs, >> "replicator"); >> struct replicator_drvdata { >> void __iomem *base; >> struct clk *atclk; >> + struct clk *pclk; >> struct coresight_device *csdev; >> spinlock_t spinlock; >> bool check_idfilter_val; >> @@ -243,6 +245,10 @@ static int replicator_probe(struct device *dev, >> struct resource *res) >> return ret; >> } >> + drvdata->pclk = coresight_get_enable_apb_pclk(dev); >> + if (IS_ERR(drvdata->pclk)) >> + return -ENODEV; >> + >> /* >> * Map the device base for dynamic-replicator, which has been >> * validated by AMBA core >> @@ -285,7 +291,6 @@ static int replicator_probe(struct device *dev, >> struct resource *res) >> } >> replicator_reset(drvdata); >> - pm_runtime_put(dev); >> out_disable_clk: >> if (ret && !IS_ERR_OR_NULL(drvdata->atclk)) >> @@ -301,29 +306,33 @@ static int replicator_remove(struct device *dev) >> return 0; >> } >> -static int static_replicator_probe(struct platform_device *pdev) >> +static int replicator_platform_probe(struct platform_device *pdev) >> { >> + struct resource *res = platform_get_resource(pdev, >> IORESOURCE_MEM, 0); >> int ret; >> pm_runtime_get_noresume(&pdev->dev); >> pm_runtime_set_active(&pdev->dev); >> pm_runtime_enable(&pdev->dev); >> - /* Static replicators do not have programming base */ >> - ret = replicator_probe(&pdev->dev, NULL); >> - >> - if (ret) { >> - pm_runtime_put_noidle(&pdev->dev); >> + ret = replicator_probe(&pdev->dev, res); >> + pm_runtime_put(&pdev->dev); >> + if (ret) >> pm_runtime_disable(&pdev->dev); >> - } >> return ret; >> } >> -static void static_replicator_remove(struct platform_device *pdev) >> +static void replicator_platform_remove(struct platform_device *pdev) >> { >> - replicator_remove(&pdev->dev); >> + struct replicator_drvdata *drvdata = dev_get_drvdata(&pdev->dev); >> + >> + if (drvdata) >> + replicator_remove(&pdev->dev); >> + >> pm_runtime_disable(&pdev->dev); >> + if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) >> + clk_put(drvdata->pclk); >> } >> #ifdef CONFIG_PM >> @@ -334,6 +343,8 @@ static int replicator_runtime_suspend(struct >> device *dev) >> if (drvdata && !IS_ERR(drvdata->atclk)) >> clk_disable_unprepare(drvdata->atclk); >> + if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) >> + clk_disable_unprepare(drvdata->pclk); >> return 0; >> } >> @@ -344,6 +355,8 @@ static int replicator_runtime_resume(struct device >> *dev) >> if (drvdata && !IS_ERR(drvdata->atclk)) >> clk_prepare_enable(drvdata->atclk); >> + if (drvdata && !IS_ERR_OR_NULL(drvdata->pclk)) >> + clk_prepare_enable(drvdata->pclk); >> return 0; >> } >> #endif >> @@ -353,31 +366,32 @@ static const struct dev_pm_ops >> replicator_dev_pm_ops = { >> replicator_runtime_resume, NULL) >> }; >> -static const struct of_device_id static_replicator_match[] = { >> +static const struct of_device_id replicator_match[] = { >> {.compatible = "arm,coresight-replicator"}, >> {.compatible = "arm,coresight-static-replicator"}, >> {} >> }; >> -MODULE_DEVICE_TABLE(of, static_replicator_match); >> +MODULE_DEVICE_TABLE(of, replicator_match); >> #ifdef CONFIG_ACPI >> -static const struct acpi_device_id static_replicator_acpi_ids[] = { >> +static const struct acpi_device_id replicator_acpi_ids[] = { >> {"ARMHC985", 0}, /* ARM CoreSight Static Replicator */ >> + {"ARMHC98D", 0}, /* ARM CoreSight Dynamic Replicator */ >> {} >> }; >> -MODULE_DEVICE_TABLE(acpi, static_replicator_acpi_ids); >> +MODULE_DEVICE_TABLE(acpi, replicator_acpi_ids); >> #endif >> -static struct platform_driver static_replicator_driver = { >> - .probe = static_replicator_probe, >> - .remove_new = static_replicator_remove, >> +static struct platform_driver replicator_driver = { >> + .probe = replicator_platform_probe, >> + .remove_new = replicator_platform_remove, >> .driver = { >> - .name = "coresight-static-replicator", >> + .name = "coresight-replicator", >> /* THIS_MODULE is taken care of by >> platform_driver_register() */ >> - .of_match_table = of_match_ptr(static_replicator_match), >> - .acpi_match_table = ACPI_PTR(static_replicator_acpi_ids), >> + .of_match_table = of_match_ptr(replicator_match), >> + .acpi_match_table = ACPI_PTR(replicator_acpi_ids), >> .pm = &replicator_dev_pm_ops, >> .suppress_bind_attrs = true, >> }, >> @@ -386,7 +400,13 @@ static struct platform_driver >> static_replicator_driver = { >> static int dynamic_replicator_probe(struct amba_device *adev, >> const struct amba_id *id) >> { >> - return replicator_probe(&adev->dev, &adev->res); >> + int ret; >> + >> + ret = replicator_probe(&adev->dev, &adev->res); >> + if (!ret) >> + pm_runtime_put(&adev->dev); >> + >> + return ret; >> } >> static void dynamic_replicator_remove(struct amba_device *adev) >> @@ -416,27 +436,12 @@ static struct amba_driver >> dynamic_replicator_driver = { >> static int __init replicator_init(void) >> { >> - int ret; >> - >> - ret = platform_driver_register(&static_replicator_driver); >> - if (ret) { >> - pr_info("Error registering platform driver\n"); >> - return ret; >> - } >> - >> - ret = amba_driver_register(&dynamic_replicator_driver); >> - if (ret) { >> - pr_info("Error registering amba driver\n"); >> - platform_driver_unregister(&static_replicator_driver); >> - } >> - >> - return ret; >> + return coresight_init_driver("replicator", >> &dynamic_replicator_driver, &replicator_driver); >> } >> static void __exit replicator_exit(void) >> { >> - platform_driver_unregister(&static_replicator_driver); >> - amba_driver_unregister(&dynamic_replicator_driver); >> + coresight_remove_driver(&dynamic_replicator_driver, >> &replicator_driver); >> } >> module_init(replicator_init); >
| |