lkml.org 
[lkml]   [2017]   [Apr]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids
From
Date

Hi Alexey,

On 04/03/2017 11:07 PM, Alexey Klimov wrote:
> (adding Prashanth to c/c)
>
> Hi George,
>
> On Fri, Mar 31, 2017 at 06:24:02AM +0000, George Cherian wrote:
>> 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. Also maintain a global total_mpar_count
>> which is a sum of per subspace id mpar value.
>
> Could you please provide clarification on why sum of total_mpar_count is
> required? Do you assume that there always will be only one single firmware CPU
> that handles PCC commands on another side?

Yes you are right the total_mpar_count should be removed and should be
handled per subspace id. Moreover the current logic of not sending the
command to PCC and returning with -EIO is also flawed. It should
actually have a retry mechanism instead of returning -EIO even without
submitting the request to the channel.
>
> Theoretically different PCC channels can be connected to different platform CPUs
> on other end (imagine NUMA systems in case of CPPC) so it's not clear why transport
> layer of PCC should use that global count. Also, ACPI spec 6.1 (page 701) in
> in description of MPAR states "The maximum number of periodic requests that the subspace
> channel can support".
>
>
>
>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>> ---
>> drivers/acpi/cppc_acpi.c | 189 ++++++++++++++++++++++++++---------------------
>> 1 file changed, 105 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 3ca0729..7ba05ac 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -77,12 +77,16 @@ 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 quiet possible that multiple CPU's can share
>> + * same subspace ids. The cpu_pcc_subspace_idx maintains
>> + * the cpu to pcc subspace id map.
>> + */
>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>
>> +static int total_mpar_count;
>> /*
>> * The cpc_desc structure contains the ACPI register details
>> * as described in the per CPU _CPC tables. The details
>> @@ -93,7 +97,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 +188,17 @@ 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 cpunum, 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);
>> -
>> - if (!pcc_data.platform_owns_pcc)
>> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>> + 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_ss_data->platform_owns_pcc)
>> return 0;
>>
>> /* Retry in case the remote processor was too slow to catch up. */
>> @@ -214,7 +223,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 +234,13 @@ 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 cpunum, u16 cmd)
>
>
> I don't like the direction of where it's going.
>
> To send commands through PCC channel you don't need to know CPU number.
> Ideally, send_pcc_cmd() shouldn't care a lot about software entity that uses
> it (CPPC, RASF, MPST, etc) and passing some CPU number to this function you
> bind it to CPPC interfaces while it shouldn't depend on it.
> Maybe you can pass subspace it here instead.
Actually if you look inside send_pcc_cmd it does a mapping of cpu to
subspace id. To avoid confusion it is better to pass the subspace id to
this function than the cpu number.
>
> BTW, is it possible to make separate mailbox PCC client and move it out from
> CPPC code?

Why do you think it is necessary? Currently CPPC driver itself is the
client for mailbox/pcc driver.

>
>
> [...]
>
>
> Best regards,
> Alexey Klimov.
>
>

\
 
 \ /
  Last update: 2017-04-04 12:53    [W:0.600 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site