From: "liuqi (BA)" <liuqi115@huawei.com> To: John Garry <john.garry@huawei.com>, <will@kernel.org>, <mark.rutland@arm.com>, <acme@kernel.org> Cc: <linux-perf-users@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linuxarm@huawei.com> Subject: Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU Date: Tue, 8 Mar 2022 17:32:09 +0800 [thread overview] Message-ID: <9a5db958-2fef-ff5e-c83f-394039e5bd0e@huawei.com> (raw) In-Reply-To: <816e5d86-9d7d-5762-d1d7-afcd0ee34af3@huawei.com> Hi John, On 2022/3/8 1:50, John Garry wrote: > >>>> + >>>> +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? > >> PM is enabled by default. yes, we could disable it in probe function and restore in remove function, I'll change this in next version. Thanks, Qi >>>> +{ >>>> + 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 > .
WARNING: multiple messages have this Message-ID (diff)
From: "liuqi (BA)" <liuqi115@huawei.com> To: John Garry <john.garry@huawei.com>, <will@kernel.org>, <mark.rutland@arm.com>, <acme@kernel.org> Cc: <linux-perf-users@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linuxarm@huawei.com> Subject: Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU Date: Tue, 8 Mar 2022 17:32:09 +0800 [thread overview] Message-ID: <9a5db958-2fef-ff5e-c83f-394039e5bd0e@huawei.com> (raw) In-Reply-To: <816e5d86-9d7d-5762-d1d7-afcd0ee34af3@huawei.com> Hi John, On 2022/3/8 1:50, John Garry wrote: > >>>> + >>>> +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? > >> PM is enabled by default. yes, we could disable it in probe function and restore in remove function, I'll change this in next version. Thanks, Qi >>>> +{ >>>> + 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 > . _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-03-08 9:32 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-24 11:11 [PATCH v2 0/2] Add Support for HiSilicon CPA PMU Qi Liu 2022-02-24 11:11 ` Qi Liu 2022-02-24 11:11 ` [PATCH v2 1/2] drivers/perf: hisi: Add Support for " Qi Liu 2022-02-24 11:11 ` Qi Liu 2022-02-28 9:56 ` John Garry 2022-02-28 9:56 ` John Garry 2022-03-03 9:02 ` liuqi (BA) 2022-03-03 9:02 ` liuqi (BA) 2022-03-07 17:50 ` John Garry 2022-03-07 17:50 ` John Garry 2022-03-08 9:32 ` liuqi (BA) [this message] 2022-03-08 9:32 ` liuqi (BA) 2022-02-24 11:11 ` [PATCH v2 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing Qi Liu 2022-02-24 11:11 ` Qi Liu 2022-02-25 18:34 ` Arnaldo Carvalho de Melo 2022-02-25 18:34 ` Arnaldo Carvalho de Melo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9a5db958-2fef-ff5e-c83f-394039e5bd0e@huawei.com \ --to=liuqi115@huawei.com \ --cc=acme@kernel.org \ --cc=john.garry@huawei.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-perf-users@vger.kernel.org \ --cc=linuxarm@huawei.com \ --cc=mark.rutland@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.