Re: [PATCH] drivers/perf: hisi: update the sccl_id/ccl_id for certain HiSilicon platform

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
> For some HiSilicon platform, the originally designed SCCL_ID and CCL_ID
> are not satisfied with much rich topology when the MT is set, so we
> extend the SCCL_ID to MPIDR[aff3] and CCL_ID to MPIDR[aff2]. Let's
> update this for HiSilicon uncore PMU driver.
> 
> Cc: John Garry <john.garry@huawei.com>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> ---
>  drivers/perf/hisilicon/hisi_uncore_pmu.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> index 79f76f8dda8e..96183e31b96a 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> @@ -15,6 +15,7 @@
>  #include <linux/errno.h>
>  #include <linux/interrupt.h>
>  
> +#include <asm/cputype.h>
>  #include <asm/local64.h>
>  
>  #include "hisi_uncore_pmu.h"
> @@ -338,8 +339,10 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
>  
>  /*
>   * Read Super CPU cluster and CPU cluster ID from MPIDR_EL1.
> - * If multi-threading is supported, CCL_ID is the low 3-bits in MPIDR[Aff2]
> - * and SCCL_ID is the upper 5-bits of Aff2 field; if not, SCCL_ID
> + * 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].
>   */

Is TSV110 in any other SoCs, where the mapping of MPIDR to SCCL/CCL IDs
differs?

The comment would be much easier to read as something like:

/*
 * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be
 * determined from the MPIDR_EL1, but the encoding varies by CPU:
 *
 * - For TSV110 (e.g. found in Kunpeng 920):
 *   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 other (non-MT) parts:
 *   SCCL is Aff2[7:0], CCL is Aff1[7:0]
 */

If TSV110 is always MT, then it would be better to structure the code
similarly to that comment:

static void hisi_read_sccl_and_ccl_id(int *scclp, int *cclp)
{
	u64 mpidr = read_cpuid_mpidr();
	int aff3 = MPIDR_AFFINITY_LEVEL(mpidr, 3);
	int aff2 = MPIDR_AFFINITY_LEVEL(mpidr, 2);
	int aff1 = MPIDR_AFFINITY_LEVEL(mpidr, 1);
	int sccl, ccl;

	if (read_cpuid_part_number() == HISI_CPU_PART_TSV110) {
		sccl = aff2 >> 3;
		ccl = aff2 & 0x7;
	} else if (mpidr & MPIDR_MT_BITMASK) {
		sccl = aff3;
		ccl = aff2;
	} else {
		sccl = aff2;
		ccl = aff1;
	}

	if (scclp)
		*scclp = sccl;
	if (cclp)
		*cclp = ccl;
}

Thanks,
Mark.

>  static void hisi_read_sccl_and_ccl_id(int *sccl_id, int *ccl_id)
> @@ -347,12 +350,19 @@ static void hisi_read_sccl_and_ccl_id(int *sccl_id, int *ccl_id)
>  	u64 mpidr = read_cpuid_mpidr();
>  
>  	if (mpidr & MPIDR_MT_BITMASK) {
> -		int aff2 = MPIDR_AFFINITY_LEVEL(mpidr, 2);
> -
> -		if (sccl_id)
> -			*sccl_id = aff2 >> 3;
> -		if (ccl_id)
> -			*ccl_id = aff2 & 0x7;
> +		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);
> +		}
>  	} else {
>  		if (sccl_id)
>  			*sccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
> -- 
> 2.7.4
> 

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