Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[John Garry]

> > > +
> > > +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
> > > +{
> > > +    u32 val;
> > > +
> > > +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> > > +    val &= ~(CPA_PERF_CTRL_EN);
> > > +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> > > +}
> > > +
> > > +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
> >
> > this seems unique for this new driver - why do we need to disable PM?
> CPA PMU doesn't work under power mangement state, so we need to disable 
> PM before using CPA PMU.

OK, so you need to disable something related to PM to use the HW. But 
why do it when you enable the counter? I mean, can we just do it once 
when we probe the HW? Why is it even enabled at all?

> > > +{
> > > +    u32 val;
> > > +
> > > +    val = readl(cpa_pmu->base + CPA_CFG_REG);
> > > +    val |= CPA_PM_CTRL;
> > > +    writel(val, cpa_pmu->base + CPA_CFG_REG);
> > > +}
> > > +
> > > +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
> > > +{
> > > +    u32 val;
> > > +
> > > +    val = readl(cpa_pmu->base + CPA_CFG_REG);
> > > +    val &= ~CPA_PM_CTRL;
> >
> > nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so 
> > please be consistent
> ]

...

> > > +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
> > > +                  struct hisi_pmu *cpa_pmu)
> > > +{
> > > +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> > > +                     &cpa_pmu->sccl_id)) {
> > > +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
> >
> > strange that the FW uses "scl-id" but driver uses sccl_id (I am 
> > talking about sccl vs scl difference)
> yes... these two names means the same thing, maybe could use a cleanup 
> patch to keep them consistent.
>
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    cpa_pmu->ccl_id = -1;
> > > +
> > > +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
> > > +                     &cpa_pmu->index_id)) {
> > > +        dev_err(&pdev->dev, "Cannot read idx-id\n");
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
> > > +    if (IS_ERR(cpa_pmu->base))
> > > +        return PTR_ERR(cpa_pmu->base);
> > > +
> > > +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
> > > +
> > > +    return 0;
> > > +}
> > > +

...

> > > +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
> > > +{
> > > +    struct hisi_pmu *cpa_pmu;
> > > +    char *name;
> > > +    int ret;
> > > +
> > > +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
> > > +    if (!cpa_pmu)
> > > +        return -ENOMEM;
> > > +
> > > +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
> > > +    if (ret)
> > > +        return ret;
> > > +
> > > +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> > > +                       &cpa_pmu->node);
> > > +    if (ret) {
> > > +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> > > +        return ret;
> > > +    }
> > > +
> > > +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
> > > "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
> >
> > sorry, but I still don't like this "- 1". From checking the chat on 
> > v1, my impression was that you agreed with me on this one.
>
> sorry, maybe I didn't express it clearly.
> CPA PMU is on IO die,

Then it is quite strange to provide the sccl id and not some sicl id 
(since it is in the IO die)

 but BIOS give CPA PMU driver the sccl-id of
> adjacent CPU die (as we need this sccl-id to pass 
> hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).


Is the PMU only ever used by one SCCL? Or can more than one SCCL access? 
If so, is there a special affinity to one particular SCCL?

> so, when naming CPA PMU, we use the sccl-id supported by BIOS to get the 
> sicl-id of IO die which CPA PMU located in, that is: cpa_pmu->sccl_id - 1.
>
> > > +                  cpa_pmu->index_id);

Thanks,
John