Re: [PATCH V2 5/7] coresight: tmc: Move ACPI support from AMBA driver to platform driver

[Anshuman Khandual]

On 12/4/23 16:24, James Clark wrote:
> 
> 
> On 01/12/2023 06:20, Anshuman Khandual wrote:
>> Add support for the tmc devices in the platform driver, which can then be
>> used on ACPI based platforms. This change would now allow runtime power
>> management for ACPI based systems. The driver would try to enable the APB
>> clock if available.
>>
> [...]
>> -module_amba_driver(tmc_driver);
>> +static int tmc_platform_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	struct tmc_drvdata *drvdata;
>> +	int ret = 0;
>> +
>> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
>> +	if (!drvdata)
>> +		return -ENOMEM;
>> +
>> +	drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
>> +	if (IS_ERR(drvdata->pclk))
>> +		return -ENODEV;
>> +
>> +	dev_set_drvdata(&pdev->dev, drvdata);
>> +	pm_runtime_get_noresume(&pdev->dev);
>> +	pm_runtime_set_active(&pdev->dev);
>> +	pm_runtime_enable(&pdev->dev);
>> +
>> +	ret = __tmc_probe(&pdev->dev, res, NULL);
>> +	if (ret) {
>> +		pm_runtime_put_noidle(&pdev->dev);
>> +		pm_runtime_disable(&pdev->dev);
>> +	}
> 
> I'm not sure if these pm_runtime()s are right because there is already a
> put inside of __tmc_probe() if it fails. If you unload and then reload

Actually there is a pm_runtime_put() on the success path, not when it
fails. So pm_runtime_put() gets called when __tmc_probe() returns 0.

__tmc_probe()
{
	....
        ret = misc_register(&drvdata->miscdev);
        if (ret)
                coresight_unregister(drvdata->csdev);
        else
                pm_runtime_put(dev);
out:
        return ret;
}

tmc_platform_probe()
{
	....
        pm_runtime_get_noresume(&pdev->dev);
        pm_runtime_set_active(&pdev->dev);
        pm_runtime_enable(&pdev->dev);

        ret = __tmc_probe(&pdev->dev, res, NULL);
        if (ret) {
                pm_runtime_put_noidle(&pdev->dev);
                pm_runtime_disable(&pdev->dev);
        }
        return ret;
}

tmc_probe()
{
	....
	return __tmc_probe(&adev->dev, &adev->res, coresight_get_uci_data(id));
}

Currently pm_runtime_put() gets called

- In success path both for AMBA and platform drivers
- In error path only for platform driver

Although the problem might be with pm_runtime_disable() instead

- pm_runtime_disable() is not required in the platform driver probe() path
- But might be required in tmc_platform_remove() along with a clk_put()

> all the coresight modules with these patches you get these errors which
> are new:
> 
>   coresight-tpiu-platform ARMHC979:00: Unbalanced pm_runtime_enable!

The code is similar in TPIU platform driver as well.

>   CSCFG registered etm0
>   coresight etm0: CPU0: etm v4.2 initialized
>   CSCFG registered etm1
>   coresight etm1: CPU1: etm v4.2 initialized
>   CSCFG registered etm2
>   coresight etm2: CPU2: etm v4.2 initialized
>   CSCFG registered etm3
>   coresight etm3: CPU3: etm v4.2 initialized
>   coresight-tmc-platform ARMHC97C:00: Unbalanced pm_runtime_enable!
>   coresight-tmc-platform ARMHC97C:01: Unbalanced pm_runtime_enable!
>   coresight-tmc-platform ARMHC97C:02: Unbalanced pm_runtime_enable!
>   coresight-tmc-platform ARMHC97C:03: Unbalanced pm_runtime_enable!
> 
> It might be worth testing all of these pm_runtime()s, including the
> error case ones, because loading and unloading the modules doesn't even
> include the error scenarios, so there are probably more bad ones in
> there too.
The code is very similar in CATU, STM as well but debug_platform_remove()
seems to be doing this right.

I am not very familiar with all the power management aspects in coresight,
please do let me know if I missing something here.