Re: [PATCH] drivers/perf: hisi: Simplify hisi_read_sccl_and_ccl_id and its comment

[Shaokun Zhang <zhangshaokun@hisilicon.com>]

Hi John,

On 2019/11/11 21:49, John Garry wrote:
> On 09/11/2019 02:51, Shaokun Zhang wrote:
>> hisi_read_sccl_and_ccl_id is not readable
> 
> That's a little strong :)
> 
>  and its comment is a little
>> confused, so simplify the function and its comment as Mark's suggestion.
>>
>> Cc: John Garry <john.garry@huawei.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Suggested-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> ---
>>   drivers/perf/hisilicon/hisi_uncore_pmu.c | 58 ++++++++++++++++++--------------
>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> index 96183e31b96a..9e9625a1f388 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> @@ -337,38 +337,44 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
>>       hisi_pmu->ops->stop_counters(hisi_pmu);
>>   }
>>   +
>>   /*
>> - * Read Super CPU cluster and CPU cluster ID from MPIDR_EL1.
>> - * If multi-threading is supported, On Huawei Kunpeng 920 SoC whose cpu
>> - * core is tsv110, CCL_ID is the low 3-bits in MPIDR[Aff2] and SCCL_ID
>> - * is the upper 5-bits of Aff2 field; while for other cpu types, SCCL_ID
>> - * is in MPIDR[Aff3] and CCL_ID is in MPIDR[Aff2], if not, SCCL_ID
>> - * is in MPIDR[Aff2] and CCL_ID is in MPIDR[Aff1].
>> + * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be
>> + * determined from the MPIDR_EL1, but the encoding varies by CPU:
>> + *
>> + * - For MT variants of TSV110 (e.g. found in Kunpeng 920):
> 
> I wish that you would drop the "found in Kunpeng 920", as I find it confusing/misleading.
> 

How about * - For MT variants of TSV110 (e.g. found in Kunpeng 920 if the CPU variant is 0x1): ?
If this is also confusing, I will drop it.

Thanks,
Shaokun

> Thanks,
> John
> 
>> + *   SCCL is Aff2[7:3], CCL is Aff2[2:0]
>> + *
>> + * - For other MT parts:
>> + *   SCCL is Aff3[7:0], CCL is Aff2[7:0]
>> + *
>> + * - For non-MT parts:
>> + *   SCCL is Aff2[7:0], CCL is Aff1[7:0]
>>    */
>> -static void hisi_read_sccl_and_ccl_id(int *sccl_id, int *ccl_id)
>> +static void hisi_read_sccl_and_ccl_id(int *scclp, int *cclp)
>>   {
>>       u64 mpidr = read_cpuid_mpidr();
>> -
>> -    if (mpidr & MPIDR_MT_BITMASK) {
>> -        if (read_cpuid_part_number() == HISI_CPU_PART_TSV110) {
>> -            int aff2 = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>> -
>> -            if (sccl_id)
>> -                *sccl_id = aff2 >> 3;
>> -            if (ccl_id)
>> -                *ccl_id = aff2 & 0x7;
>> -        } else {
>> -            if (sccl_id)
>> -                *sccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 3);
>> -            if (ccl_id)
>> -                *ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>> -        }
>> +    int aff3 = MPIDR_AFFINITY_LEVEL(mpidr, 3);
>> +    int aff2 = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>> +    int aff1 = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +    bool mt = mpidr & MPIDR_MT_BITMASK;
>> +    int sccl, ccl;
>> +
>> +    if (mt && read_cpuid_part_number() == HISI_CPU_PART_TSV110) {
>> +        sccl = aff2 >> 3;
>> +        ccl = aff2 & 0x7;
>> +    } else if (mt) {
>> +        sccl = aff3;
>> +        ccl = aff2;
>>       } else {
>> -        if (sccl_id)
>> -            *sccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>> -        if (ccl_id)
>> -            *ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +        sccl = aff2;
>> +        ccl = aff1;
>>       }
>> +
>> +    if (scclp)
>> +        *scclp = sccl;
>> +    if (cclp)
>> +        *cclp = ccl;
>>   }
>>     /*
>>
> 
> 
> .
> 


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