drivers/perf: hisi: update the sccl_id/ccl_id when MT is supported

PATCH New Shaokun Zhang 2 participants 4 messages Jul 19, 2018 – Jul 24, 2018

Subsystems (2)

Arm PMU profiling and debugging HiSilicon PMU driver

lore

Share & tools

lore

Shaokun Zhang

author

2018-07-19 11:26 UTC #0 lore raw

PATCH 1/1 drivers/perf: hisi: update the sccl_id/ccl_id when MT is supported

MT bit in MPIDR_EL1 is now supported in certain HiSilicon platforms, so the mapping between sccl_id/ccl_id and affinity level needs to be updated from the generic encoding we originally used.

Cc John Garry <[email protected]>

Cc Will Deacon <[email protected]>

Cc Mark Rutland <[email protected]>

Signed-off-by Shaokun Zhang <[email protected]>

---
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/perf/hisilicon/hisi_uncore_pmu.c +8 -5

index 44df613..b3c5ae9 100644
@@ -350,19 +350,22 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
 
 /*
  * Read Super CPU cluster and CPU cluster ID from MPIDR_EL1.
- * If multi-threading is supported, 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]. If this changes in future, this shall be updated.
+ * 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
+ * is in MPIDR[Aff2] and CCL_ID is in MPIDR[Aff1]. If this changes in
+ * future, this shall be updated.
  */
 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 = MPIDR_AFFINITY_LEVEL(mpidr, 3);
+			*sccl_id = aff2 >> 3;
 		if (ccl_id)
-			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
+			*ccl_id = aff2 & 0x7;
 	} else {
 		if (sccl_id)
 			*sccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);

Signature · 1 line

2.7.4

Will Deacon

2018-07-23 14:42 UTC #1 lore raw

PATCH 1/1 drivers/perf: hisi: update the sccl_id/ccl_id when MT is supported

Quoted 23 lines · Shaokun Zhang

MT bit in MPIDR_EL1 is now supported in certain HiSilicon platforms, so the mapping between sccl_id/ccl_id and affinity level needs to be updated from the generic encoding we originally used.
Cc John Garry <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Signed-off-by Shaokun Zhang <[email protected]>
---
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/perf/hisilicon/hisi_uncore_pmu.c +0 -3
index 44df613..b3c5ae9 100644
@@ -350,19 +350,22 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
 
 /*
  * Read Super CPU cluster and CPU cluster ID from MPIDR_EL1.
- * If multi-threading is supported, 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]. If this changes in future, this shall be updated.

drivers/perf/hisilicon/hisi_uncore_pmu.c void hisi_uncore_pmu_disable(struct pmu *pmu) · line 362

Quoted 10 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c +4 -0

+ * 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
+ * is in MPIDR[Aff2] and CCL_ID is in MPIDR[Aff1]. If this changes in
+ * future, this shall be updated.
  */
 static void hisi_read_sccl_and_ccl_id(int *sccl_id, int *ccl_id)
 {
 	u64 mpidr = read_cpuid_mpidr();
 
 	if (mpidr & MPIDR_MT_BITMASK) {

So, to be clear, you're saying that the MT bit was not set in any previous SoC with this PMU, and therefore changing this logic doesn't introduce a functional regression?

Assuming that's the case, and you now have an SoC with MT set, then I don't think the comment above which says "If this changes in future, this shall be updated" can be correct, because changing the mapping will regress an older platform, won't it?

In my opinion, you'd be much better off describing the PMU topology explicitly in firmware tables.

Will

Zhangshaokun

author

2018-07-24 11:06 UTC #2 lore raw

PATCH 1/1 drivers/perf: hisi: update the sccl_id/ccl_id when MT is supported

Hi Will,

Quoted 33 lines · Will Deacon

Quoted 32 lines · Shaokun Zhang
MT bit in MPIDR_EL1 is now supported in certain HiSilicon platforms, so the mapping between sccl_id/ccl_id and affinity level needs to be updated from the generic encoding we originally used.
Cc John Garry <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Signed-off-by Shaokun Zhang <[email protected]>
---
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/perf/hisilicon/hisi_uncore_pmu.c +4 -3
index 44df613..b3c5ae9 100644
@@ -350,19 +350,22 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
 
 /*
  * Read Super CPU cluster and CPU cluster ID from MPIDR_EL1.
- * If multi-threading is supported, 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]. If this changes in future, this shall be updated.
+ * 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
+ * is in MPIDR[Aff2] and CCL_ID is in MPIDR[Aff1]. If this changes in
+ * future, this shall be updated.
  */
 static void hisi_read_sccl_and_ccl_id(int *sccl_id, int *ccl_id)
 {
 	u64 mpidr = read_cpuid_mpidr();
 

Quoted 6 lines

drivers/perf/hisilicon/hisi_uncore_pmu.c void hisi_uncore_pmu_disable(struct pmu *pmu) · line 362
Quoted 1 line
drivers/perf/hisilicon/hisi_uncore_pmu.c +0 -0
 	if (mpidr & MPIDR_MT_BITMASK) {
So, to be clear, you're saying that the MT bit was not set in any previous SoC with this PMU, and therefore changing this logic doesn't introduce a functional regression?

Yes, you are right. When the driver was developed, the MT bit was not supported and I was told that it may be set later in certain HiSilicon platform, therefore I checked this field in the code.
Now the MT bit is set in one platform, but the mapping between sccl_id/ccl_id and affinity level is different from the origin encoding and needs to be updated. Of course, there is no functional regression.

Quoted 5 lines

Assuming that's the case, and you now have an SoC with MT set, then I don't think the comment above which says "If this changes in future, this shall be updated" can be correct, because changing the mapping will regress an older platform, won't it?

Agree, at the beginning, I am not sure about it in future. I will take this information to the SoC team and drop this comment in next version.

Quoted 3 lines

In my opinion, you'd be much better off describing the PMU topology explicitly in firmware tables.

Sound good. In driver, we read sccl_id/ccl_id from MPIDR_EL1 and compare the value from firmware tables, if both are the same, the cpu is related to PMU. So we still need this function.
I shall pay more attention on the PMU topology for next SoC and make the code will not need to be changed in future.

Thanks,
Shaokun

Quoted 4 lines

Will
.

Will Deacon

2018-07-24 14:39 UTC #3 lore raw

PATCH 1/1 drivers/perf: hisi: update the sccl_id/ccl_id when MT is supported

Quoted 48 lines · Zhangshaokun

Quoted 33 lines · Will Deacon
Quoted 32 lines · Shaokun Zhang
MT bit in MPIDR_EL1 is now supported in certain HiSilicon platforms, so the mapping between sccl_id/ccl_id and affinity level needs to be updated from the generic encoding we originally used.
Cc John Garry <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Signed-off-by Shaokun Zhang <[email protected]>
---
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/perf/hisilicon/hisi_uncore_pmu.c +4 -3
index 44df613..b3c5ae9 100644
@@ -350,19 +350,22 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
 
 /*
  * Read Super CPU cluster and CPU cluster ID from MPIDR_EL1.
- * If multi-threading is supported, 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]. If this changes in future, this shall be updated.
+ * 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
+ * is in MPIDR[Aff2] and CCL_ID is in MPIDR[Aff1]. If this changes in
+ * future, this shall be updated.
  */
 static void hisi_read_sccl_and_ccl_id(int *sccl_id, int *ccl_id)
 {
 	u64 mpidr = read_cpuid_mpidr();
 
Quoted 6 lines
drivers/perf/hisilicon/hisi_uncore_pmu.c void hisi_uncore_pmu_disable(struct pmu *pmu) · line 362
Quoted 1 line
drivers/perf/hisilicon/hisi_uncore_pmu.c +0 -0
 	if (mpidr & MPIDR_MT_BITMASK) {
So, to be clear, you're saying that the MT bit was not set in any previous SoC with this PMU, and therefore changing this logic doesn't introduce a functional regression?
Yes, you are right. When the driver was developed, the MT bit was not supported and I was told that it may be set later in certain HiSilicon platform, therefore I checked this field in the code.
Now the MT bit is set in one platform, but the mapping between sccl_id/ccl_id and affinity level is different from the origin encoding and needs to be updated. Of course, there is no functional regression.

Quoted 8 lines

Quoted 5 lines
Assuming that's the case, and you now have an SoC with MT set, then I don't think the comment above which says "If this changes in future, this shall be updated" can be correct, because changing the mapping will regress an older platform, won't it?
Agree, at the beginning, I am not sure about it in future. I will take this information to the SoC team and drop this comment in next version.

It's ok, I can drop the comment when I apply the patch.

Thanks,

Will