Re: [PATCH] coresight: cti: Add power management to CTI driver

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Good day Mike,

On Wed, Apr 15, 2020 at 09:11:16PM +0100, Mike Leach wrote:
> Add in CPU hotplug and CPU idle power management handling to CTI driver.
> 
> Only CPU bound CTIs will register with the PM routines, to enable PM of
> CTI alongside PM of ETM devices.
> 
> Need separate HP PM event CPUHP_AP_ARM_CORESIGHT_CTI_STARTING as events
> can only be registered once.
> 
> Applies to Linux 5.7-rc1, tested on platforms Juno-r1 and DB410c.
> 
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>  drivers/hwtracing/coresight/coresight-cti.c | 163 +++++++++++++++++++-
>  include/linux/cpuhotplug.h                  |   1 +

Please split this set in half, one for pm_runtime and another one for hotplug. 

>  2 files changed, 156 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
> index aa6e0249bd70..31996a20ce69 100644
> --- a/drivers/hwtracing/coresight/coresight-cti.c
> +++ b/drivers/hwtracing/coresight/coresight-cti.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/property.h>
> +#include <linux/cpu_pm.h>
>  #include "coresight-cti.h"
>  
>  /**
> @@ -27,6 +28,12 @@ static DEFINE_MUTEX(ect_mutex);
>  #define csdev_to_cti_drvdata(csdev)	\
>  	dev_get_drvdata(csdev->dev.parent)
>  
> +/* power management handling */
> +static int nr_cti_cpu;
> +
> +/* quick lookup list for CPU bound CTIs when power handling */
> +static struct cti_drvdata *cti_cpu_drvdata[NR_CPUS];
> +
>  /*
>   * CTI naming. CTI bound to cores will have the name cti_cpu<N> where
>   * N is the CPU ID. System CTIs will have the name cti_sys<I> where I
> @@ -36,7 +43,7 @@ static DEFINE_MUTEX(ect_mutex);
>   */
>  DEFINE_CORESIGHT_DEVLIST(cti_sys_devs, "cti_sys");
>  
> -/* write set of regs to hardware - call with spinlock claimed */
> +/* write set of regs to hardware - call with spinlock claimed / cpu context */
>  void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
>  {
>  	struct cti_config *config = &drvdata->config;
> @@ -73,7 +80,7 @@ static void cti_enable_hw_smp_call(void *info)
>  }
>  
>  /* write regs to hardware and enable */
> -static int cti_enable_hw(struct cti_drvdata *drvdata)
> +static int cti_enable_hw(struct cti_drvdata *drvdata, bool hp_re_enable)

s/hp_re_enable/hp_event


>  {
>  	struct cti_config *config = &drvdata->config;
>  	struct device *dev = &drvdata->csdev->dev;
> @e -82,8 +89,11 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
>  	pm_runtime_get_sync(dev->parent);
>  	spin_lock(&drvdata->spinlock);
>  
> -	/* no need to do anything if enabled or unpowered*/
> -	if (config->hw_enabled || !config->hw_powered)
> +	/* no need to do anything if enabled or unpowered */
> +	if (!hp_re_enable) {
> +		if (config->hw_enabled || !config->hw_powered)
> +			goto cti_state_unchanged;
> +	} else if (!atomic_read(&drvdata->config.enable_req_count))
>  		goto cti_state_unchanged;
>  
>  	/* claim the device */
> @@ -91,7 +101,7 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
>  	if (rc)
>  		goto cti_err_not_enabled;
>  
> -	if (drvdata->ctidev.cpu >= 0) {
> +	if (drvdata->ctidev.cpu >= 0 && !hp_re_enable) {
>  		rc = smp_call_function_single(drvdata->ctidev.cpu,
>  					      cti_enable_hw_smp_call,
>  					      drvdata, 1);
> @@ -102,12 +112,15 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
>  	}
>  
>  	config->hw_enabled = true;
> -	atomic_inc(&drvdata->config.enable_req_count);
> +	if (!hp_re_enable)
> +		atomic_inc(&drvdata->config.enable_req_count);
> +

We are dealing with 2 different variables, i.e CPU CTI vs global CTI, hotplug
event vs framework event.  I wonder if we have just enough complexity to spin
off two flavours of cti_enable_hw()...  

>  	spin_unlock(&drvdata->spinlock);
>  	return rc;
>  
>  cti_state_unchanged:
> -	atomic_inc(&drvdata->config.enable_req_count);
> +	if (!hp_re_enable)
> +		atomic_inc(&drvdata->config.enable_req_count);
>  
>  	/* cannot enable due to error */
>  cti_err_not_enabled:
> @@ -563,12 +576,127 @@ static void cti_remove_conn_xrefs(struct cti_drvdata *drvdata)
>  	}
>  }
>  
> +/** cti PM callbacks **/
> +static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
> +			     void *v)
> +{
> +	struct cti_drvdata *drvdata;
> +	unsigned int cpu = smp_processor_id();
> +
> +	if (!cti_cpu_drvdata[cpu])
> +		return NOTIFY_OK;
> +
> +	drvdata = cti_cpu_drvdata[cpu];
> +
> +	if (WARN_ON_ONCE(drvdata->ctidev.cpu != cpu))
> +		return NOTIFY_BAD;
> +
> +	switch (cmd) {
> +	case CPU_PM_ENTER:
> +		/* CTI regs all static - we have a copy & nothing to save */
> +		drvdata->config.hw_powered = false;
> +		if (drvdata->config.hw_enabled)
> +			coresight_disclaim_device(drvdata->base);
> +		break;
> +
> +	case CPU_PM_ENTER_FAILED:
> +		drvdata->config.hw_powered = true;
> +		if (drvdata->config.hw_enabled) {
> +			if (coresight_claim_device(drvdata->base))
> +				drvdata->config.hw_enabled = false;
> +		}
> +		break;
> +
> +	case CPU_PM_EXIT:
> +		/* write hardware registers to re-enable. */
> +		drvdata->config.hw_powered = true;
> +		drvdata->config.hw_enabled = false;
> +
> +		/* check enable reference count to enable HW */
> +		if (atomic_read(&drvdata->config.enable_req_count)) {
> +			/* check we can claim the device as we re-power */
> +			if (coresight_claim_device(drvdata->base))
> +				return NOTIFY_OK;
> +
> +			drvdata->config.hw_enabled = true;
> +			cti_write_all_hw_regs(drvdata);
> +		}
> +		break;
> +	default:
> +		return NOTIFY_DONE;
> +	}

For all of the above the config's hw_enabled and hw_powered are manipulated
without taking drvdata->spinlock.  Please use spin_trylock() and spin_unlock().

> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block cti_cpu_pm_nb = {
> +	.notifier_call = cti_cpu_pm_notify,
> +};
> +
> +static int cti_cpu_pm_register(void)
> +{
> +	if (IS_ENABLED(CONFIG_CPU_PM))
> +		return cpu_pm_register_notifier(&cti_cpu_pm_nb);
> +
> +	return 0;
> +}
> +
> +static void cti_cpu_pm_unregister(void)
> +{
> +	if (IS_ENABLED(CONFIG_CPU_PM))
> +		cpu_pm_unregister_notifier(&cti_cpu_pm_nb);
> +}
> +
> +/* CPU HP handlers */
> +static int cti_starting_cpu(unsigned int cpu)
> +{
> +	struct cti_drvdata *drvdata = cti_cpu_drvdata[cpu];
> +
> +	if (!drvdata)
> +		return 0;
> +
> +	spin_lock(&drvdata->spinlock);
> +	drvdata->config.hw_powered = true;
> +	spin_unlock(&drvdata->spinlock);
> +
> +	cti_enable_hw(drvdata, true);
> +	return 0;
> +}
> +
> +static int cti_dying_cpu(unsigned int cpu)
> +{
> +	struct cti_drvdata *drvdata = cti_cpu_drvdata[cpu];
> +
> +	if (!drvdata)
> +		return 0;
> +
> +	spin_lock(&drvdata->spinlock);
> +	drvdata->config.hw_powered = false;
> +	coresight_disclaim_device(drvdata->base);
> +	spin_unlock(&drvdata->spinlock);
> +	return 0;
> +}
> +
> +/* release PM registrations */
> +static void cti_pm_release(struct cti_drvdata *drvdata)
> +{
> +	if (drvdata->ctidev.cpu >= 0) {
> +		if (--nr_cti_cpu == 0) {
> +			cti_cpu_pm_unregister();
> +
> +			cpuhp_remove_state_nocalls(
> +				CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
> +		}
> +		cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;
> +	}

        if (drvdata->ctidev.cpu < 0) 
                return;

        if (--nr_cti_cpu > 0)
                return;

	cti_cpu_pm_unregister();
        cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING);
	cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL;

> +
>  /** cti ect operations **/
>  int cti_enable(struct coresight_device *csdev)
>  {
>  	struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
>  
> -	return cti_enable_hw(drvdata);
> +	return cti_enable_hw(drvdata, false);
>  }
>  
>  int cti_disable(struct coresight_device *csdev)
> @@ -598,6 +726,7 @@ static void cti_device_release(struct device *dev)
>  
>  	mutex_lock(&ect_mutex);
>  	cti_remove_conn_xrefs(drvdata);
> +	cti_pm_release(drvdata);
>  
>  	/* remove from the list */
>  	list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, node) {
> @@ -673,6 +802,23 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto err_out;
>  	}
>  
> +	/* setup CPU power management handling for CPU bound CTI devices. */
> +	if (drvdata->ctidev.cpu >= 0) {
> +		cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata;
> +		if (!nr_cti_cpu++) {
> +			cpus_read_lock();
> +			cpuhp_setup_state_nocalls_cpuslocked(
> +				CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,

Any reasons CPUHP_AP_ARM_CORESIGHT_STARTING couldn't be reused?

Thanks,
Mathieu

> +				"arm/coresight_cti:starting",
> +				cti_starting_cpu, cti_dying_cpu);
> +
> +			ret = cti_cpu_pm_register();
> +			cpus_read_unlock();
> +			if (ret)
> +				goto err_out;
> +		}
> +	}
> +
>  	/* create dynamic attributes for connections */
>  	ret = cti_create_cons_sysfs(dev, drvdata);
>  	if (ret) {
> @@ -711,6 +857,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
>  	return 0;
>  
>  err_out:
> +	cti_pm_release(drvdata);
>  	return ret;
>  }
>  
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 77d70b633531..6dc7332307ca 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -142,6 +142,7 @@ enum cpuhp_state {
>  	CPUHP_AP_ARM_XEN_STARTING,
>  	CPUHP_AP_ARM_KVMPV_STARTING,
>  	CPUHP_AP_ARM_CORESIGHT_STARTING,
> +	CPUHP_AP_ARM_CORESIGHT_CTI_STARTING,
>  	CPUHP_AP_ARM64_ISNDEP_STARTING,
>  	CPUHP_AP_SMPCFD_DYING,
>  	CPUHP_AP_X86_TBOOT_DYING,
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel