[PATCH 0/3] Exynos Performance Monitoring Counters enhancements

[PATCH 0/3] 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.
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.

Regards,
Lukasz Luba

Lukasz Luba (3):
  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

 .../bindings/devfreq/event/exynos-ppmu.txt         | 18 ++++++++++
 MAINTAINERS                                        |  5 +++
 drivers/devfreq/event/exynos-ppmu.c                | 38 +++++++++++++++-------
 include/dt-bindings/pmu/exynos_ppmu.h              | 24 ++++++++++++++
 include/linux/devfreq-event.h                      |  6 ++++
 5 files changed, 79 insertions(+), 12 deletions(-)
 create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h

-- 
2.7.4

[PATCH 1/3] 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.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 3671fde..f4aac78 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12376,6 +12376,11 @@ L:	linux-scsi@vger.kernel.org
 S:	Supported
 F:	drivers/scsi/pm8001/
 
+PMU EXYNOS DT BINDINGS
+M:	Lukasz Luba <l.luba@partner.samsung.com>
+S:	Maintained
+F:	include/dt-bindings/pmu/exynos_ppmu.h
+
 PNP SUPPORT
 M:	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
 S:	Maintained
diff --git a/include/dt-bindings/pmu/exynos_ppmu.h b/include/dt-bindings/pmu/exynos_ppmu.h
new file mode 100644
index 0000000..a6971eb
--- /dev/null
+++ b/include/dt-bindings/pmu/exynos_ppmu.h
@@ -0,0 +1,24 @@
+/* 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_EVT3_RW_DATA_CNT	0x22
+
+#endif
-- 
2.7.4

[PATCH 2/3] 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 | 38 +++++++++++++++++++++++++------------
 include/linux/devfreq-event.h       |  6 ++++++
 2 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
index c61de0b..e5f81a5 100644
--- a/drivers/devfreq/event/exynos-ppmu.c
+++ b/drivers/devfreq/event/exynos-ppmu.c
@@ -140,6 +140,7 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
 	int id = exynos_ppmu_find_ppmu_id(edev);
 	int ret;
 	u32 pmnc, cntens;
+	u32 evt_sel = edev->desc->data_type;
 
 	if (id < 0)
 		return id;
@@ -154,9 +155,12 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
 	if (ret < 0)
 		return ret;
 
-	/* Set the event of Read/Write data count  */
-	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
-				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
+	/* Check if the data type has been defined in DT, use default if not */
+	if (evt_sel == UINT_MAX)
+		evt_sel = PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT;
+
+	/* Set the event of Read or Write or both (RDWR) data count  */
+	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), evt_sel);
 	if (ret < 0)
 		return ret;
 
@@ -357,6 +361,7 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
 	unsigned int pmnc, cntens;
 	int id = exynos_ppmu_find_ppmu_id(edev);
 	int ret;
+	u32 evt_sel = edev->desc->data_type;
 
 	/* Enable all counters */
 	ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens);
@@ -368,24 +373,28 @@ 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  */
+
+	/* Set the event of proper data type counting.
+	 * Check if the data type has been defined in DT,
+	 * use default if not.
+	 */
 	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;
+		if (evt_sel == UINT_MAX)
+			evt_sel = PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT;
 		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;
+		if (evt_sel == UINT_MAX)
+			evt_sel = PPMU_V2_EVT3_RW_DATA_CNT;
 		break;
 	}
 
+	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), evt_sel);
+	if (ret < 0)
+		return ret;
+
 	/* Reset cycle counter/performance counter and enable PPMU */
 	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
 	if (ret < 0)
@@ -508,6 +517,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 +554,10 @@ 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)
+			desc[i].data_type = UINT_MAX;
 
 		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

[PATCH 3/3] 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 1/3] include: dt-bindings: add Performance Monitoring Unit for Exynos

[Chanwoo Choi]

Hi Lukasz,

On 19. 4. 17. 오후 4: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.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  MAINTAINERS                           |  5 +++++
>  include/dt-bindings/pmu/exynos_ppmu.h | 24 ++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>  create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3671fde..f4aac78 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12376,6 +12376,11 @@ L:	linux-scsi@vger.kernel.org
>  S:	Supported
>  F:	drivers/scsi/pm8001/
>  
> +PMU EXYNOS DT BINDINGS
> +M:	Lukasz Luba <l.luba@partner.samsung.com>
> +S:	Maintained
> +F:	include/dt-bindings/pmu/exynos_ppmu.h
> +

IMHO, I'm not sure that it is right of adding one dt-binding header file
to MAINTAINERS file. If right, I'm OK.


>  PNP SUPPORT
>  M:	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>  S:	Maintained
> diff --git a/include/dt-bindings/pmu/exynos_ppmu.h b/include/dt-bindings/pmu/exynos_ppmu.h
> new file mode 100644
> index 0000000..a6971eb
> --- /dev/null
> +++ b/include/dt-bindings/pmu/exynos_ppmu.h
> @@ -0,0 +1,24 @@
> +/* 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_EVT3_RW_DATA_CNT	0x22

If we can select the data type for monitoring, it looks good to me.
But, when I developed the Exynos PPMU, I has not tested other data type
except for PPMU_RO_DATA_CNT and PPMU_WO_DATA_CNT.

Do you test all data types with PPMU device?

> +
> +#endif
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Lukasz,

On 19. 4. 17. 오후 4: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 | 38 +++++++++++++++++++++++++------------
>  include/linux/devfreq-event.h       |  6 ++++++
>  2 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
> index c61de0b..e5f81a5 100644
> --- a/drivers/devfreq/event/exynos-ppmu.c
> +++ b/drivers/devfreq/event/exynos-ppmu.c
> @@ -140,6 +140,7 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>  	int id = exynos_ppmu_find_ppmu_id(edev);
>  	int ret;
>  	u32 pmnc, cntens;
> +	u32 evt_sel = edev->desc->data_type;
>  
>  	if (id < 0)
>  		return id;
> @@ -154,9 +155,12 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>  	if (ret < 0)
>  		return ret;
>  
> -	/* Set the event of Read/Write data count  */
> -	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
> +	/* Check if the data type has been defined in DT, use default if not */
> +	if (evt_sel == UINT_MAX)
> +		evt_sel = PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT;
> +
> +	/* Set the event of Read or Write or both (RDWR) data count  */
> +	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), evt_sel);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -357,6 +361,7 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>  	unsigned int pmnc, cntens;
>  	int id = exynos_ppmu_find_ppmu_id(edev);
>  	int ret;
> +	u32 evt_sel = edev->desc->data_type;
>  
>  	/* Enable all counters */
>  	ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens);
> @@ -368,24 +373,28 @@ 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  */
> +
> +	/* Set the event of proper data type counting.
> +	 * Check if the data type has been defined in DT,
> +	 * use default if not.
> +	 */
>  	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;
> +		if (evt_sel == UINT_MAX)
> +			evt_sel = PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT;
>  		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;
> +		if (evt_sel == UINT_MAX)
> +			evt_sel = PPMU_V2_EVT3_RW_DATA_CNT;
>  		break;
>  	}
>  
> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), evt_sel);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Reset cycle counter/performance counter and enable PPMU */
>  	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>  	if (ret < 0)
> @@ -508,6 +517,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 +554,10 @@ 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)
> +			desc[i].data_type = UINT_MAX;

I think that it is not proper to use 'UINT_MAX'.
If you want to use the default value, you can initialize
the 'desc[i].data_type' according to the device type or compatible name.

For all exynos SoC except for exynos5433,
	desc[i].data_type = PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT;

For exynos5433,
	desc[i].data_type = PPMU_V2_EVT3_RW_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/.

Why did you make the path name as the 'pmu' instead of 'ppmu'?

It might have some confusion because 'pmu' has
the two meaning in linux kernel.
- PMU, Power Management Unit
- PMU, Performance Monitoring Unit

>   */
>  struct devfreq_event_desc {
>  	const char *name;
> +	u32 data_type;
>  	void *driver_data;
>  
>  	const struct devfreq_event_ops *ops;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Lukasz,

On 19. 4. 17. 오후 4: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

I replied about 'pmu' name on patch2. Please check it.

> +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.
> 
Looks good to me.
But, If you want to add the new 'event-data-type' property,
you better to add 'event-data-type' to Exynos dts file including
PPMU device.

If the dts file contain the data type information,
I think it is easy to understand for the user
what is monitoring on PPMU device.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Lukasz Luba]

Hi Chanwoo,

On 4/17/19 10:10 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 17. 오후 4: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.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   MAINTAINERS                           |  5 +++++
>>   include/dt-bindings/pmu/exynos_ppmu.h | 24 ++++++++++++++++++++++++
>>   2 files changed, 29 insertions(+)
>>   create mode 100644 include/dt-bindings/pmu/exynos_ppmu.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3671fde..f4aac78 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12376,6 +12376,11 @@ L:	linux-scsi@vger.kernel.org
>>   S:	Supported
>>   F:	drivers/scsi/pm8001/
>>   
>> +PMU EXYNOS DT BINDINGS
>> +M:	Lukasz Luba <l.luba@partner.samsung.com>
>> +S:	Maintained
>> +F:	include/dt-bindings/pmu/exynos_ppmu.h
>> +
> 
> IMHO, I'm not sure that it is right of adding one dt-binding header file
> to MAINTAINERS file. If right, I'm OK.
> 
> 
>>   PNP SUPPORT
>>   M:	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>>   S:	Maintained
>> diff --git a/include/dt-bindings/pmu/exynos_ppmu.h b/include/dt-bindings/pmu/exynos_ppmu.h
>> new file mode 100644
>> index 0000000..a6971eb
>> --- /dev/null
>> +++ b/include/dt-bindings/pmu/exynos_ppmu.h
>> @@ -0,0 +1,24 @@
>> +/* 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_EVT3_RW_DATA_CNT	0x22
> 
> If we can select the data type for monitoring, it looks good to me.
> But, when I developed the Exynos PPMU, I has not tested other data type
> except for PPMU_RO_DATA_CNT and PPMU_WO_DATA_CNT.
> 
> Do you test all data types with PPMU device?
Yes, I have tested all - 1 of them on Odroid XU4 with memory controller
and they work. I haven't checked the last option for Exynos5433,
but it should work since it was default option in your driver.

Regards,
Lukasz
> 
>> +
>> +#endif
>>
> 
> 

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

[Lukasz Luba]

Hi Chanwoo,

On 4/17/19 10:22 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 17. 오후 4: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 | 38 +++++++++++++++++++++++++------------
>>   include/linux/devfreq-event.h       |  6 ++++++
>>   2 files changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>> index c61de0b..e5f81a5 100644
>> --- a/drivers/devfreq/event/exynos-ppmu.c
>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>> @@ -140,6 +140,7 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>   	int id = exynos_ppmu_find_ppmu_id(edev);
>>   	int ret;
>>   	u32 pmnc, cntens;
>> +	u32 evt_sel = edev->desc->data_type;
>>   
>>   	if (id < 0)
>>   		return id;
>> @@ -154,9 +155,12 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev)
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	/* Set the event of Read/Write data count  */
>> -	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id),
>> -				PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT);
>> +	/* Check if the data type has been defined in DT, use default if not */
>> +	if (evt_sel == UINT_MAX)
>> +		evt_sel = PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT;
>> +
>> +	/* Set the event of Read or Write or both (RDWR) data count  */
>> +	ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), evt_sel);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -357,6 +361,7 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
>>   	unsigned int pmnc, cntens;
>>   	int id = exynos_ppmu_find_ppmu_id(edev);
>>   	int ret;
>> +	u32 evt_sel = edev->desc->data_type;
>>   
>>   	/* Enable all counters */
>>   	ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens);
>> @@ -368,24 +373,28 @@ 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  */
>> +
>> +	/* Set the event of proper data type counting.
>> +	 * Check if the data type has been defined in DT,
>> +	 * use default if not.
>> +	 */
>>   	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;
>> +		if (evt_sel == UINT_MAX)
>> +			evt_sel = PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT;
>>   		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;
>> +		if (evt_sel == UINT_MAX)
>> +			evt_sel = PPMU_V2_EVT3_RW_DATA_CNT;
>>   		break;
>>   	}
>>   
>> +	ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), evt_sel);
>> +	if (ret < 0)
>> +		return ret;
>> +
>>   	/* Reset cycle counter/performance counter and enable PPMU */
>>   	ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc);
>>   	if (ret < 0)
>> @@ -508,6 +517,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 +554,10 @@ 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)
>> +			desc[i].data_type = UINT_MAX;
> 
> I think that it is not proper to use 'UINT_MAX'.
> If you want to use the default value, you can initialize
> the 'desc[i].data_type' according to the device type or compatible name.
> 
> For all exynos SoC except for exynos5433,
> 	desc[i].data_type = PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT;
> 
> For exynos5433,
> 	desc[i].data_type = PPMU_V2_EVT3_RW_DATA_CNT;
> 
OK, I will set these default values here.

>>   
>>   		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/.
> 
> Why did you make the path name as the 'pmu' instead of 'ppmu'?
The directory may contain in future other Performance Monitoring Unit
subsystems. In Exynos case we have: Platform Performance Monitoring Unit
but different SoCs might have other names, like: System PMU.
Since there is no 'pmu' in this directory, I am allocating it for
Performance Monitoring Unit and not Power Management Unit.

> 
> It might have some confusion because 'pmu' has
> the two meaning in linux kernel.
> - PMU, Power Management Unit
> - PMU, Performance Monitoring Unit
Regards,
Lukasz
> 
>>    */
>>   struct devfreq_event_desc {
>>   	const char *name;
>> +	u32 data_type;
>>   	void *driver_data;
>>   
>>   	const struct devfreq_event_ops *ops;
>>
> 
> 

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

[Lukasz Luba]

Hi Chanwoo,

On 4/17/19 10:26 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 19. 4. 17. 오후 4: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
> 
> I replied about 'pmu' name on patch2. Please check it.
Yes, I've seen it and responded.
> 
>> +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.
>>
> Looks good to me.
> But, If you want to add the new 'event-data-type' property,
> you better to add 'event-data-type' to Exynos dts file including
> PPMU device.
> 
> If the dts file contain the data type information,
> I think it is easy to understand for the user
> what is monitoring on PPMU device.

Make sense, I will do that. It will be explicitly telling the user what
is currently monitored in the PPMU device event.

Regards,
Lukasz
> 
>