Re: [PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[liuqi (BA)]

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@xxxxxxxxxx>
> > ---
> >   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 */
>
> .