Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
On 15/07/2020 09:30, Will Deacon wrote: Hi John, On Wed, Jul 15, 2020 at 09:23:19AM +0100, John Garry wrote: On Wed, Jun 17, 2020 at 09:05:11PM +0800, John Garry wrote: To allow userspace to identify the specific implementation of the device, add an "identifier" sysfs file. Encoding is as follows: hi1620: 0x0 (aka hip08) hi1630: 0x30 Signed-off-by: John Garry I'm struggling a bit to track this. If you still think it's worth pursuing, please could you post a series with a cover-letter describing what this is for, a link to the userspace changes and then patches for all the PMU drivers that need updating? There is no hi1630 userspace support yet. So what I can do is post updated userspace support (including hi1630), and then post kernel parts together for all drivers we could initially support. @Joakim, I'll pick your imx driver changes here, if you don't mind. There was an RFC from you for the SMMUv3 PMU as well, and also some other "arm64" changes. I hope to drop that RFC if an updated SMMUv3 spec helps us out. Released SMMUv3.3 spec provides a PMCG_IIDR reg, which we can now use to identify the implementation: https://developer.arm.com/documentation/ihi0070/da/?lang=en So I'll look to put the userspace and kernel parts together for this now for review. Thanks, John
Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
Hi Will, On Wed, Jun 17, 2020 at 09:05:11PM +0800, John Garry wrote: To allow userspace to identify the specific implementation of the device, add an "identifier" sysfs file. Encoding is as follows: hi1620: 0x0 (aka hip08) hi1630: 0x30 Signed-off-by: John Garry I'm struggling a bit to track this. If you still think it's worth pursuing, please could you post a series with a cover-letter describing what this is for, a link to the userspace changes and then patches for all the PMU drivers that need updating? There is no hi1630 userspace support yet. So what I can do is post updated userspace support (including hi1630), and then post kernel parts together for all drivers we could initially support. @Joakim, I'll pick your imx driver changes here, if you don't mind. There was an RFC from you for the SMMUv3 PMU as well, and also some other "arm64" changes. I hope to drop that RFC if an updated SMMUv3 spec helps us out. Cheers, John
RE: [PATCH] drivers/perf: hisi: Add identifier sysfs file
> -Original Message- > From: John Garry > Sent: 2020年7月15日 16:23 > To: Will Deacon > Cc: zhangshao...@hisilicon.com; mark.rutl...@arm.com; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Joakim > Zhang ; jo...@redhat.com; > linux...@huawei.com > Subject: Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file > > Hi Will, > > > > > On Wed, Jun 17, 2020 at 09:05:11PM +0800, John Garry wrote: > >> To allow userspace to identify the specific implementation of the > >> device, add an "identifier" sysfs file. > >> > >> Encoding is as follows: > >> hi1620: 0x0(aka hip08) > >> hi1630: 0x30 > >> > >> Signed-off-by: John Garry > > > > I'm struggling a bit to track this. If you still think it's worth > > pursuing, please could you post a series with a cover-letter > > describing what this is for, a link to the userspace changes and then > > patches for all the PMU drivers that need updating? > > There is no hi1630 userspace support yet. > > So what I can do is post updated userspace support (including hi1630), and > then post kernel parts together for all drivers we could initially support. > > @Joakim, I'll pick your imx driver changes here, if you don't mind. Of course you can. Best Regards, Joakim Zhang > There was an RFC from you for the SMMUv3 PMU as > > well, and also some other "arm64" changes. > > > > I hope to drop that RFC if an updated SMMUv3 spec helps us out. > > Cheers, > John
Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
Hi John, On Wed, Jul 15, 2020 at 09:23:19AM +0100, John Garry wrote: > > On Wed, Jun 17, 2020 at 09:05:11PM +0800, John Garry wrote: > > > To allow userspace to identify the specific implementation of the device, > > > add an "identifier" sysfs file. > > > > > > Encoding is as follows: > > > hi1620: 0x0 (aka hip08) > > > hi1630: 0x30 > > > > > > Signed-off-by: John Garry > > > > I'm struggling a bit to track this. If you still think it's worth pursuing, > > please could you post a series with a cover-letter describing what this is > > for, a link to the userspace changes and then patches for all the PMU > > drivers that need updating? > > There is no hi1630 userspace support yet. > > So what I can do is post updated userspace support (including hi1630), and > then post kernel parts together for all drivers we could initially support. > > @Joakim, I'll pick your imx driver changes here, if you don't mind. > > There was an RFC from you for the SMMUv3 PMU as > > well, and also some other "arm64" changes. > > > > I hope to drop that RFC if an updated SMMUv3 spec helps us out. Thanks, John. That should be a tonne easier to review. Will
Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
Hi John, On Wed, Jun 17, 2020 at 09:05:11PM +0800, John Garry wrote: > To allow userspace to identify the specific implementation of the device, > add an "identifier" sysfs file. > > Encoding is as follows: > hi1620: 0x0 (aka hip08) > hi1630: 0x30 > > Signed-off-by: John Garry I'm struggling a bit to track this. If you still think it's worth pursuing, please could you post a series with a cover-letter describing what this is for, a link to the userspace changes and then patches for all the PMU drivers that need updating? There was an RFC from you for the SMMUv3 PMU as well, and also some other "arm64" changes. Cheers, Will
RE: [PATCH] drivers/perf: hisi: Add identifier sysfs file
> -Original Message- > From: John Garry > Sent: 2020年7月15日 16:23 > To: Will Deacon > Cc: zhangshao...@hisilicon.com; mark.rutl...@arm.com; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Joakim > Zhang ; jo...@redhat.com; > linux...@huawei.com > Subject: Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file > > Hi Will, > > > > > On Wed, Jun 17, 2020 at 09:05:11PM +0800, John Garry wrote: > >> To allow userspace to identify the specific implementation of the > >> device, add an "identifier" sysfs file. > >> > >> Encoding is as follows: > >> hi1620: 0x0(aka hip08) > >> hi1630: 0x30 > >> > >> Signed-off-by: John Garry > > > > I'm struggling a bit to track this. If you still think it's worth > > pursuing, please could you post a series with a cover-letter > > describing what this is for, a link to the userspace changes and then > > patches for all the PMU drivers that need updating? > > There is no hi1630 userspace support yet. > > So what I can do is post updated userspace support (including hi1630), and > then > post kernel parts together for all drivers we could initially support. > > @Joakim, I'll pick your imx driver changes here, if you don't mind. Hi John, Any update about adding system pmu support in perf tool? Will you continue upstreaming it? Feel free let me know if you need I do the test. Best Regards, Joakim Zhang
Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
Hi John,
Thanks your further explaination and I'm ok on it, so for this patch:
Acked-by: Shaokun Zhang
Thanks,
Shaokun
在 2020/6/18 17:18, John Garry 写道:
> On 18/06/2020 02:40, Shaokun Zhang wrote:
>>> }
>>> + hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION);
>> Since we are now refactoring the PMU framework, the PMU version offset
>> is always the same except DDRC PMU and other uncore PMU modules will
>> also use this, how about we do it as the common code:
>>
>> #define HISI_PMU_VERSION_REG 0x1CF0
>> int hisi_uncore_pmu_version(struct hisi_pmu *hisi_pmu)
>> {
>> return readl(hisi_pmu->base + HISI_PMU_VERSION_REG);
>> }
>> EXPORT_SYMBOL_GPL(hisi_uncore_pmu_version);
>
> Hi Shaokun,
>
> Some points to make:
>
> - It's hardly worth having a separate function to do this 1-line readl()
> call, especially since it not even generic (DDRC is different)
>
> - We would have to export it (or put in a common header file with static
> inline keywords) - less exports are good
>
> - with factoring out common code, it's good to reduce total code - this
> change would increase it, AFAICS
>
> - This is HW specific. The driver is currently layered such that all HW
> specific stuff is in the HW driver (like hisi_uncore_ddrc_pmu.c), and
> not library code (hisi_uncore_pmu.c). I don't see why you want to mix
> that, like you're proposing in the framework revision you proposed
> internally.
>
> Thanks,
> John
>
>>
>> hha_pmu->identifier = hisi_uncore_pmu_version(hha_pmu);
>> we can remove the duplicated PMU version register definition in each
>> module.
>>
>> Thanks,
>> Shaokun
>>
>>> +
>>> return 0;
>>> }
>>> @@ -320,10 +323,23 @@ static const struct attribute_group
>>> hisi_hha_pmu_cpumask_attr_group = {
>>> .attrs = hisi_hha_pmu_cpumask_attrs,
>>> };
>
>
> .
RE: [PATCH] drivers/perf: hisi: Add identifier sysfs file
Hi John,
Happy to know that there is a version register in hisilicon SoC.
John, Will, Mark, is there any conclusion for other SoCs which don't have such
version register, but still want to expose identifier to userspace? Thanks a
lot.
Best Regards,
Joakim Zhang
> -Original Message-
> From: John Garry
> Sent: 2020年6月17日 21:05
> To: zhangshao...@hisilicon.com; w...@kernel.org; mark.rutl...@arm.com
> Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Joakim
> Zhang ; jo...@redhat.com;
> linux...@huawei.com; John Garry
> Subject: [PATCH] drivers/perf: hisi: Add identifier sysfs file
>
> To allow userspace to identify the specific implementation of the device, add
> an
> "identifier" sysfs file.
>
> Encoding is as follows:
> hi1620: 0x0 (aka hip08)
> hi1630: 0x30
>
> Signed-off-by: John Garry
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> index 15713faaa07e..a83d99f2662e 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> @@ -33,6 +33,7 @@
> #define DDRC_INT_MASK0x6c8
> #define DDRC_INT_STATUS 0x6cc
> #define DDRC_INT_CLEAR 0x6d0
> +#define DDRC_VERSION 0x710
>
> /* DDRC has 8-counters */
> #define DDRC_NR_COUNTERS 0x8
> @@ -267,6 +268,8 @@ static int hisi_ddrc_pmu_init_data(struct
> platform_device *pdev,
> return PTR_ERR(ddrc_pmu->base);
> }
>
> + ddrc_pmu->identifier = readl(ddrc_pmu->base + DDRC_VERSION);
> +
> return 0;
> }
>
> @@ -308,10 +311,23 @@ static const struct attribute_group
> hisi_ddrc_pmu_cpumask_attr_group = {
> .attrs = hisi_ddrc_pmu_cpumask_attrs,
> };
>
> +static struct device_attribute hisi_ddrc_pmu_identifier_attr =
> + __ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_ddrc_pmu_identifier_attrs[] = {
> + &hisi_ddrc_pmu_identifier_attr.attr,
> + NULL
> +};
> +
> +static struct attribute_group hisi_ddrc_pmu_identifier_group = {
> + .attrs = hisi_ddrc_pmu_identifier_attrs, };
> +
> static const struct attribute_group *hisi_ddrc_pmu_attr_groups[] = {
> &hisi_ddrc_pmu_format_group,
> &hisi_ddrc_pmu_events_group,
> &hisi_ddrc_pmu_cpumask_attr_group,
> + &hisi_ddrc_pmu_identifier_group,
> NULL,
> };
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> index dcc5600788a9..4fdaf1d995be 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> @@ -23,6 +23,7 @@
> #define HHA_INT_MASK 0x0804
> #define HHA_INT_STATUS 0x0808
> #define HHA_INT_CLEAR0x080C
> +#define HHA_VERSION 0x1cf0
> #define HHA_PERF_CTRL0x1E00
> #define HHA_EVENT_CTRL 0x1E04
> #define HHA_EVENT_TYPE0 0x1E80
> @@ -261,6 +262,8 @@ static int hisi_hha_pmu_init_data(struct
> platform_device *pdev,
> return PTR_ERR(hha_pmu->base);
> }
>
> + hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION);
> +
> return 0;
> }
>
> @@ -320,10 +323,23 @@ static const struct attribute_group
> hisi_hha_pmu_cpumask_attr_group = {
> .attrs = hisi_hha_pmu_cpumask_attrs,
> };
>
> +static struct device_attribute hisi_hha_pmu_identifier_attr =
> + __ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_hha_pmu_identifier_attrs[] = {
> + &hisi_hha_pmu_identifier_attr.attr,
> + NULL
> +};
> +
> +static struct attribute_group hisi_hha_pmu_identifier_group = {
> + .attrs = hisi_hha_pmu_identifier_attrs, };
> +
> static const struct attribute_group *hisi_hha_pmu_attr_groups[] = {
> &hisi_hha_pmu_format_group,
> &hisi_hha_pmu_events_group,
> &hisi_hha_pmu_cpumask_attr_group,
> + &hisi_hha_pmu_identifier_group,
> NULL,
> };
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> index 7719ae4e2c56..0e7477220be1 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> @@ -25,6 +25,7 @@
> #define L3C_INT_STATUS 0x0808
> #define L3C_INT_CLEAR0x080c
> #define L3C_EVENT_CTRL 0x1c00
> +#define L3C_VERSION 0x1cf0
> #define L3C_EVENT_TYPE0 0x1d00
> /*
> * Each counter is 48-bits and [48:63] are reserved @@ -264,6 +265,8 @@
> static int hisi_l3c_pmu_init_data(struct platform_device *pdev,
> return PTR_ERR(l3c_pmu->base);
> }
>
> + l3c_pmu->identifier = readl(l3c_pmu->base + L3C_VERSION);
> +
> return 0;
> }
>
> @@ -310,10 +313,23 @@ static const struct attribute_group
> hisi_l3c_pmu_cpumask_attr_group = {
> .attrs = hisi_l3c_
Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
Hi John,
在 2020/6/17 21:05, John Garry 写道:
> To allow userspace to identify the specific implementation of the device,
> add an "identifier" sysfs file.
>
> Encoding is as follows:
> hi1620: 0x0 (aka hip08)
> hi1630: 0x30
>
> Signed-off-by: John Garry
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> index 15713faaa07e..a83d99f2662e 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> @@ -33,6 +33,7 @@
> #define DDRC_INT_MASK0x6c8
> #define DDRC_INT_STATUS 0x6cc
> #define DDRC_INT_CLEAR 0x6d0
> +#define DDRC_VERSION 0x710
>
> /* DDRC has 8-counters */
> #define DDRC_NR_COUNTERS 0x8
> @@ -267,6 +268,8 @@ static int hisi_ddrc_pmu_init_data(struct platform_device
> *pdev,
> return PTR_ERR(ddrc_pmu->base);
> }
>
> + ddrc_pmu->identifier = readl(ddrc_pmu->base + DDRC_VERSION);
> +
> return 0;
> }
>
> @@ -308,10 +311,23 @@ static const struct attribute_group
> hisi_ddrc_pmu_cpumask_attr_group = {
> .attrs = hisi_ddrc_pmu_cpumask_attrs,
> };
>
> +static struct device_attribute hisi_ddrc_pmu_identifier_attr =
> + __ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_ddrc_pmu_identifier_attrs[] = {
> + &hisi_ddrc_pmu_identifier_attr.attr,
> + NULL
> +};
> +
> +static struct attribute_group hisi_ddrc_pmu_identifier_group = {
> + .attrs = hisi_ddrc_pmu_identifier_attrs,
> +};
> +
> static const struct attribute_group *hisi_ddrc_pmu_attr_groups[] = {
> &hisi_ddrc_pmu_format_group,
> &hisi_ddrc_pmu_events_group,
> &hisi_ddrc_pmu_cpumask_attr_group,
> + &hisi_ddrc_pmu_identifier_group,
> NULL,
> };
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> index dcc5600788a9..4fdaf1d995be 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> @@ -23,6 +23,7 @@
> #define HHA_INT_MASK 0x0804
> #define HHA_INT_STATUS 0x0808
> #define HHA_INT_CLEAR0x080C
> +#define HHA_VERSION 0x1cf0
> #define HHA_PERF_CTRL0x1E00
> #define HHA_EVENT_CTRL 0x1E04
> #define HHA_EVENT_TYPE0 0x1E80
> @@ -261,6 +262,8 @@ static int hisi_hha_pmu_init_data(struct platform_device
> *pdev,
> return PTR_ERR(hha_pmu->base);
> }
>
> + hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION);
Since we are now refactoring the PMU framework, the PMU version offset
is always the same except DDRC PMU and other uncore PMU modules will
also use this, how about we do it as the common code:
#define HISI_PMU_VERSION_REG 0x1CF0
int hisi_uncore_pmu_version(struct hisi_pmu *hisi_pmu)
{
return readl(hisi_pmu->base + HISI_PMU_VERSION_REG);
}
EXPORT_SYMBOL_GPL(hisi_uncore_pmu_version);
hha_pmu->identifier = hisi_uncore_pmu_version(hha_pmu);
we can remove the duplicated PMU version register definition in each module.
Thanks,
Shaokun
> +
> return 0;
> }
>
> @@ -320,10 +323,23 @@ static const struct attribute_group
> hisi_hha_pmu_cpumask_attr_group = {
> .attrs = hisi_hha_pmu_cpumask_attrs,
> };
>
> +static struct device_attribute hisi_hha_pmu_identifier_attr =
> + __ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_hha_pmu_identifier_attrs[] = {
> + &hisi_hha_pmu_identifier_attr.attr,
> + NULL
> +};
> +
> +static struct attribute_group hisi_hha_pmu_identifier_group = {
> + .attrs = hisi_hha_pmu_identifier_attrs,
> +};
> +
> static const struct attribute_group *hisi_hha_pmu_attr_groups[] = {
> &hisi_hha_pmu_format_group,
> &hisi_hha_pmu_events_group,
> &hisi_hha_pmu_cpumask_attr_group,
> + &hisi_hha_pmu_identifier_group,
> NULL,
> };
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> index 7719ae4e2c56..0e7477220be1 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> @@ -25,6 +25,7 @@
> #define L3C_INT_STATUS 0x0808
> #define L3C_INT_CLEAR0x080c
> #define L3C_EVENT_CTRL 0x1c00
> +#define L3C_VERSION 0x1cf0
> #define L3C_EVENT_TYPE0 0x1d00
> /*
> * Each counter is 48-bits and [48:63] are reserved
> @@ -264,6 +265,8 @@ static int hisi_l3c_pmu_init_data(struct platform_device
> *pdev,
> return PTR_ERR(l3c_pmu->base);
> }
>
> + l3c_pmu->identifier = readl(l3c_pmu->base + L3C_VERSION);
> +
> return 0;
> }
>
> @@ -310,10 +313,23 @@ static const struct attribute_group
> hisi_l3c_pmu_cpumask_attr_group = {
> .attrs = hisi_l3c_pmu_cpumas
Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file
On 18/06/2020 02:40, Shaokun Zhang wrote:
}
+ hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION);
Since we are now refactoring the PMU framework, the PMU version offset
is always the same except DDRC PMU and other uncore PMU modules will
also use this, how about we do it as the common code:
#define HISI_PMU_VERSION_REG 0x1CF0
int hisi_uncore_pmu_version(struct hisi_pmu *hisi_pmu)
{
return readl(hisi_pmu->base + HISI_PMU_VERSION_REG);
}
EXPORT_SYMBOL_GPL(hisi_uncore_pmu_version);
Hi Shaokun,
Some points to make:
- It's hardly worth having a separate function to do this 1-line readl()
call, especially since it not even generic (DDRC is different)
- We would have to export it (or put in a common header file with static
inline keywords) - less exports are good
- with factoring out common code, it's good to reduce total code - this
change would increase it, AFAICS
- This is HW specific. The driver is currently layered such that all HW
specific stuff is in the HW driver (like hisi_uncore_ddrc_pmu.c), and
not library code (hisi_uncore_pmu.c). I don't see why you want to mix
that, like you're proposing in the framework revision you proposed
internally.
Thanks,
John
hha_pmu->identifier = hisi_uncore_pmu_version(hha_pmu);
we can remove the duplicated PMU version register definition in each module.
Thanks,
Shaokun
+
return 0;
}
@@ -320,10 +323,23 @@ static const struct attribute_group hisi_hha_pmu_cpumask_attr_group = {
.attrs = hisi_hha_pmu_cpumask_attrs,
};
