[PATCH v3 0/4] Exynos Performance Monitoring Counters enhancements

[PATCH v3 0/4] Exynos Performance Monitoring Counters enhancements

[Lukasz Luba]

Hi all,

This patch set extends PPMU on Samsung Exynos by choosing type of data
which shell be counter in the PPMU registers.
It is possible to count e.g. read or write requests, read or write data
or latency.
A new field has been added in the DT 'event' node called 'event-data-type'.
It is them used during the setup of the counter. In the prevoius
implementation there was always one option used: count read+write data.
Sometimes we need more precised information and this patch set tries to
address it.

Changes:
v3:
- fixed wrong interpretation of ret value during DT parsing, which caused
  alwasy taking default value.
v2:
 - removed new entry in MAINTAINERS file as suggested by Bartek Zolnierkiewicz
   and added new file to existing list for devfreq events
 - added in the dt-bindings/pmu/exynos_ppmu.h 2 new entries for RO and WO for
   counters in Exynos5433
 - changed initialization with default values when data_type is not provided
   in DT (as sugessted by Chanwoo)
 - added 4th patch which adds 'event-data-type' to 'event' node for Exynos4412
   PPMU events (asked by Chanwoo)

Regards,
Lukasz Luba

Lukasz Luba (4):
  include: dt-bindings: add Performance Monitoring Unit for Exynos
  drivers: devfreq: events: extend events by type of counted data
  Documentation: devicetree: add PPMU events description
  DT: arm: exynos4412: add event data type which is monitored

 .../bindings/devfreq/event/exynos-ppmu.txt         | 18 +++++++
 MAINTAINERS                                        |  1 +
 arch/arm/boot/dts/exynos4412-ppmu-common.dtsi      | 10 ++++
 drivers/devfreq/event/exynos-ppmu.c                | 61 +++++++++++++++-------
 include/dt-bindings/pmu/exynos_ppmu.h              | 26 +++++++++
 include/linux/devfreq-event.h                      |  6 +++
 6 files changed, 103 insertions(+), 19 deletions(-)
 create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h

-- 
2.7.4

[PATCH v3 1/4] include: dt-bindings: add Performance Monitoring Unit for Exynos

[Lukasz Luba]

This patch add support of a new feature which can be used in DT:
Performance Monitoring Unit with defined event data type.
In this patch the event data types are defined for Exynos PPMU.
The patch also updates the MAINTAINERS file accordingly and
adds the header file to devfreq event subsystem.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 MAINTAINERS                           |  1 +
 include/dt-bindings/pmu/exynos_ppmu.h | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3671fde..1ba4b9b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4560,6 +4560,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
 S:	Supported
 F:	drivers/devfreq/event/
 F:	drivers/devfreq/devfreq-event.c
+F:	include/dt-bindings/pmu/exynos_ppmu.h
 F:	include/linux/devfreq-event.h
 F:	Documentation/devicetree/bindings/devfreq/event/
 
diff --git a/include/dt-bindings/pmu/exynos_ppmu.h b/include/dt-bindings/pmu/exynos_ppmu.h
new file mode 100644
index 0000000..08fdce9
--- /dev/null
+++ b/include/dt-bindings/pmu/exynos_ppmu.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Samsung Exynos PPMU event types for counting in regs
+ *
+ * Copyright (c) 2019, Samsung
+ * Author: Lukasz Luba <l.luba@partner.samsung.com>
+ */
+
+#ifndef __DT_BINDINGS_PMU_EXYNOS_PPMU_H
+#define __DT_BINDINGS_PMU_EXYNOS_PPMU_H
+
+
+#define PPMU_RO_BUSY_CYCLE_CNT		0x0
+#define PPMU_WO_BUSY_CYCLE_CNT		0x1
+#define PPMU_RW_BUSY_CYCLE_CNT		0x2
+#define PPMU_RO_REQUEST_CNT		0x3
+#define PPMU_WO_REQUEST_CNT		0x4
+#define PPMU_RO_DATA_CNT		0x5
+#define PPMU_WO_DATA_CNT		0x6
+#define PPMU_RO_LATENCY			0x12
+#define PPMU_WO_LATENCY			0x16
+#define PPMU_V2_RO_DATA_CNT		0x4
+#define PPMU_V2_WO_DATA_CNT		0x5
+#define PPMU_V2_EVT3_RW_DATA_CNT	0x22
+
+#endif
-- 
2.7.4

Re: [PATCH v3 1/4] include: dt-bindings: add Performance Monitoring Unit for Exynos

[Chanwoo Choi]

Hi,

I agree of this patch. But, I add the minor comments.

If you edit them according to my comment, feel free to add my following tag:
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
> This patch add support of a new feature which can be used in DT:
> Performance Monitoring Unit with defined event data type.
> In this patch the event data types are defined for Exynos PPMU.
> The patch also updates the MAINTAINERS file accordingly and
> adds the header file to devfreq event subsystem.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  MAINTAINERS                           |  1 +
>  include/dt-bindings/pmu/exynos_ppmu.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>  create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3671fde..1ba4b9b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4560,6 +4560,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
>  S:	Supported
>  F:	drivers/devfreq/event/
>  F:	drivers/devfreq/devfreq-event.c
> +F:	include/dt-bindings/pmu/exynos_ppmu.h
>  F:	include/linux/devfreq-event.h
>  F:	Documentation/devicetree/bindings/devfreq/event/
>  
> diff --git a/include/dt-bindings/pmu/exynos_ppmu.h b/include/dt-bindings/pmu/exynos_ppmu.h
> new file mode 100644
> index 0000000..08fdce9
> --- /dev/null
> +++ b/include/dt-bindings/pmu/exynos_ppmu.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Samsung Exynos PPMU event types for counting in regs
> + *
> + * Copyright (c) 2019, Samsung

Mabye, "Samsung Electronics" instead of 'Samsung'.

> + * Author: Lukasz Luba <l.luba@partner.samsung.com>
> + */
> +
> +#ifndef __DT_BINDINGS_PMU_EXYNOS_PPMU_H
> +#define __DT_BINDINGS_PMU_EXYNOS_PPMU_H
> +
> +

Remove unneeded blank line.

> +#define PPMU_RO_BUSY_CYCLE_CNT		0x0
> +#define PPMU_WO_BUSY_CYCLE_CNT		0x1
> +#define PPMU_RW_BUSY_CYCLE_CNT		0x2
> +#define PPMU_RO_REQUEST_CNT		0x3
> +#define PPMU_WO_REQUEST_CNT		0x4
> +#define PPMU_RO_DATA_CNT		0x5
> +#define PPMU_WO_DATA_CNT		0x6
> +#define PPMU_RO_LATENCY			0x12
> +#define PPMU_WO_LATENCY			0x16
> +#define PPMU_V2_RO_DATA_CNT		0x4
> +#define PPMU_V2_WO_DATA_CNT		0x5
> +#define PPMU_V2_EVT3_RW_DATA_CNT	0x22
> +
> +#endif
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 1/4] include: dt-bindings: add Performance Monitoring Unit for Exynos

[Lukasz Luba]

On 4/30/19 6:56 AM, Chanwoo Choi wrote:
> Hi,
> 
> I agree of this patch. But, I add the minor comments.
> 
> If you edit them according to my comment, feel free to add my following tag:
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>> This patch add support of a new feature which can be used in DT:
>> Performance Monitoring Unit with defined event data type.
>> In this patch the event data types are defined for Exynos PPMU.
>> The patch also updates the MAINTAINERS file accordingly and
>> adds the header file to devfreq event subsystem.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   MAINTAINERS                           |  1 +
>>   include/dt-bindings/pmu/exynos_ppmu.h | 26 ++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>   create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3671fde..1ba4b9b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4560,6 +4560,7 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git
>>   S:	Supported
>>   F:	drivers/devfreq/event/
>>   F:	drivers/devfreq/devfreq-event.c
>> +F:	include/dt-bindings/pmu/exynos_ppmu.h
>>   F:	include/linux/devfreq-event.h
>>   F:	Documentation/devicetree/bindings/devfreq/event/
>>   
>> diff --git a/include/dt-bindings/pmu/exynos_ppmu.h b/include/dt-bindings/pmu/exynos_ppmu.h
>> new file mode 100644
>> index 0000000..08fdce9
>> --- /dev/null
>> +++ b/include/dt-bindings/pmu/exynos_ppmu.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Samsung Exynos PPMU event types for counting in regs
>> + *
>> + * Copyright (c) 2019, Samsung
> 
> Mabye, "Samsung Electronics" instead of 'Samsung'.
ACK
> 
>> + * Author: Lukasz Luba <l.luba@partner.samsung.com>
>> + */
>> +
>> +#ifndef __DT_BINDINGS_PMU_EXYNOS_PPMU_H
>> +#define __DT_BINDINGS_PMU_EXYNOS_PPMU_H
>> +
>> +
> 
> Remove unneeded blank line.
OK done. I will add your 'Acked-by' in the next version.

Regards,
Lukasz

Re: [PATCH v3 1/4] include: dt-bindings: add Performance Monitoring Unit for Exynos

[Rob Herring]

On Fri, 19 Apr 2019 15:48:05 +0200, Lukasz Luba wrote:
> This patch add support of a new feature which can be used in DT:
> Performance Monitoring Unit with defined event data type.
> In this patch the event data types are defined for Exynos PPMU.
> The patch also updates the MAINTAINERS file accordingly and
> adds the header file to devfreq event subsystem.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  MAINTAINERS                           |  1 +
>  include/dt-bindings/pmu/exynos_ppmu.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
>  create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 1/4] include: dt-bindings: add Performance Monitoring Unit for Exynos

[Lukasz Luba]

On 5/1/19 12:33 AM, Rob Herring wrote:
> On Fri, 19 Apr 2019 15:48:05 +0200, Lukasz Luba wrote:
>> This patch add support of a new feature which can be used in DT:
>> Performance Monitoring Unit with defined event data type.
>> In this patch the event data types are defined for Exynos PPMU.
>> The patch also updates the MAINTAINERS file accordingly and
>> adds the header file to devfreq event subsystem.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   MAINTAINERS                           |  1 +
>>   include/dt-bindings/pmu/exynos_ppmu.h | 26 ++++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>   create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h
>>
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> 
Thank you, added to the commit message for the next patch set version.

Regards,
Lukasz

[PATCH v3 2/4] drivers: devfreq: events: extend events by type of counted data

[Lukasz Luba]

This patch adds posibility to choose what type of data should be counted
by the PPMU counter. Now the type comes from DT where the event has been
defined. When there is no 'event-data-type' the default value is used,
which is 'read data in bytes'.
It is needed when you want to know not only read+write data bytes but
i.e. only write data in byte, or number of read requests, etc.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
 include/linux/devfreq-event.h       |  6 ++++
 2 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
index c61de0b..073bf2c 100644
--- a/drivers/devfreq/event/exynos-ppmu.c
+++ b/drivers/devfreq/event/exynos-ppmu.c
@@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
 	if (ret < 0)
 		return ret;
 
-	/* Set the event of Read/Write data count  */
+	/* Set the event of proper data type monitoring */
 	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
-				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
+			   edev->desc->data_type);
 	if (ret < 0)
 		return ret;
 
@@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
 	if (ret < 0)
 		return ret;
 
-	/* Set the event of Read/Write data count  */
-	switch (id) {
-	case PPMU_PMNCNT0:
-	case PPMU_PMNCNT1:
-	case PPMU_PMNCNT2:
-		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
-				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
-		if (ret < 0)
-			return ret;
-		break;
-	case PPMU_PMNCNT3:
-		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
-				PPMU_V2_EVT3_RW_DATA_CNT);
-		if (ret < 0)
-			return ret;
-		break;
-	}
+	/* Set the event of proper data type monitoring */
+	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
+			   edev->desc->data_type);
+	if (ret < 0)
+		return ret;
 
 	/* Reset cycle counter/performance counter and enable PPMU */
 	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
@@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
 	struct device *dev = info->dev;
 	struct device_node *events_np, *node;
 	int i, j, count;
+	int ret;
 
 	events_np = of_get_child_by_name(np, "events");
 	if (!events_np) {
@@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
 		desc[j].driver_data = info;
 
 		of_property_read_string(node, "event-name", &desc[j].name);
+		ret = of_property_read_u32(node, "event-data-type",
+					   &desc[j].data_type);
+		if (ret) {
+			/* Set the event of proper data type counting.
+			 * Check if the data type has been defined in DT,
+			 * use default if not.
+			 */
+			if (of_device_is_compatible(np,
+					"samsung,exynos-ppmu-v2")) {
+				struct devfreq_event_dev edev;
+				int id;
+				/* Not all registers take the same value for
+				 * read+write data count.
+				 */
+				edev.desc = &desc[j];
+				id = exynos_ppmu_find_ppmu_id(&edev);
+
+				switch (id) {
+				case PPMU_PMNCNT0:
+				case PPMU_PMNCNT1:
+				case PPMU_PMNCNT2:
+					desc[j].data_type = PPMU_V2_RO_DATA_CNT
+						| PPMU_V2_WO_DATA_CNT;
+					break;
+				case PPMU_PMNCNT3:
+					desc[j].data_type =
+						PPMU_V2_EVT3_RW_DATA_CNT;
+					break;
+				}
+			} else {
+				desc[j].data_type = PPMU_RO_DATA_CNT |
+					PPMU_WO_DATA_CNT;
+			}
+		}
 
 		j++;
 	}
diff --git a/include/linux/devfreq-event.h b/include/linux/devfreq-event.h
index 4db00b0..cc160b1 100644
--- a/include/linux/devfreq-event.h
+++ b/include/linux/devfreq-event.h
@@ -81,14 +81,20 @@ struct devfreq_event_ops {
  * struct devfreq_event_desc - the descriptor of devfreq-event device
  *
  * @name	: the name of devfreq-event device.
+ * @data_type	: the data type which is going to be counted in the register.
  * @driver_data	: the private data for devfreq-event driver.
  * @ops		: the operation to control devfreq-event device.
  *
  * Each devfreq-event device is described with a this structure.
  * This structure contains the various data for devfreq-event device.
+ * The data_type describes what is going to be counted in the register.
+ * It might choose to count e.g. read requests, write data in bytes, etc.
+ * The full supported list of types is present in specyfic header in:
+ * include/dt-bindings/pmu/.
  */
 struct devfreq_event_desc {
 	const char *name;
+	u32 data_type;
 	void *driver_data;
 
 	const struct devfreq_event_ops *ops;
-- 
2.7.4

Re: [PATCH v3 2/4] drivers: devfreq: events: extend events by type of counted data

[Chanwoo Choi]

Hi Lukasz,

On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
> This patch adds posibility to choose what type of data should be counted
> by the PPMU counter. Now the type comes from DT where the event has been
> defined. When there is no 'event-data-type' the default value is used,
> which is 'read data in bytes'.
> It is needed when you want to know not only read+write data bytes but
> i.e. only write data in byte, or number of read requests, etc.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>  include/linux/devfreq-event.h       |  6 ++++
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
> index c61de0b..073bf2c 100644
> --- a/drivers/devfreq/event/exynos-ppmu.c
> +++ b/drivers/devfreq/event/exynos-ppmu.c
> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Set the event of Read/Write data count  */
> +	/* Set the event of proper data type monitoring */
>  	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
> +			   edev->desc->data_type);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Set the event of Read/Write data count  */
> -	switch (id) {
> -	case PPMU_PMNCNT0:
> -	case PPMU_PMNCNT1:
> -	case PPMU_PMNCNT2:
> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
> -		if (ret < 0)
> -			return ret;
> -		break;
> -	case PPMU_PMNCNT3:
> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
> -				PPMU_V2_EVT3_RW_DATA_CNT);
> -		if (ret < 0)
> -			return ret;
> -		break;
> -	}
> +	/* Set the event of proper data type monitoring */
> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
> +			   edev->desc->data_type);
> +	if (ret < 0)
> +		return ret;
>  
>  	/* Reset cycle counter/performance counter and enable PPMU */
>  	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>  	struct device *dev = info->dev;
>  	struct device_node *events_np, *node;
>  	int i, j, count;
> +	int ret;
>  
>  	events_np = of_get_child_by_name(np, "events");
>  	if (!events_np) {
> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>  		desc[j].driver_data = info;
>  
>  		of_property_read_string(node, "event-name", &desc[j].name);
> +		ret = of_property_read_u32(node, "event-data-type",
> +					   &desc[j].data_type);
> +		if (ret) {
> +			/* Set the event of proper data type counting.
> +			 * Check if the data type has been defined in DT,
> +			 * use default if not.
> +			 */
> +			if (of_device_is_compatible(np,
> +					"samsung,exynos-ppmu-v2")) {

It is not proper to compare the compatible string again
in the device driver. Instead, you can define the ppmu device type
as following and then use 'struct of_device_id' in order to
identify the device type.

	enum exynos_ppmu_type {
		EXYNOS_TYPE_PPMU,
		EXYNOS_TYPE_PPMU_V2,
	};

 static const struct of_device_id exynos_ppmu_id_match[] = {
        {
                .compatible = "samsung,exynos-ppmu",
-               .data = (void *)&exynos_ppmu_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU,
        }, {
                .compatible = "samsung,exynos-ppmu-v2",
-               .data = (void *)&exynos_ppmu_v2_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU_V2,
        },


The many device drivers in the mainline uses this code
in order to get the device type. You can refer the example
in the drivers/mfd/max14577.c.

(snip)


Example, I add the example. but it is not tested. 

--- a/drivers/devfreq/event/exynos-ppmu.c
+++ b/drivers/devfreq/event/exynos-ppmu.c
@@ -16,6 +16,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/suspend.h>
@@ -23,6 +24,11 @@
 
 #include "exynos-ppmu.h"
 
+enum exynos_ppmu_type {
+       EXYNOS_TYPE_PPMU,
+       EXYNOS_TYPE_PPMU_V2,
+};
+
 struct exynos_ppmu_data {
        struct clk *clk;
 };
@@ -36,6 +42,7 @@ struct exynos_ppmu {
        struct regmap *regmap;
 
        struct exynos_ppmu_data ppmu;
+       enum exynos_ppmu_type ppmu_type;
 };
 
 #define PPMU_EVENT(name)                       \
 
        /* Reset cycle counter/performance counter and enable PPMU */
        ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
@@ -483,31 +476,23 @@ static const struct devfreq_event_ops exynos_ppmu_v2_ops = {
 static const struct of_device_id exynos_ppmu_id_match[] = {
        {
                .compatible = "samsung,exynos-ppmu",
-               .data = (void *)&exynos_ppmu_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU,
        }, {
                .compatible = "samsung,exynos-ppmu-v2",
-               .data = (void *)&exynos_ppmu_v2_ops,
+               .data = (void *)EXYNOS_TYPE_PPMU_V2,
        },
        { /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, exynos_ppmu_id_match);
 
-static struct devfreq_event_ops *exynos_bus_get_ops(struct device_node *np)
-{
-       const struct of_device_id *match;
-
-       match = of_match_node(exynos_ppmu_id_match, np);
-       return (struct devfreq_event_ops *)match->data;
-}
-
 static int of_get_devfreq_events(struct device_node *np,
                                 struct exynos_ppmu *info)
 {
        struct devfreq_event_desc *desc;
-       struct devfreq_event_ops *event_ops;
        struct device *dev = info->dev;
        struct device_node *events_np, *node;
        int i, j, count;
 
        events_np = of_get_child_by_name(np, "events");
        if (!events_np) {
@@ -515,7 +500,6 @@ static int of_get_devfreq_events(struct device_node *np,
                        "failed to get child node of devfreq-event devices\n");
                return -EINVAL;
        }
-       event_ops = exynos_bus_get_ops(np);
 
        count = of_get_child_count(events_np);
        desc = devm_kcalloc(dev, count, sizeof(*desc), GFP_KERNEL);
@@ -540,11 +524,38 @@ static int of_get_devfreq_events(struct device_node *np,
                        continue;
                }
 
-               desc[j].ops = event_ops;
+               switch (info->ppmu_type) {
+               case EXYNOS_TYPE_PPMU:
+                       desc[j].ops = &exynos_ppmu_ops;
+                       break;
+               case EXYNOS_TYPE_PPMU_V2:
+                       desc[j].ops = &exynos_ppmu_v2_ops;
+                       break;
+               }
+


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/4] drivers: devfreq: events: extend events by type of counted data

[Lukasz Luba]

Hi Chanwoo,

On 4/30/19 9:34 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>> This patch adds posibility to choose what type of data should be counted
>> by the PPMU counter. Now the type comes from DT where the event has been
>> defined. When there is no 'event-data-type' the default value is used,
>> which is 'read data in bytes'.
>> It is needed when you want to know not only read+write data bytes but
>> i.e. only write data in byte, or number of read requests, etc.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>>   include/linux/devfreq-event.h       |  6 ++++
>>   2 files changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>> index c61de0b..073bf2c 100644
>> --- a/drivers/devfreq/event/exynos-ppmu.c
>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* Set the event of Read/Write data count  */
>> +	/* Set the event of proper data type monitoring */
>>   	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>> +			   edev->desc->data_type);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* Set the event of Read/Write data count  */
>> -	switch (id) {
>> -	case PPMU_PMNCNT0:
>> -	case PPMU_PMNCNT1:
>> -	case PPMU_PMNCNT2:
>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
>> -		if (ret < 0)
>> -			return ret;
>> -		break;
>> -	case PPMU_PMNCNT3:
>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>> -				PPMU_V2_EVT3_RW_DATA_CNT);
>> -		if (ret < 0)
>> -			return ret;
>> -		break;
>> -	}
>> +	/* Set the event of proper data type monitoring */
>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>> +			   edev->desc->data_type);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	/* Reset cycle counter/performance counter and enable PPMU */
>>   	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>   	struct device *dev = info->dev;
>>   	struct device_node *events_np, *node;
>>   	int i, j, count;
>> +	int ret;
>>   
>>   	events_np = of_get_child_by_name(np, "events");
>>   	if (!events_np) {
>> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>>   		desc[j].driver_data = info;
>>   
>>   		of_property_read_string(node, "event-name", &desc[j].name);
>> +		ret = of_property_read_u32(node, "event-data-type",
>> +					   &desc[j].data_type);
>> +		if (ret) {
>> +			/* Set the event of proper data type counting.
>> +			 * Check if the data type has been defined in DT,
>> +			 * use default if not.
>> +			 */
>> +			if (of_device_is_compatible(np,
>> +					"samsung,exynos-ppmu-v2")) {
> 
> It is not proper to compare the compatible string again
> in the device driver. Instead, you can define the ppmu device type
> as following and then use 'struct of_device_id' in order to
> identify the device type.
I have been thinking about modifying the code in similar fashion as you
did. Good to see similar approach. I'll take your changes with a small
additional code, which sets the 'info->ppmu_type' before the for
loop, as an additional patch. Would it be OK if I add you as an author
and add Sign-off-by: Chanwoo Choi <cw00.choi@samsung.com>?

Regards,
Lukasz
> 
> 	enum exynos_ppmu_type {
> 		EXYNOS_TYPE_PPMU,
> 		EXYNOS_TYPE_PPMU_V2,
> 	};
> 
>   static const struct of_device_id exynos_ppmu_id_match[] = {
>          {
>                  .compatible = "samsung,exynos-ppmu",
> -               .data = (void *)&exynos_ppmu_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU,
>          }, {
>                  .compatible = "samsung,exynos-ppmu-v2",
> -               .data = (void *)&exynos_ppmu_v2_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>          },
> 
> 
> The many device drivers in the mainline uses this code
> in order to get the device type. You can refer the example
> in the drivers/mfd/max14577.c.
> 
> (snip)
> 
> 
> Example, I add the example. but it is not tested.
> 
> --- a/drivers/devfreq/event/exynos-ppmu.c
> +++ b/drivers/devfreq/event/exynos-ppmu.c
> @@ -16,6 +16,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of_address.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   #include <linux/suspend.h>
> @@ -23,6 +24,11 @@
>   
>   #include "exynos-ppmu.h"
>   
> +enum exynos_ppmu_type {
> +       EXYNOS_TYPE_PPMU,
> +       EXYNOS_TYPE_PPMU_V2,
> +};
> +
>   struct exynos_ppmu_data {
>          struct clk *clk;
>   };
> @@ -36,6 +42,7 @@ struct exynos_ppmu {
>          struct regmap *regmap;
>   
>          struct exynos_ppmu_data ppmu;
> +       enum exynos_ppmu_type ppmu_type;
>   };
>   
>   #define PPMU_EVENT(name)                       \
>   
>          /* Reset cycle counter/performance counter and enable PPMU */
>          ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
> @@ -483,31 +476,23 @@ static const struct devfreq_event_ops exynos_ppmu_v2_ops = {
>   static const struct of_device_id exynos_ppmu_id_match[] = {
>          {
>                  .compatible = "samsung,exynos-ppmu",
> -               .data = (void *)&exynos_ppmu_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU,
>          }, {
>                  .compatible = "samsung,exynos-ppmu-v2",
> -               .data = (void *)&exynos_ppmu_v2_ops,
> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>          },
>          { /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(of, exynos_ppmu_id_match);
>   
> -static struct devfreq_event_ops *exynos_bus_get_ops(struct device_node *np)
> -{
> -       const struct of_device_id *match;
> -
> -       match = of_match_node(exynos_ppmu_id_match, np);
> -       return (struct devfreq_event_ops *)match->data;
> -}
> -
>   static int of_get_devfreq_events(struct device_node *np,
>                                   struct exynos_ppmu *info)
>   {
>          struct devfreq_event_desc *desc;
> -       struct devfreq_event_ops *event_ops;
>          struct device *dev = info->dev;
>          struct device_node *events_np, *node;
>          int i, j, count;
>   
>          events_np = of_get_child_by_name(np, "events");
>          if (!events_np) {
> @@ -515,7 +500,6 @@ static int of_get_devfreq_events(struct device_node *np,
>                          "failed to get child node of devfreq-event devices\n");
>                  return -EINVAL;
>          }
> -       event_ops = exynos_bus_get_ops(np);
>   
>          count = of_get_child_count(events_np);
>          desc = devm_kcalloc(dev, count, sizeof(*desc), GFP_KERNEL);
> @@ -540,11 +524,38 @@ static int of_get_devfreq_events(struct device_node *np,
>                          continue;
>                  }
>   
> -               desc[j].ops = event_ops;
> +               switch (info->ppmu_type) {
> +               case EXYNOS_TYPE_PPMU:
> +                       desc[j].ops = &exynos_ppmu_ops;
> +                       break;
> +               case EXYNOS_TYPE_PPMU_V2:
> +                       desc[j].ops = &exynos_ppmu_v2_ops;
> +                       break;
> +               }
> +
> 
> 

Re: [PATCH v3 2/4] drivers: devfreq: events: extend events by type of counted data

[Chanwoo Choi]

Hi Lukasz,

On 19. 5. 1. 오전 6:19, Lukasz Luba wrote:
> Hi Chanwoo,
> 
> On 4/30/19 9:34 AM, Chanwoo Choi wrote:
>> Hi Lukasz,
>>
>> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>>> This patch adds posibility to choose what type of data should be counted
>>> by the PPMU counter. Now the type comes from DT where the event has been
>>> defined. When there is no 'event-data-type' the default value is used,
>>> which is 'read data in bytes'.
>>> It is needed when you want to know not only read+write data bytes but
>>> i.e. only write data in byte, or number of read requests, etc.
>>>
>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>> ---
>>>   drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>>>   include/linux/devfreq-event.h       |  6 ++++
>>>   2 files changed, 48 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>>> index c61de0b..073bf2c 100644
>>> --- a/drivers/devfreq/event/exynos-ppmu.c
>>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>>> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>>   	if (ret < 0)
>>>   		return ret;
>>>   
>>> -	/* Set the event of Read/Write data count  */
>>> +	/* Set the event of proper data type monitoring */
>>>   	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>>> +			   edev->desc->data_type);
>>>   	if (ret < 0)
>>>   		return ret;
>>>   
>>> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>>   	if (ret < 0)
>>>   		return ret;
>>>   
>>> -	/* Set the event of Read/Write data count  */
>>> -	switch (id) {
>>> -	case PPMU_PMNCNT0:
>>> -	case PPMU_PMNCNT1:
>>> -	case PPMU_PMNCNT2:
>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		break;
>>> -	case PPMU_PMNCNT3:
>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>> -				PPMU_V2_EVT3_RW_DATA_CNT);
>>> -		if (ret < 0)
>>> -			return ret;
>>> -		break;
>>> -	}
>>> +	/* Set the event of proper data type monitoring */
>>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>> +			   edev->desc->data_type);
>>> +	if (ret < 0)
>>> +		return ret;
>>>   
>>>   	/* Reset cycle counter/performance counter and enable PPMU */
>>>   	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>>> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>>   	struct device *dev = info->dev;
>>>   	struct device_node *events_np, *node;
>>>   	int i, j, count;
>>> +	int ret;
>>>   
>>>   	events_np = of_get_child_by_name(np, "events");
>>>   	if (!events_np) {
>>> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>>>   		desc[j].driver_data = info;
>>>   
>>>   		of_property_read_string(node, "event-name", &desc[j].name);
>>> +		ret = of_property_read_u32(node, "event-data-type",
>>> +					   &desc[j].data_type);
>>> +		if (ret) {
>>> +			/* Set the event of proper data type counting.
>>> +			 * Check if the data type has been defined in DT,
>>> +			 * use default if not.
>>> +			 */
>>> +			if (of_device_is_compatible(np,
>>> +					"samsung,exynos-ppmu-v2")) {
>>
>> It is not proper to compare the compatible string again
>> in the device driver. Instead, you can define the ppmu device type
>> as following and then use 'struct of_device_id' in order to
>> identify the device type.
> I have been thinking about modifying the code in similar fashion as you
> did. Good to see similar approach. I'll take your changes with a small
> additional code, which sets the 'info->ppmu_type' before the for
> loop, as an additional patch. Would it be OK if I add you as an author
> and add Sign-off-by: Chanwoo Choi <cw00.choi@samsung.com>?

If you agree, just add the my signed-off-by on second line.

> 
> Regards,
> Lukasz
>>
>> 	enum exynos_ppmu_type {
>> 		EXYNOS_TYPE_PPMU,
>> 		EXYNOS_TYPE_PPMU_V2,
>> 	};
>>
>>   static const struct of_device_id exynos_ppmu_id_match[] = {
>>          {
>>                  .compatible = "samsung,exynos-ppmu",
>> -               .data = (void *)&exynos_ppmu_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU,
>>          }, {
>>                  .compatible = "samsung,exynos-ppmu-v2",
>> -               .data = (void *)&exynos_ppmu_v2_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>>          },
>>
>>
>> The many device drivers in the mainline uses this code
>> in order to get the device type. You can refer the example
>> in the drivers/mfd/max14577.c.
>>
>> (snip)
>>
>>
>> Example, I add the example. but it is not tested.
>>
>> --- a/drivers/devfreq/event/exynos-ppmu.c
>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of_address.h>
>> +#include <linux/of_device.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/regmap.h>
>>   #include <linux/suspend.h>
>> @@ -23,6 +24,11 @@
>>   
>>   #include "exynos-ppmu.h"
>>   
>> +enum exynos_ppmu_type {
>> +       EXYNOS_TYPE_PPMU,
>> +       EXYNOS_TYPE_PPMU_V2,
>> +};
>> +
>>   struct exynos_ppmu_data {
>>          struct clk *clk;
>>   };
>> @@ -36,6 +42,7 @@ struct exynos_ppmu {
>>          struct regmap *regmap;
>>   
>>          struct exynos_ppmu_data ppmu;
>> +       enum exynos_ppmu_type ppmu_type;
>>   };
>>   
>>   #define PPMU_EVENT(name)                       \
>>   
>>          /* Reset cycle counter/performance counter and enable PPMU */
>>          ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>> @@ -483,31 +476,23 @@ static const struct devfreq_event_ops exynos_ppmu_v2_ops = {
>>   static const struct of_device_id exynos_ppmu_id_match[] = {
>>          {
>>                  .compatible = "samsung,exynos-ppmu",
>> -               .data = (void *)&exynos_ppmu_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU,
>>          }, {
>>                  .compatible = "samsung,exynos-ppmu-v2",
>> -               .data = (void *)&exynos_ppmu_v2_ops,
>> +               .data = (void *)EXYNOS_TYPE_PPMU_V2,
>>          },
>>          { /* sentinel */ },
>>   };
>>   MODULE_DEVICE_TABLE(of, exynos_ppmu_id_match);
>>   
>> -static struct devfreq_event_ops *exynos_bus_get_ops(struct device_node *np)
>> -{
>> -       const struct of_device_id *match;
>> -
>> -       match = of_match_node(exynos_ppmu_id_match, np);
>> -       return (struct devfreq_event_ops *)match->data;
>> -}
>> -
>>   static int of_get_devfreq_events(struct device_node *np,
>>                                   struct exynos_ppmu *info)
>>   {
>>          struct devfreq_event_desc *desc;
>> -       struct devfreq_event_ops *event_ops;
>>          struct device *dev = info->dev;
>>          struct device_node *events_np, *node;
>>          int i, j, count;
>>   
>>          events_np = of_get_child_by_name(np, "events");
>>          if (!events_np) {
>> @@ -515,7 +500,6 @@ static int of_get_devfreq_events(struct device_node *np,
>>                          "failed to get child node of devfreq-event devices\n");
>>                  return -EINVAL;
>>          }
>> -       event_ops = exynos_bus_get_ops(np);
>>   
>>          count = of_get_child_count(events_np);
>>          desc = devm_kcalloc(dev, count, sizeof(*desc), GFP_KERNEL);
>> @@ -540,11 +524,38 @@ static int of_get_devfreq_events(struct device_node *np,
>>                          continue;
>>                  }
>>   
>> -               desc[j].ops = event_ops;
>> +               switch (info->ppmu_type) {
>> +               case EXYNOS_TYPE_PPMU:
>> +                       desc[j].ops = &exynos_ppmu_ops;
>> +                       break;
>> +               case EXYNOS_TYPE_PPMU_V2:
>> +                       desc[j].ops = &exynos_ppmu_v2_ops;
>> +                       break;
>> +               }
>> +
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/4] drivers: devfreq: events: extend events by type of counted data

[Lukasz Luba]

On 5/2/19 3:25 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 5. 1. 오전 6:19, Lukasz Luba wrote:
>> Hi Chanwoo,
>>
>> On 4/30/19 9:34 AM, Chanwoo Choi wrote:
>>> Hi Lukasz,
>>>
>>> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>>>> This patch adds posibility to choose what type of data should be counted
>>>> by the PPMU counter. Now the type comes from DT where the event has been
>>>> defined. When there is no 'event-data-type' the default value is used,
>>>> which is 'read data in bytes'.
>>>> It is needed when you want to know not only read+write data bytes but
>>>> i.e. only write data in byte, or number of read requests, etc.
>>>>
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    drivers/devfreq/event/exynos-ppmu.c | 61 +++++++++++++++++++++++++------------
>>>>    include/linux/devfreq-event.h       |  6 ++++
>>>>    2 files changed, 48 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>>>> index c61de0b..073bf2c 100644
>>>> --- a/drivers/devfreq/event/exynos-ppmu.c
>>>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>>>> @@ -154,9 +154,9 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> -	/* Set the event of Read/Write data count  */
>>>> +	/* Set the event of proper data type monitoring */
>>>>    	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>>>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>>>> +			   edev->desc->data_type);
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> @@ -368,23 +368,11 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>>>    	if (ret < 0)
>>>>    		return ret;
>>>>    
>>>> -	/* Set the event of Read/Write data count  */
>>>> -	switch (id) {
>>>> -	case PPMU_PMNCNT0:
>>>> -	case PPMU_PMNCNT1:
>>>> -	case PPMU_PMNCNT2:
>>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>>> -				PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -		break;
>>>> -	case PPMU_PMNCNT3:
>>>> -		ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>>> -				PPMU_V2_EVT3_RW_DATA_CNT);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -		break;
>>>> -	}
>>>> +	/* Set the event of proper data type monitoring */
>>>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id),
>>>> +			   edev->desc->data_type);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>>    
>>>>    	/* Reset cycle counter/performance counter and enable PPMU */
>>>>    	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>>>> @@ -508,6 +496,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>>>    	struct device *dev = info->dev;
>>>>    	struct device_node *events_np, *node;
>>>>    	int i, j, count;
>>>> +	int ret;
>>>>    
>>>>    	events_np = of_get_child_by_name(np, "events");
>>>>    	if (!events_np) {
>>>> @@ -544,6 +533,40 @@ static int of_get_devfreq_events(struct device_node *np,
>>>>    		desc[j].driver_data = info;
>>>>    
>>>>    		of_property_read_string(node, "event-name", &desc[j].name);
>>>> +		ret = of_property_read_u32(node, "event-data-type",
>>>> +					   &desc[j].data_type);
>>>> +		if (ret) {
>>>> +			/* Set the event of proper data type counting.
>>>> +			 * Check if the data type has been defined in DT,
>>>> +			 * use default if not.
>>>> +			 */
>>>> +			if (of_device_is_compatible(np,
>>>> +					"samsung,exynos-ppmu-v2")) {
>>>
>>> It is not proper to compare the compatible string again
>>> in the device driver. Instead, you can define the ppmu device type
>>> as following and then use 'struct of_device_id' in order to
>>> identify the device type.
>> I have been thinking about modifying the code in similar fashion as you
>> did. Good to see similar approach. I'll take your changes with a small
>> additional code, which sets the 'info->ppmu_type' before the for
>> loop, as an additional patch. Would it be OK if I add you as an author
>> and add Sign-off-by: Chanwoo Choi <cw00.choi@samsung.com>?
> 
> If you agree, just add the my signed-off-by on second line.
OK

Regards,
Lukasz

[PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

Extend the documenation by events description with new 'event-data-type'
field. Add example how the event might be defined in DT.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 .../devicetree/bindings/devfreq/event/exynos-ppmu.txt  | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
index 3e36c1d..47feb5f 100644
--- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
+++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
@@ -145,3 +145,21 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
 			reg = <0x104d0000 0x2000>;
 			status = "disabled";
 		};
+
+The 'event' type specified in the PPMU node defines 'event-name'
+which also contains 'id' number and optionally 'event-data-type'.
+
+Example:
+
+		events {
+			ppmu_leftbus_0: ppmu-event0-leftbus {
+				event-name = "ppmu-event0-leftbus";
+				event-data-type = <PPMU_RO_DATA_CNT>;
+			};
+		};
+
+The 'event-data-type' defines the type of data which shell be counted
+by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
+all possible type, i.e. count read requests, count write data in bytes,
+etc. This field is optional and when it is missing, the driver code will
+use default data type.
-- 
2.7.4

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Chanwoo Choi]

Hi Lukasz,

On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
> Extend the documenation by events description with new 'event-data-type'
> field. Add example how the event might be defined in DT.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  .../devicetree/bindings/devfreq/event/exynos-ppmu.txt  | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> index 3e36c1d..47feb5f 100644
> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> @@ -145,3 +145,21 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>  			reg = <0x104d0000 0x2000>;
>  			status = "disabled";
>  		};
> +
> +The 'event' type specified in the PPMU node defines 'event-name'
> +which also contains 'id' number and optionally 'event-data-type'.
> +
> +Example:
> +
> +		events {
> +			ppmu_leftbus_0: ppmu-event0-leftbus {
> +				event-name = "ppmu-event0-leftbus";
> +				event-data-type = <PPMU_RO_DATA_CNT>;
> +			};
> +		};
> +
> +The 'event-data-type' defines the type of data which shell be counted
> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
> +all possible type, i.e. count read requests, count write data in bytes,
> +etc. This field is optional and when it is missing, the driver code will
> +use default data type.
> 

How about editing it as following?

--- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
+++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
@@ -10,14 +10,23 @@ The Exynos PPMU driver uses the devfreq-event class to provide event data
 to various devfreq devices. The devfreq devices would use the event data when
 derterming the current state of each IP.
 
-Required properties:
+Required properties for PPMU device:
 - compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.
 - reg: physical base address of each PPMU and length of memory mapped region.
 
-Optional properties:
+Optional properties for PPMU device:
 - clock-names : the name of clock used by the PPMU, "ppmu"
 - clocks : phandles for clock specified in "clock-names" property
 
+Required properties for 'events' child node of PPMU device:
+- event-name : the unique event name among PPMU device
+Optional properties for 'events' child node of PPMU device:
+- event-data-type : Define the type of data which shell be counted
+by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
+all possible type, i.e. count read requests, count write data in bytes,
+etc. This field is optional and when it is missing, the driver code
+will use default data type.
+
 Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
 
                ppmu_dmc0: ppmu_dmc0@106a0000 {
@@ -145,3 +154,16 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
                        reg = <0x104d0000 0x2000>;
                        status = "disabled";
                };
+
+Example4 : 'event-data-type' in exynos4412-ppmu-common.dtsi are listed below.
+
+       &ppmu_dmc0 {
+               status = "okay";
+               events {
+                       ppmu_dmc0_3: ppmu-event3-dmc0 {
+                               event-name = "ppmu-event3-dmc0";
+                               event-data-type = <(PPMU_RO_DATA_CNT |
+                                               PPMU_WO_DATA_CNT)>;
+                       };
+               };
+       };


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

On 4/30/19 8:16 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>> Extend the documenation by events description with new 'event-data-type'
>> field. Add example how the event might be defined in DT.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   .../devicetree/bindings/devfreq/event/exynos-ppmu.txt  | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> index 3e36c1d..47feb5f 100644
>> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> @@ -145,3 +145,21 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>>   			reg = <0x104d0000 0x2000>;
>>   			status = "disabled";
>>   		};
>> +
>> +The 'event' type specified in the PPMU node defines 'event-name'
>> +which also contains 'id' number and optionally 'event-data-type'.
>> +
>> +Example:
>> +
>> +		events {
>> +			ppmu_leftbus_0: ppmu-event0-leftbus {
>> +				event-name = "ppmu-event0-leftbus";
>> +				event-data-type = <PPMU_RO_DATA_CNT>;
>> +			};
>> +		};
>> +
>> +The 'event-data-type' defines the type of data which shell be counted
>> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
>> +all possible type, i.e. count read requests, count write data in bytes,
>> +etc. This field is optional and when it is missing, the driver code will
>> +use default data type.
>>
> 
> How about editing it as following?
> 
> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> @@ -10,14 +10,23 @@ The Exynos PPMU driver uses the devfreq-event class to provide event data
>   to various devfreq devices. The devfreq devices would use the event data when
>   derterming the current state of each IP.
>   
> -Required properties:
> +Required properties for PPMU device:
>   - compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.
>   - reg: physical base address of each PPMU and length of memory mapped region.
>   
> -Optional properties:
> +Optional properties for PPMU device:
>   - clock-names : the name of clock used by the PPMU, "ppmu"
>   - clocks : phandles for clock specified in "clock-names" property
>   
> +Required properties for 'events' child node of PPMU device:
> +- event-name : the unique event name among PPMU device
> +Optional properties for 'events' child node of PPMU device:
> +- event-data-type : Define the type of data which shell be counted
> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
> +all possible type, i.e. count read requests, count write data in bytes,
> +etc. This field is optional and when it is missing, the driver code
> +will use default data type.
> +
>   Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
>   
>                  ppmu_dmc0: ppmu_dmc0@106a0000 {
> @@ -145,3 +154,16 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>                          reg = <0x104d0000 0x2000>;
>                          status = "disabled";
>                  };
> +
> +Example4 : 'event-data-type' in exynos4412-ppmu-common.dtsi are listed below.
> +
> +       &ppmu_dmc0 {
> +               status = "okay";
> +               events {
> +                       ppmu_dmc0_3: ppmu-event3-dmc0 {
> +                               event-name = "ppmu-event3-dmc0";
> +                               event-data-type = <(PPMU_RO_DATA_CNT |
> +                                               PPMU_WO_DATA_CNT)>;
> +                       };
> +               };
> +       };
> 
> I agree. It will be changed in the next version.

Regards,
Lukasz

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

Hi Chanwoo,

On 4/30/19 8:16 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>> Extend the documenation by events description with new 'event-data-type'
>> field. Add example how the event might be defined in DT.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   .../devicetree/bindings/devfreq/event/exynos-ppmu.txt  | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> index 3e36c1d..47feb5f 100644
>> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> @@ -145,3 +145,21 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>>   			reg = <0x104d0000 0x2000>;
>>   			status = "disabled";
>>   		};
>> +
>> +The 'event' type specified in the PPMU node defines 'event-name'
>> +which also contains 'id' number and optionally 'event-data-type'.
>> +
>> +Example:
>> +
>> +		events {
>> +			ppmu_leftbus_0: ppmu-event0-leftbus {
>> +				event-name = "ppmu-event0-leftbus";
>> +				event-data-type = <PPMU_RO_DATA_CNT>;
>> +			};
>> +		};
>> +
>> +The 'event-data-type' defines the type of data which shell be counted
>> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
>> +all possible type, i.e. count read requests, count write data in bytes,
>> +etc. This field is optional and when it is missing, the driver code will
>> +use default data type.
>>
> 
> How about editing it as following?
> 
> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> @@ -10,14 +10,23 @@ The Exynos PPMU driver uses the devfreq-event class to provide event data
>   to various devfreq devices. The devfreq devices would use the event data when
>   derterming the current state of each IP.
>   
> -Required properties:
> +Required properties for PPMU device:
>   - compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.
>   - reg: physical base address of each PPMU and length of memory mapped region.
>   
> -Optional properties:
> +Optional properties for PPMU device:
>   - clock-names : the name of clock used by the PPMU, "ppmu"
>   - clocks : phandles for clock specified in "clock-names" property
>   
> +Required properties for 'events' child node of PPMU device:
> +- event-name : the unique event name among PPMU device
> +Optional properties for 'events' child node of PPMU device:
> +- event-data-type : Define the type of data which shell be counted
> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
> +all possible type, i.e. count read requests, count write data in bytes,
> +etc. This field is optional and when it is missing, the driver code
> +will use default data type.
> +
>   Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
>   
>                  ppmu_dmc0: ppmu_dmc0@106a0000 {
> @@ -145,3 +154,16 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>                          reg = <0x104d0000 0x2000>;
>                          status = "disabled";
>                  };
> +
> +Example4 : 'event-data-type' in exynos4412-ppmu-common.dtsi are listed below.
> +
> +       &ppmu_dmc0 {
> +               status = "okay";
> +               events {
> +                       ppmu_dmc0_3: ppmu-event3-dmc0 {
> +                               event-name = "ppmu-event3-dmc0";
> +                               event-data-type = <(PPMU_RO_DATA_CNT |
> +                                               PPMU_WO_DATA_CNT)>;
> +                       };
> +               };
> +       };
> 
> 
I will also add your Signed-off-by to this patch, similar to what we
have agreed for patch 'PATCH v3 2/4'.

Regards,
Lukasz

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Rob Herring]

On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
> Extend the documenation by events description with new 'event-data-type'
> field. Add example how the event might be defined in DT.

Why do we need event types in DT? We don't do this for other h/w such as 
ARM PMU.

> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  .../devicetree/bindings/devfreq/event/exynos-ppmu.txt  | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> index 3e36c1d..47feb5f 100644
> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> @@ -145,3 +145,21 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>  			reg = <0x104d0000 0x2000>;
>  			status = "disabled";
>  		};
> +
> +The 'event' type specified in the PPMU node defines 'event-name'
> +which also contains 'id' number and optionally 'event-data-type'.
> +
> +Example:
> +
> +		events {
> +			ppmu_leftbus_0: ppmu-event0-leftbus {
> +				event-name = "ppmu-event0-leftbus";
> +				event-data-type = <PPMU_RO_DATA_CNT>;
> +			};
> +		};
> +
> +The 'event-data-type' defines the type of data which shell be counted
> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
> +all possible type, i.e. count read requests, count write data in bytes,
> +etc. This field is optional and when it is missing, the driver code will
> +use default data type.
> -- 
> 2.7.4
> 

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

Hi Rob,

On 5/1/19 12:36 AM, Rob Herring wrote:
> On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
>> Extend the documenation by events description with new 'event-data-type'
>> field. Add example how the event might be defined in DT.
> 
> Why do we need event types in DT? We don't do this for other h/w such as
> ARM PMU.
In ARM PMU all the events are hard-coded into the driver code i.e. in v7
arch/arm/kernel/perf_event_v7.c
and are seen from perf. They are different type and for different
purpose. The Ecynos PPMU events are not seen in perf, they are
for internal monitoring and must not be reset by other actors like perf.
They are used by the 'bus drivers' to made some heuristics and tune the
internal settings, like frequency.

Chanwoo has written PPMU driver which relies on DT definition.
The DT events are used by other DT devices by phandle.
In Exynos 5x SoCs we have many 'bus devices' which use events to
monitor their usage and react accordingly.
---------------8<------------------------
[    4.140923] exynos-bus: new bus device registered: soc:bus_wcore ( 
84000 KHz ~ 400000 KHz)
[    4.149179] exynos-bus: new bus device registered: soc:bus_noc ( 
67000 KHz ~ 100000 KHz)
[    4.156825] exynos-bus: new bus device registered: soc:bus_fsys_apb 
(100000 KHz ~ 200000 KHz)
[    4.165071] exynos-bus: new bus device registered: soc:bus_fsys 
(100000 KHz ~ 200000 KHz)
[    4.173577] exynos-bus: new bus device registered: soc:bus_fsys2 ( 
75000 KHz ~ 150000 KHz)
[    4.182141] exynos-bus: new bus device registered: soc:bus_mfc ( 
96000 KHz ~ 333000 KHz)
[    4.190099] exynos-bus: new bus device registered: soc:bus_gen ( 
89000 KHz ~ 267000 KHz)
[    4.197953] exynos-bus: new bus device registered: soc:bus_peri ( 
67000 KHz ~  67000 KHz)
[    4.206523] exynos-bus: new bus device registered: soc:bus_g2d ( 
84000 KHz ~ 333000 KHz)
[    4.214516] exynos-bus: new bus device registered: soc:bus_g2d_acp ( 
67000 KHz ~ 267000 KHz)
[    4.222850] exynos-bus: new bus device registered: soc:bus_jpeg ( 
75000 KHz ~ 300000 KHz)
[    4.231052] exynos-bus: new bus device registered: soc:bus_jpeg_apb ( 
84000 KHz ~ 167000 KHz)
[    4.239202] exynos-bus: new bus device registered: soc:bus_disp1_fimd 
(120000 KHz ~ 200000 KHz)
[    4.248033] exynos-bus: new bus device registered: soc:bus_disp1 
(120000 KHz ~ 300000 KHz)
[    4.256304] exynos-bus: new bus device registered: 
soc:bus_gscl_scaler (150000 KHz ~ 300000 KHz)
[    4.265397] exynos-bus: new bus device registered: soc:bus_mscl ( 
84000 KHz ~ 400000 KHz)
-------------------------->8----------------------------------------
The PPMU driver made some assumption, though. It always monitors only
'read+write data bytes' as an event data type.
Thus, it is not possible to monitor other stuff and maybe improve the
heuristics.

This simple modification allows to define different data type, which is
acquired by the counter, still being compatible with all the Exynos
drivers in the existing implementation.

Regards,
Lukasz

> 
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   .../devicetree/bindings/devfreq/event/exynos-ppmu.txt  | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> index 3e36c1d..47feb5f 100644
>> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> @@ -145,3 +145,21 @@ Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>>   			reg = <0x104d0000 0x2000>;
>>   			status = "disabled";
>>   		};
>> +
>> +The 'event' type specified in the PPMU node defines 'event-name'
>> +which also contains 'id' number and optionally 'event-data-type'.
>> +
>> +Example:
>> +
>> +		events {
>> +			ppmu_leftbus_0: ppmu-event0-leftbus {
>> +				event-name = "ppmu-event0-leftbus";
>> +				event-data-type = <PPMU_RO_DATA_CNT>;
>> +			};
>> +		};
>> +
>> +The 'event-data-type' defines the type of data which shell be counted
>> +by the counter. You can check include/dt-bindings/pmu/exynos_ppmu.h for
>> +all possible type, i.e. count read requests, count write data in bytes,
>> +etc. This field is optional and when it is missing, the driver code will
>> +use default data type.
>> -- 
>> 2.7.4
>>
> 
> 

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Rob Herring]

On Thu, May 2, 2019 at 3:52 AM Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> Hi Rob,
>
> On 5/1/19 12:36 AM, Rob Herring wrote:
> > On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
> >> Extend the documenation by events description with new 'event-data-type'
> >> field. Add example how the event might be defined in DT.
> >
> > Why do we need event types in DT? We don't do this for other h/w such as
> > ARM PMU.
> In ARM PMU all the events are hard-coded into the driver code i.e. in v7
> arch/arm/kernel/perf_event_v7.c
> and are seen from perf. They are different type and for different
> purpose. The Ecynos PPMU events are not seen in perf, they are
> for internal monitoring and must not be reset by other actors like perf.
> They are used by the 'bus drivers' to made some heuristics and tune the
> internal settings, like frequency.
>
> Chanwoo has written PPMU driver which relies on DT definition.
> The DT events are used by other DT devices by phandle.

How is that done? I don't see anything in the binding for that.

Rob

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

Hi Rob,

On 5/2/19 10:24 PM, Rob Herring wrote:
> On Thu, May 2, 2019 at 3:52 AM Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> Hi Rob,
>>
>> On 5/1/19 12:36 AM, Rob Herring wrote:
>>> On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
>>>> Extend the documenation by events description with new 'event-data-type'
>>>> field. Add example how the event might be defined in DT.
>>>
>>> Why do we need event types in DT? We don't do this for other h/w such as
>>> ARM PMU.
>> In ARM PMU all the events are hard-coded into the driver code i.e. in v7
>> arch/arm/kernel/perf_event_v7.c
>> and are seen from perf. They are different type and for different
>> purpose. The Ecynos PPMU events are not seen in perf, they are
>> for internal monitoring and must not be reset by other actors like perf.
>> They are used by the 'bus drivers' to made some heuristics and tune the
>> internal settings, like frequency.
>>
>> Chanwoo has written PPMU driver which relies on DT definition.
>> The DT events are used by other DT devices by phandle.
> 
> How is that done? I don't see anything in the binding for that.
Here are the DT devices and how they are pinned together:
- declared devfreq events:
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
- devfreq events pinned to the bus device:
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-odroid-common.dtsi#L107
- the bus device itself:
https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412.dtsi#L457

Regards,
Lukasz

> 
> Rob
> 
> 

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

Hi Rob,

On 5/6/19 12:29 PM, Lukasz Luba wrote:
> Hi Rob,
> 
> On 5/2/19 10:24 PM, Rob Herring wrote:
>> On Thu, May 2, 2019 at 3:52 AM Lukasz Luba 
>> <l.luba@partner.samsung.com> wrote:
>>>
>>> Hi Rob,
>>>
>>> On 5/1/19 12:36 AM, Rob Herring wrote:
>>>> On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
>>>>> Extend the documenation by events description with new 
>>>>> 'event-data-type'
>>>>> field. Add example how the event might be defined in DT.
>>>>
>>>> Why do we need event types in DT? We don't do this for other h/w 
>>>> such as
>>>> ARM PMU.
>>> In ARM PMU all the events are hard-coded into the driver code i.e. in v7
>>> arch/arm/kernel/perf_event_v7.c
>>> and are seen from perf. They are different type and for different
>>> purpose. The Ecynos PPMU events are not seen in perf, they are
>>> for internal monitoring and must not be reset by other actors like perf.
>>> They are used by the 'bus drivers' to made some heuristics and tune the
>>> internal settings, like frequency.
>>>
>>> Chanwoo has written PPMU driver which relies on DT definition.
>>> The DT events are used by other DT devices by phandle.
>>
>> How is that done? I don't see anything in the binding for that.
> Here are the DT devices and how they are pinned together:
> - declared devfreq events:
> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi 
> 
> - devfreq events pinned to the bus device:
> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-odroid-common.dtsi#L107 
> 
> - the bus device itself:
> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412.dtsi#L457 
> 
> 
> Regards,
> Lukasz
> 
>>
>> Rob
>>
>>
Can I send the next version of the patch set, which has Chanwoo's
suggestions, or do you have some objections to this PPMU entries?

Regards,
Lukasz

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Rob Herring]

On Tue, May 7, 2019 at 4:05 AM Lukasz Luba <l.luba@partner.samsung.com> wrote:
>
> Hi Rob,
>
> On 5/6/19 12:29 PM, Lukasz Luba wrote:
> > Hi Rob,
> >
> > On 5/2/19 10:24 PM, Rob Herring wrote:
> >> On Thu, May 2, 2019 at 3:52 AM Lukasz Luba
> >> <l.luba@partner.samsung.com> wrote:
> >>>
> >>> Hi Rob,
> >>>
> >>> On 5/1/19 12:36 AM, Rob Herring wrote:
> >>>> On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
> >>>>> Extend the documenation by events description with new
> >>>>> 'event-data-type'
> >>>>> field. Add example how the event might be defined in DT.
> >>>>
> >>>> Why do we need event types in DT? We don't do this for other h/w
> >>>> such as
> >>>> ARM PMU.
> >>> In ARM PMU all the events are hard-coded into the driver code i.e. in v7
> >>> arch/arm/kernel/perf_event_v7.c
> >>> and are seen from perf. They are different type and for different
> >>> purpose. The Ecynos PPMU events are not seen in perf, they are
> >>> for internal monitoring and must not be reset by other actors like perf.
> >>> They are used by the 'bus drivers' to made some heuristics and tune the
> >>> internal settings, like frequency.
> >>>
> >>> Chanwoo has written PPMU driver which relies on DT definition.
> >>> The DT events are used by other DT devices by phandle.
> >>
> >> How is that done? I don't see anything in the binding for that.
> > Here are the DT devices and how they are pinned together:
> > - declared devfreq events:
> > https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
> >
> > - devfreq events pinned to the bus device:
> > https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-odroid-common.dtsi#L107
> >
> > - the bus device itself:
> > https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412.dtsi#L457
> >
> >
> > Regards,
> > Lukasz
> >
> >>
> >> Rob
> >>
> >>
> Can I send the next version of the patch set, which has Chanwoo's
> suggestions, or do you have some objections to this PPMU entries?

I think the existing binding which this is based on needs some
changes, so it's pointless really for me to comment on additions.

Rob

Re: [PATCH v3 3/4] Documentation: devicetree: add PPMU events description

[Lukasz Luba]

Hi Rob,

On 5/7/19 6:50 PM, Rob Herring wrote:
> On Tue, May 7, 2019 at 4:05 AM Lukasz Luba <l.luba@partner.samsung.com> wrote:
>>
>> Hi Rob,
>>
>> On 5/6/19 12:29 PM, Lukasz Luba wrote:
>>> Hi Rob,
>>>
>>> On 5/2/19 10:24 PM, Rob Herring wrote:
>>>> On Thu, May 2, 2019 at 3:52 AM Lukasz Luba
>>>> <l.luba@partner.samsung.com> wrote:
>>>>>
>>>>> Hi Rob,
>>>>>
>>>>> On 5/1/19 12:36 AM, Rob Herring wrote:
>>>>>> On Fri, Apr 19, 2019 at 03:48:07PM +0200, Lukasz Luba wrote:
>>>>>>> Extend the documenation by events description with new
>>>>>>> 'event-data-type'
>>>>>>> field. Add example how the event might be defined in DT.
>>>>>>
>>>>>> Why do we need event types in DT? We don't do this for other h/w
>>>>>> such as
>>>>>> ARM PMU.
>>>>> In ARM PMU all the events are hard-coded into the driver code i.e. in v7
>>>>> arch/arm/kernel/perf_event_v7.c
>>>>> and are seen from perf. They are different type and for different
>>>>> purpose. The Ecynos PPMU events are not seen in perf, they are
>>>>> for internal monitoring and must not be reset by other actors like perf.
>>>>> They are used by the 'bus drivers' to made some heuristics and tune the
>>>>> internal settings, like frequency.
>>>>>
>>>>> Chanwoo has written PPMU driver which relies on DT definition.
>>>>> The DT events are used by other DT devices by phandle.
>>>>
>>>> How is that done? I don't see anything in the binding for that.
>>> Here are the DT devices and how they are pinned together:
>>> - declared devfreq events:
>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
>>>
>>> - devfreq events pinned to the bus device:
>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412-odroid-common.dtsi#L107
>>>
>>> - the bus device itself:
>>> https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/exynos4412.dtsi#L457
>>>
>>>
>>> Regards,
>>> Lukasz
>>>
>>>>
>>>> Rob
>>>>
>>>>
>> Can I send the next version of the patch set, which has Chanwoo's
>> suggestions, or do you have some objections to this PPMU entries?
> 
> I think the existing binding which this is based on needs some
> changes, so it's pointless really for me to comment on additions.
Maybe the bindings description is not perfect, but it contains
examples which might help clarifying the idea.
Regarding the real value of the patch set, it is needed for some
research. Currently, the Odroid xu3/4 is the best 'mainline' platform
with big.LITTLE, has good performance and these counters.
Willy, who is doing his PhD, wants to experiment with it. I agree that
it could be better documented but it requires more effort.
I will extend the documentation it in my free time slots (it is not my
main task for now).

For now, I have received comments and parts of code from Chanwoo which
made the next patch set mature and ready to merge (IMHO).

If you have suggestions how to improve these bindings or links for good
examples, I would really appreciate. Thank you for your time and review.

Regards,
Lukasz

[PATCH v3 4/4] DT: arm: exynos4412: add event data type which is monitored

[Lukasz Luba]

The patch adds new field in the PPMU event which shows explicitly
what kind of data the event is monitoring. It is possible to change it
using defined values in exynos_ppmu.h file.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 arch/arm/boot/dts/exynos4412-ppmu-common.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi b/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
index 3a3b2fa..549faba 100644
--- a/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
@@ -6,12 +6,16 @@
  * Author: Chanwoo Choi <cw00.choi@samsung.com>
  */
 
+#include <dt-bindings/pmu/exynos_ppmu.h>
+
 &ppmu_dmc0 {
        status = "okay";
 
        events {
 	       ppmu_dmc0_3: ppmu-event3-dmc0 {
 		       event-name = "ppmu-event3-dmc0";
+		       event-data-type = <(PPMU_RO_DATA_CNT |
+					   PPMU_WO_DATA_CNT)>;
 	       };
        };
 };
@@ -22,6 +26,8 @@
        events {
 	       ppmu_dmc1_3: ppmu-event3-dmc1 {
 		       event-name = "ppmu-event3-dmc1";
+		       event-data-type = <(PPMU_RO_DATA_CNT |
+					   PPMU_WO_DATA_CNT)>;
 	       };
        };
 };
@@ -32,6 +38,8 @@
        events {
 	       ppmu_leftbus_3: ppmu-event3-leftbus {
 		       event-name = "ppmu-event3-leftbus";
+		       event-data-type = <(PPMU_RO_DATA_CNT |
+					   PPMU_WO_DATA_CNT)>;
 	       };
        };
 };
@@ -42,6 +50,8 @@
        events {
 	       ppmu_rightbus_3: ppmu-event3-rightbus {
 		       event-name = "ppmu-event3-rightbus";
+		       event-data-type = <(PPMU_RO_DATA_CNT |
+					   PPMU_WO_DATA_CNT)>;
 	       };
        };
 };
-- 
2.7.4

Re: [PATCH v3 4/4] DT: arm: exynos4412: add event data type which is monitored

[Chanwoo Choi]

Hi,

On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
> The patch adds new field in the PPMU event which shows explicitly
> what kind of data the event is monitoring. It is possible to change it
> using defined values in exynos_ppmu.h file.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-ppmu-common.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi b/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
> index 3a3b2fa..549faba 100644
> --- a/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
> @@ -6,12 +6,16 @@
>   * Author: Chanwoo Choi <cw00.choi@samsung.com>
>   */
>  
> +#include <dt-bindings/pmu/exynos_ppmu.h>
> +
>  &ppmu_dmc0 {
>         status = "okay";
>  
>         events {
>  	       ppmu_dmc0_3: ppmu-event3-dmc0 {
>  		       event-name = "ppmu-event3-dmc0";
> +		       event-data-type = <(PPMU_RO_DATA_CNT |
> +					   PPMU_WO_DATA_CNT)>;
>  	       };
>         };
>  };
> @@ -22,6 +26,8 @@
>         events {
>  	       ppmu_dmc1_3: ppmu-event3-dmc1 {
>  		       event-name = "ppmu-event3-dmc1";
> +		       event-data-type = <(PPMU_RO_DATA_CNT |
> +					   PPMU_WO_DATA_CNT)>;
>  	       };
>         };
>  };
> @@ -32,6 +38,8 @@
>         events {
>  	       ppmu_leftbus_3: ppmu-event3-leftbus {
>  		       event-name = "ppmu-event3-leftbus";
> +		       event-data-type = <(PPMU_RO_DATA_CNT |
> +					   PPMU_WO_DATA_CNT)>;
>  	       };
>         };
>  };
> @@ -42,6 +50,8 @@
>         events {
>  	       ppmu_rightbus_3: ppmu-event3-rightbus {
>  		       event-name = "ppmu-event3-rightbus";
> +		       event-data-type = <(PPMU_RO_DATA_CNT |
> +					   PPMU_WO_DATA_CNT)>;
>  	       };
>         };
>  };
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 4/4] DT: arm: exynos4412: add event data type which is monitored

[Lukasz Luba]

On 4/30/19 8:10 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 4. 19. 오후 10:48, Lukasz Luba wrote:
>> The patch adds new field in the PPMU event which shows explicitly
>> what kind of data the event is monitoring. It is possible to change it
>> using defined values in exynos_ppmu.h file.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   arch/arm/boot/dts/exynos4412-ppmu-common.dtsi | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi b/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
>> index 3a3b2fa..549faba 100644
>> --- a/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412-ppmu-common.dtsi
>> @@ -6,12 +6,16 @@
>>    * Author: Chanwoo Choi <cw00.choi@samsung.com>
>>    */
>>   
>> +#include <dt-bindings/pmu/exynos_ppmu.h>
>> +
>>   &ppmu_dmc0 {
>>          status = "okay";
>>   
>>          events {
>>   	       ppmu_dmc0_3: ppmu-event3-dmc0 {
>>   		       event-name = "ppmu-event3-dmc0";
>> +		       event-data-type = <(PPMU_RO_DATA_CNT |
>> +					   PPMU_WO_DATA_CNT)>;
>>   	       };
>>          };
>>   };
>> @@ -22,6 +26,8 @@
>>          events {
>>   	       ppmu_dmc1_3: ppmu-event3-dmc1 {
>>   		       event-name = "ppmu-event3-dmc1";
>> +		       event-data-type = <(PPMU_RO_DATA_CNT |
>> +					   PPMU_WO_DATA_CNT)>;
>>   	       };
>>          };
>>   };
>> @@ -32,6 +38,8 @@
>>          events {
>>   	       ppmu_leftbus_3: ppmu-event3-leftbus {
>>   		       event-name = "ppmu-event3-leftbus";
>> +		       event-data-type = <(PPMU_RO_DATA_CNT |
>> +					   PPMU_WO_DATA_CNT)>;
>>   	       };
>>          };
>>   };
>> @@ -42,6 +50,8 @@
>>          events {
>>   	       ppmu_rightbus_3: ppmu-event3-rightbus {
>>   		       event-name = "ppmu-event3-rightbus";
>> +		       event-data-type = <(PPMU_RO_DATA_CNT |
>> +					   PPMU_WO_DATA_CNT)>;
>>   	       };
>>          };
>>   };
>>
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Thank you, added to the patch.

Regards,
Lukasz
> 
>