| Message ID | 20220321070124.41338-2-liuqi115@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Support for HiSilicon CPA PMU | expand |
On 21/03/2022 07:01, Qi Liu wrote: > If a PMU is in SICL, we associate it with all CPUs online, rather /s/in SICL/in a SICL/ [and all other places, including the code change], or maybe it is better mention "IO super cluster" as well for people who would not know or remember. > than CPUs in the nearest SCCL. You are not really saying why. I would mention that we do it as it is not appropiate to associate with a CPU die, and you can also mention problems that associating with a SICL creates. Please also mention how changing FW definition is ok at this stage. > > Signed-off-by: Qi Liu <liuqi115@huawei.com> > --- > drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++----------- > drivers/perf/hisilicon/hisi_uncore_pmu.c | 4 ++++ > drivers/perf/hisilicon/hisi_uncore_pmu.h | 1 + > 3 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c > index bad99d149172..54c604c0d404 100644 > --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c > +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c > @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev, > struct hisi_pmu *pa_pmu) > { > /* > - * Use the SCCL_ID and the index ID to identify the PA PMU, > - * while SCCL_ID is the nearst SCCL_ID from this SICL and > - * CPU core is chosen from this SCCL to manage this PMU. > + * As PA PMU is in SICL, use the SICL_ID and the index ID "a SICL" > + * to identify the PA PMU. > */ > if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id", > - &pa_pmu->sccl_id)) { > - dev_err(&pdev->dev, "Cannot read sccl-id!\n"); > + &pa_pmu->sicl_id)) { > + dev_err(&pdev->dev, "Cannot read sicl-id!\n"); > return -EINVAL; > } > > @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev, > } > > pa_pmu->ccl_id = -1; > + pa_pmu->sccl_id = -1; > > pa_pmu->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(pa_pmu->base)) { > @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev) > ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu); > if (ret) > return ret; > - /* > - * PA is attached in SICL and the CPU core is chosen to manage this > - * PMU which is the nearest SCCL, while its SCCL_ID is greater than > - * one with the SICL_ID. > - */ > + > name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u", > - pa_pmu->sccl_id - 1, pa_pmu->index_id); > + pa_pmu->sicl_id, pa_pmu->index_id); > if (!name) > return -ENOMEM; > > diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c > index a738aeab5c04..31930166c34b 100644 > --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c > +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c > @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu) > { > int sccl_id, ccl_id; > > + /* If SCCL_ID is -1, the PMU is in SICL and can be associated this PMU with any CPU */ too long, just have "The PMU is in a SICL and has no CPU affinity" > + if (hisi_pmu->sccl_id == -1) > + return true; > + > if (hisi_pmu->ccl_id == -1) { > /* If CCL_ID is -1, the PMU only shares the same SCCL */ > hisi_read_sccl_and_ccl_id(&sccl_id, NULL); > diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h > index 7f5841d6f592..96eeddad55ff 100644 > --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h > +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h > @@ -81,6 +81,7 @@ struct hisi_pmu { > struct device *dev; > struct hlist_node node; > int sccl_id; > + int sicl_id; > int ccl_id; > void __iomem *base; > /* the ID of the PMU modules */
Hi John, Thanks for the comments. some replies inline. On 2022/4/5 16:18, John Garry wrote: > On 21/03/2022 07:01, Qi Liu wrote: >> If a PMU is in SICL, we associate it with all CPUs online, rather > > /s/in SICL/in a SICL/ [and all other places, including the code change], > or maybe it is better mention "IO super cluster" as well for people who > would not know or remember. > got it, will change these description, thanks. >> than CPUs in the nearest SCCL. > > You are not really saying why. I would mention that we do it as it is > not appropiate to associate with a CPU die, and you can also mention > problems that associating with a SICL creates. > > Please also mention how changing FW definition is ok at this stage. ok, I'll metion this next time. > >> >> Signed-off-by: Qi Liu <liuqi115@huawei.com> >> --- >> drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++----------- >> drivers/perf/hisilicon/hisi_uncore_pmu.c | 4 ++++ >> drivers/perf/hisilicon/hisi_uncore_pmu.h | 1 + >> 3 files changed, 12 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c >> b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c >> index bad99d149172..54c604c0d404 100644 >> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c >> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c >> @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct >> platform_device *pdev, >> struct hisi_pmu *pa_pmu) >> { >> /* >> - * Use the SCCL_ID and the index ID to identify the PA PMU, >> - * while SCCL_ID is the nearst SCCL_ID from this SICL and >> - * CPU core is chosen from this SCCL to manage this PMU. >> + * As PA PMU is in SICL, use the SICL_ID and the index ID > > "a SICL" got it. > >> + * to identify the PA PMU. >> */ >> if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id", >> - &pa_pmu->sccl_id)) { >> - dev_err(&pdev->dev, "Cannot read sccl-id!\n"); >> + &pa_pmu->sicl_id)) { >> + dev_err(&pdev->dev, "Cannot read sicl-id!\n"); >> return -EINVAL; >> } >> @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct >> platform_device *pdev, >> } >> pa_pmu->ccl_id = -1; >> + pa_pmu->sccl_id = -1; >> pa_pmu->base = devm_platform_ioremap_resource(pdev, 0); >> if (IS_ERR(pa_pmu->base)) { >> @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct >> platform_device *pdev) >> ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu); >> if (ret) >> return ret; >> - /* >> - * PA is attached in SICL and the CPU core is chosen to manage this >> - * PMU which is the nearest SCCL, while its SCCL_ID is greater than >> - * one with the SICL_ID. >> - */ >> + >> name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u", >> - pa_pmu->sccl_id - 1, pa_pmu->index_id); >> + pa_pmu->sicl_id, pa_pmu->index_id); >> if (!name) >> return -ENOMEM; >> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c >> b/drivers/perf/hisilicon/hisi_uncore_pmu.c >> index a738aeab5c04..31930166c34b 100644 >> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c >> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c >> @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct >> hisi_pmu *hisi_pmu) >> { >> int sccl_id, ccl_id; >> + /* If SCCL_ID is -1, the PMU is in SICL and can be associated >> this PMU with any CPU */ > > too long, just have "The PMU is in a SICL and has no CPU affinity" ok, will fix this. Thanks, Qi > >> + if (hisi_pmu->sccl_id == -1) >> + return true; >> + >> if (hisi_pmu->ccl_id == -1) { >> /* If CCL_ID is -1, the PMU only shares the same SCCL */ >> hisi_read_sccl_and_ccl_id(&sccl_id, NULL); >> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h >> b/drivers/perf/hisilicon/hisi_uncore_pmu.h >> index 7f5841d6f592..96eeddad55ff 100644 >> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h >> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h >> @@ -81,6 +81,7 @@ struct hisi_pmu { >> struct device *dev; >> struct hlist_node node; >> int sccl_id; >> + int sicl_id; >> int ccl_id; >> void __iomem *base; >> /* the ID of the PMU modules */ > > .
diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c index bad99d149172..54c604c0d404 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev, struct hisi_pmu *pa_pmu) { /* - * Use the SCCL_ID and the index ID to identify the PA PMU, - * while SCCL_ID is the nearst SCCL_ID from this SICL and - * CPU core is chosen from this SCCL to manage this PMU. + * As PA PMU is in SICL, use the SICL_ID and the index ID + * to identify the PA PMU. */ if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id", - &pa_pmu->sccl_id)) { - dev_err(&pdev->dev, "Cannot read sccl-id!\n"); + &pa_pmu->sicl_id)) { + dev_err(&pdev->dev, "Cannot read sicl-id!\n"); return -EINVAL; } @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev, } pa_pmu->ccl_id = -1; + pa_pmu->sccl_id = -1; pa_pmu->base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(pa_pmu->base)) { @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev) ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu); if (ret) return ret; - /* - * PA is attached in SICL and the CPU core is chosen to manage this - * PMU which is the nearest SCCL, while its SCCL_ID is greater than - * one with the SICL_ID. - */ + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u", - pa_pmu->sccl_id - 1, pa_pmu->index_id); + pa_pmu->sicl_id, pa_pmu->index_id); if (!name) return -ENOMEM; diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c index a738aeab5c04..31930166c34b 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu) { int sccl_id, ccl_id; + /* If SCCL_ID is -1, the PMU is in SICL and can be associated this PMU with any CPU */ + if (hisi_pmu->sccl_id == -1) + return true; + if (hisi_pmu->ccl_id == -1) { /* If CCL_ID is -1, the PMU only shares the same SCCL */ hisi_read_sccl_and_ccl_id(&sccl_id, NULL); diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h index 7f5841d6f592..96eeddad55ff 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h @@ -81,6 +81,7 @@ struct hisi_pmu { struct device *dev; struct hlist_node node; int sccl_id; + int sicl_id; int ccl_id; void __iomem *base; /* the ID of the PMU modules */
If a PMU is in SICL, we associate it with all CPUs online, rather than CPUs in the nearest SCCL. Signed-off-by: Qi Liu <liuqi115@huawei.com> --- drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++----------- drivers/perf/hisilicon/hisi_uncore_pmu.c | 4 ++++ drivers/perf/hisilicon/hisi_uncore_pmu.h | 1 + 3 files changed, 12 insertions(+), 11 deletions(-)