| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b0c8b92e-53bc-4232-a748-ff3e6b94b112@arm.com>
Date: Thu, 15 Feb 2024 11:25:48 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Anshuman Khandual <anshuman.khandual@....com>,
linux-arm-kernel@...ts.infradead.org
Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>,
Sudeep Holla <sudeep.holla@....com>, Mike Leach <mike.leach@...aro.org>,
James Clark <james.clark@....com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH V4 05/11] coresight: replicator: Move ACPI support from
AMBA driver to platform driver
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@...nel.org>
>> Cc: Sudeep Holla <sudeep.holla@....com>
>> Cc: Suzuki K Poulose <suzuki.poulose@....com>
>> Cc: Mike Leach <mike.leach@...aro.org>
>> Cc: James Clark <james.clark@....com>
>> Cc: linux-acpi@...r.kernel.org
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: coresight@...ts.linaro.org
>> Tested-by: Sudeep Holla <sudeep.holla@....com> # Boot and driver probe
>> only
>> Acked-by: Sudeep Holla <sudeep.holla@....com> # For ACPI related changes
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....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);
>
Powered by blists - more mailing lists