Hi Krzysztof,

On 12/10/2014 06:59 PM, Krzysztof Kozlowski wrote:

On wto, 2014-12-09 at 23:13 +0900, Chanwoo Choi wrote:

quoted

This patch add exynos-ppmu devfreq event driver to provider raw data about
the utilization of each IP in Exynos SoC series.

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/Kconfig             |   9 +
 drivers/devfreq/event/Makefile      |   1 +
 drivers/devfreq/event/exynos-ppmu.c | 409 ++++++++++++++++++++++++++++++++++++
 3 files changed, 419 insertions(+)
 create mode 100644 drivers/devfreq/event/exynos-ppmu.c

I would rather see this as an incremental change for existing
exynos_ppmu.c file. However I do not insists on that.

quoted

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 4d15b62..d4559f7 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig

@@ -89,4 +89,13 @@ config ARM_EXYNOS5_BUS_DEVFREQ
 
 comment "DEVFREQ Event Drivers"
 
+config DEVFREQ_EVENT_EXYNOS_PPMU
+	bool "EXYNOS PPMU (Performance Profiling Monitoring Unit) DEVFREQ event Driver"
+	depends on ARCH_EXYNOS
+	select PM_OPP
+	help
+	 This add the DEVFREQ event driver for Exynos SoC. It provides PPMU
+	 (Performance Profiling Monitoring Unit) counters to estimate the
+	 utilization of each module.
+
 endif # PM_DEVFREQ

diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
index dc56005..be146ea 100644
--- a/drivers/devfreq/event/Makefile
+++ b/drivers/devfreq/event/Makefile

@@ -1 +1,2 @@
 # Exynos DEVFREQ Event Drivers
+obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o

diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c
new file mode 100644
index 0000000..2706f23
--- /dev/null
+++ b/drivers/devfreq/event/exynos-ppmu.c

@@ -0,0 +1,409 @@
+/*
+ * exynos_ppmu.c - EXYNOS PPMU (Performance Profiling Monitoring Units) support
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Author : Chanwoo Choi <cw00.choi@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This driver is based on drivers/devfreq/exynos/exynos_ppmu.c
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/suspend.h>
+#include <linux/devfreq.h>
+
+#define PPMU_ENABLE             BIT(0)
+#define PPMU_DISABLE            0x0
+#define PPMU_CYCLE_RESET        BIT(1)
+#define PPMU_COUNTER_RESET      BIT(2)
+
+#define PPMU_ENABLE_COUNT0      BIT(0)
+#define PPMU_ENABLE_COUNT1      BIT(1)
+#define PPMU_ENABLE_COUNT2      BIT(2)
+#define PPMU_ENABLE_COUNT3      BIT(3)
+#define PPMU_ENABLE_CYCLE       BIT(31)
+
+#define PPMU_CNTENS		0x10
+#define PPMU_FLAG		0x50
+#define PPMU_CCNT_OVERFLOW	BIT(31)
+#define PPMU_CCNT		0x100
+
+#define PPMU_PMCNT0		0x110
+#define PPMU_PMCNT_OFFSET	0x10
+#define PMCNT_OFFSET(x)		(PPMU_PMCNT0 + (PPMU_PMCNT_OFFSET * x))
+
+#define PPMU_BEVT0SEL		0x1000
+#define PPMU_BEVTSEL_OFFSET	0x100
+#define PPMU_BEVTSEL(x)		(PPMU_BEVT0SEL + (x * PPMU_BEVTSEL_OFFSET))
+
+#define RD_DATA_COUNT		0x5
+#define WR_DATA_COUNT		0x6
+#define RDWR_DATA_COUNT		0x7
+
+enum ppmu_counter {
+	PPMU_PMNCNT0,
+	PPMU_PMNCNT1,
+	PPMU_PMNCNT2,
+	PPMU_PMNCNT3,
+	PPMU_PMNCNT_MAX,
+};
+
+/* Platform data */
+struct exynos_ppmu_data {
+	struct devfreq *devfreq;

Looks like not used here.

Right. I'll remove it.

quoted

+	struct devfreq_event_dev **event_dev;
+	struct devfreq_event_desc *desc;
+	unsigned int num_events;
+
+	struct device *dev;
+	struct clk *clk_ppmu;
+	struct mutex lock;
+
+	struct __exynos_ppmu {
+		void __iomem *hw_base;
+		unsigned int ccnt;
+		unsigned int event[PPMU_PMNCNT_MAX];
+		unsigned int count[PPMU_PMNCNT_MAX];
+		unsigned long long ns;
+		ktime_t reset_time;
+		bool ccnt_overflow;
+		bool count_overflow[PPMU_PMNCNT_MAX];
+	} ppmu;
+};
+
+struct __exynos_ppmu_events {
+	char *name;
+	int id;
+} ppmu_events[] = {
+	{ "ppmu-dmc0-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-dmc0-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-dmc0-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-dmc0-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-dmc1-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-dmc1-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-dmc1-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-dmc1-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-cpu-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-cpu-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-cpu-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-cpu-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-rightbus-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-rightbus-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-rightbus-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-rightbus-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-leftbus-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-leftbus-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-leftbus-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-leftbus-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-camif-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-camif-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-camif-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-camif-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-lcd0-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-lcd0-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-lcd0-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-lcd0-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-g3d-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-g3d-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-g3d-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-g3d-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-mfc-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-mfc-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-mfc-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-mfc-pmcnt3", PPMU_PMNCNT3 },
+
+	{ "ppmu-fsys-pmcnt0", PPMU_PMNCNT0 },
+	{ "ppmu-fsys-pmcnt1", PPMU_PMNCNT1 },
+	{ "ppmu-fsys-pmcnt2", PPMU_PMNCNT2 },
+	{ "ppmu-fsys-pmcnt3", PPMU_PMNCNT3 },
+	{ /* sentinel */ },
+};
+
+static int exynos_ppmu_enable(struct devfreq_event_dev *event_dev)
+{
+	struct exynos_ppmu_data *exynos_ppmu = event_dev_get_drvdata(event_dev);
+
+	__raw_writel(PPMU_ENABLE, exynos_ppmu->ppmu.hw_base);
+
+	return 0;
+}
+
+static int exynos_ppmu_disable(struct devfreq_event_dev *event_dev)
+{
+	struct exynos_ppmu_data *exynos_ppmu = event_dev_get_drvdata(event_dev);
+
+	__raw_writel(PPMU_DISABLE, exynos_ppmu->ppmu.hw_base);
+
+	return 0;
+}
+
+static bool exynos_ppmu_is_enabled(struct devfreq_event_dev *event_dev)
+{
+	struct exynos_ppmu_data *exynos_ppmu = event_dev_get_drvdata(event_dev);
+	int val;
+
+	val = __raw_readl(exynos_ppmu->ppmu.hw_base);
+	if (val & PPMU_ENABLE)
+		return true;
+
+	return false;
+}
+
+static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *event_dev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ppmu_events); i++)
+		if (!strcmp(event_dev->desc->name, ppmu_events[i].name))
+			return ppmu_events[i].id;
+
+	return -EINVAL;
+}
+
+static int exynos_ppmu_set_event(struct devfreq_event_dev *event_dev,
+				enum devfreq_event_type event_type)
+{
+	struct exynos_ppmu_data *exynos_ppmu = event_dev_get_drvdata(event_dev);
+	void __iomem *ppmu_base = exynos_ppmu->ppmu.hw_base;
+	int id = exynos_ppmu_find_ppmu_id(event_dev);
+
+	__raw_writel(RDWR_DATA_COUNT, ppmu_base + PPMU_BEVTSEL(id));
+
+	return 0;
+}
+
+static int exynos_ppmu_get_event(struct devfreq_event_dev *event_dev,
+				enum devfreq_event_type event_type,
+				int *total_event)
+{
+	struct exynos_ppmu_data *exynos_ppmu = event_dev_get_drvdata(event_dev);
+	void __iomem *ppmu_base = exynos_ppmu->ppmu.hw_base;
+	int id = exynos_ppmu_find_ppmu_id(event_dev);
+	int cnt, total_cnt;
+
+	total_cnt = __raw_readl(ppmu_base + PPMU_CCNT);
+
+	if (id == PPMU_PMNCNT3)
+		cnt = ((__raw_readl(ppmu_base + PMCNT_OFFSET(id)) << 8) |
+			  __raw_readl(ppmu_base + PMCNT_OFFSET(id + 1)));
+	else
+		cnt = __raw_readl(ppmu_base + PMCNT_OFFSET(id));
+
+	*total_event = total_cnt;
+
+	return cnt;
+}
+
+static int exynos_ppmu_reset(struct devfreq_event_dev *event_dev)
+{
+	struct exynos_ppmu_data *exynos_ppmu = event_dev_get_drvdata(event_dev);
+	void __iomem *ppmu_base = exynos_ppmu->ppmu.hw_base;
+
+	__raw_writel(PPMU_CYCLE_RESET | PPMU_COUNTER_RESET, ppmu_base);
+	__raw_writel(PPMU_ENABLE_CYCLE  |
+		     PPMU_ENABLE_COUNT0 |
+		     PPMU_ENABLE_COUNT1 |
+		     PPMU_ENABLE_COUNT2 |
+		     PPMU_ENABLE_COUNT3,
+		     ppmu_base + PPMU_CNTENS);
+	__raw_writel(PPMU_ENABLE, exynos_ppmu->ppmu.hw_base);
+
+	return 0;
+}
+
+static struct devfreq_event_ops exynos_ppmu_ops = {
+	.enable		= exynos_ppmu_enable,
+	.disable	= exynos_ppmu_disable,
+	.is_enabled	= exynos_ppmu_is_enabled,
+	.set_event	= exynos_ppmu_set_event,
+	.get_event	= exynos_ppmu_get_event,
+	.reset		= exynos_ppmu_reset,
+};
+
+static int of_get_devfreq_events(struct device_node *np,
+				struct exynos_ppmu_data *exynos_ppmu)
+{
+	struct devfreq_event_desc *desc;
+	struct device *dev = exynos_ppmu->dev;
+	struct device_node *events_np, *node;
+	int i, j, count;
+
+	events_np = of_find_node_by_name(np, "events");

of_get_child_by_name()

OK. I'll use of_get_chilc_by_name() function to get phandle of devfreq-event device in dts.

quoted

+	if (!events_np) {
+		dev_err(dev, "Failed to find ppmus sub-node\n");
+		return -EINVAL;
+	}
+
+	count = of_get_child_count(events_np);
+	desc = devm_kzalloc(dev, sizeof(struct devfreq_event_desc) * count,
+			      GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+	exynos_ppmu->num_events = count;
+
+	j = 0;
+	for_each_child_of_node(events_np, node) {
+		for (i = 0; i < ARRAY_SIZE(ppmu_events); i++) {
+			if (!of_node_cmp(node->name, ppmu_events[i].name))
+				break;
+		}
+
+		if (i == ARRAY_SIZE(ppmu_events)) {
+			dev_warn(dev,
+				"don't know how to configure events : %s\n",
+				node->name);
+			continue;
+		}
+		desc[j].ops = &exynos_ppmu_ops;
+		desc[j].driver_data = exynos_ppmu;
+		desc[j].event_type |= DEVFREQ_EVENT_TYPE_RAW_DATA;
+		of_property_read_string(node, "event-name", &desc[j].name);
+		j++;
+	}
+	exynos_ppmu->desc = desc;

of_node_put() for 'events_np' and 'node' in loop.

OK, I'll add it.

quoted

+
+	return 0;
+}
+
+static int exynos_ppmu_parse_dt(struct exynos_ppmu_data *exynos_ppmu)
+{
+	struct device *dev = exynos_ppmu->dev;
+	struct device_node *np = dev->of_node;
+	int ret = 0;
+
+	if (!np) {
+		dev_err(dev, "Failed to find devicetree node\n");
+		return -EINVAL;
+	}
+
+	/* Maps the memory mapped IO to control PPMU register */
+	exynos_ppmu->ppmu.hw_base = of_iomap(np, 0);
+	if (IS_ERR_OR_NULL(exynos_ppmu->ppmu.hw_base)) {
+		dev_err(dev, "Failed to map memory region\n");
+		ret = -EINVAL;
+		goto err_iomap;
+	}
+
+	/* FIXME: Get the clock of ppmu and enable this clock */
+	exynos_ppmu->clk_ppmu = devm_clk_get(dev, "ppmu");
+	if (IS_ERR(exynos_ppmu->clk_ppmu))
+		dev_warn(dev, "Failed to get PPMU clock\n");
+
+	ret = of_get_devfreq_events(np, exynos_ppmu);
+	if (ret < 0) {
+		dev_err(dev, "Failed to parse exynos ppmu dt node\n");
+		goto err_clock;
+	}
+
+	return 0;
+
+err_clock:
+	clk_disable_unprepare(exynos_ppmu->clk_ppmu);

Not needed. Clock is not enabled here.

OK. I'll fix it.

quoted

+err_iomap:
+	iounmap(exynos_ppmu->ppmu.hw_base);
+
+	return ret;
+}
+
+static int exynos_ppmu_probe(struct platform_device *pdev)
+{
+	struct exynos_ppmu_data *exynos_ppmu;
+	struct devfreq_event_dev **event_dev;
+	struct devfreq_event_desc *desc;
+	int i, ret = 0, size;
+
+	/* Allocate the memory of exynos_ppmu_data and initialize it */
+	exynos_ppmu = devm_kzalloc(&pdev->dev, sizeof(struct exynos_ppmu_data),
+				   GFP_KERNEL);
+	if (!exynos_ppmu)
+		return -ENOMEM;
+
+	mutex_init(&exynos_ppmu->lock);
+	exynos_ppmu->dev = &pdev->dev;
+
+	/* Parse dt data to get resource */
+	ret = exynos_ppmu_parse_dt(exynos_ppmu);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to parse DT node for resource\n");
+		return ret;
+	}
+	desc = exynos_ppmu->desc;
+
+	size = sizeof(struct devfreq_event_dev *) * exynos_ppmu->num_events;
+	exynos_ppmu->event_dev = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!exynos_ppmu->event_dev) {
+		dev_err(&pdev->dev,
+			"Failed to allocate memory devfreq_event_dev list\n");
+		return -ENOMEM;
+	}
+	event_dev = exynos_ppmu->event_dev;
+	platform_set_drvdata(pdev, exynos_ppmu);

Missing clk_prepare_enable().

OK, I'll add it.

quoted

+
+	for (i = 0; i < exynos_ppmu->num_events; i++) {
+		event_dev[i] = devfreq_add_event_device(&pdev->dev, &desc[i]);
+		if (IS_ERR(event_dev)) {
+			ret = PTR_ERR(event_dev);
+			dev_err(&pdev->dev, "Failed to add devfreq evt dev\n");
+			goto err;
+		}
+	}
+
+	return 0;
+err:
+	clk_disable_unprepare(exynos_ppmu->clk_ppmu);
+	iounmap(exynos_ppmu->ppmu.hw_base);
+
+	return ret;
+}
+
+static int exynos_ppmu_remove(struct platform_device *pdev)
+{
+	struct exynos_ppmu_data *exynos_ppmu = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(exynos_ppmu->clk_ppmu);
+	iounmap(exynos_ppmu->ppmu.hw_base);
+
+	/* Remove devfreq instance */
+	devfreq_remove_device(exynos_ppmu->devfreq);

Shouldn't this be devfreq_remove_event_dev()?

Your right. It is my mistake. I'll fix it.

Thanks for your review.

Best Regards,
Chanwoo Choi