[PATCH 0/5] CPPC enhancements

[PATCH 0/5] CPPC enhancements

[Prashanth Prakash]

Hi,

This series has patches related to cppc that either improves the performance
or adds some features.

The CPPC batching patch has been pulled into this series after addressing
the comments from Alexey (POSTCHANGE issue) and Hoan(switch to mutex)

Thanks,
Prashanth

Ashwin Chaugule (2):
  ACPI/CPPC: restructure read/writes for efficient sys mapped reg ops
  ACPI/CPPC: add sysfs support to compute delivered performance

Prashanth Prakash (3):
  ACPI/CPPC: acquire pcc_lock only while accessing PCC subspace
  ACPI/CPPC: support for batching CPPC requests
  ACPI/CPPC: set a non-zero value for transition_latency

 drivers/acpi/cppc_acpi.c       | 427 +++++++++++++++++++++++++++++++++--------
 drivers/cpufreq/cppc_cpufreq.c |   1 +
 include/acpi/cppc_acpi.h       |   5 +-
 3 files changed, 353 insertions(+), 80 deletions(-)

-- 
Qualcomm Technologies, Inc. on behalf
of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/5] ACPI/CPPC: restructure read/writes for efficient sys mapped reg ops

[Prashanth Prakash]

From: Ashwin Chaugule <ashwin.chaugule@linaro.org>

For cases where sys mapped CPC registers need to be accessed
frequently, it helps immensly to pre-map them rather than map
and unmap for each operation. e.g. case where feedback counters
are sys mem map registers.

Restructure cpc_read/write and the cpc_regs structure to allow
pre-mapping the system addresses and unmap them when the CPU exits.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/cppc_acpi.c | 106 ++++++++++++++++++++++++++++++-----------------
 include/acpi/cppc_acpi.h |   1 +
 2 files changed, 70 insertions(+), 37 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 2e98173..28170c2 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -62,7 +62,6 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 /* This layer handles all the PCC specifics for CPPC. */
 static struct mbox_chan *pcc_channel;
 static void __iomem *pcc_comm_addr;
-static u64 comm_base_addr;
 static int pcc_subspace_idx = -1;
 static bool pcc_channel_acquired;
 static ktime_t deadline;
@@ -394,7 +393,6 @@ EXPORT_SYMBOL_GPL(acpi_get_psd_map);
 static int register_pcc_channel(int pcc_subspace_idx)
 {
 	struct acpi_pcct_hw_reduced *cppc_ss;
-	unsigned int len;
 	u64 usecs_lat;
 
 	if (pcc_subspace_idx >= 0) {
@@ -419,12 +417,6 @@ static int register_pcc_channel(int pcc_subspace_idx)
 			return -ENODEV;
 		}
 
-		/*
-		 * This is the shared communication region
-		 * for the OS and Platform to communicate over.
-		 */
-		comm_base_addr = cppc_ss->base_address;
-		len = cppc_ss->length;
 
 		/*
 		 * cppc_ss->latency is just a Nominal value. In reality
@@ -436,7 +428,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
 		pcc_mrtt = cppc_ss->min_turnaround_time;
 		pcc_mpar = cppc_ss->max_access_rate;
 
-		pcc_comm_addr = acpi_os_ioremap(comm_base_addr, len);
+		pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
 		if (!pcc_comm_addr) {
 			pr_err("Failed to ioremap PCC comm region mem\n");
 			return -ENOMEM;
@@ -545,6 +537,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 		goto out_free;
 	}
 
+	cpc_ptr->num_entries = num_ent;
+
 	/* Second entry should be revision. */
 	cpc_obj = &out_obj->package.elements[1];
 	if (cpc_obj->type == ACPI_TYPE_INTEGER)	{
@@ -585,7 +579,15 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 					pr_debug("Mismatched PCC ids.\n");
 					goto out_free;
 				}
-			} else if (gas_t->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
+				if (gas_t->address) {
+					void __iomem *addr;
+					addr = ioremap(gas_t->address, gas_t->bit_width/8);
+					if (!addr)
+						goto out_free;
+					cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;
+				}
+			} else {
 				/* Support only PCC and SYS MEM type regs */
 				pr_debug("Unsupported register type: %d\n", gas_t->space_id);
 				goto out_free;
@@ -623,6 +625,12 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	return 0;
 
 out_free:
+	/* Free all the mapped sys mem areas for this CPU */
+	for (i = 2; i < cpc_ptr->num_entries; i++) {
+		void __iomem *addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
+		if (addr)
+			iounmap(addr);
+	}
 	kfree(cpc_ptr);
 
 out_buf_free:
@@ -640,7 +648,17 @@ EXPORT_SYMBOL_GPL(acpi_cppc_processor_probe);
 void acpi_cppc_processor_exit(struct acpi_processor *pr)
 {
 	struct cpc_desc *cpc_ptr;
+	unsigned int i;
+	void __iomem *addr;
 	cpc_ptr = per_cpu(cpc_desc_ptr, pr->id);
+
+	/* Free all the mapped sys mem areas for this CPU */
+	for (i = 2; i < cpc_ptr->num_entries; i++) {
+		addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
+		if (addr)
+			iounmap(addr);
+	}
+
 	kfree(cpc_ptr);
 }
 EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
@@ -651,15 +669,27 @@ EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
  * we can directly write to it.
  */
 
-static int cpc_read(struct cpc_reg *reg, u64 *val)
+static int cpc_read(struct cpc_register_resource *reg_res, u64 *val)
 {
 	int ret_val = 0;
+	void __iomem *vaddr = 0;
+	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
+
+	if (reg_res->type == ACPI_TYPE_INTEGER) {
+		*val = reg_res->int_value;
+		return ret_val;
+	}
 
 	*val = 0;
-	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-		void __iomem *vaddr = GET_PCC_VADDR(reg->address);
+	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
+		vaddr = GET_PCC_VADDR(reg->address);
+	else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		vaddr = reg_res->sys_mem_vaddr;
+	else
+		return acpi_os_read_memory((acpi_physical_address)reg->address,
+				val, reg->bit_width);
 
-		switch (reg->bit_width) {
+	switch (reg->bit_width) {
 		case 8:
 			*val = readb_relaxed(vaddr);
 			break;
@@ -674,23 +704,28 @@ static int cpc_read(struct cpc_reg *reg, u64 *val)
 			break;
 		default:
 			pr_debug("Error: Cannot read %u bit width from PCC\n",
-				reg->bit_width);
+					reg->bit_width);
 			ret_val = -EFAULT;
-		}
-	} else
-		ret_val = acpi_os_read_memory((acpi_physical_address)reg->address,
-					val, reg->bit_width);
+	}
+
 	return ret_val;
 }
 
-static int cpc_write(struct cpc_reg *reg, u64 val)
+static int cpc_write(struct cpc_register_resource *reg_res, u64 val)
 {
 	int ret_val = 0;
+	void __iomem *vaddr = 0;
+	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
-	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-		void __iomem *vaddr = GET_PCC_VADDR(reg->address);
+	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
+		vaddr = GET_PCC_VADDR(reg->address);
+	else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
+		vaddr = reg_res->sys_mem_vaddr;
+	else
+		return acpi_os_write_memory((acpi_physical_address)reg->address,
+				val, reg->bit_width);
 
-		switch (reg->bit_width) {
+	switch (reg->bit_width) {
 		case 8:
 			writeb_relaxed(val, vaddr);
 			break;
@@ -705,13 +740,11 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
 			break;
 		default:
 			pr_debug("Error: Cannot write %u bit width to PCC\n",
-				reg->bit_width);
+					reg->bit_width);
 			ret_val = -EFAULT;
 			break;
-		}
-	} else
-		ret_val = acpi_os_write_memory((acpi_physical_address)reg->address,
-				val, reg->bit_width);
+	}
+
 	return ret_val;
 }
 
@@ -754,16 +787,16 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 		}
 	}
 
-	cpc_read(&highest_reg->cpc_entry.reg, &high);
+	cpc_read(highest_reg, &high);
 	perf_caps->highest_perf = high;
 
-	cpc_read(&lowest_reg->cpc_entry.reg, &low);
+	cpc_read(lowest_reg, &low);
 	perf_caps->lowest_perf = low;
 
-	cpc_read(&ref_perf->cpc_entry.reg, &ref);
+	cpc_read(ref_perf, &ref);
 	perf_caps->reference_perf = ref;
 
-	cpc_read(&nom_perf->cpc_entry.reg, &nom);
+	cpc_read(nom_perf, &nom);
 	perf_caps->nominal_perf = nom;
 
 	if (!ref)
@@ -804,7 +837,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 
 	/* Are any of the regs PCC ?*/
 	if ((delivered_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-			(reference_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
+		(reference_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
 		/* Ring doorbell once to update PCC subspace */
 		if (send_pcc_cmd(CMD_READ) < 0) {
 			ret = -EIO;
@@ -812,8 +845,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 		}
 	}
 
-	cpc_read(&delivered_reg->cpc_entry.reg, &delivered);
-	cpc_read(&reference_reg->cpc_entry.reg, &reference);
+	cpc_read(delivered_reg, &delivered);
+	cpc_read(reference_reg, &reference);
 
 	if (!delivered || !reference) {
 		ret = -EFAULT;
@@ -868,7 +901,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * Skip writing MIN/MAX until Linux knows how to come up with
 	 * useful values.
 	 */
-	cpc_write(&desired_reg->cpc_entry.reg, perf_ctrls->desired_perf);
+	cpc_write(desired_reg, perf_ctrls->desired_perf);
 
 	/* Is this a PCC reg ?*/
 	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
@@ -878,7 +911,6 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	}
 busy_channel:
 	spin_unlock(&pcc_lock);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_set_perf);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 284965c..36ff5c6 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -49,6 +49,7 @@ struct cpc_reg {
  */
 struct cpc_register_resource {
 	acpi_object_type type;
+	u64 __iomem *sys_mem_vaddr;
 	union {
 		struct cpc_reg reg;
 		u64 int_value;
-- 
Qualcomm Technologies, Inc. on behalf
of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/5] ACPI/CPPC: restructure read/writes for efficient sys mapped reg ops

[Hoan Tran]

Hi Prashanth,

On Thu, Jun 30, 2016 at 11:02 AM, Prashanth Prakash
<pprakash@codeaurora.org> wrote:
> From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
>
> For cases where sys mapped CPC registers need to be accessed
> frequently, it helps immensly to pre-map them rather than map
> and unmap for each operation. e.g. case where feedback counters
> are sys mem map registers.
>
> Restructure cpc_read/write and the cpc_regs structure to allow
> pre-mapping the system addresses and unmap them when the CPU exits.
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> ---
>  drivers/acpi/cppc_acpi.c | 106 ++++++++++++++++++++++++++++++-----------------
>  include/acpi/cppc_acpi.h |   1 +
>  2 files changed, 70 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 2e98173..28170c2 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -62,7 +62,6 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>  /* This layer handles all the PCC specifics for CPPC. */
>  static struct mbox_chan *pcc_channel;
>  static void __iomem *pcc_comm_addr;
> -static u64 comm_base_addr;
>  static int pcc_subspace_idx = -1;
>  static bool pcc_channel_acquired;
>  static ktime_t deadline;
> @@ -394,7 +393,6 @@ EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>  static int register_pcc_channel(int pcc_subspace_idx)
>  {
>         struct acpi_pcct_hw_reduced *cppc_ss;
> -       unsigned int len;
>         u64 usecs_lat;
>
>         if (pcc_subspace_idx >= 0) {
> @@ -419,12 +417,6 @@ static int register_pcc_channel(int pcc_subspace_idx)
>                         return -ENODEV;
>                 }
>
> -               /*
> -                * This is the shared communication region
> -                * for the OS and Platform to communicate over.
> -                */
> -               comm_base_addr = cppc_ss->base_address;
> -               len = cppc_ss->length;
>
>                 /*
>                  * cppc_ss->latency is just a Nominal value. In reality
> @@ -436,7 +428,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>                 pcc_mrtt = cppc_ss->min_turnaround_time;
>                 pcc_mpar = cppc_ss->max_access_rate;
>
> -               pcc_comm_addr = acpi_os_ioremap(comm_base_addr, len);
> +               pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>                 if (!pcc_comm_addr) {
>                         pr_err("Failed to ioremap PCC comm region mem\n");
>                         return -ENOMEM;
> @@ -545,6 +537,8 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>                 goto out_free;
>         }
>
> +       cpc_ptr->num_entries = num_ent;
> +
>         /* Second entry should be revision. */
>         cpc_obj = &out_obj->package.elements[1];
>         if (cpc_obj->type == ACPI_TYPE_INTEGER) {
> @@ -585,7 +579,15 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>                                         pr_debug("Mismatched PCC ids.\n");
>                                         goto out_free;
>                                 }
> -                       } else if (gas_t->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +                       } else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +                               if (gas_t->address) {
> +                                       void __iomem *addr;
> +                                       addr = ioremap(gas_t->address, gas_t->bit_width/8);
> +                                       if (!addr)
> +                                               goto out_free;
> +                                       cpc_ptr->cpc_regs[i-2].sys_mem_vaddr = addr;
> +                               }
> +                       } else {
>                                 /* Support only PCC and SYS MEM type regs */
>                                 pr_debug("Unsupported register type: %d\n", gas_t->space_id);
>                                 goto out_free;
> @@ -623,6 +625,12 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>         return 0;
>
>  out_free:
> +       /* Free all the mapped sys mem areas for this CPU */
> +       for (i = 2; i < cpc_ptr->num_entries; i++) {
> +               void __iomem *addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
> +               if (addr)
> +                       iounmap(addr);
> +       }
>         kfree(cpc_ptr);
>
>  out_buf_free:
> @@ -640,7 +648,17 @@ EXPORT_SYMBOL_GPL(acpi_cppc_processor_probe);
>  void acpi_cppc_processor_exit(struct acpi_processor *pr)
>  {
>         struct cpc_desc *cpc_ptr;
> +       unsigned int i;
> +       void __iomem *addr;
>         cpc_ptr = per_cpu(cpc_desc_ptr, pr->id);
> +
> +       /* Free all the mapped sys mem areas for this CPU */
> +       for (i = 2; i < cpc_ptr->num_entries; i++) {
> +               addr = cpc_ptr->cpc_regs[i-2].sys_mem_vaddr;
> +               if (addr)
> +                       iounmap(addr);
> +       }
> +
>         kfree(cpc_ptr);
>  }
>  EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
> @@ -651,15 +669,27 @@ EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
>   * we can directly write to it.
>   */
>
> -static int cpc_read(struct cpc_reg *reg, u64 *val)
> +static int cpc_read(struct cpc_register_resource *reg_res, u64 *val)
>  {
>         int ret_val = 0;
> +       void __iomem *vaddr = 0;
> +       struct cpc_reg *reg = &reg_res->cpc_entry.reg;
> +
> +       if (reg_res->type == ACPI_TYPE_INTEGER) {
> +               *val = reg_res->int_value;

It should be "*val = reg_res->cpc_entry.int_value;", isn't it ?

Thanks
Hoan

> +               return ret_val;
> +       }
>
>         *val = 0;
> -       if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
> -               void __iomem *vaddr = GET_PCC_VADDR(reg->address);
> +       if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
> +               vaddr = GET_PCC_VADDR(reg->address);
> +       else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +               vaddr = reg_res->sys_mem_vaddr;
> +       else
> +               return acpi_os_read_memory((acpi_physical_address)reg->address,
> +                               val, reg->bit_width);
>
> -               switch (reg->bit_width) {
> +       switch (reg->bit_width) {
>                 case 8:
>                         *val = readb_relaxed(vaddr);
>                         break;
> @@ -674,23 +704,28 @@ static int cpc_read(struct cpc_reg *reg, u64 *val)
>                         break;
>                 default:
>                         pr_debug("Error: Cannot read %u bit width from PCC\n",
> -                               reg->bit_width);
> +                                       reg->bit_width);
>                         ret_val = -EFAULT;
> -               }
> -       } else
> -               ret_val = acpi_os_read_memory((acpi_physical_address)reg->address,
> -                                       val, reg->bit_width);
> +       }
> +
>         return ret_val;
>  }
>
> -static int cpc_write(struct cpc_reg *reg, u64 val)
> +static int cpc_write(struct cpc_register_resource *reg_res, u64 val)
>  {
>         int ret_val = 0;
> +       void __iomem *vaddr = 0;
> +       struct cpc_reg *reg = &reg_res->cpc_entry.reg;
>
> -       if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
> -               void __iomem *vaddr = GET_PCC_VADDR(reg->address);
> +       if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
> +               vaddr = GET_PCC_VADDR(reg->address);
> +       else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> +               vaddr = reg_res->sys_mem_vaddr;
> +       else
> +               return acpi_os_write_memory((acpi_physical_address)reg->address,
> +                               val, reg->bit_width);
>
> -               switch (reg->bit_width) {
> +       switch (reg->bit_width) {
>                 case 8:
>                         writeb_relaxed(val, vaddr);
>                         break;
> @@ -705,13 +740,11 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
>                         break;
>                 default:
>                         pr_debug("Error: Cannot write %u bit width to PCC\n",
> -                               reg->bit_width);
> +                                       reg->bit_width);
>                         ret_val = -EFAULT;
>                         break;
> -               }
> -       } else
> -               ret_val = acpi_os_write_memory((acpi_physical_address)reg->address,
> -                               val, reg->bit_width);
> +       }
> +
>         return ret_val;
>  }
>
> @@ -754,16 +787,16 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>                 }
>         }
>
> -       cpc_read(&highest_reg->cpc_entry.reg, &high);
> +       cpc_read(highest_reg, &high);
>         perf_caps->highest_perf = high;
>
> -       cpc_read(&lowest_reg->cpc_entry.reg, &low);
> +       cpc_read(lowest_reg, &low);
>         perf_caps->lowest_perf = low;
>
> -       cpc_read(&ref_perf->cpc_entry.reg, &ref);
> +       cpc_read(ref_perf, &ref);
>         perf_caps->reference_perf = ref;
>
> -       cpc_read(&nom_perf->cpc_entry.reg, &nom);
> +       cpc_read(nom_perf, &nom);
>         perf_caps->nominal_perf = nom;
>
>         if (!ref)
> @@ -804,7 +837,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
>
>         /* Are any of the regs PCC ?*/
>         if ((delivered_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
> -                       (reference_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
> +               (reference_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
>                 /* Ring doorbell once to update PCC subspace */
>                 if (send_pcc_cmd(CMD_READ) < 0) {
>                         ret = -EIO;
> @@ -812,8 +845,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
>                 }
>         }
>
> -       cpc_read(&delivered_reg->cpc_entry.reg, &delivered);
> -       cpc_read(&reference_reg->cpc_entry.reg, &reference);
> +       cpc_read(delivered_reg, &delivered);
> +       cpc_read(reference_reg, &reference);
>
>         if (!delivered || !reference) {
>                 ret = -EFAULT;
> @@ -868,7 +901,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>          * Skip writing MIN/MAX until Linux knows how to come up with
>          * useful values.
>          */
> -       cpc_write(&desired_reg->cpc_entry.reg, perf_ctrls->desired_perf);
> +       cpc_write(desired_reg, perf_ctrls->desired_perf);
>
>         /* Is this a PCC reg ?*/
>         if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
> @@ -878,7 +911,6 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>         }
>  busy_channel:
>         spin_unlock(&pcc_lock);
> -
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(cppc_set_perf);
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 284965c..36ff5c6 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -49,6 +49,7 @@ struct cpc_reg {
>   */
>  struct cpc_register_resource {
>         acpi_object_type type;
> +       u64 __iomem *sys_mem_vaddr;
>         union {
>                 struct cpc_reg reg;
>                 u64 int_value;
> --
> Qualcomm Technologies, Inc. on behalf
> of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc.
> is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
>

Re: [PATCH 1/5] ACPI/CPPC: restructure read/writes for efficient sys mapped reg ops

[Prakash, Prashanth]

Hi Hoan,
>> -static int cpc_read(struct cpc_reg *reg, u64 *val)
>> +static int cpc_read(struct cpc_register_resource *reg_res, u64 *val)
>>  {
>>         int ret_val = 0;
>> +       void __iomem *vaddr = 0;
>> +       struct cpc_reg *reg = &reg_res->cpc_entry.reg;
>> +
>> +       if (reg_res->type == ACPI_TYPE_INTEGER) {
>> +               *val = reg_res->int_value;
> It should be "*val = reg_res->cpc_entry.int_value;", isn't it ?
>
Yes, that's correct. I will fix it in the next version. Thanks!
Interestingly, I don't think I saw an error or warning during compile:)

Thanks,
Prashanth

[PATCH 2/5] ACPI/CPPC: acquire pcc_lock only while accessing PCC subspace

[Prashanth Prakash]

We need to acquire pcc_lock only when we are accessing registers
that are in the PCC subspsace.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/cppc_acpi.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 28170c2..a4b2b50 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -761,7 +761,7 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	struct cpc_register_resource *highest_reg, *lowest_reg, *ref_perf,
 								 *nom_perf;
 	u64 high, low, ref, nom;
-	int ret = 0;
+	int ret = 0, regs_in_pcc = 0;
 
 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -773,13 +773,13 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	ref_perf = &cpc_desc->cpc_regs[REFERENCE_PERF];
 	nom_perf = &cpc_desc->cpc_regs[NOMINAL_PERF];
 
-	spin_lock(&pcc_lock);
-
 	/* Are any of the regs PCC ?*/
 	if ((highest_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-			(lowest_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-			(ref_perf->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-			(nom_perf->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
+		(lowest_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
+		(ref_perf->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
+		(nom_perf->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
+		spin_lock(&pcc_lock);
+		regs_in_pcc = 1;
 		/* Ring doorbell once to update PCC subspace */
 		if (send_pcc_cmd(CMD_READ) < 0) {
 			ret = -EIO;
@@ -806,7 +806,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 		ret = -EFAULT;
 
 out_err:
-	spin_unlock(&pcc_lock);
+	if (regs_in_pcc)
+		spin_unlock(&pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
@@ -823,7 +824,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
 	struct cpc_register_resource *delivered_reg, *reference_reg;
 	u64 delivered, reference;
-	int ret = 0;
+	int ret = 0, regs_in_pcc = 0;
 
 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -833,11 +834,11 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	delivered_reg = &cpc_desc->cpc_regs[DELIVERED_CTR];
 	reference_reg = &cpc_desc->cpc_regs[REFERENCE_CTR];
 
-	spin_lock(&pcc_lock);
-
 	/* Are any of the regs PCC ?*/
 	if ((delivered_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
 		(reference_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
+		spin_lock(&pcc_lock);
+		regs_in_pcc = 1;
 		/* Ring doorbell once to update PCC subspace */
 		if (send_pcc_cmd(CMD_READ) < 0) {
 			ret = -EIO;
@@ -863,7 +864,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	perf_fb_ctrs->prev_reference = reference;
 
 out_err:
-	spin_unlock(&pcc_lock);
+	if (regs_in_pcc)
+		spin_unlock(&pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
@@ -888,10 +890,9 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 
 	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
 
-	spin_lock(&pcc_lock);
-
 	/* If this is PCC reg, check if channel is free before writing */
 	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
+		spin_lock(&pcc_lock);
 		ret = check_pcc_chan();
 		if (ret)
 			goto busy_channel;
@@ -910,7 +911,8 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 			ret = -EIO;
 	}
 busy_channel:
-	spin_unlock(&pcc_lock);
+	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
+		spin_unlock(&pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_set_perf);
-- 
Qualcomm Technologies, Inc. on behalf
of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/5] ACPI/CPPC: support for batching CPPC requests

[Prashanth Prakash]

CPPC defined in section 8.4.7 of ACPI 6.0 specification suggests
"To amortize the cost of PCC transactions, OSPM should read or write
all PCC registers via a single read or write command when possible"
This patch enables opportunistic batching of frequency transition
requests whenever the request happen to overlap in time.

Currently the access to pcc is serialized by a spin lock which does
not scale well as we increase the number of cores in the system. This
patch improves the scalability by allowing the differnt CPU cores to
update PCC subspace in parallel and by batching requests which will
reduce the certain types of operation(checking command completion bit,
ringing doorbell) by a significant margin.

Profiling shows significant improvement in the overall effeciency
to service freq. transition requests. With this patch we observe close
to 30% of the frequency transition requests being batched with other
requests while running apache bench on a ARM platform with 6
independent domains(or sets of related cpus).

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/cppc_acpi.c | 176 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 147 insertions(+), 29 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index a4b2b50..8dee6d5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -40,15 +40,35 @@
 #include <linux/cpufreq.h>
 #include <linux/delay.h>
 #include <linux/ktime.h>
+#include <linux/rwsem.h>
+#include <linux/wait.h>
 
 #include <acpi/cppc_acpi.h>
+
 /*
- * Lock to provide mutually exclusive access to the PCC
- * channel. e.g. When the remote updates the shared region
- * with new data, the reader needs to be protected from
- * other CPUs activity on the same channel.
+ * Lock to provide controlled access to the PCC channel.
+ *
+ * For performance critical usecases(currently cppc_set_perf)
+ *	We need to take read_lock and check if channel belongs to OSPM before
+ * reading or writing to PCC subspace
+ *	We need to take write_lock before transferring the channel ownership to
+ * the platform via a Doorbell
+ *	This allows us to batch a number of CPPC requests if they happen to
+ * originate in about the same time
+ *
+ * For non-performance critical usecases(init)
+ *	Take write_lock for all purposes which gives exclusive access
  */
-static DEFINE_SPINLOCK(pcc_lock);
+static DECLARE_RWSEM(pcc_lock);
+
+/* Indicates if there are any pending/batched PCC write commands */
+static bool pending_pcc_write_cmd;
+
+/* Wait queue for CPUs whose requests were batched */
+static DECLARE_WAIT_QUEUE_HEAD(pcc_write_wait_q);
+
+/* Used to identify if a batched request is delivered to platform */
+static unsigned int pcc_write_cnt;
 
 /*
  * The cpc_desc structure contains the ACPI register details
@@ -70,6 +90,11 @@ static unsigned int pcc_mpar, pcc_mrtt;
 /* pcc mapped address + header size + offset within PCC subspace */
 #define GET_PCC_VADDR(offs) (pcc_comm_addr + 0x8 + (offs))
 
+/* Check if a CPC regsiter is in PCC */
+#define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
+				(cpc)->cpc_entry.reg.space_id ==	\
+				ACPI_ADR_SPACE_PLATFORM_COMM)
+
 /*
  * Arbitrary Retries in case the remote processor is slow to respond
  * to PCC commands. Keeping it high enough to cover emulators where
@@ -104,6 +129,10 @@ static int check_pcc_chan(void)
 	return ret;
 }
 
+/*
+ * This function transfers the ownership of the PCC to the platform
+ * So it must be called while holding write_lock(pcc_lock)
+ */
 static int send_pcc_cmd(u16 cmd)
 {
 	int ret = -EIO;
@@ -118,6 +147,16 @@ static int send_pcc_cmd(u16 cmd)
 	 * the channel before writing to PCC space
 	 */
 	if (cmd == CMD_READ) {
+		/*
+		 * If there are pending cpc_writes, then we stole the channel
+		 * before write completion, so first send a WRITE command to
+		 * platform
+		 */
+		if (pending_pcc_write_cmd) {
+			pending_pcc_write_cmd = FALSE;
+			send_pcc_cmd(CMD_WRITE);
+		}
+
 		ret = check_pcc_chan();
 		if (ret)
 			return ret;
@@ -191,6 +230,12 @@ static int send_pcc_cmd(u16 cmd)
 	}
 
 	mbox_client_txdone(pcc_channel, ret);
+
+	if (cmd == CMD_WRITE) {
+		pcc_write_cnt++;
+		wake_up_all(&pcc_write_wait_q);
+	}
+
 	return ret;
 }
 
@@ -774,12 +819,10 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	nom_perf = &cpc_desc->cpc_regs[NOMINAL_PERF];
 
 	/* Are any of the regs PCC ?*/
-	if ((highest_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-		(lowest_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-		(ref_perf->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-		(nom_perf->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
-		spin_lock(&pcc_lock);
+	if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
+		CPC_IN_PCC(ref_perf) || CPC_IN_PCC(nom_perf)) {
 		regs_in_pcc = 1;
+		down_write(&pcc_lock);
 		/* Ring doorbell once to update PCC subspace */
 		if (send_pcc_cmd(CMD_READ) < 0) {
 			ret = -EIO;
@@ -807,7 +850,7 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 
 out_err:
 	if (regs_in_pcc)
-		spin_unlock(&pcc_lock);
+		up_write(&pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
@@ -835,9 +878,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	reference_reg = &cpc_desc->cpc_regs[REFERENCE_CTR];
 
 	/* Are any of the regs PCC ?*/
-	if ((delivered_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) ||
-		(reference_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)) {
-		spin_lock(&pcc_lock);
+	if (CPC_IN_PCC(delivered_reg) || CPC_IN_PCC(reference_reg)) {
+		down_write(&pcc_lock);
 		regs_in_pcc = 1;
 		/* Ring doorbell once to update PCC subspace */
 		if (send_pcc_cmd(CMD_READ) < 0) {
@@ -865,7 +907,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 
 out_err:
 	if (regs_in_pcc)
-		spin_unlock(&pcc_lock);
+		up_write(&pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
@@ -882,6 +924,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
 	struct cpc_register_resource *desired_reg;
 	int ret = 0;
+	unsigned int local_cnt = 0;
 
 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
@@ -890,12 +933,35 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 
 	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
 
-	/* If this is PCC reg, check if channel is free before writing */
-	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-		spin_lock(&pcc_lock);
-		ret = check_pcc_chan();
-		if (ret)
-			goto busy_channel;
+	/*
+	 * This is Phase-I where we want to write to CPC registers
+	 * -> We want all CPUs to be able to execute this phase in parallel
+	 *
+	 * Since read_lock can be acquired by multiple CPUs simultaneously we
+	 * achieve that goal here
+	 */
+	if (CPC_IN_PCC(desired_reg)) {
+		down_read(&pcc_lock);	/* BEGIN Phase-I */
+		/*
+		 * If there are pending write commands i.e pending_pcc_write_cmd
+		 * is TRUE, then we know OSPM owns the channel as another CPU
+		 * has already checked for command completion bit and updated
+		 * the corresponding CPC registers
+		 */
+		if (!pending_pcc_write_cmd) {
+			ret = check_pcc_chan();
+			if (ret) {
+				up_read(&pcc_lock);
+				return ret;
+			}
+			/*
+			 * Update the pending_write to make sure a PCC CMD_READ
+			 * will not arrive and steal the channel during the
+			 * transition to write lock
+			 */
+			pending_pcc_write_cmd = TRUE;
+		}
+		local_cnt = pcc_write_cnt;
 	}
 
 	/*
@@ -904,15 +970,67 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 */
 	cpc_write(desired_reg, perf_ctrls->desired_perf);
 
-	/* Is this a PCC reg ?*/
-	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-		/* Ring doorbell so Remote can get our perf request. */
-		if (send_pcc_cmd(CMD_WRITE) < 0)
-			ret = -EIO;
+	if (CPC_IN_PCC(desired_reg))
+		up_read(&pcc_lock);	/* END Phase-I */
+	/*
+	 * This is Phase-II where we transfer the ownership of PCC to Platform
+	 *
+	 * Short Summary: Basically if we think of a group of cppc_set_perf
+	 * requests that happened in short overlapping interval. The last CPU to
+	 * come out of Phase-I will enter Phase-II and ring the doorbell.
+	 *
+	 * We have the following requirements for Phase-II:
+	 *     1. We want to execute Phase-II only when there are no CPUs
+	 * currently executing in Phase-I
+	 *     2. Once we start Phase-II we want to avoid all other CPUs from
+	 * entering Phase-I.
+	 *     3. We want only one CPU among all those who went through Phase-I
+	 * to run phase-II
+	 *
+	 * If write_trylock fails to get the lock and doesn't transfer the
+	 * PCC ownership to the platform, then one of the following will be TRUE
+	 *     1. There is at-least one CPU in Phase-I which will later execute
+	 * write_trylock, so the CPUs in Phase-I will be responsible for
+	 * executing the Phase-II.
+	 *     2. Some other CPU has beaten this CPU to successfully execute the
+	 * write_trylock and has already acquired the write_lock. We know for a
+	 * fact it(other CPU acquiring the write_lock) couldn't have happened
+	 * before this CPU's Phase-I as we held the read_lock.
+	 *     3. Some other CPU executing pcc CMD_READ has stolen the
+	 * down_write, in which case, send_pcc_cmd will check for pending
+	 * CMD_WRITE commands by checking the pending_pcc_write_cmd.
+	 * So this CPU can be certain that its request will be delivered
+	 *    So in all cases, this CPU knows that its request will be delivered
+	 * by another CPU and can return
+	 *
+	 * After getting the down_write we still need to check for
+	 * pending_pcc_write_cmd to take care of the following scenario
+	 *    The thread running this code could be scheduled out between
+	 * Phase-I and Phase-II. Before it is scheduled back on, another CPU
+	 * could have delivered the request to Platform by triggering the
+	 * doorbell and transferred the ownership of PCC to platform. So this
+	 * avoids triggering an unnecessary doorbell and more importantly before
+	 * triggering the doorbell it makes sure that the PCC channel ownership
+	 * is still with OSPM.
+	 *   pending_pcc_write_cmd can also be cleared by a different CPU, if
+	 * there was a pcc CMD_READ waiting on down_write and it steals the lock
+	 * before the pcc CMD_WRITE is completed. pcc_send_cmd checks for this
+	 * case during a CMD_READ and if there are pending writes it delivers
+	 * the write command before servicing the read command
+	 */
+	if (CPC_IN_PCC(desired_reg)) {
+		if (down_write_trylock(&pcc_lock)) {		/* BEGIN Phase-II */
+			/* Update only if there are pending write commands */
+			if (pending_pcc_write_cmd) {
+				pending_pcc_write_cmd = FALSE;
+				if (send_pcc_cmd(CMD_WRITE) < 0)
+					ret = -EIO;
+			}
+			up_write(&pcc_lock);	 		/* END Phase-II */
+		} else
+			/* Wait until pcc_write_cnt is updated by send_pcc_cmd */
+			wait_event(pcc_write_wait_q, local_cnt != pcc_write_cnt);
 	}
-busy_channel:
-	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
-		spin_unlock(&pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_set_perf);
-- 
Qualcomm Technologies, Inc. on behalf
of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 4/5] ACPI/CPPC: set a non-zero value for transition_latency

[Prashanth Prakash]

Compute the expected transition latency for frequency transitions
using the values from the PCCT tables when the desired perf
register is in PCC.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 drivers/acpi/cppc_acpi.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
 drivers/cpufreq/cppc_cpufreq.c |  1 +
 include/acpi/cppc_acpi.h       |  1 +
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 8dee6d5..7844e4c 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -85,7 +85,7 @@ static void __iomem *pcc_comm_addr;
 static int pcc_subspace_idx = -1;
 static bool pcc_channel_acquired;
 static ktime_t deadline;
-static unsigned int pcc_mpar, pcc_mrtt;
+static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
 
 /* pcc mapped address + header size + offset within PCC subspace */
 #define GET_PCC_VADDR(offs) (pcc_comm_addr + 0x8 + (offs))
@@ -462,7 +462,6 @@ static int register_pcc_channel(int pcc_subspace_idx)
 			return -ENODEV;
 		}
 
-
 		/*
 		 * cppc_ss->latency is just a Nominal value. In reality
 		 * the remote processor could be much slower to reply.
@@ -472,6 +471,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
 		deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
 		pcc_mrtt = cppc_ss->min_turnaround_time;
 		pcc_mpar = cppc_ss->max_access_rate;
+		pcc_nominal = cppc_ss->latency;
 
 		pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
 		if (!pcc_comm_addr) {
@@ -1034,3 +1034,46 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_set_perf);
+
+/**
+ * cppc_get_transition_latency - returns frequency transition latency in ns
+ *
+ * ACPI CPPC does not explicitly specifiy how a platform can specify the
+ * transition latency for perfromance change requests. The closest we have
+ * is the timing information from the PCCT tables which provides the info
+ * on the number and frequency of PCC commands the platform can handle.
+ */
+unsigned int cppc_get_transition_latency(int cpu_num)
+{
+	/*
+	 * Expected transition latency is based on the PCCT timing values
+	 * Below are definition from ACPI spec:
+	 * pcc_nominal- Expected latency to process a command, in microseconds
+	 * pcc_mpar   - The maximum number of periodic requests that the subspace
+	 *              channel can support, reported in commands per minute. 0
+	 *              indicates no limitation.
+	 * pcc_mrtt   - The minimum amount of time that OSPM must wait after the
+	 *              completion of a command before issuing the next command,
+	 *              in microseconds.
+	 */
+	unsigned int latency_ns = 0;
+	struct cpc_desc *cpc_desc;
+	struct cpc_register_resource *desired_reg;
+
+	cpc_desc = per_cpu(cpc_desc_ptr, cpu_num);
+	if (!cpc_desc)
+		return CPUFREQ_ETERNAL;
+
+	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
+	if (!CPC_IN_PCC(desired_reg))
+		return CPUFREQ_ETERNAL;
+
+	if (pcc_mpar)
+		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_mpar);
+
+	latency_ns = max(latency_ns, pcc_nominal * 1000);
+	latency_ns = max(latency_ns, pcc_mrtt * 1000);
+
+	return latency_ns;
+}
+EXPORT_SYMBOL_GPL(cppc_get_transition_latency);
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 8882b8e..e6a3359 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -98,6 +98,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	policy->max = cpu->perf_caps.highest_perf;
 	policy->cpuinfo.min_freq = policy->min;
 	policy->cpuinfo.max_freq = policy->max;
+	policy->cpuinfo.transition_latency = cppc_get_transition_latency(cpu_num);
 	policy->shared_type = cpu->shared_type;
 
 	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 36ff5c6..7b7e2e1 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -129,5 +129,6 @@ extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
 extern int acpi_get_psd_map(struct cpudata **);
+extern unsigned int cppc_get_transition_latency(int cpu);
 
 #endif /* _CPPC_ACPI_H*/
-- 
Qualcomm Technologies, Inc. on behalf
of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 4/5] ACPI/CPPC: set a non-zero value for transition_latency

[Alexey Klimov]

Hi Prashanth,

On Thu, Jun 30, 2016 at 12:02:37PM -0600, Prashanth Prakash wrote:
> Compute the expected transition latency for frequency transitions
> using the values from the PCCT tables when the desired perf
> register is in PCC.
> 
> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> ---
>  drivers/acpi/cppc_acpi.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
>  drivers/cpufreq/cppc_cpufreq.c |  1 +
>  include/acpi/cppc_acpi.h       |  1 +
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 8dee6d5..7844e4c 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -85,7 +85,7 @@ static void __iomem *pcc_comm_addr;
>  static int pcc_subspace_idx = -1;
>  static bool pcc_channel_acquired;
>  static ktime_t deadline;
> -static unsigned int pcc_mpar, pcc_mrtt;
> +static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
>  
>  /* pcc mapped address + header size + offset within PCC subspace */
>  #define GET_PCC_VADDR(offs) (pcc_comm_addr + 0x8 + (offs))
> @@ -462,7 +462,6 @@ static int register_pcc_channel(int pcc_subspace_idx)
>  			return -ENODEV;
>  		}
>  
> -
>  		/*
>  		 * cppc_ss->latency is just a Nominal value. In reality
>  		 * the remote processor could be much slower to reply.
> @@ -472,6 +471,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>  		deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>  		pcc_mrtt = cppc_ss->min_turnaround_time;
>  		pcc_mpar = cppc_ss->max_access_rate;
> +		pcc_nominal = cppc_ss->latency;
>  
>  		pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>  		if (!pcc_comm_addr) {
> @@ -1034,3 +1034,46 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cppc_set_perf);
> +
> +/**
> + * cppc_get_transition_latency - returns frequency transition latency in ns
> + *
> + * ACPI CPPC does not explicitly specifiy how a platform can specify the
> + * transition latency for perfromance change requests. The closest we have
> + * is the timing information from the PCCT tables which provides the info
> + * on the number and frequency of PCC commands the platform can handle.
> + */
> +unsigned int cppc_get_transition_latency(int cpu_num)
> +{
> +	/*
> +	 * Expected transition latency is based on the PCCT timing values
> +	 * Below are definition from ACPI spec:
> +	 * pcc_nominal- Expected latency to process a command, in microseconds
> +	 * pcc_mpar   - The maximum number of periodic requests that the subspace
> +	 *              channel can support, reported in commands per minute. 0
> +	 *              indicates no limitation.
> +	 * pcc_mrtt   - The minimum amount of time that OSPM must wait after the
> +	 *              completion of a command before issuing the next command,
> +	 *              in microseconds.
> +	 */
> +	unsigned int latency_ns = 0;
> +	struct cpc_desc *cpc_desc;
> +	struct cpc_register_resource *desired_reg;
> +
> +	cpc_desc = per_cpu(cpc_desc_ptr, cpu_num);
> +	if (!cpc_desc)
> +		return CPUFREQ_ETERNAL;
> +
> +	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
> +	if (!CPC_IN_PCC(desired_reg))
> +		return CPUFREQ_ETERNAL;
> +
> +	if (pcc_mpar)
> +		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_mpar);
> +
> +	latency_ns = max(latency_ns, pcc_nominal * 1000);
> +	latency_ns = max(latency_ns, pcc_mrtt * 1000);

How transition latency is used?
If it is used in the sense of "wait this amount of time before sending next request"
or this behaviour is inherited because of using this latency as a sample time,
then I would say it should be a sum of pcc_nominal and pcc_mrtt here.

Best regards,
Alexey

Re: [PATCH 4/5] ACPI/CPPC: set a non-zero value for transition_latency

[Prakash, Prashanth]

Hi Alexey,

On 7/20/2016 9:51 AM, Alexey Klimov wrote:
> Hi Prashanth,
>
> On Thu, Jun 30, 2016 at 12:02:37PM -0600, Prashanth Prakash wrote:
>> Compute the expected transition latency for frequency transitions
>> using the values from the PCCT tables when the desired perf
>> register is in PCC.
>>
>> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
>> ---
>>  drivers/acpi/cppc_acpi.c       | 47 ++++++++++++++++++++++++++++++++++++++++--
>>  drivers/cpufreq/cppc_cpufreq.c |  1 +
>>  include/acpi/cppc_acpi.h       |  1 +
>>  3 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 8dee6d5..7844e4c 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -85,7 +85,7 @@ static void __iomem *pcc_comm_addr;
>>  static int pcc_subspace_idx = -1;
>>  static bool pcc_channel_acquired;
>>  static ktime_t deadline;
>> -static unsigned int pcc_mpar, pcc_mrtt;
>> +static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
>>  
>>  /* pcc mapped address + header size + offset within PCC subspace */
>>  #define GET_PCC_VADDR(offs) (pcc_comm_addr + 0x8 + (offs))
>> @@ -462,7 +462,6 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>  			return -ENODEV;
>>  		}
>>  
>> -
>>  		/*
>>  		 * cppc_ss->latency is just a Nominal value. In reality
>>  		 * the remote processor could be much slower to reply.
>> @@ -472,6 +471,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>  		deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>  		pcc_mrtt = cppc_ss->min_turnaround_time;
>>  		pcc_mpar = cppc_ss->max_access_rate;
>> +		pcc_nominal = cppc_ss->latency;
>>  
>>  		pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>  		if (!pcc_comm_addr) {
>> @@ -1034,3 +1034,46 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(cppc_set_perf);
>> +
>> +/**
>> + * cppc_get_transition_latency - returns frequency transition latency in ns
>> + *
>> + * ACPI CPPC does not explicitly specifiy how a platform can specify the
>> + * transition latency for perfromance change requests. The closest we have
>> + * is the timing information from the PCCT tables which provides the info
>> + * on the number and frequency of PCC commands the platform can handle.
>> + */
>> +unsigned int cppc_get_transition_latency(int cpu_num)
>> +{
>> +	/*
>> +	 * Expected transition latency is based on the PCCT timing values
>> +	 * Below are definition from ACPI spec:
>> +	 * pcc_nominal- Expected latency to process a command, in microseconds
>> +	 * pcc_mpar   - The maximum number of periodic requests that the subspace
>> +	 *              channel can support, reported in commands per minute. 0
>> +	 *              indicates no limitation.
>> +	 * pcc_mrtt   - The minimum amount of time that OSPM must wait after the
>> +	 *              completion of a command before issuing the next command,
>> +	 *              in microseconds.
>> +	 */
>> +	unsigned int latency_ns = 0;
>> +	struct cpc_desc *cpc_desc;
>> +	struct cpc_register_resource *desired_reg;
>> +
>> +	cpc_desc = per_cpu(cpc_desc_ptr, cpu_num);
>> +	if (!cpc_desc)
>> +		return CPUFREQ_ETERNAL;
>> +
>> +	desired_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
>> +	if (!CPC_IN_PCC(desired_reg))
>> +		return CPUFREQ_ETERNAL;
>> +
>> +	if (pcc_mpar)
>> +		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_mpar);
>> +
>> +	latency_ns = max(latency_ns, pcc_nominal * 1000);
>> +	latency_ns = max(latency_ns, pcc_mrtt * 1000);
> How transition latency is used?
transition latency ends up deciding the sampling_rate in case of ondemand
governor and in case of schedutil it sets up minimum period between 2 requests.
> If it is used in the sense of "wait this amount of time before sending next request"
> or this behaviour is inherited because of using this latency as a sample time,
> then I would say it should be a sum of pcc_nominal and pcc_mrtt here.
Yes, that makes sense. I will update it in next version, so we will have
latency_ns = max ( mrtt+nominal, value from mpar)

Thanks,
Prashanth

[PATCH 5/5] ACPI/CPPC: add sysfs support to compute delivered performance

[Prashanth Prakash]

From: Ashwin Chaugule <ashwin.chaugule@linaro.org>

The CPPC tables contain entries for per CPU feedback counters which
allows us to compute the delivered performance over a given interval
of time.

The math for delivered performance per the CPPCv5.0+ spec is:
  reference perf * delta(delivered perf ctr)/delta(ref perf ctr)

Maintaining deltas of the counters in the kernel is messy, as it
depends on when the reads are triggered. (e.g. via the cpufreq
->get() interface). Also the ->get() interace only returns one
value, so cant return raw values. So instead, leave it to userspace
to keep track of raw values and do its math for CPUs it cares about.

delivered and reference perf counters are exposed via the same
sysfs file to avoid the potential "skid", if these values are read
individually from userspace.

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/acpi/cppc_acpi.c | 94 +++++++++++++++++++++++++++++++++++++++++++-----
 include/acpi/cppc_acpi.h |  3 +-
 2 files changed, 86 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 7844e4c..8bd501f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -95,6 +95,17 @@ static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
 				(cpc)->cpc_entry.reg.space_id ==	\
 				ACPI_ADR_SPACE_PLATFORM_COMM)
 
+/* Evalutes to True if reg is a NULL register descriptor */
+#define IS_NULL_REG(reg) ((reg)->space_id ==  ACPI_ADR_SPACE_SYSTEM_MEMORY && \
+				(reg)->address == 0 &&			\
+				(reg)->bit_width == 0 &&		\
+				(reg)->bit_offset == 0 &&		\
+				(reg)->access_width == 0)
+
+/* Evalutes to True if an optional cpc field is supported */
+#define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?		\
+				!!(cpc)->cpc_entry.int_value :			\
+				!IS_NULL_REG(&(cpc)->cpc_entry.reg))
 /*
  * Arbitrary Retries in case the remote processor is slow to respond
  * to PCC commands. Keeping it high enough to cover emulators where
@@ -102,6 +113,61 @@ static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
  */
 #define NUM_RETRIES 500
 
+struct cppc_attr {
+	struct attribute attr;
+	ssize_t (*show)(struct kobject *kobj,
+			struct attribute *attr, char *buf);
+	ssize_t (*store)(struct kobject *kobj,
+			struct attribute *attr, const char *c, ssize_t count);
+};
+
+#define define_one_cppc_ro(_name)		\
+static struct cppc_attr _name =			\
+__ATTR(_name, 0444, show_##_name, NULL)
+
+#define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
+
+static ssize_t show_feedback_ctrs(struct kobject *kobj,
+		struct attribute *attr, char *buf)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	struct cppc_perf_fb_ctrs fb_ctrs = {0};
+
+	cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
+
+	return scnprintf(buf, PAGE_SIZE, "ref:%lld del:%lld\n",
+			fb_ctrs.reference, fb_ctrs.delivered);
+}
+define_one_cppc_ro(feedback_ctrs);
+
+static ssize_t show_reference_perf(struct kobject *kobj,
+		struct attribute *attr, char *buf)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	struct cppc_perf_caps perf_caps;
+
+	cppc_get_perf_caps(cpc_ptr->cpu_id, &perf_caps);
+
+	if (perf_caps.reference_perf)
+		return scnprintf(buf, PAGE_SIZE, "%u\n",
+				perf_caps.reference_perf);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n",
+			perf_caps.nominal_perf);
+}
+define_one_cppc_ro(reference_perf);
+
+static struct attribute *cppc_attrs[] = {
+	&feedback_ctrs.attr,
+	&reference_perf.attr,
+	NULL
+};
+
+static struct kobj_type cppc_ktype = {
+	.sysfs_ops = &kobj_sysfs_ops,
+	.default_attrs = cppc_attrs,
+};
+
 static int check_pcc_chan(void)
 {
 	int ret = -EIO;
@@ -544,6 +610,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	union acpi_object *out_obj, *cpc_obj;
 	struct cpc_desc *cpc_ptr;
 	struct cpc_reg *gas_t;
+	struct device *cpu_dev;
 	acpi_handle handle = pr->handle;
 	unsigned int num_ent, i, cpc_rev;
 	acpi_status status;
@@ -666,6 +733,16 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	/* Everything looks okay */
 	pr_debug("Parsed CPC struct for CPU: %d\n", pr->id);
 
+	/* Add per logical CPU nodes for reading its feedback counters. */
+	cpu_dev = get_cpu_device(pr->id);
+	if (!cpu_dev)
+		goto out_free;
+
+	ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
+			"acpi_cppc");
+	if (ret)
+		goto out_free;
+
 	kfree(output.pointer);
 	return 0;
 
@@ -695,6 +772,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
 	struct cpc_desc *cpc_ptr;
 	unsigned int i;
 	void __iomem *addr;
+
 	cpc_ptr = per_cpu(cpc_desc_ptr, pr->id);
 
 	/* Free all the mapped sys mem areas for this CPU */
@@ -704,6 +782,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
 			iounmap(addr);
 	}
 
+	kobject_put(&cpc_ptr->kobj);
 	kfree(cpc_ptr);
 }
 EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
@@ -721,7 +800,7 @@ static int cpc_read(struct cpc_register_resource *reg_res, u64 *val)
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
 	if (reg_res->type == ACPI_TYPE_INTEGER) {
-		*val = reg_res->int_value;
+		*val = reg_res->cpc_entry.int_value;
 		return ret_val;
 	}
 
@@ -836,8 +915,11 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	cpc_read(lowest_reg, &low);
 	perf_caps->lowest_perf = low;
 
-	cpc_read(ref_perf, &ref);
-	perf_caps->reference_perf = ref;
+	perf_caps->reference_perf = 0;
+	if (CPC_SUPPORTED(ref_perf)) {
+		cpc_read(ref_perf, &ref);
+		perf_caps->reference_perf = ref;
+	}
 
 	cpc_read(nom_perf, &nom);
 	perf_caps->nominal_perf = nom;
@@ -899,12 +981,6 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	perf_fb_ctrs->delivered = delivered;
 	perf_fb_ctrs->reference = reference;
 
-	perf_fb_ctrs->delivered -= perf_fb_ctrs->prev_delivered;
-	perf_fb_ctrs->reference -= perf_fb_ctrs->prev_reference;
-
-	perf_fb_ctrs->prev_delivered = delivered;
-	perf_fb_ctrs->prev_reference = reference;
-
 out_err:
 	if (regs_in_pcc)
 		up_write(&pcc_lock);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 7b7e2e1..2bf9db1 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -63,6 +63,7 @@ struct cpc_desc {
 	int cpu_id;
 	struct cpc_register_resource cpc_regs[MAX_CPC_REG_ENT];
 	struct acpi_psd_package domain_info;
+	struct kobject kobj;
 };
 
 /* These are indexes into the per-cpu cpc_regs[]. Order is important. */
@@ -109,9 +110,7 @@ struct cppc_perf_ctrls {
 
 struct cppc_perf_fb_ctrs {
 	u64 reference;
-	u64 prev_reference;
 	u64 delivered;
-	u64 prev_delivered;
 };
 
 /* Per CPU container for runtime CPPC management. */
-- 
Qualcomm Technologies, Inc. on behalf
of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 5/5] ACPI/CPPC: add sysfs support to compute delivered performance

[Alexey Klimov]

Hi Prashanth and Ashwin,

On Thu, Jun 30, 2016 at 12:02:38PM -0600, Prashanth Prakash wrote:
> From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> 
> The CPPC tables contain entries for per CPU feedback counters which
> allows us to compute the delivered performance over a given interval
> of time.
> 
> The math for delivered performance per the CPPCv5.0+ spec is:
>   reference perf * delta(delivered perf ctr)/delta(ref perf ctr)
> 
> Maintaining deltas of the counters in the kernel is messy, as it
> depends on when the reads are triggered. (e.g. via the cpufreq
> ->get() interface). Also the ->get() interace only returns one
> value, so cant return raw values. So instead, leave it to userspace
> to keep track of raw values and do its math for CPUs it cares about.
> 
> delivered and reference perf counters are exposed via the same
> sysfs file to avoid the potential "skid", if these values are read
> individually from userspace.
> 
> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/acpi/cppc_acpi.c | 94 +++++++++++++++++++++++++++++++++++++++++++-----
>  include/acpi/cppc_acpi.h |  3 +-
>  2 files changed, 86 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 7844e4c..8bd501f 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -95,6 +95,17 @@ static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
>  				(cpc)->cpc_entry.reg.space_id ==	\
>  				ACPI_ADR_SPACE_PLATFORM_COMM)
>  
> +/* Evalutes to True if reg is a NULL register descriptor */
> +#define IS_NULL_REG(reg) ((reg)->space_id ==  ACPI_ADR_SPACE_SYSTEM_MEMORY && \
> +				(reg)->address == 0 &&			\
> +				(reg)->bit_width == 0 &&		\
> +				(reg)->bit_offset == 0 &&		\
> +				(reg)->access_width == 0)
> +
> +/* Evalutes to True if an optional cpc field is supported */
> +#define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?		\
> +				!!(cpc)->cpc_entry.int_value :			\
> +				!IS_NULL_REG(&(cpc)->cpc_entry.reg))
>  /*
>   * Arbitrary Retries in case the remote processor is slow to respond
>   * to PCC commands. Keeping it high enough to cover emulators where
> @@ -102,6 +113,61 @@ static unsigned int pcc_mpar, pcc_mrtt, pcc_nominal;
>   */
>  #define NUM_RETRIES 500
>  
> +struct cppc_attr {
> +	struct attribute attr;
> +	ssize_t (*show)(struct kobject *kobj,
> +			struct attribute *attr, char *buf);
> +	ssize_t (*store)(struct kobject *kobj,
> +			struct attribute *attr, const char *c, ssize_t count);
> +};
> +
> +#define define_one_cppc_ro(_name)		\
> +static struct cppc_attr _name =			\
> +__ATTR(_name, 0444, show_##_name, NULL)
> +
> +#define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
> +
> +static ssize_t show_feedback_ctrs(struct kobject *kobj,
> +		struct attribute *attr, char *buf)
> +{
> +	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
> +	struct cppc_perf_fb_ctrs fb_ctrs = {0};
> +
> +	cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
> +
> +	return scnprintf(buf, PAGE_SIZE, "ref:%lld del:%lld\n",
> +			fb_ctrs.reference, fb_ctrs.delivered);
> +}
> +define_one_cppc_ro(feedback_ctrs);
> +
> +static ssize_t show_reference_perf(struct kobject *kobj,
> +		struct attribute *attr, char *buf)
> +{
> +	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
> +	struct cppc_perf_caps perf_caps;
> +
> +	cppc_get_perf_caps(cpc_ptr->cpu_id, &perf_caps);
> +
> +	if (perf_caps.reference_perf)
> +		return scnprintf(buf, PAGE_SIZE, "%u\n",
> +				perf_caps.reference_perf);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
> +			perf_caps.nominal_perf);
> +}

What do you think about exporting Counter Wraparound Time register
as well if it's present?

Best regards,
Alexey Klimov


> +define_one_cppc_ro(reference_perf);
> +
> +static struct attribute *cppc_attrs[] = {
> +	&feedback_ctrs.attr,
> +	&reference_perf.attr,
> +	NULL
> +};
> +
> +static struct kobj_type cppc_ktype = {
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_attrs = cppc_attrs,
> +};
> +
>  static int check_pcc_chan(void)
>  {
>  	int ret = -EIO;
> @@ -544,6 +610,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>  	union acpi_object *out_obj, *cpc_obj;
>  	struct cpc_desc *cpc_ptr;
>  	struct cpc_reg *gas_t;
> +	struct device *cpu_dev;
>  	acpi_handle handle = pr->handle;
>  	unsigned int num_ent, i, cpc_rev;
>  	acpi_status status;
> @@ -666,6 +733,16 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>  	/* Everything looks okay */
>  	pr_debug("Parsed CPC struct for CPU: %d\n", pr->id);
>  
> +	/* Add per logical CPU nodes for reading its feedback counters. */
> +	cpu_dev = get_cpu_device(pr->id);
> +	if (!cpu_dev)
> +		goto out_free;
> +
> +	ret = kobject_init_and_add(&cpc_ptr->kobj, &cppc_ktype, &cpu_dev->kobj,
> +			"acpi_cppc");
> +	if (ret)
> +		goto out_free;
> +
>  	kfree(output.pointer);
>  	return 0;
>  
> @@ -695,6 +772,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
>  	struct cpc_desc *cpc_ptr;
>  	unsigned int i;
>  	void __iomem *addr;
> +
>  	cpc_ptr = per_cpu(cpc_desc_ptr, pr->id);
>  
>  	/* Free all the mapped sys mem areas for this CPU */
> @@ -704,6 +782,7 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
>  			iounmap(addr);
>  	}
>  
> +	kobject_put(&cpc_ptr->kobj);
>  	kfree(cpc_ptr);
>  }
>  EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
> @@ -721,7 +800,7 @@ static int cpc_read(struct cpc_register_resource *reg_res, u64 *val)
>  	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
>  
>  	if (reg_res->type == ACPI_TYPE_INTEGER) {
> -		*val = reg_res->int_value;
> +		*val = reg_res->cpc_entry.int_value;
>  		return ret_val;
>  	}
>  
> @@ -836,8 +915,11 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
>  	cpc_read(lowest_reg, &low);
>  	perf_caps->lowest_perf = low;
>  
> -	cpc_read(ref_perf, &ref);
> -	perf_caps->reference_perf = ref;
> +	perf_caps->reference_perf = 0;
> +	if (CPC_SUPPORTED(ref_perf)) {
> +		cpc_read(ref_perf, &ref);
> +		perf_caps->reference_perf = ref;
> +	}
>  
>  	cpc_read(nom_perf, &nom);
>  	perf_caps->nominal_perf = nom;
> @@ -899,12 +981,6 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
>  	perf_fb_ctrs->delivered = delivered;
>  	perf_fb_ctrs->reference = reference;
>  
> -	perf_fb_ctrs->delivered -= perf_fb_ctrs->prev_delivered;
> -	perf_fb_ctrs->reference -= perf_fb_ctrs->prev_reference;
> -
> -	perf_fb_ctrs->prev_delivered = delivered;
> -	perf_fb_ctrs->prev_reference = reference;
> -
>  out_err:
>  	if (regs_in_pcc)
>  		up_write(&pcc_lock);
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 7b7e2e1..2bf9db1 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -63,6 +63,7 @@ struct cpc_desc {
>  	int cpu_id;
>  	struct cpc_register_resource cpc_regs[MAX_CPC_REG_ENT];
>  	struct acpi_psd_package domain_info;
> +	struct kobject kobj;
>  };
>  
>  /* These are indexes into the per-cpu cpc_regs[]. Order is important. */
> @@ -109,9 +110,7 @@ struct cppc_perf_ctrls {
>  
>  struct cppc_perf_fb_ctrs {
>  	u64 reference;
> -	u64 prev_reference;
>  	u64 delivered;
> -	u64 prev_delivered;
>  };
>  
>  /* Per CPU container for runtime CPPC management. */
> -- 
> Qualcomm Technologies, Inc. on behalf
> of the Qualcomm Innovation Center, Inc.  The Qualcomm Innovation Center, Inc. 
> is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

Re: [PATCH 5/5] ACPI/CPPC: add sysfs support to compute delivered performance

[Prakash, Prashanth]

Hi Alexey,
>> +
>> +static ssize_t show_reference_perf(struct kobject *kobj,
>> +		struct attribute *attr, char *buf)
>> +{
>> +	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
>> +	struct cppc_perf_caps perf_caps;
>> +
>> +	cppc_get_perf_caps(cpc_ptr->cpu_id, &perf_caps);
>> +
>> +	if (perf_caps.reference_perf)
>> +		return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +				perf_caps.reference_perf);
>> +
>> +	return scnprintf(buf, PAGE_SIZE, "%u\n",
>> +			perf_caps.nominal_perf);
>> +}
> What do you think about exporting Counter Wraparound Time register
> as well if it's present?

Yes, that's a good point. I will add it in the next version.

Thanks,
Prashanth