From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Cherian Subject: Re: [PATCH 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids Date: Fri, 14 Jul 2017 16:32:19 +0530 Message-ID: <5968A4BB.6070200@caviumnetworks.com> References: <1497363431-10002-1-git-send-email-george.cherian@cavium.com> <1497363431-10002-3-git-send-email-george.cherian@cavium.com> <84ba5386-d33b-b7e7-9295-c15f02fe1d01@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-bl2nam02on0081.outbound.protection.outlook.com ([104.47.38.81]:47712 "EHLO NAM02-BL2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753827AbdGNLCl (ORCPT ); Fri, 14 Jul 2017 07:02:41 -0400 In-Reply-To: <84ba5386-d33b-b7e7-9295-c15f02fe1d01@codeaurora.org> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: "Prakash, Prashanth" , George Cherian , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, devel@acpica.org Cc: rjw@rjwysocki.net, lenb@kernel.org, jassisinghbrar@gmail.com, robert.moore@intel.com, lv.zheng@intel.com Hi Prashanth, Thanks for the review. On 07/14/2017 03:14 AM, Prakash, Prashanth wrote: > Hi George, > > On 6/13/2017 8:17 AM, 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 >> --- >> drivers/acpi/cppc_acpi.c | 179 +++++++++++++++++++++++++---------------------- >> 1 file changed, 96 insertions(+), 83 deletions(-) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 3ca0729..79c35b1 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -77,11 +77,10 @@ struct cppc_pcc_data { >> wait_queue_head_t pcc_write_wait_q; >> }; >> >> -/* 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]; > It is unlikely we will end up using all 256 PCC subspaces, so can we > please make it a an array(size=MAX_PCC_SUBSPACES) of cppc_pcc_data > pointers. We can dynamically allocate the cppc_pcc_data structures and > maintain their references in this array. Yes, I will do that in next version. >> +/* 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 +92,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 regsiter is in PCC */ >> #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER && \ >> @@ -183,13 +183,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. */ >> @@ -214,7 +217,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); >> >> @@ -225,11 +228,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; >> + (struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr; >> static ktime_t last_cmd_cmpl_time, last_mpar_reset; >> static int mpar_count; > Since we had only one PCC channel until ACPI6.1, it was ok to maintain > these (mpar_count, last_cmd_cmpl_time, last_mpar_reset) here as static > variables. > Now that we are supporting multiple PCC subspaces these should be > maintained per PCC subspaces. Can you please move these over to the > cppc_pcc_data? Yes I will move these to be maintained as per subspace. >> unsigned int time_delta; >> @@ -244,24 +248,24 @@ 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) { >> + if (pcc_ss_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) >> + udelay(pcc_ss_data->pcc_mrtt - time_delta); >> } >> >> /* >> @@ -275,16 +279,16 @@ 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 (pcc_ss_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 ((time_delta < 60 * MSEC_PER_SEC) && 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; >> + mpar_count = pcc_ss_data->pcc_mpar; >> } >> mpar_count--; >> } > The above mpar and mrtt checks should be modified to use per subspace variables. Will do. >> @@ -295,10 +299,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); >> @@ -306,15 +310,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) >> + if (pcc_ss_data->pcc_mrtt) >> 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) { >> @@ -324,12 +328,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; >> @@ -531,16 +535,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; >> } >> @@ -551,7 +555,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"); >> @@ -564,19 +568,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; >> @@ -656,6 +661,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; >> + unsigned int pcc_subspace_id = 0; >> acpi_status status; >> int ret = -EFAULT; >> >> @@ -728,12 +734,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"); >> - goto out_free; >> - } >> + pcc_subspace_id = gas_t->access_width; >> + pcc_data[pcc_subspace_id].pcc_subspace_idx = gas_t->access_width; >> + per_cpu(cpu_pcc_subspace_idx, pr->id) = gas_t->access_width; > pcc_data[pcc_subspace_id].pcc_subspace_idx = pcc_subspace_id; > per_cpu(cpu_pcc_subspace_idx, pr->id) = pcc_subspace_id; > > I don't see a need for pcc_subspace_idx to be in struct cppc_pcc_data > anymore. If you agree, may be we can remove it. Okay, I agree. > >> } else if (gas_t->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) { >> if (gas_t->address) { >> void __iomem *addr; >> @@ -766,14 +769,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_data[pcc_subspace_id].pcc_channel_acquired) { > Looks like we assume a PCC subspace is always present and looking at the > 6.2 spec it doesn't look like a mandatory requirement. So, the correct check > here should be: > if (pcc_subspace_id is valid && !pcc_data[pcc_subspace_id].pcc_channel_acquired) > > We need to make sure pcc_subspace_id is set to an invalid value until we find a valid > PCC subspace within the _CPC package. You can correct this in next version or I can > submit a small patch. Okay I will do the necessary changes. >> + ret = register_pcc_channel(pcc_data[pcc_subspace_id].pcc_subspace_idx); >> 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 */ >> @@ -883,6 +886,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 = ®_res->cpc_entry.reg; >> >> if (reg_res->type == ACPI_TYPE_INTEGER) { >> @@ -892,7 +896,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) >> @@ -927,10 +931,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 = ®_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) >> @@ -974,6 +979,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps) >> struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum); >> struct cpc_register_resource *highest_reg, *lowest_reg, *ref_perf, >> *nom_perf; >> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum); >> + struct cppc_pcc_data *pcc_ss_data = &pcc_data[pcc_ss_id]; >> u64 high, low, nom; >> int ret = 0, regs_in_pcc = 0; >> >> @@ -991,9 +998,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(ref_perf) || CPC_IN_PCC(nom_perf)) { >> 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; >> } >> @@ -1013,7 +1020,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); >> @@ -1030,6 +1037,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; >> >> @@ -1053,10 +1062,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; >> } >> @@ -1086,7 +1095,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs) >> perf_fb_ctrs->ctr_wrap_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); >> @@ -1102,6 +1111,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) { >> @@ -1119,11 +1130,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; >> } >> } >> @@ -1131,8 +1142,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; >> } >> >> @@ -1143,7 +1154,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 >> * >> @@ -1191,15 +1202,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; >> @@ -1232,6 +1243,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) >> @@ -1241,11 +1254,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; >> } > > When you send the next version with updates, can you please add a version > to the patches ex: [PATCH v2 2/2]? I think this series is v2, as it addresses some > of Alexey's comments on the initial version. Not only Alexey's comments, Initially this approach was rejected due to the ACPI spec(6.1) limitations. Now that the spec changes for multiple subspaces are adapted I thought of reseting the patch version to v1. Going forward I will maintain the versions. > > -- > Thanks, > Prashanth Regards, -George > From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============8732277283548946252==" MIME-Version: 1.0 From: George Cherian Subject: Re: [Devel] [PATCH 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids Date: Fri, 14 Jul 2017 16:32:19 +0530 Message-ID: <5968A4BB.6070200@caviumnetworks.com> In-Reply-To: 84ba5386-d33b-b7e7-9295-c15f02fe1d01@codeaurora.org List-ID: To: devel@acpica.org --===============8732277283548946252== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Prashanth, Thanks for the review. On 07/14/2017 03:14 AM, Prakash, Prashanth wrote: > Hi George, > > On 6/13/2017 8:17 AM, 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_mp= ar_reset >> and mpar_count is initialized properly. >> >> Signed-off-by: George Cherian >> --- >> drivers/acpi/cppc_acpi.c | 179 +++++++++++++++++++++++++--------------= -------- >> 1 file changed, 96 insertions(+), 83 deletions(-) >> >> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c >> index 3ca0729..79c35b1 100644 >> --- a/drivers/acpi/cppc_acpi.c >> +++ b/drivers/acpi/cppc_acpi.c >> @@ -77,11 +77,10 @@ struct cppc_pcc_data { >> wait_queue_head_t pcc_write_wait_q; >> }; >> >> -/* Structure to represent the single PCC channel */ >> -static struct cppc_pcc_data pcc_data =3D { >> - .pcc_subspace_idx =3D -1, >> - .platform_owns_pcc =3D true, >> -}; >> +/* Array to represent the PCC channel per subspace id */ >> +static struct cppc_pcc_data pcc_data[MAX_PCC_SUBSPACES]; > It is unlikely we will end up using all 256 PCC subspaces, so can we > please make it a an array(size=3DMAX_PCC_SUBSPACES) of cppc_pcc_data > pointers. We can dynamically allocate the cppc_pcc_data structures and > maintain their references in this array. Yes, I will do that in next version. >> +/* 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 +92,8 @@ static struct cppc_pcc_data pcc_data =3D { >> 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_ad= dr + \ >> + 0x8 + (offs)) >> >> /* Check if a CPC regsiter is in PCC */ >> #define CPC_IN_PCC(cpc) ((cpc)->type =3D=3D ACPI_TYPE_BUFFER && \ >> @@ -183,13 +183,16 @@ static struct kobj_type cppc_ktype =3D { >> .default_attrs =3D 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 =3D -EIO, status =3D 0; >> - struct acpi_pcct_shared_memory __iomem *generic_comm_base =3D pcc_data= .pcc_comm_addr; >> - ktime_t next_deadline =3D ktime_add(ktime_get(), pcc_data.deadline); >> + struct cppc_pcc_data *pcc_ss_data =3D &pcc_data[pcc_ss_id]; >> + struct acpi_pcct_shared_memory __iomem *generic_comm_base =3D >> + pcc_ss_data->pcc_comm_addr; >> + ktime_t next_deadline =3D 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. */ >> @@ -214,7 +217,7 @@ static int check_pcc_chan(bool chk_err_bit) >> } >> >> if (likely(!ret)) >> - pcc_data.platform_owns_pcc =3D false; >> + pcc_ss_data->platform_owns_pcc =3D false; >> else >> pr_err("PCC check channel failed. Status=3D%x\n", status); >> >> @@ -225,11 +228,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 =3D -EIO, i; >> + struct cppc_pcc_data *pcc_ss_data =3D &pcc_data[pcc_ss_id]; >> struct acpi_pcct_shared_memory *generic_comm_base =3D >> - (struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr; >> + (struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr; >> static ktime_t last_cmd_cmpl_time, last_mpar_reset; >> static int mpar_count; > Since we had only one PCC channel until ACPI6.1, it was ok to maintain > these (mpar_count, last_cmd_cmpl_time, last_mpar_reset) here as static > variables. > Now that we are supporting multiple PCC subspaces these should be > maintained per PCC subspaces. Can you please move these over to the > cppc_pcc_data? Yes I will move these to be maintained as per subspace. >> unsigned int time_delta; >> @@ -244,24 +248,24 @@ 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 =3D check_pcc_chan(false); >> + ret =3D check_pcc_chan(pcc_ss_id, false); >> if (ret) >> goto end; >> } else /* CMD_WRITE */ >> - pcc_data.pending_pcc_write_cmd =3D FALSE; >> + pcc_ss_data->pending_pcc_write_cmd =3D FALSE; >> >> /* >> * Handle the Minimum Request Turnaround Time(MRTT) >> * "The minimum amount of time that OSPM must wait after the completi= on >> * of a command before issuing the next command, in microseconds" >> */ >> - if (pcc_data.pcc_mrtt) { >> + if (pcc_ss_data->pcc_mrtt) { >> time_delta =3D 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) >> + udelay(pcc_ss_data->pcc_mrtt - time_delta); >> } >> >> /* >> @@ -275,16 +279,16 @@ 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 (pcc_ss_data->pcc_mpar) { >> if (mpar_count =3D=3D 0) { >> time_delta =3D ktime_ms_delta(ktime_get(), last_mpar_reset); >> - if (time_delta < 60 * MSEC_PER_SEC) { >> + if ((time_delta < 60 * MSEC_PER_SEC) && last_mpar_reset) { >> pr_debug("PCC cmd not sent due to MPAR limit"); >> ret =3D -EIO; >> goto end; >> } >> last_mpar_reset =3D ktime_get(); >> - mpar_count =3D pcc_data.pcc_mpar; >> + mpar_count =3D pcc_ss_data->pcc_mpar; >> } >> mpar_count--; >> } > The above mpar and mrtt checks should be modified to use per subspace var= iables. Will do. >> @@ -295,10 +299,10 @@ static int send_pcc_cmd(u16 cmd) >> /* Flip CMD COMPLETE bit */ >> writew_relaxed(0, &generic_comm_base->status); >> >> - pcc_data.platform_owns_pcc =3D true; >> + pcc_ss_data->platform_owns_pcc =3D true; >> >> /* Ring doorbell */ >> - ret =3D mbox_send_message(pcc_data.pcc_channel, &cmd); >> + ret =3D 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); >> @@ -306,15 +310,15 @@ static int send_pcc_cmd(u16 cmd) >> } >> >> /* wait for completion and check for PCC errro bit */ >> - ret =3D check_pcc_chan(true); >> + ret =3D check_pcc_chan(pcc_ss_id, true); >> >> - if (pcc_data.pcc_mrtt) >> + if (pcc_ss_data->pcc_mrtt) >> last_cmd_cmpl_time =3D 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 =3D=3D CMD_WRITE) { >> @@ -324,12 +328,12 @@ static int send_pcc_cmd(u16 cmd) >> if (!desc) >> continue; >> >> - if (desc->write_cmd_id =3D=3D pcc_data.pcc_write_cnt) >> + if (desc->write_cmd_id =3D=3D pcc_ss_data->pcc_write_cnt) >> desc->write_cmd_status =3D 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; >> @@ -531,16 +535,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 >=3D 0) { >> - pcc_data.pcc_channel =3D pcc_mbox_request_channel(&cppc_mbox_cl, >> - pcc_subspace_idx); >> + if (pcc_ss_idx >=3D 0) { >> + pcc_data[pcc_ss_idx].pcc_channel =3D >> + 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; >> } >> @@ -551,7 +555,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 =3D (pcc_data.pcc_channel)->con_priv; >> + cppc_ss =3D (pcc_data[pcc_ss_idx].pcc_channel)->con_priv; >> >> if (!cppc_ss) { >> pr_err("No PCC subspace found for CPPC\n"); >> @@ -564,19 +568,20 @@ static int register_pcc_channel(int pcc_subspace_i= dx) >> * So add an arbitrary amount of wait on top of Nominal. >> */ >> usecs_lat =3D NUM_RETRIES * cppc_ss->latency; >> - pcc_data.deadline =3D ns_to_ktime(usecs_lat * NSEC_PER_USEC); >> - pcc_data.pcc_mrtt =3D cppc_ss->min_turnaround_time; >> - pcc_data.pcc_mpar =3D cppc_ss->max_access_rate; >> - pcc_data.pcc_nominal =3D cppc_ss->latency; >> - >> - pcc_data.pcc_comm_addr =3D acpi_os_ioremap(cppc_ss->base_address, cpp= c_ss->length); >> - if (!pcc_data.pcc_comm_addr) { >> + pcc_data[pcc_ss_idx].deadline =3D ns_to_ktime(usecs_lat * NSEC_PER_US= EC); >> + pcc_data[pcc_ss_idx].pcc_mrtt =3D cppc_ss->min_turnaround_time; >> + pcc_data[pcc_ss_idx].pcc_mpar =3D cppc_ss->max_access_rate; >> + pcc_data[pcc_ss_idx].pcc_nominal =3D cppc_ss->latency; >> + >> + pcc_data[pcc_ss_idx].pcc_comm_addr =3D >> + 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 =3D true; >> + pcc_data[pcc_ss_idx].pcc_channel_acquired =3D true; >> } >> >> return 0; >> @@ -656,6 +661,7 @@ int acpi_cppc_processor_probe(struct acpi_processor = *pr) >> struct device *cpu_dev; >> acpi_handle handle =3D pr->handle; >> unsigned int num_ent, i, cpc_rev; >> + unsigned int pcc_subspace_id =3D 0; >> acpi_status status; >> int ret =3D -EFAULT; >> >> @@ -728,12 +734,9 @@ int acpi_cppc_processor_probe(struct acpi_processor= *pr) >> * so extract it only once. >> */ >> if (gas_t->space_id =3D=3D ACPI_ADR_SPACE_PLATFORM_COMM) { >> - if (pcc_data.pcc_subspace_idx < 0) >> - pcc_data.pcc_subspace_idx =3D gas_t->access_width; >> - else if (pcc_data.pcc_subspace_idx !=3D gas_t->access_width) { >> - pr_debug("Mismatched PCC ids.\n"); >> - goto out_free; >> - } >> + pcc_subspace_id =3D gas_t->access_width; >> + pcc_data[pcc_subspace_id].pcc_subspace_idx =3D gas_t->access_width; >> + per_cpu(cpu_pcc_subspace_idx, pr->id) =3D gas_t->access_width; > pcc_data[pcc_subspace_id].pcc_subspace_idx =3D pcc_subspace_id; > per_cpu(cpu_pcc_subspace_idx, pr->id) =3D pcc_subspace_id; > > I don't see a need for pcc_subspace_idx to be in struct cppc_pcc_data > anymore. If you agree, may be we can remove it. Okay, I agree. > >> } else if (gas_t->space_id =3D=3D ACPI_ADR_SPACE_SYSTEM_MEMORY) { >> if (gas_t->address) { >> void __iomem *addr; >> @@ -766,14 +769,14 @@ int acpi_cppc_processor_probe(struct acpi_processo= r *pr) >> if (ret) >> goto out_free; >> >> - /* Register PCC channel once for all CPUs. */ >> - if (!pcc_data.pcc_channel_acquired) { >> - ret =3D register_pcc_channel(pcc_data.pcc_subspace_idx); >> + /* Register PCC channel once for all PCC subspace id. */ >> + if (!pcc_data[pcc_subspace_id].pcc_channel_acquired) { > Looks like we assume a PCC subspace is always present and looking at the > 6.2 spec it doesn't look like a mandatory requirement. So, the correct ch= eck > here should be: > if (pcc_subspace_id is valid && !pcc_data[pcc_subspace_id].pcc_channel_ac= quired) > > We need to make sure pcc_subspace_id is set to an invalid value until we = find a valid > PCC subspace within the _CPC package. You can correct this in next versio= n or I can > submit a small patch. Okay I will do the necessary changes. >> + ret =3D register_pcc_channel(pcc_data[pcc_subspace_id].pcc_subspace_i= dx); >> 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 */ >> @@ -883,6 +886,7 @@ static int cpc_read(int cpu, struct cpc_register_res= ource *reg_res, u64 *val) >> { >> int ret_val =3D 0; >> void __iomem *vaddr =3D 0; >> + int pcc_ss_id =3D per_cpu(cpu_pcc_subspace_idx, cpu); >> struct cpc_reg *reg =3D ®_res->cpc_entry.reg; >> >> if (reg_res->type =3D=3D ACPI_TYPE_INTEGER) { >> @@ -892,7 +896,7 @@ static int cpc_read(int cpu, struct cpc_register_res= ource *reg_res, u64 *val) >> >> *val =3D 0; >> if (reg->space_id =3D=3D ACPI_ADR_SPACE_PLATFORM_COMM) >> - vaddr =3D GET_PCC_VADDR(reg->address); >> + vaddr =3D GET_PCC_VADDR(reg->address, pcc_ss_id); >> else if (reg->space_id =3D=3D ACPI_ADR_SPACE_SYSTEM_MEMORY) >> vaddr =3D reg_res->sys_mem_vaddr; >> else if (reg->space_id =3D=3D ACPI_ADR_SPACE_FIXED_HARDWARE) >> @@ -927,10 +931,11 @@ static int cpc_write(int cpu, struct cpc_register_= resource *reg_res, u64 val) >> { >> int ret_val =3D 0; >> void __iomem *vaddr =3D 0; >> + int pcc_ss_id =3D per_cpu(cpu_pcc_subspace_idx, cpu); >> struct cpc_reg *reg =3D ®_res->cpc_entry.reg; >> >> if (reg->space_id =3D=3D ACPI_ADR_SPACE_PLATFORM_COMM) >> - vaddr =3D GET_PCC_VADDR(reg->address); >> + vaddr =3D GET_PCC_VADDR(reg->address, pcc_ss_id); >> else if (reg->space_id =3D=3D ACPI_ADR_SPACE_SYSTEM_MEMORY) >> vaddr =3D reg_res->sys_mem_vaddr; >> else if (reg->space_id =3D=3D ACPI_ADR_SPACE_FIXED_HARDWARE) >> @@ -974,6 +979,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_= caps *perf_caps) >> struct cpc_desc *cpc_desc =3D per_cpu(cpc_desc_ptr, cpunum); >> struct cpc_register_resource *highest_reg, *lowest_reg, *ref_perf, >> *nom_perf; >> + int pcc_ss_id =3D per_cpu(cpu_pcc_subspace_idx, cpunum); >> + struct cppc_pcc_data *pcc_ss_data =3D &pcc_data[pcc_ss_id]; >> u64 high, low, nom; >> int ret =3D 0, regs_in_pcc =3D 0; >> >> @@ -991,9 +998,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(ref_perf) || CPC_IN_PCC(nom_perf)) { >> regs_in_pcc =3D 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 =3D -EIO; >> goto out_err; >> } >> @@ -1013,7 +1020,7 @@ int cppc_get_perf_caps(int cpunum, struct cppc_per= f_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); >> @@ -1030,6 +1037,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_per= f_fb_ctrs *perf_fb_ctrs) >> struct cpc_desc *cpc_desc =3D per_cpu(cpc_desc_ptr, cpunum); >> struct cpc_register_resource *delivered_reg, *reference_reg, >> *ref_perf_reg, *ctr_wrap_reg; >> + int pcc_ss_id =3D per_cpu(cpu_pcc_subspace_idx, cpunum); >> + struct cppc_pcc_data *pcc_ss_data =3D &pcc_data[pcc_ss_id]; >> u64 delivered, reference, ref_perf, ctr_wrap_time; >> int ret =3D 0, regs_in_pcc =3D 0; >> >> @@ -1053,10 +1062,10 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_p= erf_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 =3D 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 =3D -EIO; >> goto out_err; >> } >> @@ -1086,7 +1095,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_per= f_fb_ctrs *perf_fb_ctrs) >> perf_fb_ctrs->ctr_wrap_time =3D 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); >> @@ -1102,6 +1111,8 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls = *perf_ctrls) >> { >> struct cpc_desc *cpc_desc =3D per_cpu(cpc_desc_ptr, cpu); >> struct cpc_register_resource *desired_reg; >> + int pcc_ss_id =3D per_cpu(cpu_pcc_subspace_idx, cpu); >> + struct cppc_pcc_data *pcc_ss_data =3D &pcc_data[pcc_ss_id]; >> int ret =3D 0; >> >> if (!cpc_desc) { >> @@ -1119,11 +1130,11 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrl= s *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 =3D check_pcc_chan(false); >> + down_read(&pcc_ss_data->pcc_lock); /* BEGIN Phase-I */ >> + if (pcc_ss_data->platform_owns_pcc) { >> + ret =3D check_pcc_chan(pcc_ss_id, false); >> if (ret) { >> - up_read(&pcc_data.pcc_lock); >> + up_read(&pcc_ss_data->pcc_lock); >> return ret; >> } >> } >> @@ -1131,8 +1142,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 =3D true; >> - cpc_desc->write_cmd_id =3D pcc_data.pcc_write_cnt; >> + pcc_ss_data->pending_pcc_write_cmd =3D true; >> + cpc_desc->write_cmd_id =3D pcc_ss_data->pcc_write_cnt; >> cpc_desc->write_cmd_status =3D 0; >> } >> >> @@ -1143,7 +1154,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 >> * >> @@ -1191,15 +1202,15 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrl= s *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 !=3D pcc_data.pcc_write_cnt); >> + wait_event(pcc_ss_data->pcc_write_wait_q, >> + cpc_desc->write_cmd_id !=3D pcc_ss_data->pcc_write_cnt); >> >> /* send_pcc_cmd updates the status in case of failure */ >> ret =3D cpc_desc->write_cmd_status; >> @@ -1232,6 +1243,8 @@ unsigned int cppc_get_transition_latency(int cpu_n= um) >> unsigned int latency_ns =3D 0; >> struct cpc_desc *cpc_desc; >> struct cpc_register_resource *desired_reg; >> + int pcc_ss_id =3D per_cpu(cpu_pcc_subspace_idx, cpu_num); >> + struct cppc_pcc_data *pcc_ss_data =3D &pcc_data[pcc_ss_id]; >> >> cpc_desc =3D per_cpu(cpc_desc_ptr, cpu_num); >> if (!cpc_desc) >> @@ -1241,11 +1254,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 =3D 60 * (1000 * 1000 * 1000 / pcc_data.pcc_mpar); >> + if (pcc_ss_data->pcc_mpar) >> + latency_ns =3D 60 * (1000 * 1000 * 1000 / pcc_ss_data->pcc_mpar); >> >> - latency_ns =3D max(latency_ns, pcc_data.pcc_nominal * 1000); >> - latency_ns =3D max(latency_ns, pcc_data.pcc_mrtt * 1000); >> + latency_ns =3D max(latency_ns, pcc_ss_data->pcc_nominal * 1000); >> + latency_ns =3D max(latency_ns, pcc_ss_data->pcc_mrtt * 1000); >> >> return latency_ns; >> } > > When you send the next version with updates, can you please add a version > to the patches ex: [PATCH v2 2/2]? I think this series is v2, as it addre= sses some > of Alexey's comments on the initial version. Not only Alexey's comments, Initially this approach was rejected due to = the ACPI spec(6.1) limitations. Now that the spec changes for multiple subspaces are adapted I thought = of reseting the patch version to v1. Going forward I will maintain the = versions. > > -- > Thanks, > Prashanth Regards, -George > --===============8732277283548946252==--