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

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

[Shaokun Zhang]

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].
  */
 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

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

[Mark Rutland]

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

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

[Will Deacon]

Hi,

On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
> @@ -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].
>   */
>  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);
> +		}

[I prefer Mark's version, so please reply to indicate whether or not it
 works for you]

So I'll take this, but the lesson here seems to be that it's a terrible idea
to infer system topology from CPU ID registers. In future, I'm going to
insist that this comes from firmware tables because hacks like the above are
not sustainable.

Will

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

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

[John Garry]

On 07/11/2019 11:40, Will Deacon wrote:
> Hi,
> 
> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>> @@ -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].
>>    */
>>   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);
>> +		}
> 
> [I prefer Mark's version, so please reply to indicate whether or not it
>   works for you]

Replying on Shaokun's behalf as he appears offline now.

In response to "> If TSV110 is always MT, ":

It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes 
TaishanV110 aka TSV110: one has the MT bit set and the other without.

We did ask for this not to be changed...

> 
> So I'll take this, but the lesson here seems to be that it's a terrible idea
> to infer system topology from CPU ID registers. In future, I'm going to
> insist that this comes from firmware tables because hacks like the above are
> not sustainable.
> 

Hopefully it will not change again, but maybe we can use PPTT instead.

Thanks,
John

> Will
> .
> 


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

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

[Mark Rutland]

On Thu, Nov 07, 2019 at 11:50:30AM +0000, John Garry wrote:
> On 07/11/2019 11:40, Will Deacon wrote:
> > Hi,
> > 
> > On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
> > > @@ -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].
> > >    */
> > >   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);
> > > +		}
> > 
> > [I prefer Mark's version, so please reply to indicate whether or not it
> >   works for you]
> 
> Replying on Shaokun's behalf as he appears offline now.
> 
> In response to "> If TSV110 is always MT, ":
> 
> It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
> TaishanV110 aka TSV110: one has the MT bit set and the other without.

Just to check, for the non-MT variant is the SCCL/CCL assignment
Aff2/Aff1 as with other non-MT parts?

Thanks,
Mark.

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

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

[John Garry]

On 07/11/2019 11:56, Mark Rutland wrote:
> On Thu, Nov 07, 2019 at 11:50:30AM +0000, John Garry wrote:
>> On 07/11/2019 11:40, Will Deacon wrote:
>>> Hi,
>>>
>>> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>>>> @@ -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].
>>>>     */
>>>>    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);
>>>> +		}
>>>
>>> [I prefer Mark's version, so please reply to indicate whether or not it
>>>    works for you]
>>
>> Replying on Shaokun's behalf as he appears offline now.
>>
>> In response to "> If TSV110 is always MT, ":
>>
>> It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
>> TaishanV110 aka TSV110: one has the MT bit set and the other without.
> 
> Just to check, for the non-MT variant is the SCCL/CCL assignment
> Aff2/Aff1 as with other non-MT parts?

We don't support any other non-MT parts for this driver.

Thanks,
John

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

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

[Mark Rutland]

On Thu, Nov 07, 2019 at 12:06:24PM +0000, John Garry wrote:
> On 07/11/2019 11:56, Mark Rutland wrote:
> > On Thu, Nov 07, 2019 at 11:50:30AM +0000, John Garry wrote:
> > > On 07/11/2019 11:40, Will Deacon wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
> > > > > @@ -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].
> > > > >     */
> > > > >    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);
> > > > > +		}
> > > > 
> > > > [I prefer Mark's version, so please reply to indicate whether or not it
> > > >    works for you]
> > > 
> > > Replying on Shaokun's behalf as he appears offline now.
> > > 
> > > In response to "> If TSV110 is always MT, ":
> > > 
> > > It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
> > > TaishanV110 aka TSV110: one has the MT bit set and the other without.
> > 
> > Just to check, for the non-MT variant is the SCCL/CCL assignment
> > Aff2/Aff1 as with other non-MT parts?
> 
> We don't support any other non-MT parts for this driver.

The driver claimed to support non-MT parts before TSV110 came around, so that
statement confuses me.

For a non-MT TSV110, is Aff2 the SCCL and Aff1 the CCL? That's what the
existing code (and Shaokun's patch) assumed.

Assuming that is the case, I'd suggest we have the following:

/*
 * 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):
 *   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 *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);
	bool mt = mpdir & 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 {
		sccl = aff2;
		ccl = aff1;
	}

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

Thanks,
Mark.

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

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

[John Garry]

>>>>> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>>>>>> @@ -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].
>>>>>>      */
>>>>>>     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);
>>>>>> +		}
>>>>>
>>>>> [I prefer Mark's version, so please reply to indicate whether or not it
>>>>>     works for you]
>>>>
>>>> Replying on Shaokun's behalf as he appears offline now.
>>>>
>>>> In response to "> If TSV110 is always MT, ":
>>>>
>>>> It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
>>>> TaishanV110 aka TSV110: one has the MT bit set and the other without.
>>>
>>> Just to check, for the non-MT variant is the SCCL/CCL assignment
>>> Aff2/Aff1 as with other non-MT parts?
>>
>> We don't support any other non-MT parts for this driver.
> 
> The driver claimed to support non-MT parts before TSV110 came around, so that
> statement confuses me.

A couple of points on this:

- We gave up on upstreaming support in this driver for the predecessor 
SoC, which included an A72. You may remember the infamous djtag bus.

- The wording in the comment "If multi-threading is supported, On Huawei 
Kunpeng 920 SoC " is misleading, as it implies that the part found in 
Huawei Kunpeng 920 is MT, which it isn't always.

> 
> For a non-MT TSV110, is Aff2 the SCCL and Aff1 the CCL? 

Yes,

That's what the
> existing code (and Shaokun's patch) assumed.

well I'm going with that as well. Shaokun can confirm the layout.

> 
> Assuming that is the case, I'd suggest we have the following:
> 
> /*
>   * 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):

Again, this implies that the part found in Kunpeng 920 is MT, which it 
isn't always.

BTW, As I understand, the MIDR variant bits differ between the 2 revs of 
TSV110, so maybe that is a better method to differentiate, but I don't 
see an API exported for this.

>   *   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 *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);
> 	bool mt = mpdir & 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 {
> 		sccl = aff2;
> 		ccl = aff1;
> 	}
> 
> 	if (scclp)
> 		*scclp = sccl;
> 	if (cclp)
> 		*cclp = ccl;
> }
> 
> Thanks,
> Mark.

Thanks,
John

> .
> 


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

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

[Will Deacon]

On Thu, Nov 07, 2019 at 01:04:34PM +0000, John Garry wrote:
> > > > > > On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
> > > > > > [I prefer Mark's version, so please reply to indicate whether or not it
> > > > > >     works for you]
> > > > > 
> > > > > Replying on Shaokun's behalf as he appears offline now.
> > > > > 
> > > > > In response to "> If TSV110 is always MT, ":
> > > > > 
> > > > > It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
> > > > > TaishanV110 aka TSV110: one has the MT bit set and the other without.
> > > > 
> > > > Just to check, for the non-MT variant is the SCCL/CCL assignment
> > > > Aff2/Aff1 as with other non-MT parts?
> > > 
> > > We don't support any other non-MT parts for this driver.
> > 
> > The driver claimed to support non-MT parts before TSV110 came around, so that
> > statement confuses me.
> 
> A couple of points on this:
> 
> - We gave up on upstreaming support in this driver for the predecessor SoC,
> which included an A72. You may remember the infamous djtag bus.
> 
> - The wording in the comment "If multi-threading is supported, On Huawei
> Kunpeng 920 SoC " is misleading, as it implies that the part found in Huawei
> Kunpeng 920 is MT, which it isn't always.
> 
> > 
> > For a non-MT TSV110, is Aff2 the SCCL and Aff1 the CCL?
> 
> Yes,
> 
> That's what the
> > existing code (and Shaokun's patch) assumed.
> 
> well I'm going with that as well. Shaokun can confirm the layout.

I'll queue Shaokun's patch for now, since I want to get this to Catalin
tomorrow for 5.5 after spending the night in -next. We can always simplify
the logic later if Mark's patch ends up working out.

Thanks,

Will

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

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

[Shaokun Zhang]

Hi Mark,

On 2019/11/7 20:11, Mark Rutland wrote:
> On Thu, Nov 07, 2019 at 12:06:24PM +0000, John Garry wrote:
>> On 07/11/2019 11:56, Mark Rutland wrote:
>>> On Thu, Nov 07, 2019 at 11:50:30AM +0000, John Garry wrote:
>>>> On 07/11/2019 11:40, Will Deacon wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>>>>>> @@ -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].
>>>>>>     */
>>>>>>    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);
>>>>>> +		}
>>>>>
>>>>> [I prefer Mark's version, so please reply to indicate whether or not it
>>>>>    works for you]
>>>>
>>>> Replying on Shaokun's behalf as he appears offline now.
>>>>
>>>> In response to "> If TSV110 is always MT, ":
>>>>
>>>> It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
>>>> TaishanV110 aka TSV110: one has the MT bit set and the other without.
>>>
>>> Just to check, for the non-MT variant is the SCCL/CCL assignment
>>> Aff2/Aff1 as with other non-MT parts?
>>
>> We don't support any other non-MT parts for this driver.
> 
> The driver claimed to support non-MT parts before TSV110 came around, so that
> statement confuses me.
> 

Apologies that I reply a little later because of stepout and other things, I was
not online.

My description is a little obscure so the comment is really confused:
Under the condition that MT field is set, TSV110 core on Kunpeng 920:
SCCL is Aff2[7:3], CCL is Aff2[2:0];

If MT field is not, TSV110 core on Kunpeng 920:
SCCL is Aff2[7:0], CCL is Aff1[7:0]
And as John said that "We don't support any other non-MT parts for this driver."

> For a non-MT TSV110, is Aff2 the SCCL and Aff1 the CCL? That's what the

Right.

> existing code (and Shaokun's patch) assumed.
> 
> Assuming that is the case, I'd suggest we have the following:
> 
> /*
>  * 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):
>  *   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 *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);
> 	bool mt = mpdir & 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 {
> 		sccl = aff2;
> 		ccl = aff1;
> 	}
> 
> 	if (scclp)
> 		*scclp = sccl;
> 	if (cclp)
> 		*cclp = ccl;
> }
> 

It works and Thanks your nice comment.

> Thanks,
> Mark.
> 
> .
> 


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

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

[Shaokun Zhang]

Hi John,

On 2019/11/7 21:04, John Garry wrote:
> 
>>>>>> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>>>>>>> @@ -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].
>>>>>>>      */
>>>>>>>     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);
>>>>>>> +        }
>>>>>>
>>>>>> [I prefer Mark's version, so please reply to indicate whether or not it
>>>>>>     works for you]
>>>>>
>>>>> Replying on Shaokun's behalf as he appears offline now.
>>>>>
>>>>> In response to "> If TSV110 is always MT, ":
>>>>>
>>>>> It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
>>>>> TaishanV110 aka TSV110: one has the MT bit set and the other without.
>>>>
>>>> Just to check, for the non-MT variant is the SCCL/CCL assignment
>>>> Aff2/Aff1 as with other non-MT parts?
>>>
>>> We don't support any other non-MT parts for this driver.
>>
>> The driver claimed to support non-MT parts before TSV110 came around, so that
>> statement confuses me.
> 
> A couple of points on this:
> 
> - We gave up on upstreaming support in this driver for the predecessor SoC, which included an A72. You may remember the infamous djtag bus.
> 
> - The wording in the comment "If multi-threading is supported, On Huawei Kunpeng 920 SoC " is misleading, as it implies that the part found in Huawei Kunpeng 920 is MT, which it isn't always.
> 
>>
>> For a non-MT TSV110, is Aff2 the SCCL and Aff1 the CCL? 
> 
> Yes,
> 
> That's what the
>> existing code (and Shaokun's patch) assumed.
> 
> well I'm going with that as well. Shaokun can confirm the layout.
> 

Right, it works.

>>
>> Assuming that is the case, I'd suggest we have the following:
>>
>> /*
>>   * 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):
> 
> Again, this implies that the part found in Kunpeng 920 is MT, which it isn't always.
> 
> BTW, As I understand, the MIDR variant bits differ between the 2 revs of TSV110, so maybe that is a better method to differentiate, but I don't see an API exported for this.
> 

Yes, the CPU variant is different.

Thanks,
Shaokun

>>   *   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 *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);
>>     bool mt = mpdir & 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 {
>>         sccl = aff2;
>>         ccl = aff1;
>>     }
>>
>>     if (scclp)
>>         *scclp = sccl;
>>     if (cclp)
>>         *cclp = ccl;
>> }
>>
>> Thanks,
>> Mark.
> 
> Thanks,
> John
> 
>> .
>>
> 
> 
> .
> 


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

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

[Shaokun Zhang]

Hi Will,

On 2019/11/7 21:09, Will Deacon wrote:
> On Thu, Nov 07, 2019 at 01:04:34PM +0000, John Garry wrote:
>>>>>>> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>>>>>>> [I prefer Mark's version, so please reply to indicate whether or not it
>>>>>>>     works for you]
>>>>>>
>>>>>> Replying on Shaokun's behalf as he appears offline now.
>>>>>>
>>>>>> In response to "> If TSV110 is always MT, ":
>>>>>>
>>>>>> It isn't. There are 2 spins of Huawei Kunpeng 920 SoC which includes
>>>>>> TaishanV110 aka TSV110: one has the MT bit set and the other without.
>>>>>
>>>>> Just to check, for the non-MT variant is the SCCL/CCL assignment
>>>>> Aff2/Aff1 as with other non-MT parts?
>>>>
>>>> We don't support any other non-MT parts for this driver.
>>>
>>> The driver claimed to support non-MT parts before TSV110 came around, so that
>>> statement confuses me.
>>
>> A couple of points on this:
>>
>> - We gave up on upstreaming support in this driver for the predecessor SoC,
>> which included an A72. You may remember the infamous djtag bus.
>>
>> - The wording in the comment "If multi-threading is supported, On Huawei
>> Kunpeng 920 SoC " is misleading, as it implies that the part found in Huawei
>> Kunpeng 920 is MT, which it isn't always.
>>
>>>
>>> For a non-MT TSV110, is Aff2 the SCCL and Aff1 the CCL?
>>
>> Yes,
>>
>> That's what the
>>> existing code (and Shaokun's patch) assumed.
>>
>> well I'm going with that as well. Shaokun can confirm the layout.
> 
> I'll queue Shaokun's patch for now, since I want to get this to Catalin

Thanks Will.

> tomorrow for 5.5 after spending the night in -next. We can always simplify
> the logic later if Mark's patch ends up working out.

I have checked that it has been in your for-next/perf branch. I will simplify
it later as Mark's suggestion.
Or if it is permitted, I can post v2 and simplify this.

Thanks,
Shaokun

> 
> Thanks,
> 
> Will
> 
> .
> 


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

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

[Shaokun Zhang]

Hi Will,

On 2019/11/7 19:40, Will Deacon wrote:
> Hi,
> 
> On Thu, Nov 07, 2019 at 03:56:04PM +0800, Shaokun Zhang wrote:
>> @@ -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].
>>   */
>>  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);
>> +		}
> 
> [I prefer Mark's version, so please reply to indicate whether or not it
>  works for you]
> 
> So I'll take this, but the lesson here seems to be that it's a terrible idea
> to infer system topology from CPU ID registers. In future, I'm going to
> insist that this comes from firmware tables because hacks like the above are

My bad, I shall do it in future.

Thanks,
Shaokun

> not sustainable.
> 
> Will
> 
> .
> 


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

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

[Will Deacon]

On Fri, Nov 08, 2019 at 09:25:29AM +0800, Shaokun Zhang wrote:
> On 2019/11/7 21:09, Will Deacon wrote:
> > I'll queue Shaokun's patch for now, since I want to get this to Catalin
> 
> Thanks Will.
> 
> > tomorrow for 5.5 after spending the night in -next. We can always simplify
> > the logic later if Mark's patch ends up working out.
> 
> I have checked that it has been in your for-next/perf branch. I will simplify
> it later as Mark's suggestion.
> Or if it is permitted, I can post v2 and simplify this.

Please just send a patch on top, since I don't want to rebase that branch
now.

Cheers,

Will

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

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

[Shaokun Zhang]

hisi_read_sccl_and_ccl_id is not readable 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):
+ *   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;
 }
 
 /*
-- 
2.7.4


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

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

[John Garry]

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.

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

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

[Shaokun Zhang]

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