Re: [PATCH V4 05/11] coresight: replicator: Move ACPI support from AMBA driver to platform driver

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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);
>