[PATCH 0/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2

[PATCH 0/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2

[Chanwoo Choi]

This patch-set add the support of PPMU ((Platform Performance Monitoring Unit)
version 2.0 which is used on Exynos5433. The exynos-ppmu.c driver supports
both PPMUv1.1 and PPMUv2. This patch-set is testd on Exynos5433-based board.

The SoC list using PPMUv1.1
- Exynos4210/4212/4412, Exynos3250, Exynos5260
The SoC list using PPMUv2
- Exynos5433, Exynos7420

Chanwoo Choi (2):
  PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433
  PM / devfreq: exynos-ppmu: Update documentation to support PPMUv2

 .../bindings/devfreq/event/exynos-ppmu.txt         |  42 +++++-
 drivers/devfreq/event/exynos-ppmu.c                | 168 ++++++++++++++++++++-
 drivers/devfreq/event/exynos-ppmu.h                |  70 +++++++++
 3 files changed, 271 insertions(+), 9 deletions(-)

-- 
1.8.5.5

[PATCH 1/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433

[Chanwoo Choi]

This patch adds the support for PPMU (Platform Performance Monitoring Unit)
version 2.0 for Exynos5433 SoC. Exynos5433 SoC must need PPMUv2 which is
quite different from PPMUv1.1. The exynos-ppmu.c driver supports both PPMUv1.1
and PPMUv2.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/event/exynos-ppmu.c | 168 ++++++++++++++++++++++++++++++++++--
 drivers/devfreq/event/exynos-ppmu.h |  70 +++++++++++++++
 2 files changed, 231 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
index 7d99d13bacd8..6066dc18fc73 100644
--- a/drivers/devfreq/event/exynos-ppmu.c
+++ b/drivers/devfreq/event/exynos-ppmu.c
@@ -1,7 +1,7 @@
 /*
  * exynos_ppmu.c - EXYNOS PPMU (Platform Performance Monitoring Unit) support
  *
- * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
  * Author : Chanwoo Choi <cw00.choi@samsung.com>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -82,6 +82,15 @@ struct __exynos_ppmu_events {
 	PPMU_EVENT(mscl),
 	PPMU_EVENT(fimd0x),
 	PPMU_EVENT(fimd1x),
+
+	/* Only for Exynos5433 SoCs */
+	PPMU_EVENT(d0-cpu),
+	PPMU_EVENT(d0-general),
+	PPMU_EVENT(d0-rt),
+	PPMU_EVENT(d1-cpu),
+	PPMU_EVENT(d1-general),
+	PPMU_EVENT(d1-rt),
+
 	{ /* sentinel */ },
 };
 
@@ -96,6 +105,9 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev)
 	return -EINVAL;
 }
 
+/*
+ * The devfreq-event ops structure for PPMU v1.1
+ */
 static int exynos_ppmu_disable(struct devfreq_event_dev *edev)
 {
 	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
@@ -200,6 +212,153 @@ static const struct devfreq_event_ops exynos_ppmu_ops = {
 	.get_event = exynos_ppmu_get_event,
 };
 
+/*
+ * The devfreq-event ops structure for PPMU v2.0
+ */
+static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev)
+{
+	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
+	u32 pmnc, clear;
+
+	/* Disable all counters */
+	clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK
+		| PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK);
+
+	__raw_writel(clear, info->ppmu.base + PPMUv2_FLAG);
+	__raw_writel(clear, info->ppmu.base + PPMUv2_INTENC);
+	__raw_writel(clear, info->ppmu.base + PPMUv2_CNTENC);
+	__raw_writel(clear, info->ppmu.base + PPMUv2_CNT_RESET);
+
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_CFG0);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_CFG1);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_CFG2);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_RESULT);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CNT_AUTO);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV0_TYPE);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV1_TYPE);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV2_TYPE);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV3_TYPE);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_ID_V);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_ID_A);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_OTHERS_V);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_OTHERS_A);
+	__raw_writel(0x0, info->ppmu.base + PPMUv2_INTERRUPT_RESET);
+
+	/* Disable PPMU */
+	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
+	pmnc &= ~PPMU_PMNC_ENABLE_MASK;
+	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
+
+	return 0;
+}
+
+static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
+{
+	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
+	int id = exynos_ppmu_find_ppmu_id(edev);
+	u32 pmnc, cntens;
+
+	/* Enable all counters */
+	cntens = __raw_readl(info->ppmu.base + PPMUv2_CNTENS);
+	cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id));
+	__raw_writel(cntens, info->ppmu.base + PPMUv2_CNTENS);
+
+	/* Set the event of Read/Write data count  */
+	switch (id) {
+	case PPMU_PMNCNT0:
+	case PPMU_PMNCNT1:
+	case PPMU_PMNCNT2:
+		__raw_writel(PPMUv2_RO_DATA_CNT | PPMUv2_WO_DATA_CNT,
+				info->ppmu.base + PPMUv2_CH_EVx_TYPE(id));
+		break;
+	case PPMU_PMNCNT3:
+		__raw_writel(PPMUv2_EVT3_RW_DATA_CNT,
+				info->ppmu.base + PPMUv2_CH_EVx_TYPE(id));
+		break;
+	}
+
+	/* Reset cycle counter/performance counter and enable PPMU */
+	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
+	pmnc &= ~(PPMU_PMNC_ENABLE_MASK
+			| PPMU_PMNC_COUNTER_RESET_MASK
+			| PPMU_PMNC_CC_RESET_MASK
+			| PPMU_PMNC_CC_DIVIDER_MASK
+			| PPMUv2_PMNC_START_MODE_MASK);
+	pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT);
+	pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT);
+	pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT);
+	pmnc |= (PPMUv2_MODE_MANUAL << PPMUv2_PMNC_START_MODE_SHIFT);
+	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
+
+	return 0;
+}
+
+static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev,
+				    struct devfreq_event_data *edata)
+{
+	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
+	int id = exynos_ppmu_find_ppmu_id(edev);
+	u32 pmnc, cntenc;
+	u32 pmcnt_high, pmcnt_low;
+	u64 load_count = 0;
+
+	/* Disable PPMU */
+	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
+	pmnc &= ~PPMU_PMNC_ENABLE_MASK;
+	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
+
+	/* Read cycle count and performance count */
+	edata->total_count = __raw_readl(info->ppmu.base + PPMUv2_CCNT);
+
+	switch (id) {
+	case PPMU_PMNCNT0:
+	case PPMU_PMNCNT1:
+	case PPMU_PMNCNT2:
+		load_count = __raw_readl(info->ppmu.base + PPMUv2_PMNCT(id));
+		break;
+	case PPMU_PMNCNT3:
+		pmcnt_high = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_HIGH);
+		pmcnt_low = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_LOW);
+		load_count = (u64)((pmcnt_high & 0xff) << 32) + (u64)pmcnt_low;
+		break;
+	}
+	edata->load_count = load_count;
+
+	/* Disable all counters */
+	cntenc = __raw_readl(info->ppmu.base + PPMUv2_CNTENC);
+	cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id));
+	__raw_writel(cntenc, info->ppmu.base + PPMUv2_CNTENC);
+
+	dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name,
+					edata->load_count, edata->total_count);
+	return 0;
+}
+
+static struct devfreq_event_ops exynos_ppmu_v2_ops = {
+	.disable = exynos_ppmu_v2_disable,
+	.set_event = exynos_ppmu_v2_set_event,
+	.get_event = exynos_ppmu_v2_get_event,
+};
+
+static struct of_device_id exynos_ppmu_id_match[] = {
+	{
+		.compatible = "samsung,exynos-ppmu",
+		.data = (void *)&exynos_ppmu_ops,
+	}, {
+		.compatible = "samsung,exynos-ppmu-v2",
+		.data = (void *)&exynos_ppmu_v2_ops,
+	},
+	{ /* sentinel */ },
+};
+
+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)
 {
@@ -238,7 +397,7 @@ static int of_get_devfreq_events(struct device_node *np,
 			continue;
 		}
 
-		desc[j].ops = &exynos_ppmu_ops;
+		desc[j].ops = exynos_bus_get_ops(np);
 		desc[j].driver_data = info;
 
 		of_property_read_string(node, "event-name", &desc[j].name);
@@ -354,11 +513,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct of_device_id exynos_ppmu_id_match[] = {
-	{ .compatible = "samsung,exynos-ppmu", },
-	{ /* sentinel */ },
-};
-
 static struct platform_driver exynos_ppmu_driver = {
 	.probe	= exynos_ppmu_probe,
 	.remove	= exynos_ppmu_remove,
diff --git a/drivers/devfreq/event/exynos-ppmu.h b/drivers/devfreq/event/exynos-ppmu.h
index 4e831d48c138..9a7cf6394f37 100644
--- a/drivers/devfreq/event/exynos-ppmu.h
+++ b/drivers/devfreq/event/exynos-ppmu.h
@@ -26,6 +26,9 @@ enum ppmu_counter {
 	PPMU_PMNCNT_MAX,
 };
 
+/***
+ * PPMUv1.1 Definitions
+ */
 enum ppmu_event_type {
 	PPMU_RO_BUSY_CYCLE_CNT	= 0x0,
 	PPMU_WO_BUSY_CYCLE_CNT	= 0x1,
@@ -90,4 +93,71 @@ enum ppmu_reg {
 #define PPMU_PMNCT(x)			(PPMU_PMCNT0 + (0x10 * x))
 #define PPMU_BEVTxSEL(x)		(PPMU_BEVT0SEL + (0x100 * x))
 
+/***
+ * PPMUv2.0 definitions
+ */
+enum ppmuv2_mode {
+	PPMUv2_MODE_MANUAL = 0,
+	PPMUv2_MODE_AUTO = 1,
+	PPMUv2_MODE_CIG = 2,	/* CIG (Conditional Interrupt Generation) */
+};
+
+enum ppmuv2_event_type {
+	PPMUv2_RO_DATA_CNT	= 0x4,
+	PPMUv2_WO_DATA_CNT	= 0x5,
+
+	PPMUv2_EVT3_RW_DATA_CNT = 0x22,	/* Only for Event3 */
+};
+
+enum ppmuv2_reg {
+	/* PPC control register */
+	PPMUv2_PMNC		= 0x04,
+	PPMUv2_CNTENS		= 0x08,
+	PPMUv2_CNTENC		= 0x0c,
+	PPMUv2_INTENS		= 0x10,
+	PPMUv2_INTENC		= 0x14,
+	PPMUv2_FLAG		= 0x18,
+
+	/* Cycle Counter and Performance Event Counter Register */
+	PPMUv2_CCNT		= 0x48,
+	PPMUv2_PMCNT0		= 0x34,
+	PPMUv2_PMCNT1		= 0x38,
+	PPMUv2_PMCNT2		= 0x3c,
+	PPMUv2_PMCNT3_LOW	= 0x40,
+	PPMUv2_PMCNT3_HIGH	= 0x44,
+
+	/* Bus Event Generator */
+	PPMUv2_CIG_CFG0		= 0x1c,
+	PPMUv2_CIG_CFG1		= 0x20,
+	PPMUv2_CIG_CFG2		= 0x24,
+	PPMUv2_CIG_RESULT	= 0x28,
+	PPMUv2_CNT_RESET	= 0x2c,
+	PPMUv2_CNT_AUTO		= 0x30,
+	PPMUv2_CH_EV0_TYPE	= 0x200,
+	PPMUv2_CH_EV1_TYPE	= 0x204,
+	PPMUv2_CH_EV2_TYPE	= 0x208,
+	PPMUv2_CH_EV3_TYPE	= 0x20c,
+	PPMUv2_SM_ID_V		= 0x220,
+	PPMUv2_SM_ID_A		= 0x224,
+	PPMUv2_SM_OTHERS_V	= 0x228,
+	PPMUv2_SM_OTHERS_A	= 0x22c,
+	PPMUv2_INTERRUPT_RESET	= 0x260,
+};
+
+/* PMNC register */
+#define PPMUv2_PMNC_START_MODE_SHIFT	20
+#define PPMUv2_PMNC_START_MODE_MASK	(0x3 << PPMUv2_PMNC_START_MODE_SHIFT)
+
+#define PPMU_PMNC_CC_RESET_SHIFT	2
+#define PPMU_PMNC_COUNTER_RESET_SHIFT	1
+#define PPMU_PMNC_ENABLE_SHIFT		0
+#define PPMU_PMNC_START_MODE_MASK	BIT(16)
+#define PPMU_PMNC_CC_DIVIDER_MASK	BIT(3)
+#define PPMU_PMNC_CC_RESET_MASK		BIT(2)
+#define PPMU_PMNC_COUNTER_RESET_MASK	BIT(1)
+#define PPMU_PMNC_ENABLE_MASK		BIT(0)
+
+#define PPMUv2_PMNCT(x)			(PPMUv2_PMCNT0 + (0x4 * x))
+#define PPMUv2_CH_EVx_TYPE(x)		(PPMUv2_CH_EV0_TYPE + (0x4 * x))
+
 #endif /* __EXYNOS_PPMU_H__ */
-- 
1.8.5.5

[PATCH 2/2] PM / devfreq: exynos-ppmu: Update documentation to support PPMUv2

[Chanwoo Choi]

This patch updates the documentation to include the information of PPMUv2.
The PPMUv2 is used for Exynos5433 and Exynos7420 to monitor the performance
of each IP in Exynos SoC.

Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 .../bindings/devfreq/event/exynos-ppmu.txt         | 42 ++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
index b54bf3a2ff57..e8fa6b6a1827 100644
--- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
+++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
@@ -11,7 +11,7 @@ to various devfreq devices. The devfreq devices would use the event data when
 derterming the current state of each IP.
 
 Required properties:
-- compatible: Should be "samsung,exynos-ppmu".
+- 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:
@@ -19,7 +19,7 @@ Optional properties:
 - clocks : phandles for clock specified in "clock-names" property
 - #clock-cells: should be 1.
 
-Example1 : PPMU nodes in exynos3250.dtsi are listed below.
+Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
 
 		ppmu_dmc0: ppmu_dmc0@106a0000 {
 			compatible = "samsung,exynos-ppmu";
@@ -108,3 +108,41 @@ Example2 : Events of each PPMU node in exynos3250-rinato.dts are listed below.
 			};
 		};
 	};
+
+Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
+
+		ppmu_d0_cpu: ppmu_d0_cpu@10480000 {
+			compatible = "samsung,exynos-ppmu-v2";
+			reg = <0x10480000 0x2000>;
+			status = "disabled";
+		};
+
+		ppmu_d0_general: ppmu_d0_general@10490000 {
+			compatible = "samsung,exynos-ppmu-v2";
+			reg = <0x10490000 0x2000>;
+			status = "disabled";
+		};
+
+		ppmu_d0_rt: ppmu_d0_rt@104a0000 {
+			compatible = "samsung,exynos-ppmu-v2";
+			reg = <0x104a0000 0x2000>;
+			status = "disabled";
+		};
+
+		ppmu_d1_cpu: ppmu_d1_cpu@104b0000 {
+			compatible = "samsung,exynos-ppmu-v2";
+			reg = <0x104b0000 0x2000>;
+			status = "disabled";
+		};
+
+		ppmu_d1_general: ppmu_d1_general@104c0000 {
+			compatible = "samsung,exynos-ppmu-v2";
+			reg = <0x104c0000 0x2000>;
+			status = "disabled";
+		};
+
+		ppmu_d1_rt: ppmu_d1_rt@104d0000 {
+			compatible = "samsung,exynos-ppmu-v2";
+			reg = <0x104d0000 0x2000>;
+			status = "disabled";
+		};
-- 
1.8.5.5

Re: [PATCH 1/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433

[Krzysztof Kozlowski]

On 23.07.2015 10:57, Chanwoo Choi wrote:
> This patch adds the support for PPMU (Platform Performance Monitoring Unit)
> version 2.0 for Exynos5433 SoC. Exynos5433 SoC must need PPMUv2 which is
> quite different from PPMUv1.1. The exynos-ppmu.c driver supports both PPMUv1.1
> and PPMUv2.
> 
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>

Hi,

Few comments at the end.

> ---
>  drivers/devfreq/event/exynos-ppmu.c | 168 ++++++++++++++++++++++++++++++++++--
>  drivers/devfreq/event/exynos-ppmu.h |  70 +++++++++++++++
>  2 files changed, 231 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
> index 7d99d13bacd8..6066dc18fc73 100644
> --- a/drivers/devfreq/event/exynos-ppmu.c
> +++ b/drivers/devfreq/event/exynos-ppmu.c
> @@ -1,7 +1,7 @@
>  /*
>   * exynos_ppmu.c - EXYNOS PPMU (Platform Performance Monitoring Unit) support
>   *
> - * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
>   * Author : Chanwoo Choi <cw00.choi@samsung.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -82,6 +82,15 @@ struct __exynos_ppmu_events {
>  	PPMU_EVENT(mscl),
>  	PPMU_EVENT(fimd0x),
>  	PPMU_EVENT(fimd1x),
> +
> +	/* Only for Exynos5433 SoCs */
> +	PPMU_EVENT(d0-cpu),
> +	PPMU_EVENT(d0-general),
> +	PPMU_EVENT(d0-rt),
> +	PPMU_EVENT(d1-cpu),
> +	PPMU_EVENT(d1-general),
> +	PPMU_EVENT(d1-rt),
> +
>  	{ /* sentinel */ },
>  };
>  
> @@ -96,6 +105,9 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev)
>  	return -EINVAL;
>  }
>  
> +/*
> + * The devfreq-event ops structure for PPMU v1.1
> + */
>  static int exynos_ppmu_disable(struct devfreq_event_dev *edev)
>  {
>  	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
> @@ -200,6 +212,153 @@ static const struct devfreq_event_ops exynos_ppmu_ops = {
>  	.get_event = exynos_ppmu_get_event,
>  };
>  
> +/*
> + * The devfreq-event ops structure for PPMU v2.0
> + */
> +static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev)
> +{
> +	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
> +	u32 pmnc, clear;
> +
> +	/* Disable all counters */
> +	clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK
> +		| PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK);
> +
> +	__raw_writel(clear, info->ppmu.base + PPMUv2_FLAG);
> +	__raw_writel(clear, info->ppmu.base + PPMUv2_INTENC);
> +	__raw_writel(clear, info->ppmu.base + PPMUv2_CNTENC);
> +	__raw_writel(clear, info->ppmu.base + PPMUv2_CNT_RESET);
> +
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_CFG0);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_CFG1);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_CFG2);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CIG_RESULT);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CNT_AUTO);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV0_TYPE);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV1_TYPE);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV2_TYPE);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_CH_EV3_TYPE);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_ID_V);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_ID_A);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_OTHERS_V);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_SM_OTHERS_A);
> +	__raw_writel(0x0, info->ppmu.base + PPMUv2_INTERRUPT_RESET);
> +
> +	/* Disable PPMU */
> +	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
> +	pmnc &= ~PPMU_PMNC_ENABLE_MASK;
> +	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
> +
> +	return 0;
> +}
> +
> +static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev)
> +{
> +	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
> +	int id = exynos_ppmu_find_ppmu_id(edev);
> +	u32 pmnc, cntens;
> +
> +	/* Enable all counters */
> +	cntens = __raw_readl(info->ppmu.base + PPMUv2_CNTENS);
> +	cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id));
> +	__raw_writel(cntens, info->ppmu.base + PPMUv2_CNTENS);
> +
> +	/* Set the event of Read/Write data count  */
> +	switch (id) {
> +	case PPMU_PMNCNT0:
> +	case PPMU_PMNCNT1:
> +	case PPMU_PMNCNT2:
> +		__raw_writel(PPMUv2_RO_DATA_CNT | PPMUv2_WO_DATA_CNT,
> +				info->ppmu.base + PPMUv2_CH_EVx_TYPE(id));
> +		break;
> +	case PPMU_PMNCNT3:
> +		__raw_writel(PPMUv2_EVT3_RW_DATA_CNT,
> +				info->ppmu.base + PPMUv2_CH_EVx_TYPE(id));
> +		break;
> +	}
> +
> +	/* Reset cycle counter/performance counter and enable PPMU */
> +	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
> +	pmnc &= ~(PPMU_PMNC_ENABLE_MASK
> +			| PPMU_PMNC_COUNTER_RESET_MASK
> +			| PPMU_PMNC_CC_RESET_MASK
> +			| PPMU_PMNC_CC_DIVIDER_MASK
> +			| PPMUv2_PMNC_START_MODE_MASK);
> +	pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT);
> +	pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT);
> +	pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT);
> +	pmnc |= (PPMUv2_MODE_MANUAL << PPMUv2_PMNC_START_MODE_SHIFT);
> +	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
> +
> +	return 0;
> +}
> +
> +static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev,
> +				    struct devfreq_event_data *edata)
> +{
> +	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
> +	int id = exynos_ppmu_find_ppmu_id(edev);
> +	u32 pmnc, cntenc;
> +	u32 pmcnt_high, pmcnt_low;
> +	u64 load_count = 0;
> +
> +	/* Disable PPMU */
> +	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
> +	pmnc &= ~PPMU_PMNC_ENABLE_MASK;
> +	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
> +
> +	/* Read cycle count and performance count */
> +	edata->total_count = __raw_readl(info->ppmu.base + PPMUv2_CCNT);
> +
> +	switch (id) {
> +	case PPMU_PMNCNT0:
> +	case PPMU_PMNCNT1:
> +	case PPMU_PMNCNT2:
> +		load_count = __raw_readl(info->ppmu.base + PPMUv2_PMNCT(id));
> +		break;
> +	case PPMU_PMNCNT3:
> +		pmcnt_high = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_HIGH);
> +		pmcnt_low = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_LOW);
> +		load_count = (u64)((pmcnt_high & 0xff) << 32) + (u64)pmcnt_low;
> +		break;
> +	}
> +	edata->load_count = load_count;
> +
> +	/* Disable all counters */
> +	cntenc = __raw_readl(info->ppmu.base + PPMUv2_CNTENC);
> +	cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id));
> +	__raw_writel(cntenc, info->ppmu.base + PPMUv2_CNTENC);
> +
> +	dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name,
> +					edata->load_count, edata->total_count);
> +	return 0;
> +}
> +
> +static struct devfreq_event_ops exynos_ppmu_v2_ops = {

This can be const.

> +	.disable = exynos_ppmu_v2_disable,
> +	.set_event = exynos_ppmu_v2_set_event,
> +	.get_event = exynos_ppmu_v2_get_event,
> +};
> +
> +static struct of_device_id exynos_ppmu_id_match[] = {

In the previous patch this was not present but now it can be made 'const'.

> +	{
> +		.compatible = "samsung,exynos-ppmu",
> +		.data = (void *)&exynos_ppmu_ops,
> +	}, {
> +		.compatible = "samsung,exynos-ppmu-v2",
> +		.data = (void *)&exynos_ppmu_v2_ops,
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +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)
>  {
> @@ -238,7 +397,7 @@ static int of_get_devfreq_events(struct device_node *np,
>  			continue;
>  		}
>  
> -		desc[j].ops = &exynos_ppmu_ops;
> +		desc[j].ops = exynos_bus_get_ops(np);

So for each ops callback (get/set/disable) you will have another layer
of indirection where you will be matching the device each time?

That seems like a waste of CPU time. Just match it once here and use
either old ops or new ops_v2.


>  		desc[j].driver_data = info;
>  
>  		of_property_read_string(node, "event-name", &desc[j].name);
> @@ -354,11 +513,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static struct of_device_id exynos_ppmu_id_match[] = {
> -	{ .compatible = "samsung,exynos-ppmu", },
> -	{ /* sentinel */ },
> -};
> -
>  static struct platform_driver exynos_ppmu_driver = {
>  	.probe	= exynos_ppmu_probe,
>  	.remove	= exynos_ppmu_remove,
> diff --git a/drivers/devfreq/event/exynos-ppmu.h b/drivers/devfreq/event/exynos-ppmu.h
> index 4e831d48c138..9a7cf6394f37 100644
> --- a/drivers/devfreq/event/exynos-ppmu.h
> +++ b/drivers/devfreq/event/exynos-ppmu.h
> @@ -26,6 +26,9 @@ enum ppmu_counter {
>  	PPMU_PMNCNT_MAX,
>  };
>  
> +/***
> + * PPMUv1.1 Definitions
> + */
>  enum ppmu_event_type {
>  	PPMU_RO_BUSY_CYCLE_CNT	= 0x0,
>  	PPMU_WO_BUSY_CYCLE_CNT	= 0x1,
> @@ -90,4 +93,71 @@ enum ppmu_reg {
>  #define PPMU_PMNCT(x)			(PPMU_PMCNT0 + (0x10 * x))
>  #define PPMU_BEVTxSEL(x)		(PPMU_BEVT0SEL + (0x100 * x))
>  
> +/***
> + * PPMUv2.0 definitions
> + */
> +enum ppmuv2_mode {
> +	PPMUv2_MODE_MANUAL = 0,
> +	PPMUv2_MODE_AUTO = 1,
> +	PPMUv2_MODE_CIG = 2,	/* CIG (Conditional Interrupt Generation) */

Mixing lower-upper case looks odd to me. How about:
PPMU2_MODE_MANUAL = 0,
or
PPMU_V2_MODE_MANUAL = 0,
?

(here and everywhere below)

Best regards,
Krzysztof

> +};
> +
> +enum ppmuv2_event_type {
> +	PPMUv2_RO_DATA_CNT	= 0x4,
> +	PPMUv2_WO_DATA_CNT	= 0x5,
> +
> +	PPMUv2_EVT3_RW_DATA_CNT = 0x22,	/* Only for Event3 */
> +};
> +
> +enum ppmuv2_reg {
> +	/* PPC control register */
> +	PPMUv2_PMNC		= 0x04,
> +	PPMUv2_CNTENS		= 0x08,
> +	PPMUv2_CNTENC		= 0x0c,
> +	PPMUv2_INTENS		= 0x10,
> +	PPMUv2_INTENC		= 0x14,
> +	PPMUv2_FLAG		= 0x18,
> +
> +	/* Cycle Counter and Performance Event Counter Register */
> +	PPMUv2_CCNT		= 0x48,
> +	PPMUv2_PMCNT0		= 0x34,
> +	PPMUv2_PMCNT1		= 0x38,
> +	PPMUv2_PMCNT2		= 0x3c,
> +	PPMUv2_PMCNT3_LOW	= 0x40,
> +	PPMUv2_PMCNT3_HIGH	= 0x44,
> +
> +	/* Bus Event Generator */
> +	PPMUv2_CIG_CFG0		= 0x1c,
> +	PPMUv2_CIG_CFG1		= 0x20,
> +	PPMUv2_CIG_CFG2		= 0x24,
> +	PPMUv2_CIG_RESULT	= 0x28,
> +	PPMUv2_CNT_RESET	= 0x2c,
> +	PPMUv2_CNT_AUTO		= 0x30,
> +	PPMUv2_CH_EV0_TYPE	= 0x200,
> +	PPMUv2_CH_EV1_TYPE	= 0x204,
> +	PPMUv2_CH_EV2_TYPE	= 0x208,
> +	PPMUv2_CH_EV3_TYPE	= 0x20c,
> +	PPMUv2_SM_ID_V		= 0x220,
> +	PPMUv2_SM_ID_A		= 0x224,
> +	PPMUv2_SM_OTHERS_V	= 0x228,
> +	PPMUv2_SM_OTHERS_A	= 0x22c,
> +	PPMUv2_INTERRUPT_RESET	= 0x260,
> +};
> +
> +/* PMNC register */
> +#define PPMUv2_PMNC_START_MODE_SHIFT	20
> +#define PPMUv2_PMNC_START_MODE_MASK	(0x3 << PPMUv2_PMNC_START_MODE_SHIFT)
> +
> +#define PPMU_PMNC_CC_RESET_SHIFT	2
> +#define PPMU_PMNC_COUNTER_RESET_SHIFT	1
> +#define PPMU_PMNC_ENABLE_SHIFT		0
> +#define PPMU_PMNC_START_MODE_MASK	BIT(16)
> +#define PPMU_PMNC_CC_DIVIDER_MASK	BIT(3)
> +#define PPMU_PMNC_CC_RESET_MASK		BIT(2)
> +#define PPMU_PMNC_COUNTER_RESET_MASK	BIT(1)
> +#define PPMU_PMNC_ENABLE_MASK		BIT(0)
> +
> +#define PPMUv2_PMNCT(x)			(PPMUv2_PMCNT0 + (0x4 * x))
> +#define PPMUv2_CH_EVx_TYPE(x)		(PPMUv2_CH_EV0_TYPE + (0x4 * x))
> +
>  #endif /* __EXYNOS_PPMU_H__ */
> 

Re: [PATCH 2/2] PM / devfreq: exynos-ppmu: Update documentation to support PPMUv2

[Krzysztof Kozlowski]

On 23.07.2015 10:57, Chanwoo Choi wrote:
> This patch updates the documentation to include the information of PPMUv2.
> The PPMUv2 is used for Exynos5433 and Exynos7420 to monitor the performance
> of each IP in Exynos SoC.
> 
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  .../bindings/devfreq/event/exynos-ppmu.txt         | 42 ++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> index b54bf3a2ff57..e8fa6b6a1827 100644
> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
> @@ -11,7 +11,7 @@ to various devfreq devices. The devfreq devices would use the event data when
>  derterming the current state of each IP.
>  
>  Required properties:
> -- compatible: Should be "samsung,exynos-ppmu".
> +- compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.

I have doubts about "-v2" suffix. Why not using SoCs compatible suffix?
Is it related to ARM Performance Monitoring Unit v2 or maybe Samsung
just labelled it v2 for marketing purposes?

Best regards,
Krzysztof

>  - reg: physical base address of each PPMU and length of memory mapped region.
>  
>  Optional properties:
> @@ -19,7 +19,7 @@ Optional properties:
>  - clocks : phandles for clock specified in "clock-names" property
>  - #clock-cells: should be 1.
>  
> -Example1 : PPMU nodes in exynos3250.dtsi are listed below.
> +Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
>  
>  		ppmu_dmc0: ppmu_dmc0@106a0000 {
>  			compatible = "samsung,exynos-ppmu";
> @@ -108,3 +108,41 @@ Example2 : Events of each PPMU node in exynos3250-rinato.dts are listed below.
>  			};
>  		};
>  	};
> +
> +Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
> +
> +		ppmu_d0_cpu: ppmu_d0_cpu@10480000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x10480000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d0_general: ppmu_d0_general@10490000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x10490000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d0_rt: ppmu_d0_rt@104a0000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x104a0000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d1_cpu: ppmu_d1_cpu@104b0000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x104b0000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d1_general: ppmu_d1_general@104c0000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x104c0000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d1_rt: ppmu_d1_rt@104d0000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x104d0000 0x2000>;
> +			status = "disabled";
> +		};
> 

Re: [PATCH 2/2] PM / devfreq: exynos-ppmu: Update documentation to support PPMUv2

[Chanwoo Choi]

On 07/23/2015 04:37 PM, Krzysztof Kozlowski wrote:
> On 23.07.2015 10:57, Chanwoo Choi wrote:
>> This patch updates the documentation to include the information of PPMUv2.
>> The PPMUv2 is used for Exynos5433 and Exynos7420 to monitor the performance
>> of each IP in Exynos SoC.
>>
>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  .../bindings/devfreq/event/exynos-ppmu.txt         | 42 ++++++++++++++++++++--
>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> index b54bf3a2ff57..e8fa6b6a1827 100644
>> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>> @@ -11,7 +11,7 @@ to various devfreq devices. The devfreq devices would use the event data when
>>  derterming the current state of each IP.
>>  
>>  Required properties:
>> -- compatible: Should be "samsung,exynos-ppmu".
>> +- compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.
> 
> I have doubts about "-v2" suffix. Why not using SoCs compatible suffix?
> Is it related to ARM Performance Monitoring Unit v2 or maybe Samsung
> just labelled it v2 for marketing purposes?

As I knew, the version of PPMU was decided by SoC designer without any dependency.

If we prefer to use the SoC name instead of '-v2' version on compatible name,
we can change them as following:
- samsung,exynos-ppmu -> samsung,exynos4210-ppmu
  (because Exynos4210 used the PPMU v1 for the first time)
- samsung,exynos-ppmu-v2 -> samsung,exynos5433-ppmu
  (because Exynos5433 used the PPMU v2 for the first time)

Best Regards,
Chanwoo Choi

> 
> Best regards,
> Krzysztof
> 
>>  - reg: physical base address of each PPMU and length of memory mapped region.
>>  
>>  Optional properties:
>> @@ -19,7 +19,7 @@ Optional properties:
>>  - clocks : phandles for clock specified in "clock-names" property
>>  - #clock-cells: should be 1.
>>  
>> -Example1 : PPMU nodes in exynos3250.dtsi are listed below.
>> +Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
>>  
>>  		ppmu_dmc0: ppmu_dmc0@106a0000 {
>>  			compatible = "samsung,exynos-ppmu";
>> @@ -108,3 +108,41 @@ Example2 : Events of each PPMU node in exynos3250-rinato.dts are listed below.
>>  			};
>>  		};
>>  	};
>> +
>> +Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>> +
>> +		ppmu_d0_cpu: ppmu_d0_cpu@10480000 {
>> +			compatible = "samsung,exynos-ppmu-v2";
>> +			reg = <0x10480000 0x2000>;
>> +			status = "disabled";
>> +		};
>> +
>> +		ppmu_d0_general: ppmu_d0_general@10490000 {
>> +			compatible = "samsung,exynos-ppmu-v2";
>> +			reg = <0x10490000 0x2000>;
>> +			status = "disabled";
>> +		};
>> +
>> +		ppmu_d0_rt: ppmu_d0_rt@104a0000 {
>> +			compatible = "samsung,exynos-ppmu-v2";
>> +			reg = <0x104a0000 0x2000>;
>> +			status = "disabled";
>> +		};
>> +
>> +		ppmu_d1_cpu: ppmu_d1_cpu@104b0000 {
>> +			compatible = "samsung,exynos-ppmu-v2";
>> +			reg = <0x104b0000 0x2000>;
>> +			status = "disabled";
>> +		};
>> +
>> +		ppmu_d1_general: ppmu_d1_general@104c0000 {
>> +			compatible = "samsung,exynos-ppmu-v2";
>> +			reg = <0x104c0000 0x2000>;
>> +			status = "disabled";
>> +		};
>> +
>> +		ppmu_d1_rt: ppmu_d1_rt@104d0000 {
>> +			compatible = "samsung,exynos-ppmu-v2";
>> +			reg = <0x104d0000 0x2000>;
>> +			status = "disabled";
>> +		};
>>
> 
> 

Re: [PATCH 2/2] PM / devfreq: exynos-ppmu: Update documentation to support PPMUv2

[Krzysztof Kozlowski]

On 23.07.2015 16:48, Chanwoo Choi wrote:
> On 07/23/2015 04:37 PM, Krzysztof Kozlowski wrote:
>> On 23.07.2015 10:57, Chanwoo Choi wrote:
>>> This patch updates the documentation to include the information of PPMUv2.
>>> The PPMUv2 is used for Exynos5433 and Exynos7420 to monitor the performance
>>> of each IP in Exynos SoC.
>>>
>>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  .../bindings/devfreq/event/exynos-ppmu.txt         | 42 ++++++++++++++++++++--
>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>>> index b54bf3a2ff57..e8fa6b6a1827 100644
>>> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>>> @@ -11,7 +11,7 @@ to various devfreq devices. The devfreq devices would use the event data when
>>>  derterming the current state of each IP.
>>>  
>>>  Required properties:
>>> -- compatible: Should be "samsung,exynos-ppmu".
>>> +- compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.
>>
>> I have doubts about "-v2" suffix. Why not using SoCs compatible suffix?
>> Is it related to ARM Performance Monitoring Unit v2 or maybe Samsung
>> just labelled it v2 for marketing purposes?
> 
> As I knew, the version of PPMU was decided by SoC designer without any dependency.
> 
> If we prefer to use the SoC name instead of '-v2' version on compatible name,
> we can change them as following:
> - samsung,exynos-ppmu -> samsung,exynos4210-ppmu
>   (because Exynos4210 used the PPMU v1 for the first time)
> - samsung,exynos-ppmu-v2 -> samsung,exynos5433-ppmu
>   (because Exynos5433 used the PPMU v2 for the first time)

Does the Exynos5433 datasheet mention the "version 2"? I could not find
such annotation in mine.

Krzysztof

> 
> Best Regards,
> Chanwoo Choi
> 
>>
>> Best regards,
>> Krzysztof
>>
>>>  - reg: physical base address of each PPMU and length of memory mapped region.
>>>  
>>>  Optional properties:
>>> @@ -19,7 +19,7 @@ Optional properties:
>>>  - clocks : phandles for clock specified in "clock-names" property
>>>  - #clock-cells: should be 1.
>>>  
>>> -Example1 : PPMU nodes in exynos3250.dtsi are listed below.
>>> +Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
>>>  
>>>  		ppmu_dmc0: ppmu_dmc0@106a0000 {
>>>  			compatible = "samsung,exynos-ppmu";
>>> @@ -108,3 +108,41 @@ Example2 : Events of each PPMU node in exynos3250-rinato.dts are listed below.
>>>  			};
>>>  		};
>>>  	};
>>> +
>>> +Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>>> +
>>> +		ppmu_d0_cpu: ppmu_d0_cpu@10480000 {
>>> +			compatible = "samsung,exynos-ppmu-v2";
>>> +			reg = <0x10480000 0x2000>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		ppmu_d0_general: ppmu_d0_general@10490000 {
>>> +			compatible = "samsung,exynos-ppmu-v2";
>>> +			reg = <0x10490000 0x2000>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		ppmu_d0_rt: ppmu_d0_rt@104a0000 {
>>> +			compatible = "samsung,exynos-ppmu-v2";
>>> +			reg = <0x104a0000 0x2000>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		ppmu_d1_cpu: ppmu_d1_cpu@104b0000 {
>>> +			compatible = "samsung,exynos-ppmu-v2";
>>> +			reg = <0x104b0000 0x2000>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		ppmu_d1_general: ppmu_d1_general@104c0000 {
>>> +			compatible = "samsung,exynos-ppmu-v2";
>>> +			reg = <0x104c0000 0x2000>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		ppmu_d1_rt: ppmu_d1_rt@104d0000 {
>>> +			compatible = "samsung,exynos-ppmu-v2";
>>> +			reg = <0x104d0000 0x2000>;
>>> +			status = "disabled";
>>> +		};
>>>
>>
>>
> 
> 

Re: [PATCH 2/2] PM / devfreq: exynos-ppmu: Update documentation to support PPMUv2

[Chanwoo Choi]

On 07/23/2015 04:58 PM, Krzysztof Kozlowski wrote:
> On 23.07.2015 16:48, Chanwoo Choi wrote:
>> On 07/23/2015 04:37 PM, Krzysztof Kozlowski wrote:
>>> On 23.07.2015 10:57, Chanwoo Choi wrote:
>>>> This patch updates the documentation to include the information of PPMUv2.
>>>> The PPMUv2 is used for Exynos5433 and Exynos7420 to monitor the performance
>>>> of each IP in Exynos SoC.
>>>>
>>>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  .../bindings/devfreq/event/exynos-ppmu.txt         | 42 ++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>>>> index b54bf3a2ff57..e8fa6b6a1827 100644
>>>> --- a/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>>>> +++ b/Documentation/devicetree/bindings/devfreq/event/exynos-ppmu.txt
>>>> @@ -11,7 +11,7 @@ to various devfreq devices. The devfreq devices would use the event data when
>>>>  derterming the current state of each IP.
>>>>  
>>>>  Required properties:
>>>> -- compatible: Should be "samsung,exynos-ppmu".
>>>> +- compatible: Should be "samsung,exynos-ppmu" or "samsung,exynos-ppmu-v2.
>>>
>>> I have doubts about "-v2" suffix. Why not using SoCs compatible suffix?
>>> Is it related to ARM Performance Monitoring Unit v2 or maybe Samsung
>>> just labelled it v2 for marketing purposes?
>>
>> As I knew, the version of PPMU was decided by SoC designer without any dependency.
>>
>> If we prefer to use the SoC name instead of '-v2' version on compatible name,
>> we can change them as following:
>> - samsung,exynos-ppmu -> samsung,exynos4210-ppmu
>>   (because Exynos4210 used the PPMU v1 for the first time)
>> - samsung,exynos-ppmu-v2 -> samsung,exynos5433-ppmu
>>   (because Exynos5433 used the PPMU v2 for the first time)
> 
> Does the Exynos5433 datasheet mention the "version 2"? I could not find
> such annotation in mine.

Yes, Exynos5433 datasheet don't include the detailed information of PPMU.
I check the version of PPMU on the released kernel source by System L.S.I.

> 
> Krzysztof
> 
>>
>> Best Regards,
>> Chanwoo Choi
>>
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>>>  - reg: physical base address of each PPMU and length of memory mapped region.
>>>>  
>>>>  Optional properties:
>>>> @@ -19,7 +19,7 @@ Optional properties:
>>>>  - clocks : phandles for clock specified in "clock-names" property
>>>>  - #clock-cells: should be 1.
>>>>  
>>>> -Example1 : PPMU nodes in exynos3250.dtsi are listed below.
>>>> +Example1 : PPMUv1 nodes in exynos3250.dtsi are listed below.
>>>>  
>>>>  		ppmu_dmc0: ppmu_dmc0@106a0000 {
>>>>  			compatible = "samsung,exynos-ppmu";
>>>> @@ -108,3 +108,41 @@ Example2 : Events of each PPMU node in exynos3250-rinato.dts are listed below.
>>>>  			};
>>>>  		};
>>>>  	};
>>>> +
>>>> +Example3 : PPMUv2 nodes in exynos5433.dtsi are listed below.
>>>> +
>>>> +		ppmu_d0_cpu: ppmu_d0_cpu@10480000 {
>>>> +			compatible = "samsung,exynos-ppmu-v2";
>>>> +			reg = <0x10480000 0x2000>;
>>>> +			status = "disabled";
>>>> +		};
>>>> +
>>>> +		ppmu_d0_general: ppmu_d0_general@10490000 {
>>>> +			compatible = "samsung,exynos-ppmu-v2";
>>>> +			reg = <0x10490000 0x2000>;
>>>> +			status = "disabled";
>>>> +		};
>>>> +
>>>> +		ppmu_d0_rt: ppmu_d0_rt@104a0000 {
>>>> +			compatible = "samsung,exynos-ppmu-v2";
>>>> +			reg = <0x104a0000 0x2000>;
>>>> +			status = "disabled";
>>>> +		};
>>>> +
>>>> +		ppmu_d1_cpu: ppmu_d1_cpu@104b0000 {
>>>> +			compatible = "samsung,exynos-ppmu-v2";
>>>> +			reg = <0x104b0000 0x2000>;
>>>> +			status = "disabled";
>>>> +		};
>>>> +
>>>> +		ppmu_d1_general: ppmu_d1_general@104c0000 {
>>>> +			compatible = "samsung,exynos-ppmu-v2";
>>>> +			reg = <0x104c0000 0x2000>;
>>>> +			status = "disabled";
>>>> +		};
>>>> +
>>>> +		ppmu_d1_rt: ppmu_d1_rt@104d0000 {
>>>> +			compatible = "samsung,exynos-ppmu-v2";
>>>> +			reg = <0x104d0000 0x2000>;
>>>> +			status = "disabled";
>>>> +		};
>>>>
>>>
>>>
>>
>>
> 
> 

Re: [PATCH 1/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433

[Chanwoo Choi]

Hi Krzysztof,

On 07/23/2015 04:28 PM, Krzysztof Kozlowski wrote:
> On 23.07.2015 10:57, Chanwoo Choi wrote:
>> This patch adds the support for PPMU (Platform Performance Monitoring Unit)
>> version 2.0 for Exynos5433 SoC. Exynos5433 SoC must need PPMUv2 which is
>> quite different from PPMUv1.1. The exynos-ppmu.c driver supports both PPMUv1.1
>> and PPMUv2.
>>
>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> Hi,
> 
> Few comments at the end.
> 
>> ---
>>  drivers/devfreq/event/exynos-ppmu.c | 168 ++++++++++++++++++++++++++++++++++--
>>  drivers/devfreq/event/exynos-ppmu.h |  70 +++++++++++++++
>>  2 files changed, 231 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>> index 7d99d13bacd8..6066dc18fc73 100644
>> --- a/drivers/devfreq/event/exynos-ppmu.c
>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * exynos_ppmu.c - EXYNOS PPMU (Platform Performance Monitoring Unit) support
>>   *
>> - * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
>>   * Author : Chanwoo Choi <cw00.choi@samsung.com>
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>> @@ -82,6 +82,15 @@ struct __exynos_ppmu_events {
>>  	PPMU_EVENT(mscl),
>>  	PPMU_EVENT(fimd0x),
>>  	PPMU_EVENT(fimd1x),

(snip)

>> +static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev,
>> +				    struct devfreq_event_data *edata)
>> +{
>> +	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
>> +	int id = exynos_ppmu_find_ppmu_id(edev);
>> +	u32 pmnc, cntenc;
>> +	u32 pmcnt_high, pmcnt_low;
>> +	u64 load_count = 0;
>> +
>> +	/* Disable PPMU */
>> +	pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
>> +	pmnc &= ~PPMU_PMNC_ENABLE_MASK;
>> +	__raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
>> +
>> +	/* Read cycle count and performance count */
>> +	edata->total_count = __raw_readl(info->ppmu.base + PPMUv2_CCNT);
>> +
>> +	switch (id) {
>> +	case PPMU_PMNCNT0:
>> +	case PPMU_PMNCNT1:
>> +	case PPMU_PMNCNT2:
>> +		load_count = __raw_readl(info->ppmu.base + PPMUv2_PMNCT(id));
>> +		break;
>> +	case PPMU_PMNCNT3:
>> +		pmcnt_high = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_HIGH);
>> +		pmcnt_low = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_LOW);
>> +		load_count = (u64)((pmcnt_high & 0xff) << 32) + (u64)pmcnt_low;
>> +		break;
>> +	}
>> +	edata->load_count = load_count;
>> +
>> +	/* Disable all counters */
>> +	cntenc = __raw_readl(info->ppmu.base + PPMUv2_CNTENC);
>> +	cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id));
>> +	__raw_writel(cntenc, info->ppmu.base + PPMUv2_CNTENC);
>> +
>> +	dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name,
>> +					edata->load_count, edata->total_count);
>> +	return 0;
>> +}
>> +
>> +static struct devfreq_event_ops exynos_ppmu_v2_ops = {
> 
> This can be const.

OK.

> 
>> +	.disable = exynos_ppmu_v2_disable,
>> +	.set_event = exynos_ppmu_v2_set_event,
>> +	.get_event = exynos_ppmu_v2_get_event,
>> +};
>> +
>> +static struct of_device_id exynos_ppmu_id_match[] = {
> 
> In the previous patch this was not present but now it can be made 'const'.

OK for adding the 'const'. 
The original 'exynos_ppmu_id_match' is located on below of this patch.

> 
>> +	{
>> +		.compatible = "samsung,exynos-ppmu",
>> +		.data = (void *)&exynos_ppmu_ops,
>> +	}, {
>> +		.compatible = "samsung,exynos-ppmu-v2",
>> +		.data = (void *)&exynos_ppmu_v2_ops,
>> +	},
>> +	{ /* sentinel */ },
>> +};
>> +
>> +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)
>>  {
>> @@ -238,7 +397,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>  			continue;
>>  		}
>>  
>> -		desc[j].ops = &exynos_ppmu_ops;
>> +		desc[j].ops = exynos_bus_get_ops(np);
> 
> So for each ops callback (get/set/disable) you will have another layer
> of indirection where you will be matching the device each time?
> 
> That seems like a waste of CPU time. Just match it once here and use
> either old ops or new ops_v2.

OK. I'll rework to reduce the unneeded operation.

> 
> 
>>  		desc[j].driver_data = info;
>>  
>>  		of_property_read_string(node, "event-name", &desc[j].name);
>> @@ -354,11 +513,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> -static struct of_device_id exynos_ppmu_id_match[] = {
>> -	{ .compatible = "samsung,exynos-ppmu", },
>> -	{ /* sentinel */ },
>> -};
>> -
>>  static struct platform_driver exynos_ppmu_driver = {
>>  	.probe	= exynos_ppmu_probe,
>>  	.remove	= exynos_ppmu_remove,
>> diff --git a/drivers/devfreq/event/exynos-ppmu.h b/drivers/devfreq/event/exynos-ppmu.h
>> index 4e831d48c138..9a7cf6394f37 100644
>> --- a/drivers/devfreq/event/exynos-ppmu.h
>> +++ b/drivers/devfreq/event/exynos-ppmu.h
>> @@ -26,6 +26,9 @@ enum ppmu_counter {
>>  	PPMU_PMNCNT_MAX,
>>  };
>>  
>> +/***
>> + * PPMUv1.1 Definitions
>> + */
>>  enum ppmu_event_type {
>>  	PPMU_RO_BUSY_CYCLE_CNT	= 0x0,
>>  	PPMU_WO_BUSY_CYCLE_CNT	= 0x1,
>> @@ -90,4 +93,71 @@ enum ppmu_reg {
>>  #define PPMU_PMNCT(x)			(PPMU_PMCNT0 + (0x10 * x))
>>  #define PPMU_BEVTxSEL(x)		(PPMU_BEVT0SEL + (0x100 * x))
>>  
>> +/***
>> + * PPMUv2.0 definitions
>> + */
>> +enum ppmuv2_mode {
>> +	PPMUv2_MODE_MANUAL = 0,
>> +	PPMUv2_MODE_AUTO = 1,
>> +	PPMUv2_MODE_CIG = 2,	/* CIG (Conditional Interrupt Generation) */
> 
> Mixing lower-upper case looks odd to me. How about:
> PPMU2_MODE_MANUAL = 0,
> or
> PPMU_V2_MODE_MANUAL = 0,
> ?
> 
> (here and everywhere below)

As you know, after deciding the compatible name about using '-v2',
we can decide the correct register name.

Best Regards,
Chanwoo Choi

Re: [PATCH 1/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433

[Dan Carpenter]

On Thu, Jul 23, 2015 at 10:57:18AM +0900, Chanwoo Choi wrote:
> +static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev)
> +{
> +	struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
> +	u32 pmnc, clear;
> +
> +	/* Disable all counters */
> +	clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK
> +		| PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK);
> +
> +	__raw_writel(clear, info->ppmu.base + PPMUv2_FLAG);

Why aren't you using normal readl()/writel()?  What are the endiannesses
here?

regards,
dan carpenter

Re: [PATCH 1/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433

[Chanwoo Choi]

On Thu, Jul 23, 2015 at 10:14 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Hi Krzysztof,
>
> On 07/23/2015 04:28 PM, Krzysztof Kozlowski wrote:
>> On 23.07.2015 10:57, Chanwoo Choi wrote:
>>> This patch adds the support for PPMU (Platform Performance Monitoring Unit)
>>> version 2.0 for Exynos5433 SoC. Exynos5433 SoC must need PPMUv2 which is
>>> quite different from PPMUv1.1. The exynos-ppmu.c driver supports both PPMUv1.1
>>> and PPMUv2.
>>>
>>> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> Hi,
>>
>> Few comments at the end.
>>
>>> ---
>>>  drivers/devfreq/event/exynos-ppmu.c | 168 ++++++++++++++++++++++++++++++++++--
>>>  drivers/devfreq/event/exynos-ppmu.h |  70 +++++++++++++++
>>>  2 files changed, 231 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
>>> index 7d99d13bacd8..6066dc18fc73 100644
>>> --- a/drivers/devfreq/event/exynos-ppmu.c
>>> +++ b/drivers/devfreq/event/exynos-ppmu.c
>>> @@ -1,7 +1,7 @@
>>>  /*
>>>   * exynos_ppmu.c - EXYNOS PPMU (Platform Performance Monitoring Unit) support
>>>   *
>>> - * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>>> + * Copyright (c) 2014-2015 Samsung Electronics Co., Ltd.
>>>   * Author : Chanwoo Choi <cw00.choi@samsung.com>
>>>   *
>>>   * This program is free software; you can redistribute it and/or modify
>>> @@ -82,6 +82,15 @@ struct __exynos_ppmu_events {
>>>      PPMU_EVENT(mscl),
>>>      PPMU_EVENT(fimd0x),
>>>      PPMU_EVENT(fimd1x),
>
> (snip)
>
>>> +static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev,
>>> +                                struct devfreq_event_data *edata)
>>> +{
>>> +    struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
>>> +    int id = exynos_ppmu_find_ppmu_id(edev);
>>> +    u32 pmnc, cntenc;
>>> +    u32 pmcnt_high, pmcnt_low;
>>> +    u64 load_count = 0;
>>> +
>>> +    /* Disable PPMU */
>>> +    pmnc = __raw_readl(info->ppmu.base + PPMUv2_PMNC);
>>> +    pmnc &= ~PPMU_PMNC_ENABLE_MASK;
>>> +    __raw_writel(pmnc, info->ppmu.base + PPMUv2_PMNC);
>>> +
>>> +    /* Read cycle count and performance count */
>>> +    edata->total_count = __raw_readl(info->ppmu.base + PPMUv2_CCNT);
>>> +
>>> +    switch (id) {
>>> +    case PPMU_PMNCNT0:
>>> +    case PPMU_PMNCNT1:
>>> +    case PPMU_PMNCNT2:
>>> +            load_count = __raw_readl(info->ppmu.base + PPMUv2_PMNCT(id));
>>> +            break;
>>> +    case PPMU_PMNCNT3:
>>> +            pmcnt_high = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_HIGH);
>>> +            pmcnt_low = __raw_readl(info->ppmu.base + PPMUv2_PMCNT3_LOW);
>>> +            load_count = (u64)((pmcnt_high & 0xff) << 32) + (u64)pmcnt_low;
>>> +            break;
>>> +    }
>>> +    edata->load_count = load_count;
>>> +
>>> +    /* Disable all counters */
>>> +    cntenc = __raw_readl(info->ppmu.base + PPMUv2_CNTENC);
>>> +    cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id));
>>> +    __raw_writel(cntenc, info->ppmu.base + PPMUv2_CNTENC);
>>> +
>>> +    dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name,
>>> +                                    edata->load_count, edata->total_count);
>>> +    return 0;
>>> +}
>>> +
>>> +static struct devfreq_event_ops exynos_ppmu_v2_ops = {
>>
>> This can be const.
>
> OK.
>
>>
>>> +    .disable = exynos_ppmu_v2_disable,
>>> +    .set_event = exynos_ppmu_v2_set_event,
>>> +    .get_event = exynos_ppmu_v2_get_event,
>>> +};
>>> +
>>> +static struct of_device_id exynos_ppmu_id_match[] = {
>>
>> In the previous patch this was not present but now it can be made 'const'.
>
> OK for adding the 'const'.
> The original 'exynos_ppmu_id_match' is located on below of this patch.
>
>>
>>> +    {
>>> +            .compatible = "samsung,exynos-ppmu",
>>> +            .data = (void *)&exynos_ppmu_ops,
>>> +    }, {
>>> +            .compatible = "samsung,exynos-ppmu-v2",
>>> +            .data = (void *)&exynos_ppmu_v2_ops,
>>> +    },
>>> +    { /* sentinel */ },
>>> +};
>>> +
>>> +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)
>>>  {
>>> @@ -238,7 +397,7 @@ static int of_get_devfreq_events(struct device_node *np,
>>>                      continue;
>>>              }
>>>
>>> -            desc[j].ops = &exynos_ppmu_ops;
>>> +            desc[j].ops = exynos_bus_get_ops(np);
>>
>> So for each ops callback (get/set/disable) you will have another layer
>> of indirection where you will be matching the device each time?
>>
>> That seems like a waste of CPU time. Just match it once here and use
>> either old ops or new ops_v2.
>
> OK. I'll rework to reduce the unneeded operation.
>
>>
>>
>>>              desc[j].driver_data = info;
>>>
>>>              of_property_read_string(node, "event-name", &desc[j].name);
>>> @@ -354,11 +513,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev)
>>>      return 0;
>>>  }
>>>
>>> -static struct of_device_id exynos_ppmu_id_match[] = {
>>> -    { .compatible = "samsung,exynos-ppmu", },
>>> -    { /* sentinel */ },
>>> -};
>>> -
>>>  static struct platform_driver exynos_ppmu_driver = {
>>>      .probe  = exynos_ppmu_probe,
>>>      .remove = exynos_ppmu_remove,
>>> diff --git a/drivers/devfreq/event/exynos-ppmu.h b/drivers/devfreq/event/exynos-ppmu.h
>>> index 4e831d48c138..9a7cf6394f37 100644
>>> --- a/drivers/devfreq/event/exynos-ppmu.h
>>> +++ b/drivers/devfreq/event/exynos-ppmu.h
>>> @@ -26,6 +26,9 @@ enum ppmu_counter {
>>>      PPMU_PMNCNT_MAX,
>>>  };
>>>
>>> +/***
>>> + * PPMUv1.1 Definitions
>>> + */
>>>  enum ppmu_event_type {
>>>      PPMU_RO_BUSY_CYCLE_CNT  = 0x0,
>>>      PPMU_WO_BUSY_CYCLE_CNT  = 0x1,
>>> @@ -90,4 +93,71 @@ enum ppmu_reg {
>>>  #define PPMU_PMNCT(x)                       (PPMU_PMCNT0 + (0x10 * x))
>>>  #define PPMU_BEVTxSEL(x)            (PPMU_BEVT0SEL + (0x100 * x))
>>>
>>> +/***
>>> + * PPMUv2.0 definitions
>>> + */
>>> +enum ppmuv2_mode {
>>> +    PPMUv2_MODE_MANUAL = 0,
>>> +    PPMUv2_MODE_AUTO = 1,
>>> +    PPMUv2_MODE_CIG = 2,    /* CIG (Conditional Interrupt Generation) */
>>
>> Mixing lower-upper case looks odd to me. How about:
>> PPMU2_MODE_MANUAL = 0,
>> or
>> PPMU_V2_MODE_MANUAL = 0,
>> ?
>>
>> (here and everywhere below)
>
> As you know, after deciding the compatible name about using '-v2',
> we can decide the correct register name.

I modify the register name as PPMU_V2_* style.

Thanks,
Chanwoo Choi

Re: [PATCH 1/2] PM / devfreq: exynos-ppmu: Add the support of PPMUv2 for Exynos5433

[Chanwoo Choi]

On Thu, Jul 23, 2015 at 11:43 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> On Thu, Jul 23, 2015 at 10:57:18AM +0900, Chanwoo Choi wrote:
>> +static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev)
>> +{
>> +     struct exynos_ppmu *info = devfreq_event_get_drvdata(edev);
>> +     u32 pmnc, clear;
>> +
>> +     /* Disable all counters */
>> +     clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK
>> +             | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK);
>> +
>> +     __raw_writel(clear, info->ppmu.base + PPMUv2_FLAG);
>
> Why aren't you using normal readl()/writel()?  What are the endiannesses
> here?

There are no special reason. Usually I used the __raw_readl/writel to
access the SFR registers

Thanks,
Chanwoo Choi