[PATCH v4 0/2] Make cppc acpi driver aware of pcc subspace ids

[PATCH v4 0/2] Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

The current cppc acpi driver works with only one pcc subspace id.
It maintains and registers only one pcc channel even if the acpi table has 
different pcc subspace ids. 

As per ACPI 6.2 spec  all PCC registers, for all processors in the same 
performance domain (as defined by _PSD), must be defined to be in the same
subspace. The series tries to address the same by making cppc acpi driver 
aware of multiple possible pcc subspace ids.

Patch 1 : In preparation to share the MAX_PCC_SUBSPACE definition with cppc acpi
          driver
Patch 2 : Make the cppc acpi driver aware of multiple pcc subspace ids.

Changes from v3:
  - Address Issue reported by kbuild-robot.

Changes from v2:
  - Addressed Prashanth's comments on
    * Not to use local variables to update mpar_count, last_mpar_reset and 
      last_cmd_cmpl_time
    * Add check for kzalloc failure in pcc_data_alloc()
    * Initialize pcc_subspace_id to -1 in acpi_cppc_processor_probe()
    * Check for pcc_subspace_id validity before registering pcc_channel

Changes from v1:
  - Add last_cmd_cmpl_time, last_mpar_reset, mpar_count to the cppc_pcc_data to
make it per subspace.
  - PCC per subspace dynamic allocation support added instead of static
allocation
  - Added a new function pcc_data_alloc, In instances where CPU's with SMT
support same PCC subspace would be used for all CPU's belonging to same 
physical core. This function adds the pcc_subspace refcounting and allocates
the cppc_pcc_data per unique subspace idx.
  - Added cleanup in acpi_cppc_processor_exit. Free the mbox channel and free
the cppc_pcc_data in case refcount is zero.

George Cherian (2):
  mailbox: PCC: Move the MAX_PCC_SUBSPACES definition to header file
  ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

 drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
 drivers/mailbox/pcc.c    |   1 -
 include/acpi/pcc.h       |   1 +
 3 files changed, 154 insertions(+), 91 deletions(-)

-- 
2.1.4

[Devel] [PATCH v4 0/2] Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

The current cppc acpi driver works with only one pcc subspace id.
It maintains and registers only one pcc channel even if the acpi table has 
different pcc subspace ids. 

As per ACPI 6.2 spec  all PCC registers, for all processors in the same 
performance domain (as defined by _PSD), must be defined to be in the same
subspace. The series tries to address the same by making cppc acpi driver 
aware of multiple possible pcc subspace ids.

Patch 1 : In preparation to share the MAX_PCC_SUBSPACE definition with cppc acpi
          driver
Patch 2 : Make the cppc acpi driver aware of multiple pcc subspace ids.

Changes from v3:
  - Address Issue reported by kbuild-robot.

Changes from v2:
  - Addressed Prashanth's comments on
    * Not to use local variables to update mpar_count, last_mpar_reset and 
      last_cmd_cmpl_time
    * Add check for kzalloc failure in pcc_data_alloc()
    * Initialize pcc_subspace_id to -1 in acpi_cppc_processor_probe()
    * Check for pcc_subspace_id validity before registering pcc_channel

Changes from v1:
  - Add last_cmd_cmpl_time, last_mpar_reset, mpar_count to the cppc_pcc_data to
make it per subspace.
  - PCC per subspace dynamic allocation support added instead of static
allocation
  - Added a new function pcc_data_alloc, In instances where CPU's with SMT
support same PCC subspace would be used for all CPU's belonging to same 
physical core. This function adds the pcc_subspace refcounting and allocates
the cppc_pcc_data per unique subspace idx.
  - Added cleanup in acpi_cppc_processor_exit. Free the mbox channel and free
the cppc_pcc_data in case refcount is zero.

George Cherian (2):
  mailbox: PCC: Move the MAX_PCC_SUBSPACES definition to header file
  ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

 drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
 drivers/mailbox/pcc.c    |   1 -
 include/acpi/pcc.h       |   1 +
 3 files changed, 154 insertions(+), 91 deletions(-)

-- 
2.1.4

[PATCH v4 1/2] mailbox: PCC: Move the MAX_PCC_SUBSPACES definition to header file

[George Cherian]

Move the MAX_PCC_SUBSPACES definition to acpi/pcc.h file. In preparation to add
subspace id support for cppc_acpi driver.

Signed-off-by: George Cherian <george.cherian@cavium.com>
---
 drivers/mailbox/pcc.c | 1 -
 include/acpi/pcc.h    | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 9b7005e..e5a6967 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -69,7 +69,6 @@
 
 #include "mailbox.h"
 
-#define MAX_PCC_SUBSPACES	256
 #define MBOX_IRQ_NAME		"pcc-mbox"
 
 static struct mbox_chan *pcc_mbox_channels;
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 8caa79c..cd6ef45 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -13,6 +13,7 @@
 #include <linux/mailbox_controller.h>
 #include <linux/mailbox_client.h>
 
+#define MAX_PCC_SUBSPACES	256
 #ifdef CONFIG_PCC
 extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
 						  int subspace_id);
-- 
2.1.4

[Devel] [PATCH v4 1/2] mailbox: PCC: Move the MAX_PCC_SUBSPACES definition to header file

[George Cherian]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

Move the MAX_PCC_SUBSPACES definition to acpi/pcc.h file. In preparation to add
subspace id support for cppc_acpi driver.

Signed-off-by: George Cherian <george.cherian(a)cavium.com>
---
 drivers/mailbox/pcc.c | 1 -
 include/acpi/pcc.h    | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 9b7005e..e5a6967 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -69,7 +69,6 @@
 
 #include "mailbox.h"
 
-#define MAX_PCC_SUBSPACES	256
 #define MBOX_IRQ_NAME		"pcc-mbox"
 
 static struct mbox_chan *pcc_mbox_channels;
diff --git a/include/acpi/pcc.h b/include/acpi/pcc.h
index 8caa79c..cd6ef45 100644
--- a/include/acpi/pcc.h
+++ b/include/acpi/pcc.h
@@ -13,6 +13,7 @@
 #include <linux/mailbox_controller.h>
 #include <linux/mailbox_client.h>
 
+#define MAX_PCC_SUBSPACES	256
 #ifdef CONFIG_PCC
 extern struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
 						  int subspace_id);
-- 
2.1.4

[PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
all PCC registers, for all processors in the same performance
domain (as defined by _PSD), must be defined to be in the same subspace.
Based on Section 14.1 of ACPI specification, it is possible to have a
maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
instead of using a single global pcc_data structure.

While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
and mpar_count is initialized properly.

Signed-off-by: George Cherian <george.cherian@cavium.com>
---
 drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
 1 file changed, 153 insertions(+), 90 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index e5b47f0..3ae79ef 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -75,13 +75,16 @@ struct cppc_pcc_data {
 
 	/* Wait queue for CPUs whose requests were batched */
 	wait_queue_head_t pcc_write_wait_q;
+	ktime_t last_cmd_cmpl_time;
+	ktime_t last_mpar_reset;
+	int mpar_count;
+	int refcount;
 };
 
-/* Structure to represent the single PCC channel */
-static struct cppc_pcc_data pcc_data = {
-	.pcc_subspace_idx = -1,
-	.platform_owns_pcc = true,
-};
+/* Array  to represent the PCC channel per subspace id */
+static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
+/* The cpu_pcc_subspace_idx containsper CPU subspace id */
+static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
 
 /*
  * The cpc_desc structure contains the ACPI register details
@@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
 static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 
 /* pcc mapped address + header size + offset within PCC subspace */
-#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
+#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
+						0x8 + (offs))
 
 /* Check if a CPC register is in PCC */
 #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
@@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
 	.default_attrs = cppc_attrs,
 };
 
-static int check_pcc_chan(bool chk_err_bit)
+static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
 {
 	int ret = -EIO, status = 0;
-	struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
-	ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+		pcc_ss_data->pcc_comm_addr;
+	ktime_t next_deadline = ktime_add(ktime_get(),
+					  pcc_ss_data->deadline);
 
-	if (!pcc_data.platform_owns_pcc)
+	if (!pcc_ss_data->platform_owns_pcc)
 		return 0;
 
 	/* Retry in case the remote processor was too slow to catch up. */
@@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
 	}
 
 	if (likely(!ret))
-		pcc_data.platform_owns_pcc = false;
+		pcc_ss_data->platform_owns_pcc = false;
 	else
 		pr_err("PCC check channel failed. Status=%x\n", status);
 
@@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
  * 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)
+static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 {
 	int ret = -EIO, i;
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	struct acpi_pcct_shared_memory *generic_comm_base =
-		(struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
-	static ktime_t last_cmd_cmpl_time, last_mpar_reset;
-	static int mpar_count;
+		(struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
 	unsigned int time_delta;
 
 	/*
@@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
 		 * before write completion, so first send a WRITE command to
 		 * platform
 		 */
-		if (pcc_data.pending_pcc_write_cmd)
-			send_pcc_cmd(CMD_WRITE);
+		if (pcc_ss_data->pending_pcc_write_cmd)
+			send_pcc_cmd(pcc_ss_id, CMD_WRITE);
 
-		ret = check_pcc_chan(false);
+		ret = check_pcc_chan(pcc_ss_id, false);
 		if (ret)
 			goto end;
 	} else /* CMD_WRITE */
-		pcc_data.pending_pcc_write_cmd = FALSE;
+		pcc_ss_data->pending_pcc_write_cmd = FALSE;
 
 	/*
 	 * Handle the Minimum Request Turnaround Time(MRTT)
 	 * "The minimum amount of time that OSPM must wait after the completion
 	 * of a command before issuing the next command, in microseconds"
 	 */
-	if (pcc_data.pcc_mrtt) {
-		time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
-		if (pcc_data.pcc_mrtt > time_delta)
-			udelay(pcc_data.pcc_mrtt - time_delta);
+	if (pcc_ss_data->pcc_mrtt) {
+		time_delta = ktime_us_delta(ktime_get(),
+					    pcc_ss_data->last_cmd_cmpl_time);
+		if (pcc_ss_data->pcc_mrtt > time_delta)
+			udelay(pcc_ss_data->pcc_mrtt - time_delta);
 	}
 
 	/*
@@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
 	 * not send the request to the platform after hitting the MPAR limit in
 	 * any 60s window
 	 */
-	if (pcc_data.pcc_mpar) {
-		if (mpar_count == 0) {
-			time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
-			if (time_delta < 60 * MSEC_PER_SEC) {
+	if (pcc_ss_data->pcc_mpar) {
+		if (pcc_ss_data->mpar_count == 0) {
+			time_delta = ktime_ms_delta(ktime_get(),
+						    pcc_ss_data->last_mpar_reset);
+			if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
 				pr_debug("PCC cmd not sent due to MPAR limit");
 				ret = -EIO;
 				goto end;
 			}
-			last_mpar_reset = ktime_get();
-			mpar_count = pcc_data.pcc_mpar;
+			pcc_ss_data->last_mpar_reset = ktime_get();
+			pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
 		}
-		mpar_count--;
+		pcc_ss_data->mpar_count--;
 	}
 
 	/* Write to the shared comm region. */
@@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
 	/* Flip CMD COMPLETE bit */
 	writew_relaxed(0, &generic_comm_base->status);
 
-	pcc_data.platform_owns_pcc = true;
+	pcc_ss_data->platform_owns_pcc = true;
 
 	/* Ring doorbell */
-	ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
+	ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
 	if (ret < 0) {
 		pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
 				cmd, ret);
@@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
 	}
 
 	/* wait for completion and check for PCC errro bit */
-	ret = check_pcc_chan(true);
+	ret = check_pcc_chan(pcc_ss_id, true);
 
-	if (pcc_data.pcc_mrtt)
-		last_cmd_cmpl_time = ktime_get();
+	if (pcc_ss_data->pcc_mrtt)
+		pcc_ss_data->last_cmd_cmpl_time = ktime_get();
 
-	if (pcc_data.pcc_channel->mbox->txdone_irq)
-		mbox_chan_txdone(pcc_data.pcc_channel, ret);
+	if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
+		mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
 	else
-		mbox_client_txdone(pcc_data.pcc_channel, ret);
+		mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
 
 end:
 	if (cmd == CMD_WRITE) {
@@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
 				if (!desc)
 					continue;
 
-				if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
+				if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
 					desc->write_cmd_status = ret;
 			}
 		}
-		pcc_data.pcc_write_cnt++;
-		wake_up_all(&pcc_data.pcc_write_wait_q);
+		pcc_ss_data->pcc_write_cnt++;
+		wake_up_all(&pcc_ss_data->pcc_write_wait_q);
 	}
 
 	return ret;
@@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
 }
 EXPORT_SYMBOL_GPL(acpi_get_psd_map);
 
-static int register_pcc_channel(int pcc_subspace_idx)
+static int register_pcc_channel(int pcc_ss_idx)
 {
 	struct acpi_pcct_hw_reduced *cppc_ss;
 	u64 usecs_lat;
 
-	if (pcc_subspace_idx >= 0) {
-		pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
-				pcc_subspace_idx);
+	if (pcc_ss_idx >= 0) {
+		pcc_data[pcc_ss_idx]->pcc_channel =
+			pcc_mbox_request_channel(&cppc_mbox_cl,	pcc_ss_idx);
 
-		if (IS_ERR(pcc_data.pcc_channel)) {
+		if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
 			pr_err("Failed to find PCC communication channel\n");
 			return -ENODEV;
 		}
@@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
 		 * PCC channels) and stored pointers to the
 		 * subspace communication region in con_priv.
 		 */
-		cppc_ss = (pcc_data.pcc_channel)->con_priv;
+		cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
 
 		if (!cppc_ss) {
 			pr_err("No PCC subspace found for CPPC\n");
@@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
 		 * So add an arbitrary amount of wait on top of Nominal.
 		 */
 		usecs_lat = NUM_RETRIES * cppc_ss->latency;
-		pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
-		pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
-		pcc_data.pcc_mpar = cppc_ss->max_access_rate;
-		pcc_data.pcc_nominal = cppc_ss->latency;
-
-		pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
-		if (!pcc_data.pcc_comm_addr) {
+		pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
+		pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
+		pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
+		pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
+
+		pcc_data[pcc_ss_idx]->pcc_comm_addr =
+			acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
+		if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
 			pr_err("Failed to ioremap PCC comm region mem\n");
 			return -ENOMEM;
 		}
 
 		/* Set flag so that we dont come here for each CPU. */
-		pcc_data.pcc_channel_acquired = true;
+		pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
 	}
 
 	return 0;
@@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
 	return false;
 }
 
+
+/**
+ * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
+ *
+ * Check and allocate the cppc_pcc_data memory.
+ * In some processor configurations it is possible that same subspace
+ * is shared between multiple CPU's. This is seen especially in CPU's
+ * with hardware multi-threading support.
+ *
+ * Return: 0 for success, errno for failure
+ */
+int pcc_data_alloc(int pcc_ss_id)
+{
+	int loop;
+
+	if (pcc_ss_id < 0)
+		return -EINVAL;
+
+	for (loop = 0; pcc_data[loop] != NULL; loop++) {
+		if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
+			pcc_data[loop]->refcount++;
+			return 0;
+		}
+	}
+
+	pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
+	if (!pcc_data[pcc_ss_id])
+		return -ENOMEM;
+	pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
+	pcc_data[pcc_ss_id]->refcount++;
+
+	return 0;
+}
 /*
  * An example CPC table looks like the following.
  *
@@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	struct device *cpu_dev;
 	acpi_handle handle = pr->handle;
 	unsigned int num_ent, i, cpc_rev;
+	int pcc_subspace_id = -1;
 	acpi_status status;
 	int ret = -EFAULT;
 
@@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 			 * so extract it only once.
 			 */
 			if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-				if (pcc_data.pcc_subspace_idx < 0)
-					pcc_data.pcc_subspace_idx = gas_t->access_width;
-				else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
-					pr_debug("Mismatched PCC ids.\n");
+				pcc_subspace_id = gas_t->access_width;
+				if (pcc_data_alloc(pcc_subspace_id))
 					goto out_free;
-				}
 			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
 				if (gas_t->address) {
 					void __iomem *addr;
@@ -763,6 +803,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 			goto out_free;
 		}
 	}
+	per_cpu(cpu_pcc_subspace_idx, pr->id) = pcc_subspace_id;
 	/* Store CPU Logical ID */
 	cpc_ptr->cpu_id = pr->id;
 
@@ -771,14 +812,14 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	if (ret)
 		goto out_free;
 
-	/* Register PCC channel once for all CPUs. */
-	if (!pcc_data.pcc_channel_acquired) {
-		ret = register_pcc_channel(pcc_data.pcc_subspace_idx);
+	/* Register PCC channel once for all PCC subspace id. */
+	if (pcc_subspace_id >= 0 && !pcc_data[pcc_subspace_id]->pcc_channel_acquired) {
+		ret = register_pcc_channel(pcc_subspace_id);
 		if (ret)
 			goto out_free;
 
-		init_rwsem(&pcc_data.pcc_lock);
-		init_waitqueue_head(&pcc_data.pcc_write_wait_q);
+		init_rwsem(&pcc_data[pcc_subspace_id]->pcc_lock);
+		init_waitqueue_head(&pcc_data[pcc_subspace_id]->pcc_write_wait_q);
 	}
 
 	/* Everything looks okay */
@@ -831,6 +872,18 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
 	struct cpc_desc *cpc_ptr;
 	unsigned int i;
 	void __iomem *addr;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, pr->id);
+
+	if (pcc_ss_id >=0 && pcc_data[pcc_ss_id]) {
+		if (pcc_data[pcc_ss_id]->pcc_channel_acquired) {
+			pcc_data[pcc_ss_id]->refcount--;
+			if (!pcc_data[pcc_ss_id]->refcount) {
+				pcc_mbox_free_channel(pcc_data[pcc_ss_id]->pcc_channel);
+				pcc_data[pcc_ss_id]->pcc_channel_acquired = 0;
+				kfree(pcc_data[pcc_ss_id]);
+			}
+		}
+	}
 
 	cpc_ptr = per_cpu(cpc_desc_ptr, pr->id);
 	if (!cpc_ptr)
@@ -888,6 +941,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 {
 	int ret_val = 0;
 	void __iomem *vaddr = 0;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
 	if (reg_res->type == ACPI_TYPE_INTEGER) {
@@ -897,7 +951,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 
 	*val = 0;
 	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
-		vaddr = GET_PCC_VADDR(reg->address);
+		vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id);
 	else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
 		vaddr = reg_res->sys_mem_vaddr;
 	else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
@@ -932,10 +986,11 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 {
 	int ret_val = 0;
 	void __iomem *vaddr = 0;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
 	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
-		vaddr = GET_PCC_VADDR(reg->address);
+		vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id);
 	else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
 		vaddr = reg_res->sys_mem_vaddr;
 	else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
@@ -980,6 +1035,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	struct cpc_register_resource *highest_reg, *lowest_reg,
 		*lowest_non_linear_reg, *nominal_reg;
 	u64 high, low, nom, min_nonlinear;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	int ret = 0, regs_in_pcc = 0;
 
 	if (!cpc_desc) {
@@ -996,9 +1053,9 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
 		CPC_IN_PCC(lowest_non_linear_reg) || CPC_IN_PCC(nominal_reg)) {
 		regs_in_pcc = 1;
-		down_write(&pcc_data.pcc_lock);
+		down_write(&pcc_ss_data->pcc_lock);
 		/* Ring doorbell once to update PCC subspace */
-		if (send_pcc_cmd(CMD_READ) < 0) {
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
 			ret = -EIO;
 			goto out_err;
 		}
@@ -1021,7 +1078,7 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 
 out_err:
 	if (regs_in_pcc)
-		up_write(&pcc_data.pcc_lock);
+		up_write(&pcc_ss_data->pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
@@ -1038,6 +1095,8 @@ 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,
 		*ref_perf_reg, *ctr_wrap_reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	u64 delivered, reference, ref_perf, ctr_wrap_time;
 	int ret = 0, regs_in_pcc = 0;
 
@@ -1061,10 +1120,10 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	/* Are any of the regs PCC ?*/
 	if (CPC_IN_PCC(delivered_reg) || CPC_IN_PCC(reference_reg) ||
 		CPC_IN_PCC(ctr_wrap_reg) || CPC_IN_PCC(ref_perf_reg)) {
-		down_write(&pcc_data.pcc_lock);
+		down_write(&pcc_ss_data->pcc_lock);
 		regs_in_pcc = 1;
 		/* Ring doorbell once to update PCC subspace */
-		if (send_pcc_cmd(CMD_READ) < 0) {
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
 			ret = -EIO;
 			goto out_err;
 		}
@@ -1094,7 +1153,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	perf_fb_ctrs->wraparound_time = ctr_wrap_time;
 out_err:
 	if (regs_in_pcc)
-		up_write(&pcc_data.pcc_lock);
+		up_write(&pcc_ss_data->pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
@@ -1110,6 +1169,8 @@ 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 pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	int ret = 0;
 
 	if (!cpc_desc) {
@@ -1127,11 +1188,11 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * achieve that goal here
 	 */
 	if (CPC_IN_PCC(desired_reg)) {
-		down_read(&pcc_data.pcc_lock);	/* BEGIN Phase-I */
-		if (pcc_data.platform_owns_pcc) {
-			ret = check_pcc_chan(false);
+		down_read(&pcc_ss_data->pcc_lock); /* BEGIN Phase-I */
+		if (pcc_ss_data->platform_owns_pcc) {
+			ret = check_pcc_chan(pcc_ss_id, false);
 			if (ret) {
-				up_read(&pcc_data.pcc_lock);
+				up_read(&pcc_ss_data->pcc_lock);
 				return ret;
 			}
 		}
@@ -1139,8 +1200,8 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 		 * Update the pending_write to make sure a PCC CMD_READ will not
 		 * arrive and steal the channel during the switch to write lock
 		 */
-		pcc_data.pending_pcc_write_cmd = true;
-		cpc_desc->write_cmd_id = pcc_data.pcc_write_cnt;
+		pcc_ss_data->pending_pcc_write_cmd = true;
+		cpc_desc->write_cmd_id = pcc_ss_data->pcc_write_cnt;
 		cpc_desc->write_cmd_status = 0;
 	}
 
@@ -1151,7 +1212,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
 
 	if (CPC_IN_PCC(desired_reg))
-		up_read(&pcc_data.pcc_lock);	/* END Phase-I */
+		up_read(&pcc_ss_data->pcc_lock);	/* END Phase-I */
 	/*
 	 * This is Phase-II where we transfer the ownership of PCC to Platform
 	 *
@@ -1199,15 +1260,15 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * the write command before servicing the read command
 	 */
 	if (CPC_IN_PCC(desired_reg)) {
-		if (down_write_trylock(&pcc_data.pcc_lock)) {	/* BEGIN Phase-II */
+		if (down_write_trylock(&pcc_ss_data->pcc_lock)) {/* BEGIN Phase-II */
 			/* Update only if there are pending write commands */
-			if (pcc_data.pending_pcc_write_cmd)
-				send_pcc_cmd(CMD_WRITE);
-			up_write(&pcc_data.pcc_lock);		/* END Phase-II */
+			if (pcc_ss_data->pending_pcc_write_cmd)
+				send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+			up_write(&pcc_ss_data->pcc_lock);	/* END Phase-II */
 		} else
 			/* Wait until pcc_write_cnt is updated by send_pcc_cmd */
-			wait_event(pcc_data.pcc_write_wait_q,
-				cpc_desc->write_cmd_id != pcc_data.pcc_write_cnt);
+			wait_event(pcc_ss_data->pcc_write_wait_q,
+				   cpc_desc->write_cmd_id != pcc_ss_data->pcc_write_cnt);
 
 		/* send_pcc_cmd updates the status in case of failure */
 		ret = cpc_desc->write_cmd_status;
@@ -1240,6 +1301,8 @@ unsigned int cppc_get_transition_latency(int cpu_num)
 	unsigned int latency_ns = 0;
 	struct cpc_desc *cpc_desc;
 	struct cpc_register_resource *desired_reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu_num);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 
 	cpc_desc = per_cpu(cpc_desc_ptr, cpu_num);
 	if (!cpc_desc)
@@ -1249,11 +1312,11 @@ unsigned int cppc_get_transition_latency(int cpu_num)
 	if (!CPC_IN_PCC(desired_reg))
 		return CPUFREQ_ETERNAL;
 
-	if (pcc_data.pcc_mpar)
-		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_data.pcc_mpar);
+	if (pcc_ss_data->pcc_mpar)
+		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_ss_data->pcc_mpar);
 
-	latency_ns = max(latency_ns, pcc_data.pcc_nominal * 1000);
-	latency_ns = max(latency_ns, pcc_data.pcc_mrtt * 1000);
+	latency_ns = max(latency_ns, pcc_ss_data->pcc_nominal * 1000);
+	latency_ns = max(latency_ns, pcc_ss_data->pcc_mrtt * 1000);
 
 	return latency_ns;
 }
-- 
2.1.4

[Devel] [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

[-- Attachment #1: Type: text/plain, Size: 20858 bytes --]

Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
all PCC registers, for all processors in the same performance
domain (as defined by _PSD), must be defined to be in the same subspace.
Based on Section 14.1 of ACPI specification, it is possible to have a
maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
instead of using a single global pcc_data structure.

While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
and mpar_count is initialized properly.

Signed-off-by: George Cherian <george.cherian(a)cavium.com>
---
 drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
 1 file changed, 153 insertions(+), 90 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index e5b47f0..3ae79ef 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -75,13 +75,16 @@ struct cppc_pcc_data {
 
 	/* Wait queue for CPUs whose requests were batched */
 	wait_queue_head_t pcc_write_wait_q;
+	ktime_t last_cmd_cmpl_time;
+	ktime_t last_mpar_reset;
+	int mpar_count;
+	int refcount;
 };
 
-/* Structure to represent the single PCC channel */
-static struct cppc_pcc_data pcc_data = {
-	.pcc_subspace_idx = -1,
-	.platform_owns_pcc = true,
-};
+/* Array  to represent the PCC channel per subspace id */
+static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
+/* The cpu_pcc_subspace_idx containsper CPU subspace id */
+static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
 
 /*
  * The cpc_desc structure contains the ACPI register details
@@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
 static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 
 /* pcc mapped address + header size + offset within PCC subspace */
-#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
+#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
+						0x8 + (offs))
 
 /* Check if a CPC register is in PCC */
 #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
@@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
 	.default_attrs = cppc_attrs,
 };
 
-static int check_pcc_chan(bool chk_err_bit)
+static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
 {
 	int ret = -EIO, status = 0;
-	struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
-	ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
+	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
+		pcc_ss_data->pcc_comm_addr;
+	ktime_t next_deadline = ktime_add(ktime_get(),
+					  pcc_ss_data->deadline);
 
-	if (!pcc_data.platform_owns_pcc)
+	if (!pcc_ss_data->platform_owns_pcc)
 		return 0;
 
 	/* Retry in case the remote processor was too slow to catch up. */
@@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
 	}
 
 	if (likely(!ret))
-		pcc_data.platform_owns_pcc = false;
+		pcc_ss_data->platform_owns_pcc = false;
 	else
 		pr_err("PCC check channel failed. Status=%x\n", status);
 
@@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
  * 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)
+static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
 {
 	int ret = -EIO, i;
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	struct acpi_pcct_shared_memory *generic_comm_base =
-		(struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
-	static ktime_t last_cmd_cmpl_time, last_mpar_reset;
-	static int mpar_count;
+		(struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
 	unsigned int time_delta;
 
 	/*
@@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
 		 * before write completion, so first send a WRITE command to
 		 * platform
 		 */
-		if (pcc_data.pending_pcc_write_cmd)
-			send_pcc_cmd(CMD_WRITE);
+		if (pcc_ss_data->pending_pcc_write_cmd)
+			send_pcc_cmd(pcc_ss_id, CMD_WRITE);
 
-		ret = check_pcc_chan(false);
+		ret = check_pcc_chan(pcc_ss_id, false);
 		if (ret)
 			goto end;
 	} else /* CMD_WRITE */
-		pcc_data.pending_pcc_write_cmd = FALSE;
+		pcc_ss_data->pending_pcc_write_cmd = FALSE;
 
 	/*
 	 * Handle the Minimum Request Turnaround Time(MRTT)
 	 * "The minimum amount of time that OSPM must wait after the completion
 	 * of a command before issuing the next command, in microseconds"
 	 */
-	if (pcc_data.pcc_mrtt) {
-		time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
-		if (pcc_data.pcc_mrtt > time_delta)
-			udelay(pcc_data.pcc_mrtt - time_delta);
+	if (pcc_ss_data->pcc_mrtt) {
+		time_delta = ktime_us_delta(ktime_get(),
+					    pcc_ss_data->last_cmd_cmpl_time);
+		if (pcc_ss_data->pcc_mrtt > time_delta)
+			udelay(pcc_ss_data->pcc_mrtt - time_delta);
 	}
 
 	/*
@@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
 	 * not send the request to the platform after hitting the MPAR limit in
 	 * any 60s window
 	 */
-	if (pcc_data.pcc_mpar) {
-		if (mpar_count == 0) {
-			time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
-			if (time_delta < 60 * MSEC_PER_SEC) {
+	if (pcc_ss_data->pcc_mpar) {
+		if (pcc_ss_data->mpar_count == 0) {
+			time_delta = ktime_ms_delta(ktime_get(),
+						    pcc_ss_data->last_mpar_reset);
+			if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
 				pr_debug("PCC cmd not sent due to MPAR limit");
 				ret = -EIO;
 				goto end;
 			}
-			last_mpar_reset = ktime_get();
-			mpar_count = pcc_data.pcc_mpar;
+			pcc_ss_data->last_mpar_reset = ktime_get();
+			pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
 		}
-		mpar_count--;
+		pcc_ss_data->mpar_count--;
 	}
 
 	/* Write to the shared comm region. */
@@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
 	/* Flip CMD COMPLETE bit */
 	writew_relaxed(0, &generic_comm_base->status);
 
-	pcc_data.platform_owns_pcc = true;
+	pcc_ss_data->platform_owns_pcc = true;
 
 	/* Ring doorbell */
-	ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
+	ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
 	if (ret < 0) {
 		pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
 				cmd, ret);
@@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
 	}
 
 	/* wait for completion and check for PCC errro bit */
-	ret = check_pcc_chan(true);
+	ret = check_pcc_chan(pcc_ss_id, true);
 
-	if (pcc_data.pcc_mrtt)
-		last_cmd_cmpl_time = ktime_get();
+	if (pcc_ss_data->pcc_mrtt)
+		pcc_ss_data->last_cmd_cmpl_time = ktime_get();
 
-	if (pcc_data.pcc_channel->mbox->txdone_irq)
-		mbox_chan_txdone(pcc_data.pcc_channel, ret);
+	if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
+		mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
 	else
-		mbox_client_txdone(pcc_data.pcc_channel, ret);
+		mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
 
 end:
 	if (cmd == CMD_WRITE) {
@@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
 				if (!desc)
 					continue;
 
-				if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
+				if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
 					desc->write_cmd_status = ret;
 			}
 		}
-		pcc_data.pcc_write_cnt++;
-		wake_up_all(&pcc_data.pcc_write_wait_q);
+		pcc_ss_data->pcc_write_cnt++;
+		wake_up_all(&pcc_ss_data->pcc_write_wait_q);
 	}
 
 	return ret;
@@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
 }
 EXPORT_SYMBOL_GPL(acpi_get_psd_map);
 
-static int register_pcc_channel(int pcc_subspace_idx)
+static int register_pcc_channel(int pcc_ss_idx)
 {
 	struct acpi_pcct_hw_reduced *cppc_ss;
 	u64 usecs_lat;
 
-	if (pcc_subspace_idx >= 0) {
-		pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
-				pcc_subspace_idx);
+	if (pcc_ss_idx >= 0) {
+		pcc_data[pcc_ss_idx]->pcc_channel =
+			pcc_mbox_request_channel(&cppc_mbox_cl,	pcc_ss_idx);
 
-		if (IS_ERR(pcc_data.pcc_channel)) {
+		if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
 			pr_err("Failed to find PCC communication channel\n");
 			return -ENODEV;
 		}
@@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
 		 * PCC channels) and stored pointers to the
 		 * subspace communication region in con_priv.
 		 */
-		cppc_ss = (pcc_data.pcc_channel)->con_priv;
+		cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
 
 		if (!cppc_ss) {
 			pr_err("No PCC subspace found for CPPC\n");
@@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
 		 * So add an arbitrary amount of wait on top of Nominal.
 		 */
 		usecs_lat = NUM_RETRIES * cppc_ss->latency;
-		pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
-		pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
-		pcc_data.pcc_mpar = cppc_ss->max_access_rate;
-		pcc_data.pcc_nominal = cppc_ss->latency;
-
-		pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
-		if (!pcc_data.pcc_comm_addr) {
+		pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
+		pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
+		pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
+		pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
+
+		pcc_data[pcc_ss_idx]->pcc_comm_addr =
+			acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
+		if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
 			pr_err("Failed to ioremap PCC comm region mem\n");
 			return -ENOMEM;
 		}
 
 		/* Set flag so that we dont come here for each CPU. */
-		pcc_data.pcc_channel_acquired = true;
+		pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
 	}
 
 	return 0;
@@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
 	return false;
 }
 
+
+/**
+ * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
+ *
+ * Check and allocate the cppc_pcc_data memory.
+ * In some processor configurations it is possible that same subspace
+ * is shared between multiple CPU's. This is seen especially in CPU's
+ * with hardware multi-threading support.
+ *
+ * Return: 0 for success, errno for failure
+ */
+int pcc_data_alloc(int pcc_ss_id)
+{
+	int loop;
+
+	if (pcc_ss_id < 0)
+		return -EINVAL;
+
+	for (loop = 0; pcc_data[loop] != NULL; loop++) {
+		if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
+			pcc_data[loop]->refcount++;
+			return 0;
+		}
+	}
+
+	pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
+	if (!pcc_data[pcc_ss_id])
+		return -ENOMEM;
+	pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
+	pcc_data[pcc_ss_id]->refcount++;
+
+	return 0;
+}
 /*
  * An example CPC table looks like the following.
  *
@@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	struct device *cpu_dev;
 	acpi_handle handle = pr->handle;
 	unsigned int num_ent, i, cpc_rev;
+	int pcc_subspace_id = -1;
 	acpi_status status;
 	int ret = -EFAULT;
 
@@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 			 * so extract it only once.
 			 */
 			if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-				if (pcc_data.pcc_subspace_idx < 0)
-					pcc_data.pcc_subspace_idx = gas_t->access_width;
-				else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
-					pr_debug("Mismatched PCC ids.\n");
+				pcc_subspace_id = gas_t->access_width;
+				if (pcc_data_alloc(pcc_subspace_id))
 					goto out_free;
-				}
 			} else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
 				if (gas_t->address) {
 					void __iomem *addr;
@@ -763,6 +803,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 			goto out_free;
 		}
 	}
+	per_cpu(cpu_pcc_subspace_idx, pr->id) = pcc_subspace_id;
 	/* Store CPU Logical ID */
 	cpc_ptr->cpu_id = pr->id;
 
@@ -771,14 +812,14 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 	if (ret)
 		goto out_free;
 
-	/* Register PCC channel once for all CPUs. */
-	if (!pcc_data.pcc_channel_acquired) {
-		ret = register_pcc_channel(pcc_data.pcc_subspace_idx);
+	/* Register PCC channel once for all PCC subspace id. */
+	if (pcc_subspace_id >= 0 && !pcc_data[pcc_subspace_id]->pcc_channel_acquired) {
+		ret = register_pcc_channel(pcc_subspace_id);
 		if (ret)
 			goto out_free;
 
-		init_rwsem(&pcc_data.pcc_lock);
-		init_waitqueue_head(&pcc_data.pcc_write_wait_q);
+		init_rwsem(&pcc_data[pcc_subspace_id]->pcc_lock);
+		init_waitqueue_head(&pcc_data[pcc_subspace_id]->pcc_write_wait_q);
 	}
 
 	/* Everything looks okay */
@@ -831,6 +872,18 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
 	struct cpc_desc *cpc_ptr;
 	unsigned int i;
 	void __iomem *addr;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, pr->id);
+
+	if (pcc_ss_id >=0 && pcc_data[pcc_ss_id]) {
+		if (pcc_data[pcc_ss_id]->pcc_channel_acquired) {
+			pcc_data[pcc_ss_id]->refcount--;
+			if (!pcc_data[pcc_ss_id]->refcount) {
+				pcc_mbox_free_channel(pcc_data[pcc_ss_id]->pcc_channel);
+				pcc_data[pcc_ss_id]->pcc_channel_acquired = 0;
+				kfree(pcc_data[pcc_ss_id]);
+			}
+		}
+	}
 
 	cpc_ptr = per_cpu(cpc_desc_ptr, pr->id);
 	if (!cpc_ptr)
@@ -888,6 +941,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 {
 	int ret_val = 0;
 	void __iomem *vaddr = 0;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
 	if (reg_res->type == ACPI_TYPE_INTEGER) {
@@ -897,7 +951,7 @@ static int cpc_read(int cpu, struct cpc_register_resource *reg_res, u64 *val)
 
 	*val = 0;
 	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
-		vaddr = GET_PCC_VADDR(reg->address);
+		vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id);
 	else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
 		vaddr = reg_res->sys_mem_vaddr;
 	else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
@@ -932,10 +986,11 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
 {
 	int ret_val = 0;
 	void __iomem *vaddr = 0;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
 	struct cpc_reg *reg = &reg_res->cpc_entry.reg;
 
 	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM)
-		vaddr = GET_PCC_VADDR(reg->address);
+		vaddr = GET_PCC_VADDR(reg->address, pcc_ss_id);
 	else if (reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
 		vaddr = reg_res->sys_mem_vaddr;
 	else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
@@ -980,6 +1035,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	struct cpc_register_resource *highest_reg, *lowest_reg,
 		*lowest_non_linear_reg, *nominal_reg;
 	u64 high, low, nom, min_nonlinear;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	int ret = 0, regs_in_pcc = 0;
 
 	if (!cpc_desc) {
@@ -996,9 +1053,9 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
 		CPC_IN_PCC(lowest_non_linear_reg) || CPC_IN_PCC(nominal_reg)) {
 		regs_in_pcc = 1;
-		down_write(&pcc_data.pcc_lock);
+		down_write(&pcc_ss_data->pcc_lock);
 		/* Ring doorbell once to update PCC subspace */
-		if (send_pcc_cmd(CMD_READ) < 0) {
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
 			ret = -EIO;
 			goto out_err;
 		}
@@ -1021,7 +1078,7 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 
 out_err:
 	if (regs_in_pcc)
-		up_write(&pcc_data.pcc_lock);
+		up_write(&pcc_ss_data->pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
@@ -1038,6 +1095,8 @@ 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,
 		*ref_perf_reg, *ctr_wrap_reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	u64 delivered, reference, ref_perf, ctr_wrap_time;
 	int ret = 0, regs_in_pcc = 0;
 
@@ -1061,10 +1120,10 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	/* Are any of the regs PCC ?*/
 	if (CPC_IN_PCC(delivered_reg) || CPC_IN_PCC(reference_reg) ||
 		CPC_IN_PCC(ctr_wrap_reg) || CPC_IN_PCC(ref_perf_reg)) {
-		down_write(&pcc_data.pcc_lock);
+		down_write(&pcc_ss_data->pcc_lock);
 		regs_in_pcc = 1;
 		/* Ring doorbell once to update PCC subspace */
-		if (send_pcc_cmd(CMD_READ) < 0) {
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
 			ret = -EIO;
 			goto out_err;
 		}
@@ -1094,7 +1153,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 	perf_fb_ctrs->wraparound_time = ctr_wrap_time;
 out_err:
 	if (regs_in_pcc)
-		up_write(&pcc_data.pcc_lock);
+		up_write(&pcc_ss_data->pcc_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
@@ -1110,6 +1169,8 @@ 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 pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 	int ret = 0;
 
 	if (!cpc_desc) {
@@ -1127,11 +1188,11 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * achieve that goal here
 	 */
 	if (CPC_IN_PCC(desired_reg)) {
-		down_read(&pcc_data.pcc_lock);	/* BEGIN Phase-I */
-		if (pcc_data.platform_owns_pcc) {
-			ret = check_pcc_chan(false);
+		down_read(&pcc_ss_data->pcc_lock); /* BEGIN Phase-I */
+		if (pcc_ss_data->platform_owns_pcc) {
+			ret = check_pcc_chan(pcc_ss_id, false);
 			if (ret) {
-				up_read(&pcc_data.pcc_lock);
+				up_read(&pcc_ss_data->pcc_lock);
 				return ret;
 			}
 		}
@@ -1139,8 +1200,8 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 		 * Update the pending_write to make sure a PCC CMD_READ will not
 		 * arrive and steal the channel during the switch to write lock
 		 */
-		pcc_data.pending_pcc_write_cmd = true;
-		cpc_desc->write_cmd_id = pcc_data.pcc_write_cnt;
+		pcc_ss_data->pending_pcc_write_cmd = true;
+		cpc_desc->write_cmd_id = pcc_ss_data->pcc_write_cnt;
 		cpc_desc->write_cmd_status = 0;
 	}
 
@@ -1151,7 +1212,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	cpc_write(cpu, desired_reg, perf_ctrls->desired_perf);
 
 	if (CPC_IN_PCC(desired_reg))
-		up_read(&pcc_data.pcc_lock);	/* END Phase-I */
+		up_read(&pcc_ss_data->pcc_lock);	/* END Phase-I */
 	/*
 	 * This is Phase-II where we transfer the ownership of PCC to Platform
 	 *
@@ -1199,15 +1260,15 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * the write command before servicing the read command
 	 */
 	if (CPC_IN_PCC(desired_reg)) {
-		if (down_write_trylock(&pcc_data.pcc_lock)) {	/* BEGIN Phase-II */
+		if (down_write_trylock(&pcc_ss_data->pcc_lock)) {/* BEGIN Phase-II */
 			/* Update only if there are pending write commands */
-			if (pcc_data.pending_pcc_write_cmd)
-				send_pcc_cmd(CMD_WRITE);
-			up_write(&pcc_data.pcc_lock);		/* END Phase-II */
+			if (pcc_ss_data->pending_pcc_write_cmd)
+				send_pcc_cmd(pcc_ss_id, CMD_WRITE);
+			up_write(&pcc_ss_data->pcc_lock);	/* END Phase-II */
 		} else
 			/* Wait until pcc_write_cnt is updated by send_pcc_cmd */
-			wait_event(pcc_data.pcc_write_wait_q,
-				cpc_desc->write_cmd_id != pcc_data.pcc_write_cnt);
+			wait_event(pcc_ss_data->pcc_write_wait_q,
+				   cpc_desc->write_cmd_id != pcc_ss_data->pcc_write_cnt);
 
 		/* send_pcc_cmd updates the status in case of failure */
 		ret = cpc_desc->write_cmd_status;
@@ -1240,6 +1301,8 @@ unsigned int cppc_get_transition_latency(int cpu_num)
 	unsigned int latency_ns = 0;
 	struct cpc_desc *cpc_desc;
 	struct cpc_register_resource *desired_reg;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu_num);
+	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
 
 	cpc_desc = per_cpu(cpc_desc_ptr, cpu_num);
 	if (!cpc_desc)
@@ -1249,11 +1312,11 @@ unsigned int cppc_get_transition_latency(int cpu_num)
 	if (!CPC_IN_PCC(desired_reg))
 		return CPUFREQ_ETERNAL;
 
-	if (pcc_data.pcc_mpar)
-		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_data.pcc_mpar);
+	if (pcc_ss_data->pcc_mpar)
+		latency_ns = 60 * (1000 * 1000 * 1000 / pcc_ss_data->pcc_mpar);
 
-	latency_ns = max(latency_ns, pcc_data.pcc_nominal * 1000);
-	latency_ns = max(latency_ns, pcc_data.pcc_mrtt * 1000);
+	latency_ns = max(latency_ns, pcc_ss_data->pcc_nominal * 1000);
+	latency_ns = max(latency_ns, pcc_ss_data->pcc_mrtt * 1000);
 
 	return latency_ns;
 }
-- 
2.1.4

Re: [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[Prakash, Prashanth]

Hi George,

On 9/19/2017 11:24 PM, George Cherian wrote:
> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
> all PCC registers, for all processors in the same performance
> domain (as defined by _PSD), must be defined to be in the same subspace.
> Based on Section 14.1 of ACPI specification, it is possible to have a
> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
> instead of using a single global pcc_data structure.
>
> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
> and mpar_count is initialized properly.
>
> Signed-off-by: George Cherian <george.cherian@cavium.com>
> ---
>  drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>  1 file changed, 153 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index e5b47f0..3ae79ef 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>  
>  	/* Wait queue for CPUs whose requests were batched */
>  	wait_queue_head_t pcc_write_wait_q;
> +	ktime_t last_cmd_cmpl_time;
> +	ktime_t last_mpar_reset;
> +	int mpar_count;
> +	int refcount;
>  };
>  
> -/* Structure to represent the single PCC channel */
> -static struct cppc_pcc_data pcc_data = {
> -	.pcc_subspace_idx = -1,
> -	.platform_owns_pcc = true,
> -};
> +/* Array  to represent the PCC channel per subspace id */
> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>  
>  /*
>   * The cpc_desc structure contains the ACPI register details
> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>  static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>  
>  /* pcc mapped address + header size + offset within PCC subspace */
> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
> +						0x8 + (offs))
>  
>  /* Check if a CPC register is in PCC */
>  #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>  	.default_attrs = cppc_attrs,
>  };
>  
> -static int check_pcc_chan(bool chk_err_bit)
> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>  {
>  	int ret = -EIO, status = 0;
> -	struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
> -	ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
> +	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
> +	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
> +		pcc_ss_data->pcc_comm_addr;
> +	ktime_t next_deadline = ktime_add(ktime_get(),
> +					  pcc_ss_data->deadline);
>  
> -	if (!pcc_data.platform_owns_pcc)
> +	if (!pcc_ss_data->platform_owns_pcc)
>  		return 0;
>  
>  	/* Retry in case the remote processor was too slow to catch up. */
> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>  	}
>  
>  	if (likely(!ret))
> -		pcc_data.platform_owns_pcc = false;
> +		pcc_ss_data->platform_owns_pcc = false;
>  	else
>  		pr_err("PCC check channel failed. Status=%x\n", status);
>  
> @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>   * 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)
> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>  {
>  	int ret = -EIO, i;
> +	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>  	struct acpi_pcct_shared_memory *generic_comm_base =
> -		(struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
> -	static ktime_t last_cmd_cmpl_time, last_mpar_reset;
> -	static int mpar_count;
> +		(struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>  	unsigned int time_delta;
>  
>  	/*
> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>  		 * before write completion, so first send a WRITE command to
>  		 * platform
>  		 */
> -		if (pcc_data.pending_pcc_write_cmd)
> -			send_pcc_cmd(CMD_WRITE);
> +		if (pcc_ss_data->pending_pcc_write_cmd)
> +			send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>  
> -		ret = check_pcc_chan(false);
> +		ret = check_pcc_chan(pcc_ss_id, false);
>  		if (ret)
>  			goto end;
>  	} else /* CMD_WRITE */
> -		pcc_data.pending_pcc_write_cmd = FALSE;
> +		pcc_ss_data->pending_pcc_write_cmd = FALSE;
>  
>  	/*
>  	 * Handle the Minimum Request Turnaround Time(MRTT)
>  	 * "The minimum amount of time that OSPM must wait after the completion
>  	 * of a command before issuing the next command, in microseconds"
>  	 */
> -	if (pcc_data.pcc_mrtt) {
> -		time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
> -		if (pcc_data.pcc_mrtt > time_delta)
> -			udelay(pcc_data.pcc_mrtt - time_delta);
> +	if (pcc_ss_data->pcc_mrtt) {
> +		time_delta = ktime_us_delta(ktime_get(),
> +					    pcc_ss_data->last_cmd_cmpl_time);
> +		if (pcc_ss_data->pcc_mrtt > time_delta)
> +			udelay(pcc_ss_data->pcc_mrtt - time_delta);
>  	}
>  
>  	/*
> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>  	 * not send the request to the platform after hitting the MPAR limit in
>  	 * any 60s window
>  	 */
> -	if (pcc_data.pcc_mpar) {
> -		if (mpar_count == 0) {
> -			time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
> -			if (time_delta < 60 * MSEC_PER_SEC) {
> +	if (pcc_ss_data->pcc_mpar) {
> +		if (pcc_ss_data->mpar_count == 0) {
> +			time_delta = ktime_ms_delta(ktime_get(),
> +						    pcc_ss_data->last_mpar_reset);
> +			if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>  				pr_debug("PCC cmd not sent due to MPAR limit");
>  				ret = -EIO;
>  				goto end;
>  			}
> -			last_mpar_reset = ktime_get();
> -			mpar_count = pcc_data.pcc_mpar;
> +			pcc_ss_data->last_mpar_reset = ktime_get();
> +			pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>  		}
> -		mpar_count--;
> +		pcc_ss_data->mpar_count--;
>  	}
>  
>  	/* Write to the shared comm region. */
> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>  	/* Flip CMD COMPLETE bit */
>  	writew_relaxed(0, &generic_comm_base->status);
>  
> -	pcc_data.platform_owns_pcc = true;
> +	pcc_ss_data->platform_owns_pcc = true;
>  
>  	/* Ring doorbell */
> -	ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
> +	ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>  	if (ret < 0) {
>  		pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>  				cmd, ret);
> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>  	}
>  
>  	/* wait for completion and check for PCC errro bit */
> -	ret = check_pcc_chan(true);
> +	ret = check_pcc_chan(pcc_ss_id, true);
>  
> -	if (pcc_data.pcc_mrtt)
> -		last_cmd_cmpl_time = ktime_get();
> +	if (pcc_ss_data->pcc_mrtt)
> +		pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>  
> -	if (pcc_data.pcc_channel->mbox->txdone_irq)
> -		mbox_chan_txdone(pcc_data.pcc_channel, ret);
> +	if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
> +		mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>  	else
> -		mbox_client_txdone(pcc_data.pcc_channel, ret);
> +		mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>  
>  end:
>  	if (cmd == CMD_WRITE) {
> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>  				if (!desc)
>  					continue;
>  
> -				if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
> +				if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>  					desc->write_cmd_status = ret;
>  			}
>  		}
> -		pcc_data.pcc_write_cnt++;
> -		wake_up_all(&pcc_data.pcc_write_wait_q);
> +		pcc_ss_data->pcc_write_cnt++;
> +		wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>  	}
>  
>  	return ret;
> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>  }
>  EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>  
> -static int register_pcc_channel(int pcc_subspace_idx)
> +static int register_pcc_channel(int pcc_ss_idx)
>  {
>  	struct acpi_pcct_hw_reduced *cppc_ss;
>  	u64 usecs_lat;
>  
> -	if (pcc_subspace_idx >= 0) {
> -		pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
> -				pcc_subspace_idx);
> +	if (pcc_ss_idx >= 0) {
> +		pcc_data[pcc_ss_idx]->pcc_channel =
> +			pcc_mbox_request_channel(&cppc_mbox_cl,	pcc_ss_idx);
>  
> -		if (IS_ERR(pcc_data.pcc_channel)) {
> +		if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>  			pr_err("Failed to find PCC communication channel\n");
>  			return -ENODEV;
>  		}
> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>  		 * PCC channels) and stored pointers to the
>  		 * subspace communication region in con_priv.
>  		 */
> -		cppc_ss = (pcc_data.pcc_channel)->con_priv;
> +		cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>  
>  		if (!cppc_ss) {
>  			pr_err("No PCC subspace found for CPPC\n");
> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>  		 * So add an arbitrary amount of wait on top of Nominal.
>  		 */
>  		usecs_lat = NUM_RETRIES * cppc_ss->latency;
> -		pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
> -		pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
> -		pcc_data.pcc_mpar = cppc_ss->max_access_rate;
> -		pcc_data.pcc_nominal = cppc_ss->latency;
> -
> -		pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
> -		if (!pcc_data.pcc_comm_addr) {
> +		pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
> +		pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
> +		pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
> +		pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
> +
> +		pcc_data[pcc_ss_idx]->pcc_comm_addr =
> +			acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
> +		if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>  			pr_err("Failed to ioremap PCC comm region mem\n");
>  			return -ENOMEM;
>  		}
>  
>  		/* Set flag so that we dont come here for each CPU. */
> -		pcc_data.pcc_channel_acquired = true;
> +		pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>  	}
>  
>  	return 0;
> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>  	return false;
>  }
>  
> +
> +/**
> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
> + *
> + * Check and allocate the cppc_pcc_data memory.
> + * In some processor configurations it is possible that same subspace
> + * is shared between multiple CPU's. This is seen especially in CPU's
> + * with hardware multi-threading support.
> + *
> + * Return: 0 for success, errno for failure
> + */
> +int pcc_data_alloc(int pcc_ss_id)
> +{
> +	int loop;
> +
> +	if (pcc_ss_id < 0)
Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
> +		return -EINVAL;
> +
> +	for (loop = 0; pcc_data[loop] != NULL; loop++) {
> +		if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
> +			pcc_data[loop]->refcount++;
> +			return 0;
> +		}
> +	}
Why do we need the above for loop? can't it be direct array lookup?
if (pcc_data[pcc_ss_id]) {
    //increment ref_count and return
}

Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
it is no longer useful and probably adds to confusion.
> +
> +	pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
> +	if (!pcc_data[pcc_ss_id])
> +		return -ENOMEM;
> +	pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
> +	pcc_data[pcc_ss_id]->refcount++;
> +
> +	return 0;
> +}
>  /*
>   * An example CPC table looks like the following.
>   *
> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>  	struct device *cpu_dev;
>  	acpi_handle handle = pr->handle;
>  	unsigned int num_ent, i, cpc_rev;
> +	int pcc_subspace_id = -1;
>  	acpi_status status;
>  	int ret = -EFAULT;
>  
> @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>  			 * so extract it only once.
>  			 */
>  			if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
> -				if (pcc_data.pcc_subspace_idx < 0)
> -					pcc_data.pcc_subspace_idx = gas_t->access_width;
> -				else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
> -					pr_debug("Mismatched PCC ids.\n");
We need to retain the above checks to make sure all PCC registers
within a _CPC package is under same subspace. The Spec still requires:
"If the PCC register space is used, all PCC registers, for all processors in
the same performance domain (as defined by _PSD), must be defined
to be in the same subspace."

> +				pcc_subspace_id = gas_t->access_width;
> +				if (pcc_data_alloc(pcc_subspace_id))
>  					goto out_free;
We need to call pcc_data_alloc(to increment the reference) only once per CPU,
otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
pcc_data.


--
Thanks,
Prashanth

Re: [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

Hi Prakash,

On 09/29/2017 04:49 AM, Prakash, Prashanth wrote:
> Hi George,
> 
> On 9/19/2017 11:24 PM, George Cherian wrote:
>> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
>> all PCC registers, for all processors in the same performance
>> domain (as defined by _PSD), must be defined to be in the same subspace.
>> Based on Section 14.1 of ACPI specification, it is possible to have a
>> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
>> instead of using a single global pcc_data structure.
>>
>> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
>> and mpar_count is initialized properly.
>>
>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>>   1 file changed, 153 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index e5b47f0..3ae79ef 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>>   
>>   	/* Wait queue for CPUs whose requests were batched */
>>   	wait_queue_head_t pcc_write_wait_q;
>> +	ktime_t last_cmd_cmpl_time;
>> +	ktime_t last_mpar_reset;
>> +	int mpar_count;
>> +	int refcount;
>>   };
>>   
>> -/* Structure to represent the single PCC channel */
>> -static struct cppc_pcc_data pcc_data = {
>> -	.pcc_subspace_idx = -1,
>> -	.platform_owns_pcc = true,
>> -};
>> +/* Array  to represent the PCC channel per subspace id */
>> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
>> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>   
>>   /*
>>    * The cpc_desc structure contains the ACPI register details
>> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>>   static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>   
>>   /* pcc mapped address + header size + offset within PCC subspace */
>> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
>> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
>> +						0x8 + (offs))
>>   
>>   /* Check if a CPC register is in PCC */
>>   #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
>> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>>   	.default_attrs = cppc_attrs,
>>   };
>>   
>> -static int check_pcc_chan(bool chk_err_bit)
>> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>>   {
>>   	int ret = -EIO, status = 0;
>> -	struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
>> -	ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
>> +	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>> +	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
>> +		pcc_ss_data->pcc_comm_addr;
>> +	ktime_t next_deadline = ktime_add(ktime_get(),
>> +					  pcc_ss_data->deadline);
>>   
>> -	if (!pcc_data.platform_owns_pcc)
>> +	if (!pcc_ss_data->platform_owns_pcc)
>>   		return 0;
>>   
>>   	/* Retry in case the remote processor was too slow to catch up. */
>> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>>   	}
>>   
>>   	if (likely(!ret))
>> -		pcc_data.platform_owns_pcc = false;
>> +		pcc_ss_data->platform_owns_pcc = false;
>>   	else
>>   		pr_err("PCC check channel failed. Status=%x\n", status);
>>   
>> @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>>    * 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)
>> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>>   {
>>   	int ret = -EIO, i;
>> +	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>   	struct acpi_pcct_shared_memory *generic_comm_base =
>> -		(struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
>> -	static ktime_t last_cmd_cmpl_time, last_mpar_reset;
>> -	static int mpar_count;
>> +		(struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>>   	unsigned int time_delta;
>>   
>>   	/*
>> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>>   		 * before write completion, so first send a WRITE command to
>>   		 * platform
>>   		 */
>> -		if (pcc_data.pending_pcc_write_cmd)
>> -			send_pcc_cmd(CMD_WRITE);
>> +		if (pcc_ss_data->pending_pcc_write_cmd)
>> +			send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>>   
>> -		ret = check_pcc_chan(false);
>> +		ret = check_pcc_chan(pcc_ss_id, false);
>>   		if (ret)
>>   			goto end;
>>   	} else /* CMD_WRITE */
>> -		pcc_data.pending_pcc_write_cmd = FALSE;
>> +		pcc_ss_data->pending_pcc_write_cmd = FALSE;
>>   
>>   	/*
>>   	 * Handle the Minimum Request Turnaround Time(MRTT)
>>   	 * "The minimum amount of time that OSPM must wait after the completion
>>   	 * of a command before issuing the next command, in microseconds"
>>   	 */
>> -	if (pcc_data.pcc_mrtt) {
>> -		time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
>> -		if (pcc_data.pcc_mrtt > time_delta)
>> -			udelay(pcc_data.pcc_mrtt - time_delta);
>> +	if (pcc_ss_data->pcc_mrtt) {
>> +		time_delta = ktime_us_delta(ktime_get(),
>> +					    pcc_ss_data->last_cmd_cmpl_time);
>> +		if (pcc_ss_data->pcc_mrtt > time_delta)
>> +			udelay(pcc_ss_data->pcc_mrtt - time_delta);
>>   	}
>>   
>>   	/*
>> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>>   	 * not send the request to the platform after hitting the MPAR limit in
>>   	 * any 60s window
>>   	 */
>> -	if (pcc_data.pcc_mpar) {
>> -		if (mpar_count == 0) {
>> -			time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
>> -			if (time_delta < 60 * MSEC_PER_SEC) {
>> +	if (pcc_ss_data->pcc_mpar) {
>> +		if (pcc_ss_data->mpar_count == 0) {
>> +			time_delta = ktime_ms_delta(ktime_get(),
>> +						    pcc_ss_data->last_mpar_reset);
>> +			if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>>   				pr_debug("PCC cmd not sent due to MPAR limit");
>>   				ret = -EIO;
>>   				goto end;
>>   			}
>> -			last_mpar_reset = ktime_get();
>> -			mpar_count = pcc_data.pcc_mpar;
>> +			pcc_ss_data->last_mpar_reset = ktime_get();
>> +			pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>>   		}
>> -		mpar_count--;
>> +		pcc_ss_data->mpar_count--;
>>   	}
>>   
>>   	/* Write to the shared comm region. */
>> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>>   	/* Flip CMD COMPLETE bit */
>>   	writew_relaxed(0, &generic_comm_base->status);
>>   
>> -	pcc_data.platform_owns_pcc = true;
>> +	pcc_ss_data->platform_owns_pcc = true;
>>   
>>   	/* Ring doorbell */
>> -	ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
>> +	ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>>   	if (ret < 0) {
>>   		pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>>   				cmd, ret);
>> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>>   	}
>>   
>>   	/* wait for completion and check for PCC errro bit */
>> -	ret = check_pcc_chan(true);
>> +	ret = check_pcc_chan(pcc_ss_id, true);
>>   
>> -	if (pcc_data.pcc_mrtt)
>> -		last_cmd_cmpl_time = ktime_get();
>> +	if (pcc_ss_data->pcc_mrtt)
>> +		pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>>   
>> -	if (pcc_data.pcc_channel->mbox->txdone_irq)
>> -		mbox_chan_txdone(pcc_data.pcc_channel, ret);
>> +	if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
>> +		mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>>   	else
>> -		mbox_client_txdone(pcc_data.pcc_channel, ret);
>> +		mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>>   
>>   end:
>>   	if (cmd == CMD_WRITE) {
>> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>>   				if (!desc)
>>   					continue;
>>   
>> -				if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
>> +				if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>>   					desc->write_cmd_status = ret;
>>   			}
>>   		}
>> -		pcc_data.pcc_write_cnt++;
>> -		wake_up_all(&pcc_data.pcc_write_wait_q);
>> +		pcc_ss_data->pcc_write_cnt++;
>> +		wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>>   	}
>>   
>>   	return ret;
>> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>>   }
>>   EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>>   
>> -static int register_pcc_channel(int pcc_subspace_idx)
>> +static int register_pcc_channel(int pcc_ss_idx)
>>   {
>>   	struct acpi_pcct_hw_reduced *cppc_ss;
>>   	u64 usecs_lat;
>>   
>> -	if (pcc_subspace_idx >= 0) {
>> -		pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
>> -				pcc_subspace_idx);
>> +	if (pcc_ss_idx >= 0) {
>> +		pcc_data[pcc_ss_idx]->pcc_channel =
>> +			pcc_mbox_request_channel(&cppc_mbox_cl,	pcc_ss_idx);
>>   
>> -		if (IS_ERR(pcc_data.pcc_channel)) {
>> +		if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>>   			pr_err("Failed to find PCC communication channel\n");
>>   			return -ENODEV;
>>   		}
>> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>   		 * PCC channels) and stored pointers to the
>>   		 * subspace communication region in con_priv.
>>   		 */
>> -		cppc_ss = (pcc_data.pcc_channel)->con_priv;
>> +		cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>>   
>>   		if (!cppc_ss) {
>>   			pr_err("No PCC subspace found for CPPC\n");
>> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>   		 * So add an arbitrary amount of wait on top of Nominal.
>>   		 */
>>   		usecs_lat = NUM_RETRIES * cppc_ss->latency;
>> -		pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>> -		pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
>> -		pcc_data.pcc_mpar = cppc_ss->max_access_rate;
>> -		pcc_data.pcc_nominal = cppc_ss->latency;
>> -
>> -		pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>> -		if (!pcc_data.pcc_comm_addr) {
>> +		pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>> +		pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
>> +		pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
>> +		pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
>> +
>> +		pcc_data[pcc_ss_idx]->pcc_comm_addr =
>> +			acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>> +		if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>>   			pr_err("Failed to ioremap PCC comm region mem\n");
>>   			return -ENOMEM;
>>   		}
>>   
>>   		/* Set flag so that we dont come here for each CPU. */
>> -		pcc_data.pcc_channel_acquired = true;
>> +		pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>>   	}
>>   
>>   	return 0;
>> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>>   	return false;
>>   }
>>   
>> +
>> +/**
>> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>> + *
>> + * Check and allocate the cppc_pcc_data memory.
>> + * In some processor configurations it is possible that same subspace
>> + * is shared between multiple CPU's. This is seen especially in CPU's
>> + * with hardware multi-threading support.
>> + *
>> + * Return: 0 for success, errno for failure
>> + */
>> +int pcc_data_alloc(int pcc_ss_id)
>> +{
>> +	int loop;
>> +
>> +	if (pcc_ss_id < 0)
> Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
Yes we can have this additional check.
>> +		return -EINVAL;
>> +
>> +	for (loop = 0; pcc_data[loop] != NULL; loop++) {
>> +		if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
>> +			pcc_data[loop]->refcount++;
>> +			return 0;
>> +		}
>> +	}
> Why do we need the above for loop? can't it be direct array lookup?
> if (pcc_data[pcc_ss_id]) {
>      //increment ref_count and return
> }
Yes, we could get rid of the loop and increment the reference directly 
if already allocated.

> 
> Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
> it is no longer useful and probably adds to confusion.
Please see below
>> +
>> +	pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
>> +	if (!pcc_data[pcc_ss_id])
>> +		return -ENOMEM;
>> +	pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
>> +	pcc_data[pcc_ss_id]->refcount++;
>> +
>> +	return 0;
>> +}
>>   /*
>>    * An example CPC table looks like the following.
>>    *
>> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>   	struct device *cpu_dev;
>>   	acpi_handle handle = pr->handle;
>>   	unsigned int num_ent, i, cpc_rev;
>> +	int pcc_subspace_id = -1;
>>   	acpi_status status;
>>   	int ret = -EFAULT;
>>   
>> @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>   			 * so extract it only once.
>>   			 */
>>   			if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>> -				if (pcc_data.pcc_subspace_idx < 0)
>> -					pcc_data.pcc_subspace_idx = gas_t->access_width;
>> -				else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
>> -					pr_debug("Mismatched PCC ids.\n");
> We need to retain the above checks to make sure all PCC registers
> within a _CPC package is under same subspace. The Spec still requires:
> "If the PCC register space is used, all PCC registers, for all processors in
> the same performance domain (as defined by _PSD), must be defined
> to be in the same subspace."

So that means we need to maintain the pcc_subspace_idx in cppc_pcc_data.
If so, then I can move the check inside pcc_data_alloc().
> 
>> +				pcc_subspace_id = gas_t->access_width;
>> +				if (pcc_data_alloc(pcc_subspace_id))
>>   					goto out_free;
> We need to call pcc_data_alloc(to increment the reference) only once per CPU,
> otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
> pcc_data.

It is infact called only once per CPU, The refcounting was added in case 
if multiple CPU's share the same domain for eg:
In instances, where CPU have hardware multi-threading support.
or in cases where all CPU's are controlled using single subspace (as 
earlier).

Do you see any issue with acpi_cppc_processor_exit() in the earlier 
cases, where refcount remains > 0 and the memory not getting freed-up?
> 
> 
> --
> Thanks,
> Prashanth
> 

Re: [Devel] [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

[-- Attachment #1: Type: text/plain, Size: 14440 bytes --]

Hi Prakash,

On 09/29/2017 04:49 AM, Prakash, Prashanth wrote:
> Hi George,
> 
> On 9/19/2017 11:24 PM, George Cherian wrote:
>> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
>> all PCC registers, for all processors in the same performance
>> domain (as defined by _PSD), must be defined to be in the same subspace.
>> Based on Section 14.1 of ACPI specification, it is possible to have a
>> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
>> instead of using a single global pcc_data structure.
>>
>> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
>> and mpar_count is initialized properly.
>>
>> Signed-off-by: George Cherian <george.cherian(a)cavium.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>>   1 file changed, 153 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index e5b47f0..3ae79ef 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>>   
>>   	/* Wait queue for CPUs whose requests were batched */
>>   	wait_queue_head_t pcc_write_wait_q;
>> +	ktime_t last_cmd_cmpl_time;
>> +	ktime_t last_mpar_reset;
>> +	int mpar_count;
>> +	int refcount;
>>   };
>>   
>> -/* Structure to represent the single PCC channel */
>> -static struct cppc_pcc_data pcc_data = {
>> -	.pcc_subspace_idx = -1,
>> -	.platform_owns_pcc = true,
>> -};
>> +/* Array  to represent the PCC channel per subspace id */
>> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
>> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>   
>>   /*
>>    * The cpc_desc structure contains the ACPI register details
>> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>>   static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>   
>>   /* pcc mapped address + header size + offset within PCC subspace */
>> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
>> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
>> +						0x8 + (offs))
>>   
>>   /* Check if a CPC register is in PCC */
>>   #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&		\
>> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>>   	.default_attrs = cppc_attrs,
>>   };
>>   
>> -static int check_pcc_chan(bool chk_err_bit)
>> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>>   {
>>   	int ret = -EIO, status = 0;
>> -	struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
>> -	ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
>> +	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>> +	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
>> +		pcc_ss_data->pcc_comm_addr;
>> +	ktime_t next_deadline = ktime_add(ktime_get(),
>> +					  pcc_ss_data->deadline);
>>   
>> -	if (!pcc_data.platform_owns_pcc)
>> +	if (!pcc_ss_data->platform_owns_pcc)
>>   		return 0;
>>   
>>   	/* Retry in case the remote processor was too slow to catch up. */
>> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>>   	}
>>   
>>   	if (likely(!ret))
>> -		pcc_data.platform_owns_pcc = false;
>> +		pcc_ss_data->platform_owns_pcc = false;
>>   	else
>>   		pr_err("PCC check channel failed. Status=%x\n", status);
>>   
>> @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>>    * 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)
>> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>>   {
>>   	int ret = -EIO, i;
>> +	struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>   	struct acpi_pcct_shared_memory *generic_comm_base =
>> -		(struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
>> -	static ktime_t last_cmd_cmpl_time, last_mpar_reset;
>> -	static int mpar_count;
>> +		(struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>>   	unsigned int time_delta;
>>   
>>   	/*
>> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>>   		 * before write completion, so first send a WRITE command to
>>   		 * platform
>>   		 */
>> -		if (pcc_data.pending_pcc_write_cmd)
>> -			send_pcc_cmd(CMD_WRITE);
>> +		if (pcc_ss_data->pending_pcc_write_cmd)
>> +			send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>>   
>> -		ret = check_pcc_chan(false);
>> +		ret = check_pcc_chan(pcc_ss_id, false);
>>   		if (ret)
>>   			goto end;
>>   	} else /* CMD_WRITE */
>> -		pcc_data.pending_pcc_write_cmd = FALSE;
>> +		pcc_ss_data->pending_pcc_write_cmd = FALSE;
>>   
>>   	/*
>>   	 * Handle the Minimum Request Turnaround Time(MRTT)
>>   	 * "The minimum amount of time that OSPM must wait after the completion
>>   	 * of a command before issuing the next command, in microseconds"
>>   	 */
>> -	if (pcc_data.pcc_mrtt) {
>> -		time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
>> -		if (pcc_data.pcc_mrtt > time_delta)
>> -			udelay(pcc_data.pcc_mrtt - time_delta);
>> +	if (pcc_ss_data->pcc_mrtt) {
>> +		time_delta = ktime_us_delta(ktime_get(),
>> +					    pcc_ss_data->last_cmd_cmpl_time);
>> +		if (pcc_ss_data->pcc_mrtt > time_delta)
>> +			udelay(pcc_ss_data->pcc_mrtt - time_delta);
>>   	}
>>   
>>   	/*
>> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>>   	 * not send the request to the platform after hitting the MPAR limit in
>>   	 * any 60s window
>>   	 */
>> -	if (pcc_data.pcc_mpar) {
>> -		if (mpar_count == 0) {
>> -			time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
>> -			if (time_delta < 60 * MSEC_PER_SEC) {
>> +	if (pcc_ss_data->pcc_mpar) {
>> +		if (pcc_ss_data->mpar_count == 0) {
>> +			time_delta = ktime_ms_delta(ktime_get(),
>> +						    pcc_ss_data->last_mpar_reset);
>> +			if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>>   				pr_debug("PCC cmd not sent due to MPAR limit");
>>   				ret = -EIO;
>>   				goto end;
>>   			}
>> -			last_mpar_reset = ktime_get();
>> -			mpar_count = pcc_data.pcc_mpar;
>> +			pcc_ss_data->last_mpar_reset = ktime_get();
>> +			pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>>   		}
>> -		mpar_count--;
>> +		pcc_ss_data->mpar_count--;
>>   	}
>>   
>>   	/* Write to the shared comm region. */
>> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>>   	/* Flip CMD COMPLETE bit */
>>   	writew_relaxed(0, &generic_comm_base->status);
>>   
>> -	pcc_data.platform_owns_pcc = true;
>> +	pcc_ss_data->platform_owns_pcc = true;
>>   
>>   	/* Ring doorbell */
>> -	ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
>> +	ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>>   	if (ret < 0) {
>>   		pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>>   				cmd, ret);
>> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>>   	}
>>   
>>   	/* wait for completion and check for PCC errro bit */
>> -	ret = check_pcc_chan(true);
>> +	ret = check_pcc_chan(pcc_ss_id, true);
>>   
>> -	if (pcc_data.pcc_mrtt)
>> -		last_cmd_cmpl_time = ktime_get();
>> +	if (pcc_ss_data->pcc_mrtt)
>> +		pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>>   
>> -	if (pcc_data.pcc_channel->mbox->txdone_irq)
>> -		mbox_chan_txdone(pcc_data.pcc_channel, ret);
>> +	if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
>> +		mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>>   	else
>> -		mbox_client_txdone(pcc_data.pcc_channel, ret);
>> +		mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>>   
>>   end:
>>   	if (cmd == CMD_WRITE) {
>> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>>   				if (!desc)
>>   					continue;
>>   
>> -				if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
>> +				if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>>   					desc->write_cmd_status = ret;
>>   			}
>>   		}
>> -		pcc_data.pcc_write_cnt++;
>> -		wake_up_all(&pcc_data.pcc_write_wait_q);
>> +		pcc_ss_data->pcc_write_cnt++;
>> +		wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>>   	}
>>   
>>   	return ret;
>> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>>   }
>>   EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>>   
>> -static int register_pcc_channel(int pcc_subspace_idx)
>> +static int register_pcc_channel(int pcc_ss_idx)
>>   {
>>   	struct acpi_pcct_hw_reduced *cppc_ss;
>>   	u64 usecs_lat;
>>   
>> -	if (pcc_subspace_idx >= 0) {
>> -		pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
>> -				pcc_subspace_idx);
>> +	if (pcc_ss_idx >= 0) {
>> +		pcc_data[pcc_ss_idx]->pcc_channel =
>> +			pcc_mbox_request_channel(&cppc_mbox_cl,	pcc_ss_idx);
>>   
>> -		if (IS_ERR(pcc_data.pcc_channel)) {
>> +		if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>>   			pr_err("Failed to find PCC communication channel\n");
>>   			return -ENODEV;
>>   		}
>> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>   		 * PCC channels) and stored pointers to the
>>   		 * subspace communication region in con_priv.
>>   		 */
>> -		cppc_ss = (pcc_data.pcc_channel)->con_priv;
>> +		cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>>   
>>   		if (!cppc_ss) {
>>   			pr_err("No PCC subspace found for CPPC\n");
>> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>   		 * So add an arbitrary amount of wait on top of Nominal.
>>   		 */
>>   		usecs_lat = NUM_RETRIES * cppc_ss->latency;
>> -		pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>> -		pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
>> -		pcc_data.pcc_mpar = cppc_ss->max_access_rate;
>> -		pcc_data.pcc_nominal = cppc_ss->latency;
>> -
>> -		pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>> -		if (!pcc_data.pcc_comm_addr) {
>> +		pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>> +		pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
>> +		pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
>> +		pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
>> +
>> +		pcc_data[pcc_ss_idx]->pcc_comm_addr =
>> +			acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>> +		if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>>   			pr_err("Failed to ioremap PCC comm region mem\n");
>>   			return -ENOMEM;
>>   		}
>>   
>>   		/* Set flag so that we dont come here for each CPU. */
>> -		pcc_data.pcc_channel_acquired = true;
>> +		pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>>   	}
>>   
>>   	return 0;
>> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>>   	return false;
>>   }
>>   
>> +
>> +/**
>> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>> + *
>> + * Check and allocate the cppc_pcc_data memory.
>> + * In some processor configurations it is possible that same subspace
>> + * is shared between multiple CPU's. This is seen especially in CPU's
>> + * with hardware multi-threading support.
>> + *
>> + * Return: 0 for success, errno for failure
>> + */
>> +int pcc_data_alloc(int pcc_ss_id)
>> +{
>> +	int loop;
>> +
>> +	if (pcc_ss_id < 0)
> Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
Yes we can have this additional check.
>> +		return -EINVAL;
>> +
>> +	for (loop = 0; pcc_data[loop] != NULL; loop++) {
>> +		if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
>> +			pcc_data[loop]->refcount++;
>> +			return 0;
>> +		}
>> +	}
> Why do we need the above for loop? can't it be direct array lookup?
> if (pcc_data[pcc_ss_id]) {
>      //increment ref_count and return
> }
Yes, we could get rid of the loop and increment the reference directly 
if already allocated.

> 
> Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
> it is no longer useful and probably adds to confusion.
Please see below
>> +
>> +	pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
>> +	if (!pcc_data[pcc_ss_id])
>> +		return -ENOMEM;
>> +	pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
>> +	pcc_data[pcc_ss_id]->refcount++;
>> +
>> +	return 0;
>> +}
>>   /*
>>    * An example CPC table looks like the following.
>>    *
>> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>   	struct device *cpu_dev;
>>   	acpi_handle handle = pr->handle;
>>   	unsigned int num_ent, i, cpc_rev;
>> +	int pcc_subspace_id = -1;
>>   	acpi_status status;
>>   	int ret = -EFAULT;
>>   
>> @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>   			 * so extract it only once.
>>   			 */
>>   			if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>> -				if (pcc_data.pcc_subspace_idx < 0)
>> -					pcc_data.pcc_subspace_idx = gas_t->access_width;
>> -				else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
>> -					pr_debug("Mismatched PCC ids.\n");
> We need to retain the above checks to make sure all PCC registers
> within a _CPC package is under same subspace. The Spec still requires:
> "If the PCC register space is used, all PCC registers, for all processors in
> the same performance domain (as defined by _PSD), must be defined
> to be in the same subspace."

So that means we need to maintain the pcc_subspace_idx in cppc_pcc_data.
If so, then I can move the check inside pcc_data_alloc().
> 
>> +				pcc_subspace_id = gas_t->access_width;
>> +				if (pcc_data_alloc(pcc_subspace_id))
>>   					goto out_free;
> We need to call pcc_data_alloc(to increment the reference) only once per CPU,
> otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
> pcc_data.

It is infact called only once per CPU, The refcounting was added in case 
if multiple CPU's share the same domain for eg:
In instances, where CPU have hardware multi-threading support.
or in cases where all CPU's are controlled using single subspace (as 
earlier).

Do you see any issue with acpi_cppc_processor_exit() in the earlier 
cases, where refcount remains > 0 and the memory not getting freed-up?
> 
> 
> --
> Thanks,
> Prashanth
> 

Re: [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[Prakash, Prashanth]

On 10/3/2017 5:01 AM, George Cherian wrote:
> Hi Prakash,
>
> On 09/29/2017 04:49 AM, Prakash, Prashanth wrote:
>> Hi George,
>>
>> On 9/19/2017 11:24 PM, George Cherian wrote:
>>> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
>>> all PCC registers, for all processors in the same performance
>>> domain (as defined by _PSD), must be defined to be in the same subspace.
>>> Based on Section 14.1 of ACPI specification, it is possible to have a
>>> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
>>> instead of using a single global pcc_data structure.
>>>
>>> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
>>> and mpar_count is initialized properly.
>>>
>>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>>> ---
>>>   drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>>>   1 file changed, 153 insertions(+), 90 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index e5b47f0..3ae79ef 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>>>         /* Wait queue for CPUs whose requests were batched */
>>>       wait_queue_head_t pcc_write_wait_q;
>>> +    ktime_t last_cmd_cmpl_time;
>>> +    ktime_t last_mpar_reset;
>>> +    int mpar_count;
>>> +    int refcount;
>>>   };
>>>   -/* Structure to represent the single PCC channel */
>>> -static struct cppc_pcc_data pcc_data = {
>>> -    .pcc_subspace_idx = -1,
>>> -    .platform_owns_pcc = true,
>>> -};
>>> +/* Array  to represent the PCC channel per subspace id */
>>> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
>>> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
>>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>>     /*
>>>    * The cpc_desc structure contains the ACPI register details
>>> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>>>   static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>>     /* pcc mapped address + header size + offset within PCC subspace */
>>> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
>>> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
>>> +                        0x8 + (offs))
>>>     /* Check if a CPC register is in PCC */
>>>   #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&        \
>>> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>>>       .default_attrs = cppc_attrs,
>>>   };
>>>   -static int check_pcc_chan(bool chk_err_bit)
>>> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>>>   {
>>>       int ret = -EIO, status = 0;
>>> -    struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
>>> -    ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
>>> +    struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>> +    struct acpi_pcct_shared_memory __iomem *generic_comm_base =
>>> +        pcc_ss_data->pcc_comm_addr;
>>> +    ktime_t next_deadline = ktime_add(ktime_get(),
>>> +                      pcc_ss_data->deadline);
>>>   -    if (!pcc_data.platform_owns_pcc)
>>> +    if (!pcc_ss_data->platform_owns_pcc)
>>>           return 0;
>>>         /* Retry in case the remote processor was too slow to catch up. */
>>> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>>>       }
>>>         if (likely(!ret))
>>> -        pcc_data.platform_owns_pcc = false;
>>> +        pcc_ss_data->platform_owns_pcc = false;
>>>       else
>>>           pr_err("PCC check channel failed. Status=%x\n", status);
>>>   @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>>>    * 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)
>>> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>>>   {
>>>       int ret = -EIO, i;
>>> +    struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>>       struct acpi_pcct_shared_memory *generic_comm_base =
>>> -        (struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
>>> -    static ktime_t last_cmd_cmpl_time, last_mpar_reset;
>>> -    static int mpar_count;
>>> +        (struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>>>       unsigned int time_delta;
>>>         /*
>>> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>>>            * before write completion, so first send a WRITE command to
>>>            * platform
>>>            */
>>> -        if (pcc_data.pending_pcc_write_cmd)
>>> -            send_pcc_cmd(CMD_WRITE);
>>> +        if (pcc_ss_data->pending_pcc_write_cmd)
>>> +            send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>>>   -        ret = check_pcc_chan(false);
>>> +        ret = check_pcc_chan(pcc_ss_id, false);
>>>           if (ret)
>>>               goto end;
>>>       } else /* CMD_WRITE */
>>> -        pcc_data.pending_pcc_write_cmd = FALSE;
>>> +        pcc_ss_data->pending_pcc_write_cmd = FALSE;
>>>         /*
>>>        * Handle the Minimum Request Turnaround Time(MRTT)
>>>        * "The minimum amount of time that OSPM must wait after the completion
>>>        * of a command before issuing the next command, in microseconds"
>>>        */
>>> -    if (pcc_data.pcc_mrtt) {
>>> -        time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
>>> -        if (pcc_data.pcc_mrtt > time_delta)
>>> -            udelay(pcc_data.pcc_mrtt - time_delta);
>>> +    if (pcc_ss_data->pcc_mrtt) {
>>> +        time_delta = ktime_us_delta(ktime_get(),
>>> +                        pcc_ss_data->last_cmd_cmpl_time);
>>> +        if (pcc_ss_data->pcc_mrtt > time_delta)
>>> +            udelay(pcc_ss_data->pcc_mrtt - time_delta);
>>>       }
>>>         /*
>>> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>>>        * not send the request to the platform after hitting the MPAR limit in
>>>        * any 60s window
>>>        */
>>> -    if (pcc_data.pcc_mpar) {
>>> -        if (mpar_count == 0) {
>>> -            time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
>>> -            if (time_delta < 60 * MSEC_PER_SEC) {
>>> +    if (pcc_ss_data->pcc_mpar) {
>>> +        if (pcc_ss_data->mpar_count == 0) {
>>> +            time_delta = ktime_ms_delta(ktime_get(),
>>> +                            pcc_ss_data->last_mpar_reset);
>>> +            if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>>>                   pr_debug("PCC cmd not sent due to MPAR limit");
>>>                   ret = -EIO;
>>>                   goto end;
>>>               }
>>> -            last_mpar_reset = ktime_get();
>>> -            mpar_count = pcc_data.pcc_mpar;
>>> +            pcc_ss_data->last_mpar_reset = ktime_get();
>>> +            pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>>>           }
>>> -        mpar_count--;
>>> +        pcc_ss_data->mpar_count--;
>>>       }
>>>         /* Write to the shared comm region. */
>>> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>>>       /* Flip CMD COMPLETE bit */
>>>       writew_relaxed(0, &generic_comm_base->status);
>>>   -    pcc_data.platform_owns_pcc = true;
>>> +    pcc_ss_data->platform_owns_pcc = true;
>>>         /* Ring doorbell */
>>> -    ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
>>> +    ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>>>       if (ret < 0) {
>>>           pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>>>                   cmd, ret);
>>> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>>>       }
>>>         /* wait for completion and check for PCC errro bit */
>>> -    ret = check_pcc_chan(true);
>>> +    ret = check_pcc_chan(pcc_ss_id, true);
>>>   -    if (pcc_data.pcc_mrtt)
>>> -        last_cmd_cmpl_time = ktime_get();
>>> +    if (pcc_ss_data->pcc_mrtt)
>>> +        pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>>>   -    if (pcc_data.pcc_channel->mbox->txdone_irq)
>>> -        mbox_chan_txdone(pcc_data.pcc_channel, ret);
>>> +    if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
>>> +        mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>>>       else
>>> -        mbox_client_txdone(pcc_data.pcc_channel, ret);
>>> +        mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>>>     end:
>>>       if (cmd == CMD_WRITE) {
>>> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>>>                   if (!desc)
>>>                       continue;
>>>   -                if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
>>> +                if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>>>                       desc->write_cmd_status = ret;
>>>               }
>>>           }
>>> -        pcc_data.pcc_write_cnt++;
>>> -        wake_up_all(&pcc_data.pcc_write_wait_q);
>>> +        pcc_ss_data->pcc_write_cnt++;
>>> +        wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>>>       }
>>>         return ret;
>>> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>>>   }
>>>   EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>>>   -static int register_pcc_channel(int pcc_subspace_idx)
>>> +static int register_pcc_channel(int pcc_ss_idx)
>>>   {
>>>       struct acpi_pcct_hw_reduced *cppc_ss;
>>>       u64 usecs_lat;
>>>   -    if (pcc_subspace_idx >= 0) {
>>> -        pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
>>> -                pcc_subspace_idx);
>>> +    if (pcc_ss_idx >= 0) {
>>> +        pcc_data[pcc_ss_idx]->pcc_channel =
>>> +            pcc_mbox_request_channel(&cppc_mbox_cl,    pcc_ss_idx);
>>>   -        if (IS_ERR(pcc_data.pcc_channel)) {
>>> +        if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>>>               pr_err("Failed to find PCC communication channel\n");
>>>               return -ENODEV;
>>>           }
>>> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>>            * PCC channels) and stored pointers to the
>>>            * subspace communication region in con_priv.
>>>            */
>>> -        cppc_ss = (pcc_data.pcc_channel)->con_priv;
>>> +        cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>>>             if (!cppc_ss) {
>>>               pr_err("No PCC subspace found for CPPC\n");
>>> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>>            * So add an arbitrary amount of wait on top of Nominal.
>>>            */
>>>           usecs_lat = NUM_RETRIES * cppc_ss->latency;
>>> -        pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>> -        pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
>>> -        pcc_data.pcc_mpar = cppc_ss->max_access_rate;
>>> -        pcc_data.pcc_nominal = cppc_ss->latency;
>>> -
>>> -        pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>> -        if (!pcc_data.pcc_comm_addr) {
>>> +        pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>> +        pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
>>> +        pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
>>> +        pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
>>> +
>>> +        pcc_data[pcc_ss_idx]->pcc_comm_addr =
>>> +            acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>> +        if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>>>               pr_err("Failed to ioremap PCC comm region mem\n");
>>>               return -ENOMEM;
>>>           }
>>>             /* Set flag so that we dont come here for each CPU. */
>>> -        pcc_data.pcc_channel_acquired = true;
>>> +        pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>>>       }
>>>         return 0;
>>> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>>>       return false;
>>>   }
>>>   +
>>> +/**
>>> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>>> + *
>>> + * Check and allocate the cppc_pcc_data memory.
>>> + * In some processor configurations it is possible that same subspace
>>> + * is shared between multiple CPU's. This is seen especially in CPU's
>>> + * with hardware multi-threading support.
>>> + *
>>> + * Return: 0 for success, errno for failure
>>> + */
>>> +int pcc_data_alloc(int pcc_ss_id)
>>> +{
>>> +    int loop;
>>> +
>>> +    if (pcc_ss_id < 0)
>> Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
> Yes we can have this additional check.
>>> +        return -EINVAL;
>>> +
>>> +    for (loop = 0; pcc_data[loop] != NULL; loop++) {
>>> +        if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
>>> +            pcc_data[loop]->refcount++;
>>> +            return 0;
>>> +        }
>>> +    }
>> Why do we need the above for loop? can't it be direct array lookup?
>> if (pcc_data[pcc_ss_id]) {
>>      //increment ref_count and return
>> }
> Yes, we could get rid of the loop and increment the reference directly if already allocated.
>
>>
>> Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
>> it is no longer useful and probably adds to confusion.
> Please see below
>>> +
>>> +    pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
>>> +    if (!pcc_data[pcc_ss_id])
>>> +        return -ENOMEM;
>>> +    pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
>>> +    pcc_data[pcc_ss_id]->refcount++;
>>> +
>>> +    return 0;
>>> +}
>>>   /*
>>>    * An example CPC table looks like the following.
>>>    *
>>> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>>       struct device *cpu_dev;
>>>       acpi_handle handle = pr->handle;
>>>       unsigned int num_ent, i, cpc_rev;
>>> +    int pcc_subspace_id = -1;
>>>       acpi_status status;
>>>       int ret = -EFAULT;
>>>   @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>>                * so extract it only once.
>>>                */
>>>               if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>>> -                if (pcc_data.pcc_subspace_idx < 0)
>>> -                    pcc_data.pcc_subspace_idx = gas_t->access_width;
>>> -                else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
>>> -                    pr_debug("Mismatched PCC ids.\n");
>> We need to retain the above checks to make sure all PCC registers
>> within a _CPC package is under same subspace. The Spec still requires:
>> "If the PCC register space is used, all PCC registers, for all processors in
>> the same performance domain (as defined by _PSD), must be defined
>> to be in the same subspace."
>
> So that means we need to maintain the pcc_subspace_idx in cppc_pcc_data.
> If so, then I can move the check inside pcc_data_alloc().
We need the pcc_subspace_idx for checking only withing the context of this
function, so tracking that data in a local variable will be sufficient.
>>
>>> +                pcc_subspace_id = gas_t->access_width;
>>> +                if (pcc_data_alloc(pcc_subspace_id))
>>>                       goto out_free;
>> We need to call pcc_data_alloc(to increment the reference) only once per CPU,
>> otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
>> pcc_data.
>
> It is infact called only once per CPU, The refcounting was added in case if multiple CPU's share the same domain for eg:
> In instances, where CPU have hardware multi-threading support.
> or in cases where all CPU's are controlled using single subspace (as earlier).
No, it is getting called for every PCC registers within a _CPC package, instead it
needs to be called only once per _CPC package irrespective of number of PCC
registers within the_CPC  package.
>
> Do you see any issue with acpi_cppc_processor_exit() in the earlier cases, where refcount remains > 0 and the memory not getting freed-up?
>>
>>
>> -- 
>> Thanks,
>> Prashanth
>>

Re: [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

Thanks Prakash for the clarifications.
I have updated the patch set incorporating your comments and posted v5.

Regards
-George

On 10/07/2017 02:40 AM, Prakash, Prashanth wrote:
> 
> On 10/3/2017 5:01 AM, George Cherian wrote:
>> Hi Prakash,
>>
>> On 09/29/2017 04:49 AM, Prakash, Prashanth wrote:
>>> Hi George,
>>>
>>> On 9/19/2017 11:24 PM, George Cherian wrote:
>>>> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
>>>> all PCC registers, for all processors in the same performance
>>>> domain (as defined by _PSD), must be defined to be in the same subspace.
>>>> Based on Section 14.1 of ACPI specification, it is possible to have a
>>>> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
>>>> instead of using a single global pcc_data structure.
>>>>
>>>> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
>>>> and mpar_count is initialized properly.
>>>>
>>>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>>>> ---
>>>>    drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>>>>    1 file changed, 153 insertions(+), 90 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index e5b47f0..3ae79ef 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>>>>          /* Wait queue for CPUs whose requests were batched */
>>>>        wait_queue_head_t pcc_write_wait_q;
>>>> +    ktime_t last_cmd_cmpl_time;
>>>> +    ktime_t last_mpar_reset;
>>>> +    int mpar_count;
>>>> +    int refcount;
>>>>    };
>>>>    -/* Structure to represent the single PCC channel */
>>>> -static struct cppc_pcc_data pcc_data = {
>>>> -    .pcc_subspace_idx = -1,
>>>> -    .platform_owns_pcc = true,
>>>> -};
>>>> +/* Array  to represent the PCC channel per subspace id */
>>>> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
>>>> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
>>>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>>>      /*
>>>>     * The cpc_desc structure contains the ACPI register details
>>>> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>>>>    static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>>>      /* pcc mapped address + header size + offset within PCC subspace */
>>>> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
>>>> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
>>>> +                        0x8 + (offs))
>>>>      /* Check if a CPC register is in PCC */
>>>>    #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&        \
>>>> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>>>>        .default_attrs = cppc_attrs,
>>>>    };
>>>>    -static int check_pcc_chan(bool chk_err_bit)
>>>> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>>>>    {
>>>>        int ret = -EIO, status = 0;
>>>> -    struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
>>>> -    ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
>>>> +    struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>>> +    struct acpi_pcct_shared_memory __iomem *generic_comm_base =
>>>> +        pcc_ss_data->pcc_comm_addr;
>>>> +    ktime_t next_deadline = ktime_add(ktime_get(),
>>>> +                      pcc_ss_data->deadline);
>>>>    -    if (!pcc_data.platform_owns_pcc)
>>>> +    if (!pcc_ss_data->platform_owns_pcc)
>>>>            return 0;
>>>>          /* Retry in case the remote processor was too slow to catch up. */
>>>> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>>>>        }
>>>>          if (likely(!ret))
>>>> -        pcc_data.platform_owns_pcc = false;
>>>> +        pcc_ss_data->platform_owns_pcc = false;
>>>>        else
>>>>            pr_err("PCC check channel failed. Status=%x\n", status);
>>>>    @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>>>>     * 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)
>>>> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>>>>    {
>>>>        int ret = -EIO, i;
>>>> +    struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>>>        struct acpi_pcct_shared_memory *generic_comm_base =
>>>> -        (struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
>>>> -    static ktime_t last_cmd_cmpl_time, last_mpar_reset;
>>>> -    static int mpar_count;
>>>> +        (struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>>>>        unsigned int time_delta;
>>>>          /*
>>>> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>>>>             * before write completion, so first send a WRITE command to
>>>>             * platform
>>>>             */
>>>> -        if (pcc_data.pending_pcc_write_cmd)
>>>> -            send_pcc_cmd(CMD_WRITE);
>>>> +        if (pcc_ss_data->pending_pcc_write_cmd)
>>>> +            send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>>>>    -        ret = check_pcc_chan(false);
>>>> +        ret = check_pcc_chan(pcc_ss_id, false);
>>>>            if (ret)
>>>>                goto end;
>>>>        } else /* CMD_WRITE */
>>>> -        pcc_data.pending_pcc_write_cmd = FALSE;
>>>> +        pcc_ss_data->pending_pcc_write_cmd = FALSE;
>>>>          /*
>>>>         * Handle the Minimum Request Turnaround Time(MRTT)
>>>>         * "The minimum amount of time that OSPM must wait after the completion
>>>>         * of a command before issuing the next command, in microseconds"
>>>>         */
>>>> -    if (pcc_data.pcc_mrtt) {
>>>> -        time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
>>>> -        if (pcc_data.pcc_mrtt > time_delta)
>>>> -            udelay(pcc_data.pcc_mrtt - time_delta);
>>>> +    if (pcc_ss_data->pcc_mrtt) {
>>>> +        time_delta = ktime_us_delta(ktime_get(),
>>>> +                        pcc_ss_data->last_cmd_cmpl_time);
>>>> +        if (pcc_ss_data->pcc_mrtt > time_delta)
>>>> +            udelay(pcc_ss_data->pcc_mrtt - time_delta);
>>>>        }
>>>>          /*
>>>> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>>>>         * not send the request to the platform after hitting the MPAR limit in
>>>>         * any 60s window
>>>>         */
>>>> -    if (pcc_data.pcc_mpar) {
>>>> -        if (mpar_count == 0) {
>>>> -            time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
>>>> -            if (time_delta < 60 * MSEC_PER_SEC) {
>>>> +    if (pcc_ss_data->pcc_mpar) {
>>>> +        if (pcc_ss_data->mpar_count == 0) {
>>>> +            time_delta = ktime_ms_delta(ktime_get(),
>>>> +                            pcc_ss_data->last_mpar_reset);
>>>> +            if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>>>>                    pr_debug("PCC cmd not sent due to MPAR limit");
>>>>                    ret = -EIO;
>>>>                    goto end;
>>>>                }
>>>> -            last_mpar_reset = ktime_get();
>>>> -            mpar_count = pcc_data.pcc_mpar;
>>>> +            pcc_ss_data->last_mpar_reset = ktime_get();
>>>> +            pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>>>>            }
>>>> -        mpar_count--;
>>>> +        pcc_ss_data->mpar_count--;
>>>>        }
>>>>          /* Write to the shared comm region. */
>>>> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>>>>        /* Flip CMD COMPLETE bit */
>>>>        writew_relaxed(0, &generic_comm_base->status);
>>>>    -    pcc_data.platform_owns_pcc = true;
>>>> +    pcc_ss_data->platform_owns_pcc = true;
>>>>          /* Ring doorbell */
>>>> -    ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
>>>> +    ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>>>>        if (ret < 0) {
>>>>            pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>>>>                    cmd, ret);
>>>> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>>>>        }
>>>>          /* wait for completion and check for PCC errro bit */
>>>> -    ret = check_pcc_chan(true);
>>>> +    ret = check_pcc_chan(pcc_ss_id, true);
>>>>    -    if (pcc_data.pcc_mrtt)
>>>> -        last_cmd_cmpl_time = ktime_get();
>>>> +    if (pcc_ss_data->pcc_mrtt)
>>>> +        pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>>>>    -    if (pcc_data.pcc_channel->mbox->txdone_irq)
>>>> -        mbox_chan_txdone(pcc_data.pcc_channel, ret);
>>>> +    if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
>>>> +        mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>>>>        else
>>>> -        mbox_client_txdone(pcc_data.pcc_channel, ret);
>>>> +        mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>>>>      end:
>>>>        if (cmd == CMD_WRITE) {
>>>> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>>>>                    if (!desc)
>>>>                        continue;
>>>>    -                if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
>>>> +                if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>>>>                        desc->write_cmd_status = ret;
>>>>                }
>>>>            }
>>>> -        pcc_data.pcc_write_cnt++;
>>>> -        wake_up_all(&pcc_data.pcc_write_wait_q);
>>>> +        pcc_ss_data->pcc_write_cnt++;
>>>> +        wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>>>>        }
>>>>          return ret;
>>>> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>>>>    -static int register_pcc_channel(int pcc_subspace_idx)
>>>> +static int register_pcc_channel(int pcc_ss_idx)
>>>>    {
>>>>        struct acpi_pcct_hw_reduced *cppc_ss;
>>>>        u64 usecs_lat;
>>>>    -    if (pcc_subspace_idx >= 0) {
>>>> -        pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
>>>> -                pcc_subspace_idx);
>>>> +    if (pcc_ss_idx >= 0) {
>>>> +        pcc_data[pcc_ss_idx]->pcc_channel =
>>>> +            pcc_mbox_request_channel(&cppc_mbox_cl,    pcc_ss_idx);
>>>>    -        if (IS_ERR(pcc_data.pcc_channel)) {
>>>> +        if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>>>>                pr_err("Failed to find PCC communication channel\n");
>>>>                return -ENODEV;
>>>>            }
>>>> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>>>             * PCC channels) and stored pointers to the
>>>>             * subspace communication region in con_priv.
>>>>             */
>>>> -        cppc_ss = (pcc_data.pcc_channel)->con_priv;
>>>> +        cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>>>>              if (!cppc_ss) {
>>>>                pr_err("No PCC subspace found for CPPC\n");
>>>> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>>>             * So add an arbitrary amount of wait on top of Nominal.
>>>>             */
>>>>            usecs_lat = NUM_RETRIES * cppc_ss->latency;
>>>> -        pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>>> -        pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
>>>> -        pcc_data.pcc_mpar = cppc_ss->max_access_rate;
>>>> -        pcc_data.pcc_nominal = cppc_ss->latency;
>>>> -
>>>> -        pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>>> -        if (!pcc_data.pcc_comm_addr) {
>>>> +        pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>>> +        pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
>>>> +        pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
>>>> +        pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
>>>> +
>>>> +        pcc_data[pcc_ss_idx]->pcc_comm_addr =
>>>> +            acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>>> +        if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>>>>                pr_err("Failed to ioremap PCC comm region mem\n");
>>>>                return -ENOMEM;
>>>>            }
>>>>              /* Set flag so that we dont come here for each CPU. */
>>>> -        pcc_data.pcc_channel_acquired = true;
>>>> +        pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>>>>        }
>>>>          return 0;
>>>> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>>>>        return false;
>>>>    }
>>>>    +
>>>> +/**
>>>> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>>>> + *
>>>> + * Check and allocate the cppc_pcc_data memory.
>>>> + * In some processor configurations it is possible that same subspace
>>>> + * is shared between multiple CPU's. This is seen especially in CPU's
>>>> + * with hardware multi-threading support.
>>>> + *
>>>> + * Return: 0 for success, errno for failure
>>>> + */
>>>> +int pcc_data_alloc(int pcc_ss_id)
>>>> +{
>>>> +    int loop;
>>>> +
>>>> +    if (pcc_ss_id < 0)
>>> Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
>> Yes we can have this additional check.
>>>> +        return -EINVAL;
>>>> +
>>>> +    for (loop = 0; pcc_data[loop] != NULL; loop++) {
>>>> +        if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
>>>> +            pcc_data[loop]->refcount++;
>>>> +            return 0;
>>>> +        }
>>>> +    }
>>> Why do we need the above for loop? can't it be direct array lookup?
>>> if (pcc_data[pcc_ss_id]) {
>>>       //increment ref_count and return
>>> }
>> Yes, we could get rid of the loop and increment the reference directly if already allocated.
>>
>>>
>>> Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
>>> it is no longer useful and probably adds to confusion.
>> Please see below
>>>> +
>>>> +    pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
>>>> +    if (!pcc_data[pcc_ss_id])
>>>> +        return -ENOMEM;
>>>> +    pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
>>>> +    pcc_data[pcc_ss_id]->refcount++;
>>>> +
>>>> +    return 0;
>>>> +}
>>>>    /*
>>>>     * An example CPC table looks like the following.
>>>>     *
>>>> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>>>        struct device *cpu_dev;
>>>>        acpi_handle handle = pr->handle;
>>>>        unsigned int num_ent, i, cpc_rev;
>>>> +    int pcc_subspace_id = -1;
>>>>        acpi_status status;
>>>>        int ret = -EFAULT;
>>>>    @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>>>                 * so extract it only once.
>>>>                 */
>>>>                if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>>>> -                if (pcc_data.pcc_subspace_idx < 0)
>>>> -                    pcc_data.pcc_subspace_idx = gas_t->access_width;
>>>> -                else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
>>>> -                    pr_debug("Mismatched PCC ids.\n");
>>> We need to retain the above checks to make sure all PCC registers
>>> within a _CPC package is under same subspace. The Spec still requires:
>>> "If the PCC register space is used, all PCC registers, for all processors in
>>> the same performance domain (as defined by _PSD), must be defined
>>> to be in the same subspace."
>>
>> So that means we need to maintain the pcc_subspace_idx in cppc_pcc_data.
>> If so, then I can move the check inside pcc_data_alloc().
> We need the pcc_subspace_idx for checking only withing the context of this
> function, so tracking that data in a local variable will be sufficient.
>>>
>>>> +                pcc_subspace_id = gas_t->access_width;
>>>> +                if (pcc_data_alloc(pcc_subspace_id))
>>>>                        goto out_free;
>>> We need to call pcc_data_alloc(to increment the reference) only once per CPU,
>>> otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
>>> pcc_data.
>>
>> It is infact called only once per CPU, The refcounting was added in case if multiple CPU's share the same domain for eg:
>> In instances, where CPU have hardware multi-threading support.
>> or in cases where all CPU's are controlled using single subspace (as earlier).
> No, it is getting called for every PCC registers within a _CPC package, instead it
> needs to be called only once per _CPC package irrespective of number of PCC
> registers within the_CPC  package.
>>
>> Do you see any issue with acpi_cppc_processor_exit() in the earlier cases, where refcount remains > 0 and the memory not getting freed-up?
>>>
>>>
>>> -- 
>>> Thanks,
>>> Prashanth
>>>
> 

Re: [Devel] [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[George Cherian]

[-- Attachment #1: Type: text/plain, Size: 18600 bytes --]


Thanks Prakash for the clarifications.
I have updated the patch set incorporating your comments and posted v5.

Regards
-George

On 10/07/2017 02:40 AM, Prakash, Prashanth wrote:
> 
> On 10/3/2017 5:01 AM, George Cherian wrote:
>> Hi Prakash,
>>
>> On 09/29/2017 04:49 AM, Prakash, Prashanth wrote:
>>> Hi George,
>>>
>>> On 9/19/2017 11:24 PM, George Cherian wrote:
>>>> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
>>>> all PCC registers, for all processors in the same performance
>>>> domain (as defined by _PSD), must be defined to be in the same subspace.
>>>> Based on Section 14.1 of ACPI specification, it is possible to have a
>>>> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
>>>> instead of using a single global pcc_data structure.
>>>>
>>>> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
>>>> and mpar_count is initialized properly.
>>>>
>>>> Signed-off-by: George Cherian <george.cherian(a)cavium.com>
>>>> ---
>>>>    drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>>>>    1 file changed, 153 insertions(+), 90 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index e5b47f0..3ae79ef 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>>>>          /* Wait queue for CPUs whose requests were batched */
>>>>        wait_queue_head_t pcc_write_wait_q;
>>>> +    ktime_t last_cmd_cmpl_time;
>>>> +    ktime_t last_mpar_reset;
>>>> +    int mpar_count;
>>>> +    int refcount;
>>>>    };
>>>>    -/* Structure to represent the single PCC channel */
>>>> -static struct cppc_pcc_data pcc_data = {
>>>> -    .pcc_subspace_idx = -1,
>>>> -    .platform_owns_pcc = true,
>>>> -};
>>>> +/* Array  to represent the PCC channel per subspace id */
>>>> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
>>>> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
>>>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>>>      /*
>>>>     * The cpc_desc structure contains the ACPI register details
>>>> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>>>>    static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>>>      /* pcc mapped address + header size + offset within PCC subspace */
>>>> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
>>>> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
>>>> +                        0x8 + (offs))
>>>>      /* Check if a CPC register is in PCC */
>>>>    #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&        \
>>>> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>>>>        .default_attrs = cppc_attrs,
>>>>    };
>>>>    -static int check_pcc_chan(bool chk_err_bit)
>>>> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>>>>    {
>>>>        int ret = -EIO, status = 0;
>>>> -    struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
>>>> -    ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
>>>> +    struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>>> +    struct acpi_pcct_shared_memory __iomem *generic_comm_base =
>>>> +        pcc_ss_data->pcc_comm_addr;
>>>> +    ktime_t next_deadline = ktime_add(ktime_get(),
>>>> +                      pcc_ss_data->deadline);
>>>>    -    if (!pcc_data.platform_owns_pcc)
>>>> +    if (!pcc_ss_data->platform_owns_pcc)
>>>>            return 0;
>>>>          /* Retry in case the remote processor was too slow to catch up. */
>>>> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>>>>        }
>>>>          if (likely(!ret))
>>>> -        pcc_data.platform_owns_pcc = false;
>>>> +        pcc_ss_data->platform_owns_pcc = false;
>>>>        else
>>>>            pr_err("PCC check channel failed. Status=%x\n", status);
>>>>    @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>>>>     * 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)
>>>> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>>>>    {
>>>>        int ret = -EIO, i;
>>>> +    struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>>>        struct acpi_pcct_shared_memory *generic_comm_base =
>>>> -        (struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
>>>> -    static ktime_t last_cmd_cmpl_time, last_mpar_reset;
>>>> -    static int mpar_count;
>>>> +        (struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>>>>        unsigned int time_delta;
>>>>          /*
>>>> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>>>>             * before write completion, so first send a WRITE command to
>>>>             * platform
>>>>             */
>>>> -        if (pcc_data.pending_pcc_write_cmd)
>>>> -            send_pcc_cmd(CMD_WRITE);
>>>> +        if (pcc_ss_data->pending_pcc_write_cmd)
>>>> +            send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>>>>    -        ret = check_pcc_chan(false);
>>>> +        ret = check_pcc_chan(pcc_ss_id, false);
>>>>            if (ret)
>>>>                goto end;
>>>>        } else /* CMD_WRITE */
>>>> -        pcc_data.pending_pcc_write_cmd = FALSE;
>>>> +        pcc_ss_data->pending_pcc_write_cmd = FALSE;
>>>>          /*
>>>>         * Handle the Minimum Request Turnaround Time(MRTT)
>>>>         * "The minimum amount of time that OSPM must wait after the completion
>>>>         * of a command before issuing the next command, in microseconds"
>>>>         */
>>>> -    if (pcc_data.pcc_mrtt) {
>>>> -        time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
>>>> -        if (pcc_data.pcc_mrtt > time_delta)
>>>> -            udelay(pcc_data.pcc_mrtt - time_delta);
>>>> +    if (pcc_ss_data->pcc_mrtt) {
>>>> +        time_delta = ktime_us_delta(ktime_get(),
>>>> +                        pcc_ss_data->last_cmd_cmpl_time);
>>>> +        if (pcc_ss_data->pcc_mrtt > time_delta)
>>>> +            udelay(pcc_ss_data->pcc_mrtt - time_delta);
>>>>        }
>>>>          /*
>>>> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>>>>         * not send the request to the platform after hitting the MPAR limit in
>>>>         * any 60s window
>>>>         */
>>>> -    if (pcc_data.pcc_mpar) {
>>>> -        if (mpar_count == 0) {
>>>> -            time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
>>>> -            if (time_delta < 60 * MSEC_PER_SEC) {
>>>> +    if (pcc_ss_data->pcc_mpar) {
>>>> +        if (pcc_ss_data->mpar_count == 0) {
>>>> +            time_delta = ktime_ms_delta(ktime_get(),
>>>> +                            pcc_ss_data->last_mpar_reset);
>>>> +            if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>>>>                    pr_debug("PCC cmd not sent due to MPAR limit");
>>>>                    ret = -EIO;
>>>>                    goto end;
>>>>                }
>>>> -            last_mpar_reset = ktime_get();
>>>> -            mpar_count = pcc_data.pcc_mpar;
>>>> +            pcc_ss_data->last_mpar_reset = ktime_get();
>>>> +            pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>>>>            }
>>>> -        mpar_count--;
>>>> +        pcc_ss_data->mpar_count--;
>>>>        }
>>>>          /* Write to the shared comm region. */
>>>> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>>>>        /* Flip CMD COMPLETE bit */
>>>>        writew_relaxed(0, &generic_comm_base->status);
>>>>    -    pcc_data.platform_owns_pcc = true;
>>>> +    pcc_ss_data->platform_owns_pcc = true;
>>>>          /* Ring doorbell */
>>>> -    ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
>>>> +    ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>>>>        if (ret < 0) {
>>>>            pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>>>>                    cmd, ret);
>>>> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>>>>        }
>>>>          /* wait for completion and check for PCC errro bit */
>>>> -    ret = check_pcc_chan(true);
>>>> +    ret = check_pcc_chan(pcc_ss_id, true);
>>>>    -    if (pcc_data.pcc_mrtt)
>>>> -        last_cmd_cmpl_time = ktime_get();
>>>> +    if (pcc_ss_data->pcc_mrtt)
>>>> +        pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>>>>    -    if (pcc_data.pcc_channel->mbox->txdone_irq)
>>>> -        mbox_chan_txdone(pcc_data.pcc_channel, ret);
>>>> +    if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
>>>> +        mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>>>>        else
>>>> -        mbox_client_txdone(pcc_data.pcc_channel, ret);
>>>> +        mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>>>>      end:
>>>>        if (cmd == CMD_WRITE) {
>>>> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>>>>                    if (!desc)
>>>>                        continue;
>>>>    -                if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
>>>> +                if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>>>>                        desc->write_cmd_status = ret;
>>>>                }
>>>>            }
>>>> -        pcc_data.pcc_write_cnt++;
>>>> -        wake_up_all(&pcc_data.pcc_write_wait_q);
>>>> +        pcc_ss_data->pcc_write_cnt++;
>>>> +        wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>>>>        }
>>>>          return ret;
>>>> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>>>>    -static int register_pcc_channel(int pcc_subspace_idx)
>>>> +static int register_pcc_channel(int pcc_ss_idx)
>>>>    {
>>>>        struct acpi_pcct_hw_reduced *cppc_ss;
>>>>        u64 usecs_lat;
>>>>    -    if (pcc_subspace_idx >= 0) {
>>>> -        pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
>>>> -                pcc_subspace_idx);
>>>> +    if (pcc_ss_idx >= 0) {
>>>> +        pcc_data[pcc_ss_idx]->pcc_channel =
>>>> +            pcc_mbox_request_channel(&cppc_mbox_cl,    pcc_ss_idx);
>>>>    -        if (IS_ERR(pcc_data.pcc_channel)) {
>>>> +        if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>>>>                pr_err("Failed to find PCC communication channel\n");
>>>>                return -ENODEV;
>>>>            }
>>>> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>>>             * PCC channels) and stored pointers to the
>>>>             * subspace communication region in con_priv.
>>>>             */
>>>> -        cppc_ss = (pcc_data.pcc_channel)->con_priv;
>>>> +        cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>>>>              if (!cppc_ss) {
>>>>                pr_err("No PCC subspace found for CPPC\n");
>>>> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>>>             * So add an arbitrary amount of wait on top of Nominal.
>>>>             */
>>>>            usecs_lat = NUM_RETRIES * cppc_ss->latency;
>>>> -        pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>>> -        pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
>>>> -        pcc_data.pcc_mpar = cppc_ss->max_access_rate;
>>>> -        pcc_data.pcc_nominal = cppc_ss->latency;
>>>> -
>>>> -        pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>>> -        if (!pcc_data.pcc_comm_addr) {
>>>> +        pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>>> +        pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
>>>> +        pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
>>>> +        pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
>>>> +
>>>> +        pcc_data[pcc_ss_idx]->pcc_comm_addr =
>>>> +            acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>>> +        if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>>>>                pr_err("Failed to ioremap PCC comm region mem\n");
>>>>                return -ENOMEM;
>>>>            }
>>>>              /* Set flag so that we dont come here for each CPU. */
>>>> -        pcc_data.pcc_channel_acquired = true;
>>>> +        pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>>>>        }
>>>>          return 0;
>>>> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>>>>        return false;
>>>>    }
>>>>    +
>>>> +/**
>>>> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>>>> + *
>>>> + * Check and allocate the cppc_pcc_data memory.
>>>> + * In some processor configurations it is possible that same subspace
>>>> + * is shared between multiple CPU's. This is seen especially in CPU's
>>>> + * with hardware multi-threading support.
>>>> + *
>>>> + * Return: 0 for success, errno for failure
>>>> + */
>>>> +int pcc_data_alloc(int pcc_ss_id)
>>>> +{
>>>> +    int loop;
>>>> +
>>>> +    if (pcc_ss_id < 0)
>>> Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
>> Yes we can have this additional check.
>>>> +        return -EINVAL;
>>>> +
>>>> +    for (loop = 0; pcc_data[loop] != NULL; loop++) {
>>>> +        if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
>>>> +            pcc_data[loop]->refcount++;
>>>> +            return 0;
>>>> +        }
>>>> +    }
>>> Why do we need the above for loop? can't it be direct array lookup?
>>> if (pcc_data[pcc_ss_id]) {
>>>       //increment ref_count and return
>>> }
>> Yes, we could get rid of the loop and increment the reference directly if already allocated.
>>
>>>
>>> Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
>>> it is no longer useful and probably adds to confusion.
>> Please see below
>>>> +
>>>> +    pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
>>>> +    if (!pcc_data[pcc_ss_id])
>>>> +        return -ENOMEM;
>>>> +    pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
>>>> +    pcc_data[pcc_ss_id]->refcount++;
>>>> +
>>>> +    return 0;
>>>> +}
>>>>    /*
>>>>     * An example CPC table looks like the following.
>>>>     *
>>>> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>>>        struct device *cpu_dev;
>>>>        acpi_handle handle = pr->handle;
>>>>        unsigned int num_ent, i, cpc_rev;
>>>> +    int pcc_subspace_id = -1;
>>>>        acpi_status status;
>>>>        int ret = -EFAULT;
>>>>    @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>>>                 * so extract it only once.
>>>>                 */
>>>>                if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>>>> -                if (pcc_data.pcc_subspace_idx < 0)
>>>> -                    pcc_data.pcc_subspace_idx = gas_t->access_width;
>>>> -                else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
>>>> -                    pr_debug("Mismatched PCC ids.\n");
>>> We need to retain the above checks to make sure all PCC registers
>>> within a _CPC package is under same subspace. The Spec still requires:
>>> "If the PCC register space is used, all PCC registers, for all processors in
>>> the same performance domain (as defined by _PSD), must be defined
>>> to be in the same subspace."
>>
>> So that means we need to maintain the pcc_subspace_idx in cppc_pcc_data.
>> If so, then I can move the check inside pcc_data_alloc().
> We need the pcc_subspace_idx for checking only withing the context of this
> function, so tracking that data in a local variable will be sufficient.
>>>
>>>> +                pcc_subspace_id = gas_t->access_width;
>>>> +                if (pcc_data_alloc(pcc_subspace_id))
>>>>                        goto out_free;
>>> We need to call pcc_data_alloc(to increment the reference) only once per CPU,
>>> otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
>>> pcc_data.
>>
>> It is infact called only once per CPU, The refcounting was added in case if multiple CPU's share the same domain for eg:
>> In instances, where CPU have hardware multi-threading support.
>> or in cases where all CPU's are controlled using single subspace (as earlier).
> No, it is getting called for every PCC registers within a _CPC package, instead it
> needs to be called only once per _CPC package irrespective of number of PCC
> registers within the_CPC  package.
>>
>> Do you see any issue with acpi_cppc_processor_exit() in the earlier cases, where refcount remains > 0 and the memory not getting freed-up?
>>>
>>>
>>> -- 
>>> Thanks,
>>> Prashanth
>>>
>