| Message ID | 1573113364-32531-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 8703317ae576c9bf3e07e5b97275e3f957e8d74b |
| Headers | show |
| Series | drivers/perf: hisi: update the sccl_id/ccl_id for certain HiSilicon platform | expand |
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 >
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
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 > . >
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.
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
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.
>>>>> 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 > . >
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
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. > > . >
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 > >> . >> > > > . >
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 > > . >
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 > > . >
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
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);
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(-)