[PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Chanwoo Choi]

This patchset support devicetree and use common ppmu driver instead of
individual code of exynos4_bus.c to remove duplicate code. Also this patchset
get the resources for busfreq from dt data by using DT helper function.
- PPMU register address
- PPMU clock
- Regulator for INT/MIF block

This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
To remove power-leakage in suspend state, before entering suspend state,
disable ppmu clocks.

Changes from v1:
- Add exynos4_bus.txt documentation for devicetree guide
- Fix probe failure if CONFIG_PM_OPP is disabled
- Fix typo and resource leak(regulator/clock/memory) when happening probe failure
- Add additionally comment for PPMU usage instead of previous PPC
- Split separate patch to remove ambiguous of patch

Chanwoo Choi (8):
  devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
  devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
  devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
  devfreq: exynos4: Fix bug of resource leak and code clean on probe()
  devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
  devfreq: exynos4: Fix power-leakage of clock on suspend state
  devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
  devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

 .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
 drivers/devfreq/Kconfig                            |   1 +
 drivers/devfreq/exynos/Makefile                    |   2 +-
 drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
 4 files changed, 341 insertions(+), 126 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt

-- 
1.8.0

[PATCHv2 1/8] devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC

[Chanwoo Choi]

This patch support DT(DeviceTree) method to probe exynos4_bus and get device
id of each Exynos4 SoC by using dt helper function.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/exynos4_bus.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index e07b0c6..168a7c6 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -23,6 +23,7 @@
 #include <linux/devfreq.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/of.h>
 #include <linux/module.h>
 
 /* Exynos4 ASV has been in the mailing list, but not upstreamed, yet. */
@@ -1017,6 +1018,28 @@ unlock:
 	return NOTIFY_DONE;
 }
 
+static struct of_device_id exynos4_busfreq_id_match[] = {
+	{
+		.compatible = "samsung,exynos4210-busfreq",
+		.data = (void *)TYPE_BUSF_EXYNOS4210,
+	}, {
+		.compatible = "samsung,exynos4x12-busfreq",
+		.data = (void *)TYPE_BUSF_EXYNOS4x12,
+	},
+};
+
+static int exynos4_busfreq_get_driver_data(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *match;
+
+	match = of_match_node(exynos4_busfreq_id_match, dev->of_node);
+	if (!match)
+		return -ENODEV;
+
+	return (int) match->data;
+}
+
 static int exynos4_busfreq_probe(struct platform_device *pdev)
 {
 	struct busfreq_data *data;
@@ -1030,7 +1053,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	data->type = pdev->id_entry->driver_data;
+	data->type = exynos4_busfreq_get_driver_data(pdev);
 	data->dmc[0].hw_base = S5P_VA_DMC0;
 	data->dmc[1].hw_base = S5P_VA_DMC1;
 	data->pm_notifier.notifier_call = exynos4_busfreq_pm_notifier_event;
@@ -1135,6 +1158,7 @@ static struct platform_driver exynos4_busfreq_driver = {
 		.name	= "exynos4-busfreq",
 		.owner	= THIS_MODULE,
 		.pm	= &exynos4_busfreq_pm,
+		.of_match_table = exynos4_busfreq_id_match,
 	},
 };
 
-- 
1.8.0

[PATCHv2 2/8] devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data

[Chanwoo Choi]

This patch use common ppmu driver of exynos_ppmu.c driver instead of individual
function related to PPC because PPMU is integrated module with both PPC and
Bus event generator. When using PPMU to get bus performance read/write event,
exynos4_bus.c don't need to consider memory type.

And get ppmu address from dt data by using dt helper function (of_iomap).
And then this patch delete duplicate defined structure/enum.

For example,
busfreq@106A0000 {
	compatible = "samsung,exynos4x12-busfreq";
	reg = <0x106A0000 0x2000>, <0x106B0000 0x2000>;
	regs-name = "PPMU_DMC0", "PPMU_DMC1";
};

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/Makefile      |   2 +-
 drivers/devfreq/exynos/exynos4_bus.c | 222 ++++++++++++++++++-----------------
 2 files changed, 117 insertions(+), 107 deletions(-)

diff --git a/drivers/devfreq/exynos/Makefile b/drivers/devfreq/exynos/Makefile
index bfaaf5b..49bc917 100644
--- a/drivers/devfreq/exynos/Makefile
+++ b/drivers/devfreq/exynos/Makefile
@@ -1,3 +1,3 @@
 # Exynos DEVFREQ Drivers
-obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)	+= exynos4_bus.o
+obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)	+= exynos_ppmu.o exynos4_bus.o
 obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)	+= exynos_ppmu.o exynos5_bus.o
diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 168a7c6..1a0effa 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -24,17 +24,19 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/module.h>
 
+#include <mach/map.h>
+
+#include "exynos_ppmu.h"
+#include "exynos4_bus.h"
+
 /* Exynos4 ASV has been in the mailing list, but not upstreamed, yet. */
 #ifdef CONFIG_EXYNOS_ASV
 extern unsigned int exynos_result_of_asv;
 #endif
 
-#include <mach/map.h>
-
-#include "exynos4_bus.h"
-
 #define MAX_SAFEVOLT	1200000 /* 1.2V */
 
 enum exynos4_busf_type {
@@ -45,22 +47,6 @@ enum exynos4_busf_type {
 /* Assume that the bus is saturated if the utilization is 40% */
 #define BUS_SATURATION_RATIO	40
 
-enum ppmu_counter {
-	PPMU_PMNCNT0 = 0,
-	PPMU_PMCCNT1,
-	PPMU_PMNCNT2,
-	PPMU_PMNCNT3,
-	PPMU_PMNCNT_MAX,
-};
-struct exynos4_ppmu {
-	void __iomem *hw_base;
-	unsigned int ccnt;
-	unsigned int event;
-	unsigned int count[PPMU_PMNCNT_MAX];
-	bool ccnt_overflow;
-	bool count_overflow[PPMU_PMNCNT_MAX];
-};
-
 enum busclk_level_idx {
 	LV_0 = 0,
 	LV_1,
@@ -69,6 +55,13 @@ enum busclk_level_idx {
 	LV_4,
 	_LV_END
 };
+
+enum exynos_ppmu_idx {
+	PPMU_DMC0,
+	PPMU_DMC1,
+	PPMU_END,
+};
+
 #define EX4210_LV_MAX	LV_2
 #define EX4x12_LV_MAX	LV_4
 #define EX4210_LV_NUM	(LV_2 + 1)
@@ -92,7 +85,7 @@ struct busfreq_data {
 	struct regulator *vdd_int;
 	struct regulator *vdd_mif; /* Exynos4412/4212 only */
 	struct busfreq_opp_info curr_oppinfo;
-	struct exynos4_ppmu dmc[2];
+	struct exynos_ppmu ppmu[PPMU_END];
 
 	struct notifier_block pm_notifier;
 	struct mutex lock;
@@ -102,12 +95,6 @@ struct busfreq_data {
 	unsigned int top_divtable[_LV_END];
 };
 
-struct bus_opp_table {
-	unsigned int idx;
-	unsigned long clk;
-	unsigned long volt;
-};
-
 /* 4210 controls clock of mif and voltage of int */
 static struct bus_opp_table exynos4210_busclk_table[] = {
 	{LV_0, 400000, 1150000},
@@ -525,27 +512,22 @@ static int exynos4x12_set_busclk(struct busfreq_data *data,
 	return 0;
 }
 
-
 static void busfreq_mon_reset(struct busfreq_data *data)
 {
 	unsigned int i;
 
-	for (i = 0; i < 2; i++) {
-		void __iomem *ppmu_base = data->dmc[i].hw_base;
+	for (i = 0; i < PPMU_END; i++) {
+		void __iomem *ppmu_base = data->ppmu[i].hw_base;
 
-		/* Reset PPMU */
-		__raw_writel(0x8000000f, ppmu_base + 0xf010);
-		__raw_writel(0x8000000f, ppmu_base + 0xf050);
-		__raw_writel(0x6, ppmu_base + 0xf000);
-		__raw_writel(0x0, ppmu_base + 0xf100);
+		/* Reset the performance and cycle counters */
+		exynos_ppmu_reset(ppmu_base);
 
-		/* Set PPMU Event */
-		data->dmc[i].event = 0x6;
-		__raw_writel(((data->dmc[i].event << 12) | 0x1),
-			     ppmu_base + 0xfc);
+		/* Setup count registers to monitor read/write transactions */
+		data->ppmu[i].event[PPMU_PMNCNT3] = RDWR_DATA_COUNT;
+		exynos_ppmu_setevent(ppmu_base, PPMU_PMNCNT3,
+					data->ppmu[i].event[PPMU_PMNCNT3]);
 
-		/* Start PPMU */
-		__raw_writel(0x1, ppmu_base + 0xf000);
+		exynos_ppmu_start(ppmu_base);
 	}
 }
 
@@ -553,23 +535,20 @@ static void exynos4_read_ppmu(struct busfreq_data *data)
 {
 	int i, j;
 
-	for (i = 0; i < 2; i++) {
-		void __iomem *ppmu_base = data->dmc[i].hw_base;
-		u32 overflow;
+	for (i = 0; i < PPMU_END; i++) {
+		void __iomem *ppmu_base = data->ppmu[i].hw_base;
 
-		/* Stop PPMU */
-		__raw_writel(0x0, ppmu_base + 0xf000);
+		exynos_ppmu_stop(ppmu_base);
 
 		/* Update local data from PPMU */
-		overflow = __raw_readl(ppmu_base + 0xf050);
-
-		data->dmc[i].ccnt = __raw_readl(ppmu_base + 0xf100);
-		data->dmc[i].ccnt_overflow = overflow & (1 << 31);
-
-		for (j = 0; j < PPMU_PMNCNT_MAX; j++) {
-			data->dmc[i].count[j] = __raw_readl(
-					ppmu_base + (0xf110 + (0x10 * j)));
-			data->dmc[i].count_overflow[j] = overflow & (1 << j);
+		data->ppmu[i].ccnt = __raw_readl(ppmu_base + PPMU_CCNT);
+
+		for (j = PPMU_PMNCNT0; j < PPMU_PMNCNT_MAX; j++) {
+			if (data->ppmu[i].event[j] == 0)
+				data->ppmu[i].count[j] = 0;
+			else
+				data->ppmu[i].count[j] =
+					exynos_ppmu_read(ppmu_base, j);
 		}
 	}
 
@@ -699,66 +678,42 @@ out:
 	return err;
 }
 
-static int exynos4_get_busier_dmc(struct busfreq_data *data)
+static int exynos4_get_busier_ppmu(struct busfreq_data *data)
 {
-	u64 p0 = data->dmc[0].count[0];
-	u64 p1 = data->dmc[1].count[0];
-
-	p0 *= data->dmc[1].ccnt;
-	p1 *= data->dmc[0].ccnt;
-
-	if (data->dmc[1].ccnt == 0)
-		return 0;
+	int i, j;
+	int busy = 0;
+	unsigned int temp = 0;
+
+	for (i = 0; i < PPMU_END; i++) {
+		for (j = PPMU_PMNCNT0; j < PPMU_PMNCNT_MAX; j++) {
+			if (data->ppmu[i].count[j] > temp) {
+				temp = data->ppmu[i].count[j];
+				busy = i;
+			}
+		}
+	}
 
-	if (p0 > p1)
-		return 0;
-	return 1;
+	return busy;
 }
 
 static int exynos4_bus_get_dev_status(struct device *dev,
 				      struct devfreq_dev_status *stat)
 {
 	struct busfreq_data *data = dev_get_drvdata(dev);
-	int busier_dmc;
-	int cycles_x2 = 2; /* 2 x cycles */
-	void __iomem *addr;
-	u32 timing;
-	u32 memctrl;
+	int busier;
 
 	exynos4_read_ppmu(data);
-	busier_dmc = exynos4_get_busier_dmc(data);
+	busier = exynos4_get_busier_ppmu(data);
 	stat->current_frequency = data->curr_oppinfo.rate;
 
-	if (busier_dmc)
-		addr = S5P_VA_DMC1;
-	else
-		addr = S5P_VA_DMC0;
-
-	memctrl = __raw_readl(addr + 0x04); /* one of DDR2/3/LPDDR2 */
-	timing = __raw_readl(addr + 0x38); /* CL or WL/RL values */
-
-	switch ((memctrl >> 8) & 0xf) {
-	case 0x4: /* DDR2 */
-		cycles_x2 = ((timing >> 16) & 0xf) * 2;
-		break;
-	case 0x5: /* LPDDR2 */
-	case 0x6: /* DDR3 */
-		cycles_x2 = ((timing >> 8) & 0xf) + ((timing >> 0) & 0xf);
-		break;
-	default:
-		pr_err("%s: Unknown Memory Type(%d).\n", __func__,
-		       (memctrl >> 8) & 0xf);
-		return -EINVAL;
-	}
-
 	/* Number of cycles spent on memory access */
-	stat->busy_time = data->dmc[busier_dmc].count[0] / 2 * (cycles_x2 + 2);
+	stat->busy_time = data->ppmu[busier].count[PPMU_PMNCNT3];
 	stat->busy_time *= 100 / BUS_SATURATION_RATIO;
-	stat->total_time = data->dmc[busier_dmc].ccnt;
+	stat->total_time = data->ppmu[busier].ccnt;
 
 	/* If the counters have overflown, retry */
-	if (data->dmc[busier_dmc].ccnt_overflow ||
-	    data->dmc[busier_dmc].count_overflow[0])
+	if (data->ppmu[busier].ccnt_overflow ||
+	    data->ppmu[busier].count_overflow[0])
 		return -EAGAIN;
 
 	return 0;
@@ -1028,6 +983,39 @@ static struct of_device_id exynos4_busfreq_id_match[] = {
 	},
 };
 
+static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
+{
+	struct device *dev = data->dev;
+	struct device_node *np = dev->of_node;
+	int i, ret;
+
+	if (!np) {
+		dev_err(dev, "Failed to find devicetree node\n");
+		return -EINVAL;
+	}
+
+	/* Maps the memory mapped IO to control PPMU register */
+	for (i = 0; i < PPMU_END; i++) {
+		data->ppmu[i].hw_base = of_iomap(np, i);
+		if (IS_ERR_OR_NULL(data->ppmu[i].hw_base)) {
+			dev_err(dev, "Failed to map memory region\n");
+			data->ppmu[i].hw_base = NULL;
+			ret = -EINVAL;
+			goto err_iomap;
+		}
+	}
+
+	return 0;
+
+err_iomap:
+	for (i = 0; i < PPMU_END; i++) {
+		if (data->ppmu[i].hw_base)
+			iounmap(data->ppmu[i].hw_base);
+	}
+
+	return ret;
+}
+
 static int exynos4_busfreq_get_driver_data(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1045,7 +1033,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	struct busfreq_data *data;
 	struct dev_pm_opp *opp;
 	struct device *dev = &pdev->dev;
-	int err = 0;
+	int i, err = 0;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(struct busfreq_data), GFP_KERNEL);
 	if (data == NULL) {
@@ -1054,10 +1042,16 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	}
 
 	data->type = exynos4_busfreq_get_driver_data(pdev);
-	data->dmc[0].hw_base = S5P_VA_DMC0;
-	data->dmc[1].hw_base = S5P_VA_DMC1;
 	data->pm_notifier.notifier_call = exynos4_busfreq_pm_notifier_event;
 	data->dev = dev;
+
+	/* Parse dt data to get register/regulator */
+	err = exynos4_busfreq_parse_dt(data);
+	if (err < 0) {
+		dev_err(dev, "Failed to parse dt for resource\n");
+		return err;
+	}
+
 	mutex_init(&data->lock);
 
 	switch (data->type) {
@@ -1102,12 +1096,20 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	busfreq_mon_reset(data);
-
+	/* Reigster Exynos4's devfreq instance with 'simple_ondemand' gov */
 	data->devfreq = devfreq_add_device(dev, &exynos4_devfreq_profile,
 					   "simple_ondemand", NULL);
-	if (IS_ERR(data->devfreq))
-		return PTR_ERR(data->devfreq);
+	if (IS_ERR(data->devfreq)) {
+		dev_err(dev, "Failed to add devfreq device\n");
+		err = PTR_ERR(data->devfreq);
+		goto err_opp;
+	}
+
+	/*
+	 * Start PPMU(Performance Profiling Monitoring Unit) to check
+	 * utilization of each IP in the Exynos4 SoC.
+	 */
+	busfreq_mon_reset(data);
 
 	devfreq_register_opp_notifier(dev, data->devfreq);
 
@@ -1119,6 +1121,14 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	}
 
 	return 0;
+
+err_opp:
+	for (i = 0; i < PPMU_END; i++) {
+		if (data->ppmu[i].hw_base)
+			iounmap(data->ppmu[i].hw_base);
+	}
+
+	return err;
 }
 
 static int exynos4_busfreq_remove(struct platform_device *pdev)
-- 
1.8.0

[PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Chanwoo Choi]

There are not the clock controller of ppmudmc0/1. This patch control the clock
of ppmudmc0/1 which is used for monitoring memory bus utilization.

Also, this patch code clean about regulator control and free resource
when calling exit/remove function.

For example,
busfreq@106A0000 {
	compatible = "samsung,exynos4x12-busfreq";

	/* Clock for PPMUDMC0/1 */
	clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
	clock-names = "ppmudmc0", "ppmudmc1";

	/* Regulator for MIF/INT block */
	vdd_mif-supply = <&buck1_reg>;
	vdd_int-supply = <&buck3_reg>;
};

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/exynos4_bus.c | 114 ++++++++++++++++++++++++++++++-----
 1 file changed, 100 insertions(+), 14 deletions(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 1a0effa..a2a3a47 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
 	PPMU_END,
 };
 
+static const char *exynos_ppmu_clk_name[] = {
+	[PPMU_DMC0]	= "ppmudmc0",
+	[PPMU_DMC1]	= "ppmudmc1",
+};
+
 #define EX4210_LV_MAX	LV_2
 #define EX4x12_LV_MAX	LV_4
 #define EX4210_LV_NUM	(LV_2 + 1)
@@ -86,6 +91,7 @@ struct busfreq_data {
 	struct regulator *vdd_mif; /* Exynos4412/4212 only */
 	struct busfreq_opp_info curr_oppinfo;
 	struct exynos_ppmu ppmu[PPMU_END];
+	struct clk *clk_ppmu[PPMU_END];
 
 	struct notifier_block pm_notifier;
 	struct mutex lock;
@@ -722,8 +728,26 @@ static int exynos4_bus_get_dev_status(struct device *dev,
 static void exynos4_bus_exit(struct device *dev)
 {
 	struct busfreq_data *data = dev_get_drvdata(dev);
+	int i;
+
+	/*
+	 * Un-map memory map and disable regulator/clocks
+	 * to prevent power leakage.
+	 */
+	regulator_disable(data->vdd_int);
+	if (data->type == TYPE_BUSF_EXYNOS4x12)
+		regulator_disable(data->vdd_mif);
+
+	for (i = 0; i < PPMU_END; i++) {
+		if (data->clk_ppmu[i])
+			clk_disable_unprepare(data->clk_ppmu[i]);
+	}
 
-	devfreq_unregister_opp_notifier(dev, data->devfreq);
+	for (i = 0; i < PPMU_END; i++) {
+		if (data->ppmu[i].hw_base)
+			iounmap(data->ppmu[i].hw_base);
+
+	}
 }
 
 static struct devfreq_dev_profile exynos4_devfreq_profile = {
@@ -987,6 +1011,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
 {
 	struct device *dev = data->dev;
 	struct device_node *np = dev->of_node;
+	const char **clk_name = exynos_ppmu_clk_name;
 	int i, ret;
 
 	if (!np) {
@@ -1005,8 +1030,70 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
 		}
 	}
 
+	/*
+	 * Get PPMU's clocks to control them. But, if PPMU's clocks
+	 * is default 'pass' state, this driver don't need control
+	 * PPMU's clock.
+	 */
+	for (i = 0; i < PPMU_END; i++) {
+		data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
+		if (IS_ERR_OR_NULL(data->clk_ppmu[i])) {
+			dev_warn(dev, "Cannot get %s clock\n", clk_name[i]);
+			data->clk_ppmu[i] = NULL;
+		}
+
+		ret = clk_prepare_enable(data->clk_ppmu[i]);
+		if (ret < 0) {
+			dev_warn(dev, "Cannot enable %s clock\n", clk_name[i]);
+			data->clk_ppmu[i] = NULL;
+			goto err_clocks;
+		}
+	}
+
+	/* Get regulator to control voltage of int block */
+	data->vdd_int = devm_regulator_get(dev, "vdd_int");
+	if (IS_ERR(data->vdd_int)) {
+		dev_err(dev, "Failed to get the regulator of vdd_int\n");
+		ret = PTR_ERR(data->vdd_int);
+		goto err_clocks;
+	}
+	ret = regulator_enable(data->vdd_int);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulator of vdd_int\n");
+		goto err_clocks;
+	}
+
+	switch (data->type) {
+	case TYPE_BUSF_EXYNOS4210:
+		break;
+	case TYPE_BUSF_EXYNOS4x12:
+		/* Get regulator to control voltage of mif blk if Exynos4x12 */
+		data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
+		if (IS_ERR(data->vdd_mif)) {
+			dev_err(dev, "Failed to get the regulator vdd_mif\n");
+			ret = PTR_ERR(data->vdd_mif);
+			goto err_regulator;
+		}
+		ret = regulator_enable(data->vdd_mif);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
+			goto err_regulator;
+		}
+		break;
+	default:
+		dev_err(dev, "Unknown device type : %d\n", data->type);
+		return -EINVAL;
+	};
+
 	return 0;
 
+err_regulator:
+	regulator_disable(data->vdd_int);
+err_clocks:
+	for (i = 0; i < PPMU_END; i++) {
+		if (data->clk_ppmu[i])
+			clk_disable_unprepare(data->clk_ppmu[i]);
+	}
 err_iomap:
 	for (i = 0; i < PPMU_END; i++) {
 		if (data->ppmu[i].hw_base)
@@ -1068,19 +1155,6 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	data->vdd_int = devm_regulator_get(dev, "vdd_int");
-	if (IS_ERR(data->vdd_int)) {
-		dev_err(dev, "Cannot get the regulator \"vdd_int\"\n");
-		return PTR_ERR(data->vdd_int);
-	}
-	if (data->type == TYPE_BUSF_EXYNOS4x12) {
-		data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
-		if (IS_ERR(data->vdd_mif)) {
-			dev_err(dev, "Cannot get the regulator \"vdd_mif\"\n");
-			return PTR_ERR(data->vdd_mif);
-		}
-	}
-
 	rcu_read_lock();
 	opp = dev_pm_opp_find_freq_floor(dev,
 					 &exynos4_devfreq_profile.initial_freq);
@@ -1123,6 +1197,15 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	return 0;
 
 err_opp:
+	regulator_disable(data->vdd_int);
+	if (data->type == TYPE_BUSF_EXYNOS4x12)
+		regulator_disable(data->vdd_mif);
+
+	for (i = 0; i < PPMU_END; i++) {
+		if (data->clk_ppmu[i])
+			clk_disable_unprepare(data->clk_ppmu[i]);
+	}
+
 	for (i = 0; i < PPMU_END; i++) {
 		if (data->ppmu[i].hw_base)
 			iounmap(data->ppmu[i].hw_base);
@@ -1135,7 +1218,10 @@ static int exynos4_busfreq_remove(struct platform_device *pdev)
 {
 	struct busfreq_data *data = platform_get_drvdata(pdev);
 
+	/* Unregister all of notifier chain */
 	unregister_pm_notifier(&data->pm_notifier);
+	devfreq_unregister_opp_notifier(data->dev, data->devfreq);
+
 	devfreq_remove_device(data->devfreq);
 
 	return 0;
-- 
1.8.0

Re: [PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Tomasz Figa]

Hi Chanwoo,

On 13.03.2014 09:17, Chanwoo Choi wrote:
> There are not the clock controller of ppmudmc0/1. This patch control the clock
> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>
> Also, this patch code clean about regulator control and free resource
> when calling exit/remove function.
>
> For example,
> busfreq@106A0000 {
> 	compatible = "samsung,exynos4x12-busfreq";
>
> 	/* Clock for PPMUDMC0/1 */
> 	clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
> 	clock-names = "ppmudmc0", "ppmudmc1";
>
> 	/* Regulator for MIF/INT block */
> 	vdd_mif-supply = <&buck1_reg>;
> 	vdd_int-supply = <&buck3_reg>;
> };
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>   drivers/devfreq/exynos/exynos4_bus.c | 114 ++++++++++++++++++++++++++++++-----
>   1 file changed, 100 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
> index 1a0effa..a2a3a47 100644
> --- a/drivers/devfreq/exynos/exynos4_bus.c
> +++ b/drivers/devfreq/exynos/exynos4_bus.c
> @@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
>   	PPMU_END,
>   };
>
> +static const char *exynos_ppmu_clk_name[] = {
> +	[PPMU_DMC0]	= "ppmudmc0",
> +	[PPMU_DMC1]	= "ppmudmc1",
> +};
> +
>   #define EX4210_LV_MAX	LV_2
>   #define EX4x12_LV_MAX	LV_4
>   #define EX4210_LV_NUM	(LV_2 + 1)
> @@ -86,6 +91,7 @@ struct busfreq_data {
>   	struct regulator *vdd_mif; /* Exynos4412/4212 only */
>   	struct busfreq_opp_info curr_oppinfo;
>   	struct exynos_ppmu ppmu[PPMU_END];
> +	struct clk *clk_ppmu[PPMU_END];
>
>   	struct notifier_block pm_notifier;
>   	struct mutex lock;
> @@ -722,8 +728,26 @@ static int exynos4_bus_get_dev_status(struct device *dev,
>   static void exynos4_bus_exit(struct device *dev)
>   {
>   	struct busfreq_data *data = dev_get_drvdata(dev);
> +	int i;
> +
> +	/*
> +	 * Un-map memory map and disable regulator/clocks
> +	 * to prevent power leakage.
> +	 */
> +	regulator_disable(data->vdd_int);
> +	if (data->type == TYPE_BUSF_EXYNOS4x12)
> +		regulator_disable(data->vdd_mif);
> +
> +	for (i = 0; i < PPMU_END; i++) {
> +		if (data->clk_ppmu[i])

This check is invalid. Clock pointers must be checked for validity using 
the IS_ERR() macro, because NULL is a valid clock pointer value 
indicating a dummy clock.

> +			clk_disable_unprepare(data->clk_ppmu[i]);
> +	}
>
> -	devfreq_unregister_opp_notifier(dev, data->devfreq);
> +	for (i = 0; i < PPMU_END; i++) {
> +		if (data->ppmu[i].hw_base)

Can this even happen? Is there a PPMU without registers?

> +			iounmap(data->ppmu[i].hw_base);
> +
> +	}
>   }
>
>   static struct devfreq_dev_profile exynos4_devfreq_profile = {
> @@ -987,6 +1011,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>   {
>   	struct device *dev = data->dev;
>   	struct device_node *np = dev->of_node;
> +	const char **clk_name = exynos_ppmu_clk_name;
>   	int i, ret;
>
>   	if (!np) {
> @@ -1005,8 +1030,70 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>   		}
>   	}
>
> +	/*
> +	 * Get PPMU's clocks to control them. But, if PPMU's clocks
> +	 * is default 'pass' state, this driver don't need control
> +	 * PPMU's clock.
> +	 */
> +	for (i = 0; i < PPMU_END; i++) {
> +		data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
> +		if (IS_ERR_OR_NULL(data->clk_ppmu[i])) {

Again, this check is invalid. Only IS_ERR() is the correct way to check 
whether returned clock pointer is valid.

> +			dev_warn(dev, "Cannot get %s clock\n", clk_name[i]);
> +			data->clk_ppmu[i] = NULL;

This assignment is wrong. To allow further checking whether the clock 
was found the value returned from devm_clk_get() must be retained and 
then IS_ERR() used in further code.

However, I believe it should be an error if a clock is not provided. The 
driver must make sure that PPMU clocks are ungated before trying to 
access them, otherwise the system might hang.

> +		}
> +
> +		ret = clk_prepare_enable(data->clk_ppmu[i]);

The code above allows the clock to be skipped, but this line doesn't 
check whether it is valid. Still, I think the clock should be always 
required.

> +		if (ret < 0) {
> +			dev_warn(dev, "Cannot enable %s clock\n", clk_name[i]);
> +			data->clk_ppmu[i] = NULL;
> +			goto err_clocks;
> +		}
> +	}
> +
> +	/* Get regulator to control voltage of int block */
> +	data->vdd_int = devm_regulator_get(dev, "vdd_int");
> +	if (IS_ERR(data->vdd_int)) {
> +		dev_err(dev, "Failed to get the regulator of vdd_int\n");
> +		ret = PTR_ERR(data->vdd_int);
> +		goto err_clocks;
> +	}
> +	ret = regulator_enable(data->vdd_int);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulator of vdd_int\n");
> +		goto err_clocks;
> +	}
> +
> +	switch (data->type) {
> +	case TYPE_BUSF_EXYNOS4210:
> +		break;
> +	case TYPE_BUSF_EXYNOS4x12:
> +		/* Get regulator to control voltage of mif blk if Exynos4x12 */
> +		data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
> +		if (IS_ERR(data->vdd_mif)) {
> +			dev_err(dev, "Failed to get the regulator vdd_mif\n");
> +			ret = PTR_ERR(data->vdd_mif);
> +			goto err_regulator;
> +		}
> +		ret = regulator_enable(data->vdd_mif);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
> +			goto err_regulator;
> +		}
> +		break;
> +	default:
> +		dev_err(dev, "Unknown device type : %d\n", data->type);
> +		return -EINVAL;
> +	};
> +
>   	return 0;
>
> +err_regulator:
> +	regulator_disable(data->vdd_int);
> +err_clocks:
> +	for (i = 0; i < PPMU_END; i++) {
> +		if (data->clk_ppmu[i])

Invalid check.

Best regards,
Tomasz

Re: [PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Chanwoo Choi]

Hi Tomasz,

On 03/15/2014 02:42 AM, Tomasz Figa wrote:
> Hi Chanwoo,
> 
> On 13.03.2014 09:17, Chanwoo Choi wrote:
>> There are not the clock controller of ppmudmc0/1. This patch control the clock
>> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>>
>> Also, this patch code clean about regulator control and free resource
>> when calling exit/remove function.
>>
>> For example,
>> busfreq@106A0000 {
>>     compatible = "samsung,exynos4x12-busfreq";
>>
>>     /* Clock for PPMUDMC0/1 */
>>     clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
>>     clock-names = "ppmudmc0", "ppmudmc1";
>>
>>     /* Regulator for MIF/INT block */
>>     vdd_mif-supply = <&buck1_reg>;
>>     vdd_int-supply = <&buck3_reg>;
>> };
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>   drivers/devfreq/exynos/exynos4_bus.c | 114 ++++++++++++++++++++++++++++++-----
>>   1 file changed, 100 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>> index 1a0effa..a2a3a47 100644
>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>> @@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
>>       PPMU_END,
>>   };
>>
>> +static const char *exynos_ppmu_clk_name[] = {
>> +    [PPMU_DMC0]    = "ppmudmc0",
>> +    [PPMU_DMC1]    = "ppmudmc1",
>> +};
>> +
>>   #define EX4210_LV_MAX    LV_2
>>   #define EX4x12_LV_MAX    LV_4
>>   #define EX4210_LV_NUM    (LV_2 + 1)
>> @@ -86,6 +91,7 @@ struct busfreq_data {
>>       struct regulator *vdd_mif; /* Exynos4412/4212 only */
>>       struct busfreq_opp_info curr_oppinfo;
>>       struct exynos_ppmu ppmu[PPMU_END];
>> +    struct clk *clk_ppmu[PPMU_END];
>>
>>       struct notifier_block pm_notifier;
>>       struct mutex lock;
>> @@ -722,8 +728,26 @@ static int exynos4_bus_get_dev_status(struct device *dev,
>>   static void exynos4_bus_exit(struct device *dev)
>>   {
>>       struct busfreq_data *data = dev_get_drvdata(dev);
>> +    int i;
>> +
>> +    /*
>> +     * Un-map memory map and disable regulator/clocks
>> +     * to prevent power leakage.
>> +     */
>> +    regulator_disable(data->vdd_int);
>> +    if (data->type == TYPE_BUSF_EXYNOS4x12)
>> +        regulator_disable(data->vdd_mif);
>> +
>> +    for (i = 0; i < PPMU_END; i++) {
>> +        if (data->clk_ppmu[i])
> 
> This check is invalid. Clock pointers must be checked for validity using the IS_ERR() macro, because NULL is a valid clock pointer value indicating a dummy clock.

OK, I'll check it by using the IS_ERR() macro as following:

	if (IS_ERR(data->clk_ppmu[i]) {


> 
>> +            clk_disable_unprepare(data->clk_ppmu[i]);
>> +    }
>>
>> -    devfreq_unregister_opp_notifier(dev, data->devfreq);
>> +    for (i = 0; i < PPMU_END; i++) {
>> +        if (data->ppmu[i].hw_base)
> 
> Can this even happen? Is there a PPMU without registers?
> 
>> +            iounmap(data->ppmu[i].hw_base);
>> +
>> +    }
>>   }
>>
>>   static struct devfreq_dev_profile exynos4_devfreq_profile = {
>> @@ -987,6 +1011,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>   {
>>       struct device *dev = data->dev;
>>       struct device_node *np = dev->of_node;
>> +    const char **clk_name = exynos_ppmu_clk_name;
>>       int i, ret;
>>
>>       if (!np) {
>> @@ -1005,8 +1030,70 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>           }
>>       }
>>
>> +    /*
>> +     * Get PPMU's clocks to control them. But, if PPMU's clocks
>> +     * is default 'pass' state, this driver don't need control
>> +     * PPMU's clock.
>> +     */
>> +    for (i = 0; i < PPMU_END; i++) {
>> +        data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
>> +        if (IS_ERR_OR_NULL(data->clk_ppmu[i])) {
> 
> Again, this check is invalid. Only IS_ERR() is the correct way to check whether returned clock pointer is valid.

ditto.
	if (IS_ERR(data->clk_ppmu[i]) {

> 
>> +            dev_warn(dev, "Cannot get %s clock\n", clk_name[i]);
>> +            data->clk_ppmu[i] = NULL;
> 
> This assignment is wrong. To allow further checking whether the clock was found the value returned from devm_clk_get() must be retained and then IS_ERR() used in further code.
> 
> However, I believe it should be an error if a clock is not provided. The driver must make sure that PPMU clocks are ungated before trying to access them, otherwise the system might hang.

OK, I'll use IS_ERR() macro when checking / handling clock instance of 'data->clk_ppmu[i]'.
And If this driver can't get the clock of ppmu, handel error exception.

> 
>> +        }
>> +
>> +        ret = clk_prepare_enable(data->clk_ppmu[i]);
> 
> The code above allows the clock to be skipped, but this line doesn't check whether it is valid. Still, I think the clock should be always required.

OK, I'll require clock of ppmu without exception.

> 
>> +        if (ret < 0) {
>> +            dev_warn(dev, "Cannot enable %s clock\n", clk_name[i]);
>> +            data->clk_ppmu[i] = NULL;
>> +            goto err_clocks;
>> +        }
>> +    }
>> +
>> +    /* Get regulator to control voltage of int block */
>> +    data->vdd_int = devm_regulator_get(dev, "vdd_int");
>> +    if (IS_ERR(data->vdd_int)) {
>> +        dev_err(dev, "Failed to get the regulator of vdd_int\n");
>> +        ret = PTR_ERR(data->vdd_int);
>> +        goto err_clocks;
>> +    }
>> +    ret = regulator_enable(data->vdd_int);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to enable regulator of vdd_int\n");
>> +        goto err_clocks;
>> +    }
>> +
>> +    switch (data->type) {
>> +    case TYPE_BUSF_EXYNOS4210:
>> +        break;
>> +    case TYPE_BUSF_EXYNOS4x12:
>> +        /* Get regulator to control voltage of mif blk if Exynos4x12 */
>> +        data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
>> +        if (IS_ERR(data->vdd_mif)) {
>> +            dev_err(dev, "Failed to get the regulator vdd_mif\n");
>> +            ret = PTR_ERR(data->vdd_mif);
>> +            goto err_regulator;
>> +        }
>> +        ret = regulator_enable(data->vdd_mif);
>> +        if (ret < 0) {
>> +            dev_err(dev, "Failed to enable regulator of vdd_mif\n");
>> +            goto err_regulator;
>> +        }
>> +        break;
>> +    default:
>> +        dev_err(dev, "Unknown device type : %d\n", data->type);
>> +        return -EINVAL;
>> +    };
>> +
>>       return 0;
>>
>> +err_regulator:
>> +    regulator_disable(data->vdd_int);
>> +err_clocks:
>> +    for (i = 0; i < PPMU_END; i++) {
>> +        if (data->clk_ppmu[i])
> 
> Invalid check.

Modify it as following:

	if (!IS_ERR(data->clk_ppmu[i]) {


Best Regards,
Chanwoo Choi

Re: [PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Chanwoo Choi]

Hi Tomasz,

On 03/17/2014 11:51 AM, Chanwoo Choi wrote:
> Hi Tomasz,
> 
> On 03/15/2014 02:42 AM, Tomasz Figa wrote:
>> Hi Chanwoo,
>>
>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>> There are not the clock controller of ppmudmc0/1. This patch control the clock
>>> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>>>
>>> Also, this patch code clean about regulator control and free resource
>>> when calling exit/remove function.
>>>
>>> For example,
>>> busfreq@106A0000 {
>>>     compatible = "samsung,exynos4x12-busfreq";
>>>
>>>     /* Clock for PPMUDMC0/1 */
>>>     clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
>>>     clock-names = "ppmudmc0", "ppmudmc1";
>>>
>>>     /* Regulator for MIF/INT block */
>>>     vdd_mif-supply = <&buck1_reg>;
>>>     vdd_int-supply = <&buck3_reg>;
>>> };
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>   drivers/devfreq/exynos/exynos4_bus.c | 114 ++++++++++++++++++++++++++++++-----
>>>   1 file changed, 100 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>>> index 1a0effa..a2a3a47 100644
>>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>>> @@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
>>>       PPMU_END,
>>>   };
>>>
>>> +static const char *exynos_ppmu_clk_name[] = {
>>> +    [PPMU_DMC0]    = "ppmudmc0",
>>> +    [PPMU_DMC1]    = "ppmudmc1",
>>> +};
>>> +
>>>   #define EX4210_LV_MAX    LV_2
>>>   #define EX4x12_LV_MAX    LV_4
>>>   #define EX4210_LV_NUM    (LV_2 + 1)
>>> @@ -86,6 +91,7 @@ struct busfreq_data {
>>>       struct regulator *vdd_mif; /* Exynos4412/4212 only */
>>>       struct busfreq_opp_info curr_oppinfo;
>>>       struct exynos_ppmu ppmu[PPMU_END];
>>> +    struct clk *clk_ppmu[PPMU_END];
>>>
>>>       struct notifier_block pm_notifier;
>>>       struct mutex lock;
>>> @@ -722,8 +728,26 @@ static int exynos4_bus_get_dev_status(struct device *dev,
>>>   static void exynos4_bus_exit(struct device *dev)
>>>   {
>>>       struct busfreq_data *data = dev_get_drvdata(dev);
>>> +    int i;
>>> +
>>> +    /*
>>> +     * Un-map memory map and disable regulator/clocks
>>> +     * to prevent power leakage.
>>> +     */
>>> +    regulator_disable(data->vdd_int);
>>> +    if (data->type == TYPE_BUSF_EXYNOS4x12)
>>> +        regulator_disable(data->vdd_mif);
>>> +
>>> +    for (i = 0; i < PPMU_END; i++) {
>>> +        if (data->clk_ppmu[i])
>>
>> This check is invalid. Clock pointers must be checked for validity using the IS_ERR() macro, because NULL is a valid clock pointer value indicating a dummy clock.
> 
> OK, I'll check it by using the IS_ERR() macro as following:
> 

I'll modify it as following:

	for (i = 0; i < PPMU_END; i++) {
		if (IS_ERR(data->clk_ppmu[i])
			continue;
		else
			clk_unprepare_disable(data->clk_ppmu[i]);
	}


> 	if (IS_ERR(data->clk_ppmu[i]) {
> 
> 
>>
>>> +            clk_disable_unprepare(data->clk_ppmu[i]);
>>> +    }
>>>
>>> -    devfreq_unregister_opp_notifier(dev, data->devfreq);
>>> +    for (i = 0; i < PPMU_END; i++) {
>>> +        if (data->ppmu[i].hw_base)
>>
>> Can this even happen? Is there a PPMU without registers?

OK, I'll always unmap the ppmu address.

>>
>>> +            iounmap(data->ppmu[i].hw_base);
>>> +
>>> +    }
>>>   }
>>>
>>>   static struct devfreq_dev_profile exynos4_devfreq_profile = {
>>> @@ -987,6 +1011,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>>   {
>>>       struct device *dev = data->dev;
>>>       struct device_node *np = dev->of_node;
>>> +    const char **clk_name = exynos_ppmu_clk_name;
>>>       int i, ret;
>>>
>>>       if (!np) {
>>> @@ -1005,8 +1030,70 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>>           }
>>>       }
>>>
>>> +    /*
>>> +     * Get PPMU's clocks to control them. But, if PPMU's clocks
>>> +     * is default 'pass' state, this driver don't need control
>>> +     * PPMU's clock.
>>> +     */
>>> +    for (i = 0; i < PPMU_END; i++) {
>>> +        data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
>>> +        if (IS_ERR_OR_NULL(data->clk_ppmu[i])) {
>>
>> Again, this check is invalid. Only IS_ERR() is the correct way to check whether returned clock pointer is valid.
> 
> ditto.
> 	if (IS_ERR(data->clk_ppmu[i]) {
> 
>>
>>> +            dev_warn(dev, "Cannot get %s clock\n", clk_name[i]);
>>> +            data->clk_ppmu[i] = NULL;
>>
>> This assignment is wrong. To allow further checking whether the clock was found the value returned from devm_clk_get() must be retained and then IS_ERR() used in further code.
>>
>> However, I believe it should be an error if a clock is not provided. The driver must make sure that PPMU clocks are ungated before trying to access them, otherwise the system might hang.
> 
> OK, I'll use IS_ERR() macro when checking / handling clock instance of 'data->clk_ppmu[i]'.
> And If this driver can't get the clock of ppmu, handel error exception.
> 
>>
>>> +        }
>>> +
>>> +        ret = clk_prepare_enable(data->clk_ppmu[i]);
>>
>> The code above allows the clock to be skipped, but this line doesn't check whether it is valid. Still, I think the clock should be always required.
> 
> OK, I'll require clock of ppmu without exception.
> 
>>
>>> +        if (ret < 0) {
>>> +            dev_warn(dev, "Cannot enable %s clock\n", clk_name[i]);
>>> +            data->clk_ppmu[i] = NULL;
>>> +            goto err_clocks;
>>> +        }
>>> +    }
>>> +
>>> +    /* Get regulator to control voltage of int block */
>>> +    data->vdd_int = devm_regulator_get(dev, "vdd_int");
>>> +    if (IS_ERR(data->vdd_int)) {
>>> +        dev_err(dev, "Failed to get the regulator of vdd_int\n");
>>> +        ret = PTR_ERR(data->vdd_int);
>>> +        goto err_clocks;
>>> +    }
>>> +    ret = regulator_enable(data->vdd_int);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "Failed to enable regulator of vdd_int\n");
>>> +        goto err_clocks;
>>> +    }
>>> +
>>> +    switch (data->type) {
>>> +    case TYPE_BUSF_EXYNOS4210:
>>> +        break;
>>> +    case TYPE_BUSF_EXYNOS4x12:
>>> +        /* Get regulator to control voltage of mif blk if Exynos4x12 */
>>> +        data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
>>> +        if (IS_ERR(data->vdd_mif)) {
>>> +            dev_err(dev, "Failed to get the regulator vdd_mif\n");
>>> +            ret = PTR_ERR(data->vdd_mif);
>>> +            goto err_regulator;
>>> +        }
>>> +        ret = regulator_enable(data->vdd_mif);
>>> +        if (ret < 0) {
>>> +            dev_err(dev, "Failed to enable regulator of vdd_mif\n");
>>> +            goto err_regulator;
>>> +        }
>>> +        break;
>>> +    default:
>>> +        dev_err(dev, "Unknown device type : %d\n", data->type);
>>> +        return -EINVAL;
>>> +    };
>>> +
>>>       return 0;
>>>
>>> +err_regulator:
>>> +    regulator_disable(data->vdd_int);
>>> +err_clocks:
>>> +    for (i = 0; i < PPMU_END; i++) {
>>> +        if (data->clk_ppmu[i])
>>
>> Invalid check.
> 
> Modify it as following:
> 
> 	if (!IS_ERR(data->clk_ppmu[i]) {


	for (i = 0; i < PPMU_END; i++) {
		if (IS_ERR(data->clk_ppmu[i])
			continue;
		else
			clk_unprepare_disable(data->clk_ppmu[i]);
	}

Best Regards,
Chanwoo Choi

Re: [PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Chanwoo Choi]

Hi Tomasz,

On 03/17/2014 11:51 AM, Chanwoo Choi wrote:
> Hi Tomasz,
> 
> On 03/15/2014 02:42 AM, Tomasz Figa wrote:
>> Hi Chanwoo,
>>
>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>> There are not the clock controller of ppmudmc0/1. This patch control the clock
>>> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>>>
>>> Also, this patch code clean about regulator control and free resource
>>> when calling exit/remove function.
>>>
>>> For example,
>>> busfreq@106A0000 {
>>>     compatible = "samsung,exynos4x12-busfreq";
>>>
>>>     /* Clock for PPMUDMC0/1 */
>>>     clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
>>>     clock-names = "ppmudmc0", "ppmudmc1";
>>>
>>>     /* Regulator for MIF/INT block */
>>>     vdd_mif-supply = <&buck1_reg>;
>>>     vdd_int-supply = <&buck3_reg>;
>>> };
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>

I modify this patch according your comment as following:

Best Regards,
Chanwoo Choi

>From c8f2fbc4c1166ec02fb2ad46164bc7ed9118721b Mon Sep 17 00:00:00 2001
From: Chanwoo Choi <cw00.choi@samsung.com>
Date: Fri, 14 Mar 2014 12:05:54 +0900
Subject: [PATCH] devfreq: exynos4: Add ppmu's clock control and code clean
 about regulator control

There are not the clock controller of ppmudmc0/1. This patch control the clock
of ppmudmc0/1 which is used for monitoring memory bus utilization.

Also, this patch code clean about regulator control and free resource
when calling exit/remove function.

For example,
busfreq@106A0000 {
	compatible = "samsung,exynos4x12-busfreq";

	/* Clock for PPMUDMC0/1 */
	clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
	clock-names = "ppmudmc0", "ppmudmc1";

	/* Regulator for MIF/INT block */
	vdd_mif-supply = <&buck1_reg>;
	vdd_int-supply = <&buck3_reg>;
};

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/exynos4_bus.c | 97 +++++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 13 deletions(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 4c630fb..3956bcc 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
 	PPMU_END,
 };
 
+static const char *exynos_ppmu_clk_name[] = {
+	[PPMU_DMC0]	= "ppmudmc0",
+	[PPMU_DMC1]	= "ppmudmc1",
+};
+
 #define EX4210_LV_MAX	LV_2
 #define EX4x12_LV_MAX	LV_4
 #define EX4210_LV_NUM	(LV_2 + 1)
@@ -86,6 +91,7 @@ struct busfreq_data {
 	struct regulator *vdd_mif; /* Exynos4412/4212 only */
 	struct busfreq_opp_info curr_oppinfo;
 	struct exynos_ppmu ppmu[PPMU_END];
+	struct clk *clk_ppmu[PPMU_END];
 
 	struct notifier_block pm_notifier;
 	struct mutex lock;
@@ -724,6 +730,17 @@ static void exynos4_bus_exit(struct device *dev)
 	struct busfreq_data *data = dev_get_drvdata(dev);
 	int i;
 
+	/*
+	 * Un-map memory map and disable regulator/clocks
+	 * to prevent power leakage.
+	 */
+	regulator_disable(data->vdd_int);
+	if (data->type == TYPE_BUSF_EXYNOS4x12)
+		regulator_disable(data->vdd_mif);
+
+	for (i = 0; i < PPMU_END; i++)
+		clk_disable_unprepare(data->clk_ppmu[i]);
+
 	for (i = 0; i < PPMU_END; i++)
 		iounmap(data->ppmu[i].hw_base);
 }
@@ -989,6 +1006,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
 {
 	struct device *dev = data->dev;
 	struct device_node *np = dev->of_node;
+	const char **clk_name = exynos_ppmu_clk_name;
 	int i, ret = 0;
 
 	if (!np) {
@@ -1006,8 +1024,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
 		}
 	}
 
+	for (i = 0; i < PPMU_END; i++) {
+		data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
+		if (IS_ERR(data->clk_ppmu[i])) {
+			dev_warn(dev, "Failed to get %s clock\n", clk_name[i]);
+			goto err_clocks;
+		}
+
+		ret = clk_prepare_enable(data->clk_ppmu[i]);
+		if (ret < 0) {
+			dev_warn(dev, "Failed to enable %s clock\n", clk_name[i]);
+			data->clk_ppmu[i] = NULL;
+			goto err_clocks;
+		}
+	}
+
+	/* Get regulator to control voltage of int block */
+	data->vdd_int = devm_regulator_get(dev, "vdd_int");
+	if (IS_ERR(data->vdd_int)) {
+		dev_err(dev, "Failed to get the regulator of vdd_int\n");
+		ret = PTR_ERR(data->vdd_int);
+		goto err_clocks;
+	}
+	ret = regulator_enable(data->vdd_int);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulator of vdd_int\n");
+		goto err_clocks;
+	}
+
+	switch (data->type) {
+	case TYPE_BUSF_EXYNOS4210:
+		break;
+	case TYPE_BUSF_EXYNOS4x12:
+		/* Get regulator to control voltage of mif blk if Exynos4x12 */
+		data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
+		if (IS_ERR(data->vdd_mif)) {
+			dev_err(dev, "Failed to get the regulator vdd_mif\n");
+			ret = PTR_ERR(data->vdd_mif);
+			goto err_regulator;
+		}
+		ret = regulator_enable(data->vdd_mif);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
+			goto err_regulator;
+		}
+		break;
+	default:
+		dev_err(dev, "Unknown device type : %d\n", data->type);
+		return -EINVAL;
+	};
+
 	return 0;
 
+err_regulator:
+	regulator_disable(data->vdd_int);
+err_clocks:
+	for (i = 0; i < PPMU_END; i++) {
+		if (IS_ERR(data->clk_ppmu[i]))
+			continue;
+		else
+			clk_disable_unprepare(data->clk_ppmu[i]);
+	}
 err_iomap:
 	for (i = 0; i < PPMU_END; i++) {
 		if (IS_ERR_OR_NULL(data->ppmu[i].hw_base))
@@ -1074,19 +1151,6 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	data->vdd_int = devm_regulator_get(dev, "vdd_int");
-	if (IS_ERR(data->vdd_int)) {
-		dev_err(dev, "Cannot get the regulator \"vdd_int\"\n");
-		return PTR_ERR(data->vdd_int);
-	}
-	if (data->type == TYPE_BUSF_EXYNOS4x12) {
-		data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
-		if (IS_ERR(data->vdd_mif)) {
-			dev_err(dev, "Cannot get the regulator \"vdd_mif\"\n");
-			return PTR_ERR(data->vdd_mif);
-		}
-	}
-
 	rcu_read_lock();
 	opp = dev_pm_opp_find_freq_floor(dev,
 					 &exynos4_devfreq_profile.initial_freq);
@@ -1146,6 +1210,13 @@ err_notifier_opp:
 	return err;
 
 err_devfreq:
+	regulator_disable(data->vdd_int);
+	if (data->type == TYPE_BUSF_EXYNOS4x12)
+		regulator_disable(data->vdd_mif);
+
+	for (i = 0; i < PPMU_END; i++)
+		clk_disable_unprepare(data->clk_ppmu[i]);
+
 	for (i = 0; i < PPMU_END; i++)
 		iounmap(data->ppmu[i].hw_base);
 
-- 
1.8.0

Re: [PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Tomasz Figa]

Hi Chanwoo,

On 17.03.2014 06:59, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 03/17/2014 11:51 AM, Chanwoo Choi wrote:
>> Hi Tomasz,
>>
>> On 03/15/2014 02:42 AM, Tomasz Figa wrote:
>>> Hi Chanwoo,
>>>
>>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>>> There are not the clock controller of ppmudmc0/1. This patch control the clock
>>>> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>>>>
>>>> Also, this patch code clean about regulator control and free resource
>>>> when calling exit/remove function.
>>>>
>>>> For example,
>>>> busfreq@106A0000 {
>>>>      compatible = "samsung,exynos4x12-busfreq";
>>>>
>>>>      /* Clock for PPMUDMC0/1 */
>>>>      clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
>>>>      clock-names = "ppmudmc0", "ppmudmc1";
>>>>
>>>>      /* Regulator for MIF/INT block */
>>>>      vdd_mif-supply = <&buck1_reg>;
>>>>      vdd_int-supply = <&buck3_reg>;
>>>> };
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> I modify this patch according your comment as following:
>
> Best Regards,
> Chanwoo Choi
>
>>From c8f2fbc4c1166ec02fb2ad46164bc7ed9118721b Mon Sep 17 00:00:00 2001
> From: Chanwoo Choi <cw00.choi@samsung.com>
> Date: Fri, 14 Mar 2014 12:05:54 +0900
> Subject: [PATCH] devfreq: exynos4: Add ppmu's clock control and code clean
>   about regulator control
>
> There are not the clock controller of ppmudmc0/1. This patch control the clock
> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>
> Also, this patch code clean about regulator control and free resource
> when calling exit/remove function.
>
> For example,
> busfreq@106A0000 {
> 	compatible = "samsung,exynos4x12-busfreq";
>
> 	/* Clock for PPMUDMC0/1 */
> 	clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
> 	clock-names = "ppmudmc0", "ppmudmc1";
>
> 	/* Regulator for MIF/INT block */
> 	vdd_mif-supply = <&buck1_reg>;
> 	vdd_int-supply = <&buck3_reg>;
> };
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>   drivers/devfreq/exynos/exynos4_bus.c | 97 +++++++++++++++++++++++++++++++-----
>   1 file changed, 84 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
> index 4c630fb..3956bcc 100644
> --- a/drivers/devfreq/exynos/exynos4_bus.c
> +++ b/drivers/devfreq/exynos/exynos4_bus.c
> @@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
>   	PPMU_END,
>   };
>
> +static const char *exynos_ppmu_clk_name[] = {
> +	[PPMU_DMC0]	= "ppmudmc0",
> +	[PPMU_DMC1]	= "ppmudmc1",
> +};
> +
>   #define EX4210_LV_MAX	LV_2
>   #define EX4x12_LV_MAX	LV_4
>   #define EX4210_LV_NUM	(LV_2 + 1)
> @@ -86,6 +91,7 @@ struct busfreq_data {
>   	struct regulator *vdd_mif; /* Exynos4412/4212 only */
>   	struct busfreq_opp_info curr_oppinfo;
>   	struct exynos_ppmu ppmu[PPMU_END];
> +	struct clk *clk_ppmu[PPMU_END];
>
>   	struct notifier_block pm_notifier;
>   	struct mutex lock;
> @@ -724,6 +730,17 @@ static void exynos4_bus_exit(struct device *dev)
>   	struct busfreq_data *data = dev_get_drvdata(dev);
>   	int i;
>
> +	/*
> +	 * Un-map memory map and disable regulator/clocks
> +	 * to prevent power leakage.
> +	 */
> +	regulator_disable(data->vdd_int);
> +	if (data->type == TYPE_BUSF_EXYNOS4x12)
> +		regulator_disable(data->vdd_mif);
> +
> +	for (i = 0; i < PPMU_END; i++)
> +		clk_disable_unprepare(data->clk_ppmu[i]);
> +
>   	for (i = 0; i < PPMU_END; i++)
>   		iounmap(data->ppmu[i].hw_base);
>   }
> @@ -989,6 +1006,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>   {
>   	struct device *dev = data->dev;
>   	struct device_node *np = dev->of_node;
> +	const char **clk_name = exynos_ppmu_clk_name;
>   	int i, ret = 0;
>
>   	if (!np) {
> @@ -1006,8 +1024,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>   		}
>   	}
>
> +	for (i = 0; i < PPMU_END; i++) {
> +		data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
> +		if (IS_ERR(data->clk_ppmu[i])) {
> +			dev_warn(dev, "Failed to get %s clock\n", clk_name[i]);

dev_err() since this is an error.

> +			goto err_clocks;
> +		}
> +
> +		ret = clk_prepare_enable(data->clk_ppmu[i]);
> +		if (ret < 0) {
> +			dev_warn(dev, "Failed to enable %s clock\n", clk_name[i]);

Ditto.

> +			data->clk_ppmu[i] = NULL;
> +			goto err_clocks;
> +		}
> +	}
> +
> +	/* Get regulator to control voltage of int block */
> +	data->vdd_int = devm_regulator_get(dev, "vdd_int");
> +	if (IS_ERR(data->vdd_int)) {
> +		dev_err(dev, "Failed to get the regulator of vdd_int\n");
> +		ret = PTR_ERR(data->vdd_int);
> +		goto err_clocks;
> +	}
> +	ret = regulator_enable(data->vdd_int);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulator of vdd_int\n");
> +		goto err_clocks;
> +	}
> +
> +	switch (data->type) {
> +	case TYPE_BUSF_EXYNOS4210:
> +		break;

Do you need to use switch here? Wouldn't a simple if (data->type == 
TYPE_BUSF_EXYNOS4x12) work as well, resulting with simpler code?

However I would expect this code to be completely removed, after 
defining DT bindings in a way allowing you to completely drop such SoC 
type enums. Please see my other reply for explanation of Exynos power 
plane DT bindings concept.

> +	case TYPE_BUSF_EXYNOS4x12:
> +		/* Get regulator to control voltage of mif blk if Exynos4x12 */
> +		data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
> +		if (IS_ERR(data->vdd_mif)) {
> +			dev_err(dev, "Failed to get the regulator vdd_mif\n");
> +			ret = PTR_ERR(data->vdd_mif);
> +			goto err_regulator;
> +		}
> +		ret = regulator_enable(data->vdd_mif);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
> +			goto err_regulator;
> +		}
> +		break;
> +	default:
> +		dev_err(dev, "Unknown device type : %d\n", data->type);
> +		return -EINVAL;
> +	};
> +
>   	return 0;
>
> +err_regulator:
> +	regulator_disable(data->vdd_int);
> +err_clocks:
> +	for (i = 0; i < PPMU_END; i++) {
> +		if (IS_ERR(data->clk_ppmu[i]))
> +			continue;
> +		else

if (!IS_ERR(...))

> +			clk_disable_unprepare(data->clk_ppmu[i]);
> +	}

Otherwise looks good.

Best regards,
Tomasz

Re: [PATCHv2 3/8] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Chanwoo Choi]

Hi Tomasz,

On 03/18/2014 08:13 PM, Tomasz Figa wrote:
> Hi Chanwoo,
> 
> On 17.03.2014 06:59, Chanwoo Choi wrote:
>> Hi Tomasz,
>>
>> On 03/17/2014 11:51 AM, Chanwoo Choi wrote:
>>> Hi Tomasz,
>>>
>>> On 03/15/2014 02:42 AM, Tomasz Figa wrote:
>>>> Hi Chanwoo,
>>>>
>>>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>>>> There are not the clock controller of ppmudmc0/1. This patch control the clock
>>>>> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>>>>>
>>>>> Also, this patch code clean about regulator control and free resource
>>>>> when calling exit/remove function.
>>>>>
>>>>> For example,
>>>>> busfreq@106A0000 {
>>>>>      compatible = "samsung,exynos4x12-busfreq";
>>>>>
>>>>>      /* Clock for PPMUDMC0/1 */
>>>>>      clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
>>>>>      clock-names = "ppmudmc0", "ppmudmc1";
>>>>>
>>>>>      /* Regulator for MIF/INT block */
>>>>>      vdd_mif-supply = <&buck1_reg>;
>>>>>      vdd_int-supply = <&buck3_reg>;
>>>>> };
>>>>>
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> I modify this patch according your comment as following:
>>
>> Best Regards,
>> Chanwoo Choi
>>
>>> From c8f2fbc4c1166ec02fb2ad46164bc7ed9118721b Mon Sep 17 00:00:00 2001
>> From: Chanwoo Choi <cw00.choi@samsung.com>
>> Date: Fri, 14 Mar 2014 12:05:54 +0900
>> Subject: [PATCH] devfreq: exynos4: Add ppmu's clock control and code clean
>>   about regulator control
>>
>> There are not the clock controller of ppmudmc0/1. This patch control the clock
>> of ppmudmc0/1 which is used for monitoring memory bus utilization.
>>
>> Also, this patch code clean about regulator control and free resource
>> when calling exit/remove function.
>>
>> For example,
>> busfreq@106A0000 {
>>     compatible = "samsung,exynos4x12-busfreq";
>>
>>     /* Clock for PPMUDMC0/1 */
>>     clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
>>     clock-names = "ppmudmc0", "ppmudmc1";
>>
>>     /* Regulator for MIF/INT block */
>>     vdd_mif-supply = <&buck1_reg>;
>>     vdd_int-supply = <&buck3_reg>;
>> };
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>   drivers/devfreq/exynos/exynos4_bus.c | 97 +++++++++++++++++++++++++++++++-----
>>   1 file changed, 84 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>> index 4c630fb..3956bcc 100644
>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>> @@ -62,6 +62,11 @@ enum exynos_ppmu_idx {
>>       PPMU_END,
>>   };
>>
>> +static const char *exynos_ppmu_clk_name[] = {
>> +    [PPMU_DMC0]    = "ppmudmc0",
>> +    [PPMU_DMC1]    = "ppmudmc1",
>> +};
>> +
>>   #define EX4210_LV_MAX    LV_2
>>   #define EX4x12_LV_MAX    LV_4
>>   #define EX4210_LV_NUM    (LV_2 + 1)
>> @@ -86,6 +91,7 @@ struct busfreq_data {
>>       struct regulator *vdd_mif; /* Exynos4412/4212 only */
>>       struct busfreq_opp_info curr_oppinfo;
>>       struct exynos_ppmu ppmu[PPMU_END];
>> +    struct clk *clk_ppmu[PPMU_END];
>>
>>       struct notifier_block pm_notifier;
>>       struct mutex lock;
>> @@ -724,6 +730,17 @@ static void exynos4_bus_exit(struct device *dev)
>>       struct busfreq_data *data = dev_get_drvdata(dev);
>>       int i;
>>
>> +    /*
>> +     * Un-map memory map and disable regulator/clocks
>> +     * to prevent power leakage.
>> +     */
>> +    regulator_disable(data->vdd_int);
>> +    if (data->type == TYPE_BUSF_EXYNOS4x12)
>> +        regulator_disable(data->vdd_mif);
>> +
>> +    for (i = 0; i < PPMU_END; i++)
>> +        clk_disable_unprepare(data->clk_ppmu[i]);
>> +
>>       for (i = 0; i < PPMU_END; i++)
>>           iounmap(data->ppmu[i].hw_base);
>>   }
>> @@ -989,6 +1006,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>   {
>>       struct device *dev = data->dev;
>>       struct device_node *np = dev->of_node;
>> +    const char **clk_name = exynos_ppmu_clk_name;
>>       int i, ret = 0;
>>
>>       if (!np) {
>> @@ -1006,8 +1024,67 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data)
>>           }
>>       }
>>
>> +    for (i = 0; i < PPMU_END; i++) {
>> +        data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]);
>> +        if (IS_ERR(data->clk_ppmu[i])) {
>> +            dev_warn(dev, "Failed to get %s clock\n", clk_name[i]);
> 
> dev_err() since this is an error.

OK, I'll fix it.

> 
>> +            goto err_clocks;
>> +        }
>> +
>> +        ret = clk_prepare_enable(data->clk_ppmu[i]);
>> +        if (ret < 0) {
>> +            dev_warn(dev, "Failed to enable %s clock\n", clk_name[i]);
> 
> Ditto.

OK, I'll fix it.

> 
>> +            data->clk_ppmu[i] = NULL;
>> +            goto err_clocks;
>> +        }
>> +    }
>> +
>> +    /* Get regulator to control voltage of int block */
>> +    data->vdd_int = devm_regulator_get(dev, "vdd_int");
>> +    if (IS_ERR(data->vdd_int)) {
>> +        dev_err(dev, "Failed to get the regulator of vdd_int\n");
>> +        ret = PTR_ERR(data->vdd_int);
>> +        goto err_clocks;
>> +    }
>> +    ret = regulator_enable(data->vdd_int);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to enable regulator of vdd_int\n");
>> +        goto err_clocks;
>> +    }
>> +
>> +    switch (data->type) {
>> +    case TYPE_BUSF_EXYNOS4210:
>> +        break;
> 
> Do you need to use switch here? Wouldn't a simple if (data->type == TYPE_BUSF_EXYNOS4x12) work as well, resulting with simpler code?

OK, I'll fix it.

> 
> However I would expect this code to be completely removed, after defining DT bindings in a way allowing you to completely drop such SoC type enums. Please see my other reply for explanation of Exynos power plane DT bindings concept.

OK, I agree your suggestion about Exynos power plane DT bindings concept.

I'll apply "Exynos power plane" to exynos4 busfreq driver on next step
after defining Exynos power plane DT bindings.

> 
>> +    case TYPE_BUSF_EXYNOS4x12:
>> +        /* Get regulator to control voltage of mif blk if Exynos4x12 */
>> +        data->vdd_mif = devm_regulator_get(dev, "vdd_mif");
>> +        if (IS_ERR(data->vdd_mif)) {
>> +            dev_err(dev, "Failed to get the regulator vdd_mif\n");
>> +            ret = PTR_ERR(data->vdd_mif);
>> +            goto err_regulator;
>> +        }
>> +        ret = regulator_enable(data->vdd_mif);
>> +        if (ret < 0) {
>> +            dev_err(dev, "Failed to enable regulator of vdd_mif\n");
>> +            goto err_regulator;
>> +        }
>> +        break;
>> +    default:
>> +        dev_err(dev, "Unknown device type : %d\n", data->type);
>> +        return -EINVAL;
>> +    };
>> +
>>       return 0;
>>
>> +err_regulator:
>> +    regulator_disable(data->vdd_int);
>> +err_clocks:
>> +    for (i = 0; i < PPMU_END; i++) {
>> +        if (IS_ERR(data->clk_ppmu[i]))
>> +            continue;
>> +        else
> 
> if (!IS_ERR(...))
> 
>> +            clk_disable_unprepare(data->clk_ppmu[i]);
>> +    }
> 
> Otherwise looks good.

Thanks for your review.

Best Regards,
Chanwoo Choi

[PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()

[Chanwoo Choi]

This patch fix bug about resource leak when happening probe fail and code clean
to add debug message.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/exynos4_bus.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index a2a3a47..152a3e9 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 		dev_err(dev, "Cannot determine the device id %d\n", data->type);
 		err = -EINVAL;
 	}
-	if (err)
+	if (err) {
+		dev_err(dev, "Cannot initialize busfreq table %d\n",
+			     data->type);
 		return err;
+	}
 
 	rcu_read_lock();
 	opp = dev_pm_opp_find_freq_floor(dev,
@@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	if (IS_ERR(data->devfreq)) {
 		dev_err(dev, "Failed to add devfreq device\n");
 		err = PTR_ERR(data->devfreq);
-		goto err_opp;
+		goto err_devfreq;
 	}
 
 	/*
@@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
 	 */
 	busfreq_mon_reset(data);
 
-	devfreq_register_opp_notifier(dev, data->devfreq);
+	/* Register opp_notifier for Exynos4 busfreq */
+	err = devfreq_register_opp_notifier(dev, data->devfreq);
+	if (err < 0) {
+		dev_err(dev, "Failed to register opp notifier\n");
+		goto err_notifier_opp;
+	}
 
+	/* Register pm_notifier for Exynos4 busfreq */
 	err = register_pm_notifier(&data->pm_notifier);
 	if (err) {
 		dev_err(dev, "Failed to setup pm notifier\n");
-		devfreq_remove_device(data->devfreq);
-		return err;
+		goto err_notifier_pm;
 	}
 
 	return 0;
 
-err_opp:
+err_notifier_pm:
+	devfreq_unregister_opp_notifier(dev, data->devfreq);
+err_notifier_opp:
+	/*
+	 * The devfreq_remove_device() would execute finally devfreq->profile
+	 * ->exit(). To avoid duplicate resource free operation, return directly
+	 * before executing resource free below 'err_devfreq' goto statement.
+	 */
+	devfreq_remove_device(data->devfreq);
+
+	return err;
+
+err_devfreq:
 	regulator_disable(data->vdd_int);
 	if (data->type == TYPE_BUSF_EXYNOS4x12)
 		regulator_disable(data->vdd_mif);
-- 
1.8.0

Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()

[Tomasz Figa]

Hi Chanwoo,

On 13.03.2014 09:17, Chanwoo Choi wrote:
> This patch fix bug about resource leak when happening probe fail and code clean
> to add debug message.
>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>   drivers/devfreq/exynos/exynos4_bus.c | 32 ++++++++++++++++++++++++++------
>   1 file changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
> index a2a3a47..152a3e9 100644
> --- a/drivers/devfreq/exynos/exynos4_bus.c
> +++ b/drivers/devfreq/exynos/exynos4_bus.c
> @@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>   		dev_err(dev, "Cannot determine the device id %d\n", data->type);
>   		err = -EINVAL;
>   	}
> -	if (err)
> +	if (err) {
> +		dev_err(dev, "Cannot initialize busfreq table %d\n",
> +			     data->type);
>   		return err;
> +	}
>
>   	rcu_read_lock();
>   	opp = dev_pm_opp_find_freq_floor(dev,
> @@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>   	if (IS_ERR(data->devfreq)) {
>   		dev_err(dev, "Failed to add devfreq device\n");
>   		err = PTR_ERR(data->devfreq);
> -		goto err_opp;
> +		goto err_devfreq;
>   	}
>
>   	/*
> @@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>   	 */
>   	busfreq_mon_reset(data);
>
> -	devfreq_register_opp_notifier(dev, data->devfreq);
> +	/* Register opp_notifier for Exynos4 busfreq */
> +	err = devfreq_register_opp_notifier(dev, data->devfreq);
> +	if (err < 0) {
> +		dev_err(dev, "Failed to register opp notifier\n");
> +		goto err_notifier_opp;
> +	}
>
> +	/* Register pm_notifier for Exynos4 busfreq */
>   	err = register_pm_notifier(&data->pm_notifier);
>   	if (err) {
>   		dev_err(dev, "Failed to setup pm notifier\n");
> -		devfreq_remove_device(data->devfreq);
> -		return err;
> +		goto err_notifier_pm;
>   	}
>
>   	return 0;
>
> -err_opp:
> +err_notifier_pm:
> +	devfreq_unregister_opp_notifier(dev, data->devfreq);
> +err_notifier_opp:
> +	/*
> +	 * The devfreq_remove_device() would execute finally devfreq->profile
> +	 * ->exit(). To avoid duplicate resource free operation, return directly
> +	 * before executing resource free below 'err_devfreq' goto statement.
> +	 */

I'm not quite sure about this. I believe that in this case 
devfreq->profile->exit() would be exynos4_bus_exit() and all it does is 
devfreq_unregister_opp_notifier(dev, data->devfreq), so all remaining 
resources (regulators, clocks, etc.) would get leaked.

I believe the correct thing to do would be to remove the .exit() 
callback from exynos4_devfreq_profile struct and handle all the clean-up 
here in error path.

Best regards,
Tomasz

Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()

[Chanwoo Choi]

Hi Tomasz,

On 03/15/2014 02:49 AM, Tomasz Figa wrote:
> Hi Chanwoo,
> 
> On 13.03.2014 09:17, Chanwoo Choi wrote:
>> This patch fix bug about resource leak when happening probe fail and code clean
>> to add debug message.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>   drivers/devfreq/exynos/exynos4_bus.c | 32 ++++++++++++++++++++++++++------
>>   1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>> index a2a3a47..152a3e9 100644
>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>> @@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>           dev_err(dev, "Cannot determine the device id %d\n", data->type);
>>           err = -EINVAL;
>>       }
>> -    if (err)
>> +    if (err) {
>> +        dev_err(dev, "Cannot initialize busfreq table %d\n",
>> +                 data->type);
>>           return err;
>> +    }
>>
>>       rcu_read_lock();
>>       opp = dev_pm_opp_find_freq_floor(dev,
>> @@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>       if (IS_ERR(data->devfreq)) {
>>           dev_err(dev, "Failed to add devfreq device\n");
>>           err = PTR_ERR(data->devfreq);
>> -        goto err_opp;
>> +        goto err_devfreq;
>>       }
>>
>>       /*
>> @@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>        */
>>       busfreq_mon_reset(data);
>>
>> -    devfreq_register_opp_notifier(dev, data->devfreq);
>> +    /* Register opp_notifier for Exynos4 busfreq */
>> +    err = devfreq_register_opp_notifier(dev, data->devfreq);
>> +    if (err < 0) {
>> +        dev_err(dev, "Failed to register opp notifier\n");
>> +        goto err_notifier_opp;
>> +    }
>>
>> +    /* Register pm_notifier for Exynos4 busfreq */
>>       err = register_pm_notifier(&data->pm_notifier);
>>       if (err) {
>>           dev_err(dev, "Failed to setup pm notifier\n");
>> -        devfreq_remove_device(data->devfreq);
>> -        return err;
>> +        goto err_notifier_pm;
>>       }
>>
>>       return 0;
>>
>> -err_opp:
>> +err_notifier_pm:
>> +    devfreq_unregister_opp_notifier(dev, data->devfreq);
>> +err_notifier_opp:
>> +    /*
>> +     * The devfreq_remove_device() would execute finally devfreq->profile
>> +     * ->exit(). To avoid duplicate resource free operation, return directly
>> +     * before executing resource free below 'err_devfreq' goto statement.
>> +     */
> 
> I'm not quite sure about this. I believe that in this case devfreq->profile->exit() would be exynos4_bus_exit() and all it does is devfreq_unregister_opp_notifier(dev, data->devfreq), so all remaining resources (regulators, clocks, etc.) would get leaked.

This patch execute following sequence to probe exynos4_busfreq.c:

1. Parse dt node to get resource(regulator/clock/memory address).
2. Enable regulator/clock and map memory.
3. Add devfreq device using devfreq_add_device().
   The devfreq_add_device() return devfreq instance(data->devfreq).
4. Register opp_notifier using devfreq instance(data->devfreq) which is created in sequence #3.

Case 1,
If an error happens in sequence #3 for registering devfreq_add_device(),

this case can't execute devfreq->profile->exit() to free resource
because this case has failed to register devfreq->profile to devfreq_list.

So, must goto 'err_devfreq' statement to free resource(regulator/clock/memory).


Case 2,
If an error happens in sequence #4 for registering opp_notifier,

In contrast
this case can execute devfreq->profile->exit() to free resource.
But, After executed devfreq->profile->exit(),
should not execute 'err_devfreq' statement to free resource.
In case, will operate twice of resource.

If my explanation is wrong, please reply your comment.

> 
> I believe the correct thing to do would be to remove the .exit() callback from exynos4_devfreq_profile struct and handle all the clean-up here in error path.
> 

Best Regards,
Chanwoo Choi

Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()

[Tomasz Figa]

On 17.03.2014 06:05, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 03/15/2014 02:49 AM, Tomasz Figa wrote:
>> Hi Chanwoo,
>>
>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>> This patch fix bug about resource leak when happening probe fail and code clean
>>> to add debug message.
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>    drivers/devfreq/exynos/exynos4_bus.c | 32 ++++++++++++++++++++++++++------
>>>    1 file changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>>> index a2a3a47..152a3e9 100644
>>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>>> @@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>>            dev_err(dev, "Cannot determine the device id %d\n", data->type);
>>>            err = -EINVAL;
>>>        }
>>> -    if (err)
>>> +    if (err) {
>>> +        dev_err(dev, "Cannot initialize busfreq table %d\n",
>>> +                 data->type);
>>>            return err;
>>> +    }
>>>
>>>        rcu_read_lock();
>>>        opp = dev_pm_opp_find_freq_floor(dev,
>>> @@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>>        if (IS_ERR(data->devfreq)) {
>>>            dev_err(dev, "Failed to add devfreq device\n");
>>>            err = PTR_ERR(data->devfreq);
>>> -        goto err_opp;
>>> +        goto err_devfreq;
>>>        }
>>>
>>>        /*
>>> @@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>>         */
>>>        busfreq_mon_reset(data);
>>>
>>> -    devfreq_register_opp_notifier(dev, data->devfreq);
>>> +    /* Register opp_notifier for Exynos4 busfreq */
>>> +    err = devfreq_register_opp_notifier(dev, data->devfreq);
>>> +    if (err < 0) {
>>> +        dev_err(dev, "Failed to register opp notifier\n");
>>> +        goto err_notifier_opp;
>>> +    }
>>>
>>> +    /* Register pm_notifier for Exynos4 busfreq */
>>>        err = register_pm_notifier(&data->pm_notifier);
>>>        if (err) {
>>>            dev_err(dev, "Failed to setup pm notifier\n");
>>> -        devfreq_remove_device(data->devfreq);
>>> -        return err;
>>> +        goto err_notifier_pm;
>>>        }
>>>
>>>        return 0;
>>>
>>> -err_opp:
>>> +err_notifier_pm:
>>> +    devfreq_unregister_opp_notifier(dev, data->devfreq);
>>> +err_notifier_opp:
>>> +    /*
>>> +     * The devfreq_remove_device() would execute finally devfreq->profile
>>> +     * ->exit(). To avoid duplicate resource free operation, return directly
>>> +     * before executing resource free below 'err_devfreq' goto statement.
>>> +     */
>>
>> I'm not quite sure about this. I believe that in this case devfreq->profile->exit() would be exynos4_bus_exit() and all it does is devfreq_unregister_opp_notifier(dev, data->devfreq), so all remaining resources (regulators, clocks, etc.) would get leaked.
>
> This patch execute following sequence to probe exynos4_busfreq.c:
>
> 1. Parse dt node to get resource(regulator/clock/memory address).
> 2. Enable regulator/clock and map memory.
> 3. Add devfreq device using devfreq_add_device().
>     The devfreq_add_device() return devfreq instance(data->devfreq).
> 4. Register opp_notifier using devfreq instance(data->devfreq) which is created in sequence #3.
>
> Case 1,
> If an error happens in sequence #3 for registering devfreq_add_device(),
>
> this case can't execute devfreq->profile->exit() to free resource
> because this case has failed to register devfreq->profile to devfreq_list.
>
> So, must goto 'err_devfreq' statement to free resource(regulator/clock/memory).
>
>
> Case 2,
> If an error happens in sequence #4 for registering opp_notifier,
>
> In contrast
> this case can execute devfreq->profile->exit() to free resource.
> But, After executed devfreq->profile->exit(),
> should not execute 'err_devfreq' statement to free resource.
> In case, will operate twice of resource.
>
> If my explanation is wrong, please reply your comment.
>

OK, it will work indeed, however the code is barely readable with this.

For consistency (and readability), resources acquired in platform 
driver's .probe() should be freed in .remove() and error path of 
.probe() should not rely on external code to do the clean-up.

So, as I proposed in my previous reply:

>>
>> I believe the correct thing to do would be to remove the .exit() callback from exynos4_devfreq_profile struct and handle all the clean-up here in error path.
>>

Best regards,
Tomasz

Re: [PATCHv2 4/8] devfreq: exynos4: Fix bug of resource leak and code clean on probe()

[Chanwoo Choi]

Hi Tomasz,

On 03/18/2014 09:18 PM, Tomasz Figa wrote:
> On 17.03.2014 06:05, Chanwoo Choi wrote:
>> Hi Tomasz,
>>
>> On 03/15/2014 02:49 AM, Tomasz Figa wrote:
>>> Hi Chanwoo,
>>>
>>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>>> This patch fix bug about resource leak when happening probe fail and code clean
>>>> to add debug message.
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>    drivers/devfreq/exynos/exynos4_bus.c | 32 ++++++++++++++++++++++++++------
>>>>    1 file changed, 26 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>>>> index a2a3a47..152a3e9 100644
>>>> --- a/drivers/devfreq/exynos/exynos4_bus.c
>>>> +++ b/drivers/devfreq/exynos/exynos4_bus.c
>>>> @@ -1152,8 +1152,11 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>>>            dev_err(dev, "Cannot determine the device id %d\n", data->type);
>>>>            err = -EINVAL;
>>>>        }
>>>> -    if (err)
>>>> +    if (err) {
>>>> +        dev_err(dev, "Cannot initialize busfreq table %d\n",
>>>> +                 data->type);
>>>>            return err;
>>>> +    }
>>>>
>>>>        rcu_read_lock();
>>>>        opp = dev_pm_opp_find_freq_floor(dev,
>>>> @@ -1176,7 +1179,7 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>>>        if (IS_ERR(data->devfreq)) {
>>>>            dev_err(dev, "Failed to add devfreq device\n");
>>>>            err = PTR_ERR(data->devfreq);
>>>> -        goto err_opp;
>>>> +        goto err_devfreq;
>>>>        }
>>>>
>>>>        /*
>>>> @@ -1185,18 +1188,35 @@ static int exynos4_busfreq_probe(struct platform_device *pdev)
>>>>         */
>>>>        busfreq_mon_reset(data);
>>>>
>>>> -    devfreq_register_opp_notifier(dev, data->devfreq);
>>>> +    /* Register opp_notifier for Exynos4 busfreq */
>>>> +    err = devfreq_register_opp_notifier(dev, data->devfreq);
>>>> +    if (err < 0) {
>>>> +        dev_err(dev, "Failed to register opp notifier\n");
>>>> +        goto err_notifier_opp;
>>>> +    }
>>>>
>>>> +    /* Register pm_notifier for Exynos4 busfreq */
>>>>        err = register_pm_notifier(&data->pm_notifier);
>>>>        if (err) {
>>>>            dev_err(dev, "Failed to setup pm notifier\n");
>>>> -        devfreq_remove_device(data->devfreq);
>>>> -        return err;
>>>> +        goto err_notifier_pm;
>>>>        }
>>>>
>>>>        return 0;
>>>>
>>>> -err_opp:
>>>> +err_notifier_pm:
>>>> +    devfreq_unregister_opp_notifier(dev, data->devfreq);
>>>> +err_notifier_opp:
>>>> +    /*
>>>> +     * The devfreq_remove_device() would execute finally devfreq->profile
>>>> +     * ->exit(). To avoid duplicate resource free operation, return directly
>>>> +     * before executing resource free below 'err_devfreq' goto statement.
>>>> +     */
>>>
>>> I'm not quite sure about this. I believe that in this case devfreq->profile->exit() would be exynos4_bus_exit() and all it does is devfreq_unregister_opp_notifier(dev, data->devfreq), so all remaining resources (regulators, clocks, etc.) would get leaked.
>>
>> This patch execute following sequence to probe exynos4_busfreq.c:
>>
>> 1. Parse dt node to get resource(regulator/clock/memory address).
>> 2. Enable regulator/clock and map memory.
>> 3. Add devfreq device using devfreq_add_device().
>>     The devfreq_add_device() return devfreq instance(data->devfreq).
>> 4. Register opp_notifier using devfreq instance(data->devfreq) which is created in sequence #3.
>>
>> Case 1,
>> If an error happens in sequence #3 for registering devfreq_add_device(),
>>
>> this case can't execute devfreq->profile->exit() to free resource
>> because this case has failed to register devfreq->profile to devfreq_list.
>>
>> So, must goto 'err_devfreq' statement to free resource(regulator/clock/memory).
>>
>>
>> Case 2,
>> If an error happens in sequence #4 for registering opp_notifier,
>>
>> In contrast
>> this case can execute devfreq->profile->exit() to free resource.
>> But, After executed devfreq->profile->exit(),
>> should not execute 'err_devfreq' statement to free resource.
>> In case, will operate twice of resource.
>>
>> If my explanation is wrong, please reply your comment.
>>
> 
> OK, it will work indeed, however the code is barely readable with this.
> 
> For consistency (and readability), resources acquired in platform driver's .probe() should be freed in .remove() and error path of .probe() should not rely on external code to do the clean-up.
> 
> So, as I proposed in my previous reply:

OK, I'll modify it according to your previous comment.

> 
>>>
>>> I believe the correct thing to do would be to remove the .exit() callback from exynos4_devfreq_profile struct and handle all the clean-up here in error path.
>>>

Best Regards,
Chanwoo Choi
 

[PATCHv2 5/8] devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro

[Chanwoo Choi]

This patch use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/exynos4_bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 152a3e9..9576ee2 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1247,6 +1247,7 @@ static int exynos4_busfreq_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
 static int exynos4_busfreq_resume(struct device *dev)
 {
 	struct busfreq_data *data = dev_get_drvdata(dev);
@@ -1254,9 +1255,10 @@ static int exynos4_busfreq_resume(struct device *dev)
 	busfreq_mon_reset(data);
 	return 0;
 }
+#endif
 
 static const struct dev_pm_ops exynos4_busfreq_pm = {
-	.resume	= exynos4_busfreq_resume,
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, exynos4_busfreq_resume)
 };
 
 static const struct platform_device_id exynos4_busfreq_id[] = {
-- 
1.8.0

[PATCHv2 6/8] devfreq: exynos4: Fix power-leakage of clock on suspend state

[Chanwoo Choi]

This patch disable ppmu clocks before entering suspend state to remove
power-leakage and enable ppmu clocks on resume function.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/exynos/exynos4_bus.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 9576ee2..3d35063 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1251,14 +1251,37 @@ static int exynos4_busfreq_remove(struct platform_device *pdev)
 static int exynos4_busfreq_resume(struct device *dev)
 {
 	struct busfreq_data *data = dev_get_drvdata(dev);
+	int i;
+
+	/* Enable clock after wake-up from suspend state */
+	for (i = 0; i < PPMU_END; i++)
+		clk_prepare_enable(data->clk_ppmu[i]);
+
+	/* Reset PPMU to check utilization again */
 
 	busfreq_mon_reset(data);
+
 	return 0;
 }
+
+static int exynos4_busfreq_suspend(struct device *dev)
+{
+	struct busfreq_data *data = dev_get_drvdata(dev);
+	int i;
+
+	/*
+	 * Disable clock before entering suspend state
+	 * to reduce leakage power on suspend state.
+	 */
+	for (i = 0; i < PPMU_END; i++)
+		clk_disable_unprepare(data->clk_ppmu[i]);
+
+	return 0;
+};
 #endif
 
 static const struct dev_pm_ops exynos4_busfreq_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(NULL, exynos4_busfreq_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(exynos4_busfreq_suspend, exynos4_busfreq_resume)
 };
 
 static const struct platform_device_id exynos4_busfreq_id[] = {
-- 
1.8.0

Re: [PATCHv2 6/8] devfreq: exynos4: Fix power-leakage of clock on suspend state

[Tomasz Figa]

Hi Chanwoo,

On 13.03.2014 09:17, Chanwoo Choi wrote:
> This patch disable ppmu clocks before entering suspend state to remove
> power-leakage and enable ppmu clocks on resume function.

I don't think there is any need for this, because all the clocks are 
stopped anyway in SLEEP mode.

Best regards,
Tomasz

Re: [PATCHv2 6/8] devfreq: exynos4: Fix power-leakage of clock on suspend state

[Chanwoo Choi]

Hi Tomasz,

On 03/15/2014 02:52 AM, Tomasz Figa wrote:
> Hi Chanwoo,
> 
> On 13.03.2014 09:17, Chanwoo Choi wrote:
>> This patch disable ppmu clocks before entering suspend state to remove
>> power-leakage and enable ppmu clocks on resume function.
> 
> I don't think there is any need for this, because all the clocks are stopped anyway in SLEEP mode.

OK, I'll discard this patch on next patchset.

Best Regards,
Chanwoo Choi

[PATCHv2 7/8] devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail

[Chanwoo Choi]

This patch add CONFIG_PM_OPP dependecy to exynos4_bus driver
to fix probe fail as following log:

[    3.721389] exynos4-busfreq busfreq.3: Fail to add opp entries.
[    3.721697] exynos4-busfreq: probe of busfreq.3 failed with error -22

If CONFIG_PM_OPP is disabled, dev_pm_opp_find_freq_floor() in xxx_probe()
will always return -EINVAL error.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 7d2f435..b2de2a1 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -70,6 +70,7 @@ config ARM_EXYNOS4_BUS_DEVFREQ
 	depends on (CPU_EXYNOS4210 || SOC_EXYNOS4212 || SOC_EXYNOS4412) && !ARCH_MULTIPLATFORM
 	select ARCH_HAS_OPP
 	select DEVFREQ_GOV_SIMPLE_ONDEMAND
+	select PM_OPP
 	help
 	  This adds the DEVFREQ driver for Exynos4210 memory bus (vdd_int)
 	  and Exynos4212/4412 memory interface and bus (vdd_mif + vdd_int).
-- 
1.8.0

[PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Chanwoo Choi]

This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
and then busfreq driver adjusts dynamically the operating frequency/voltage
by using DEVFREQ Subsystem.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt

diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
new file mode 100644
index 0000000..2a83fcc
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
@@ -0,0 +1,49 @@
+
+Exynos4210/4x12 busfreq driver
+-----------------------------
+
+Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
+scaling according to PPMU counters of memory controllers
+
+Required properties:
+- compatible	: should contain Exynos4 SoC type as follwoing:
+		  - "samsung,exynos4x12-busfreq" for Exynos4x12
+		  - "samsung,exynos4210-busfreq" for Exynos4210
+- reg		: offset and length of the ppmudmc0/1
+		  - PPMU (Performance Profiling Monitoring Units)
+		  : It is to profile performance event of DMC(Dynamic Memory
+		  Controller) So, exynos4_bus.c can check memory bus utilization
+		  by using PPMU of Exynos4 SoC.
+- clocks	: clock number of ppmudmc0/1
+- clock-names	: clock name of ppmudmc0/1
+- vdd_int-supply: regulator for interface block of Exynos4
+
+Optional properties:
+- vdd_mif-supply: regulator for DMC block of Exynos4x12 if Exynos4x12 Soc
+- regs-name	: register name of ppmudmc0/1
+
+All the required listed above must be defined under code busfreq with devfreq
+
+Exmaple:
+For Exynos4210 busfreq,
+	busfreq@106A0000 {
+		compatible = "samsung,exynos4210-busfreq";
+		reg = <0x106A0000 0x2000>, <0x106B0000 0x2000>;
+		regs-name = "PPMU_DMC0", "PPMU_DMC1";
+		clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
+		clock-names = "ppmudmc0", "ppmudmc1";
+
+		vdd_int-supply = <&buck3_reg>;
+	};
+
+For Exynos4x12 busfreq,
+	busfreq@106A0000 {
+		compatible = "samsung,exynos4x12-busfreq";
+		reg = <0x106A0000 0x2000>, <0x106B0000 0x2000>;
+		regs-name = "PPMU_DMC0", "PPMU_DMC1";
+		clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
+		clock-names = "ppmudmc0", "ppmudmc1";
+
+		vdd_mif-suppy = <&buck1_reg>;
+		vdd_int-supply = <&buck3_reg>;
+	};
-- 
1.8.0

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Bartlomiej Zolnierkiewicz]

On Thursday, March 13, 2014 05:17:29 PM Chanwoo Choi wrote:
> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface

This patch adds DT binding documentation not the driver itself.

Same comment for the patch summary line.

> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
> and then busfreq driver adjusts dynamically the operating frequency/voltage
> by using DEVFREQ Subsystem.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> new file mode 100644
> index 0000000..2a83fcc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> @@ -0,0 +1,49 @@
> +
> +Exynos4210/4x12 busfreq driver
> +-----------------------------
> +
> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage

SoC

> +scaling according to PPMU counters of memory controllers
> +
> +Required properties:
> +- compatible	: should contain Exynos4 SoC type as follwoing:

following

> +		  - "samsung,exynos4x12-busfreq" for Exynos4x12
> +		  - "samsung,exynos4210-busfreq" for Exynos4210
> +- reg		: offset and length of the ppmudmc0/1
> +		  - PPMU (Performance Profiling Monitoring Units)
> +		  : It is to profile performance event of DMC(Dynamic Memory
> +		  Controller) So, exynos4_bus.c can check memory bus utilization
> +		  by using PPMU of Exynos4 SoC.

It can be improved by changing ordering, i.e.

		  : PPMU of Exynos4 SoC is used to profile performance event of DMC (Dynamic
		  Memory Controller) so the driver can check memory bus utilization.

> +- clocks	: clock number of ppmudmc0/1
> +- clock-names	: clock name of ppmudmc0/1
> +- vdd_int-supply: regulator for interface block of Exynos4
> +
> +Optional properties:
> +- vdd_mif-supply: regulator for DMC block of Exynos4x12 if Exynos4x12 Soc

if using Exynos4x12 SoC

> +- regs-name	: register name of ppmudmc0/1
> +
> +All the required listed above must be defined under code busfreq with devfreq

required properties

> +Exmaple:

Example:

> +For Exynos4210 busfreq,

please add a newline here

> +	busfreq@106A0000 {
> +		compatible = "samsung,exynos4210-busfreq";
> +		reg = <0x106A0000 0x2000>, <0x106B0000 0x2000>;
> +		regs-name = "PPMU_DMC0", "PPMU_DMC1";
> +		clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
> +		clock-names = "ppmudmc0", "ppmudmc1";
> +
> +		vdd_int-supply = <&buck3_reg>;
> +	};
> +
> +For Exynos4x12 busfreq,

ditto

> +	busfreq@106A0000 {
> +		compatible = "samsung,exynos4x12-busfreq";
> +		reg = <0x106A0000 0x2000>, <0x106B0000 0x2000>;
> +		regs-name = "PPMU_DMC0", "PPMU_DMC1";
> +		clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>;
> +		clock-names = "ppmudmc0", "ppmudmc1";
> +
> +		vdd_mif-suppy = <&buck1_reg>;
> +		vdd_int-supply = <&buck3_reg>;
> +	};

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Mark Rutland]

On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
> and then busfreq driver adjusts dynamically the operating frequency/voltage
> by using DEVFREQ Subsystem.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> new file mode 100644
> index 0000000..2a83fcc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> @@ -0,0 +1,49 @@
> +
> +Exynos4210/4x12 busfreq driver
> +-----------------------------
> +
> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
> +scaling according to PPMU counters of memory controllers
> +
> +Required properties:
> +- compatible	: should contain Exynos4 SoC type as follwoing:
> +		  - "samsung,exynos4x12-busfreq" for Exynos4x12
> +		  - "samsung,exynos4210-busfreq" for Exynos4210

Is there a device called "busfreq"? What device does this binding
describe?

> +- reg		: offset and length of the ppmudmc0/1
> +		  - PPMU (Performance Profiling Monitoring Units)

You seem to require a particular order here. It would be good to be
explicit about it.

> +		  : It is to profile performance event of DMC(Dynamic Memory
> +		  Controller) So, exynos4_bus.c can check memory bus utilization
> +		  by using PPMU of Exynos4 SoC.

This is superfluous, and Linux-specific. The binding document shouldn't
need to refer to drivers.

> +- clocks	: clock number of ppmudmc0/1
> +- clock-names	: clock name of ppmudmc0/1

Are these two clocks, or one clock with a slash in the name?

Please list each name separately.

> +- vdd_int-supply: regulator for interface block of Exynos4
> +
> +Optional properties:
> +- vdd_mif-supply: regulator for DMC block of Exynos4x12 if Exynos4x12 Soc
> +- regs-name	: register name of ppmudmc0/1

This is completely nonstandard. Did you mean reg-names?

Please be explicit about the names you expect. Write them in full,
quoted, and describe the relationship to the reg property.

> +
> +All the required listed above must be defined under code busfreq with devfreq

I'm having some difficulty figuring out what exactly this is intended to
mean.

Thanks,
Mark.

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Chanwoo Choi]

Hi Mark,

On 03/14/2014 02:53 AM, Mark Rutland wrote:
> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>> by using DEVFREQ Subsystem.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>> new file mode 100644
>> index 0000000..2a83fcc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>> @@ -0,0 +1,49 @@
>> +
>> +Exynos4210/4x12 busfreq driver
>> +-----------------------------
>> +
>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>> +scaling according to PPMU counters of memory controllers
>> +
>> +Required properties:
>> +- compatible	: should contain Exynos4 SoC type as follwoing:
>> +		  - "samsung,exynos4x12-busfreq" for Exynos4x12
>> +		  - "samsung,exynos4210-busfreq" for Exynos4210
> 
> Is there a device called "busfreq"? What device does this binding
> describe?

I'll add detailed description of busfreq as following:

"busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
memory bus frequency/voltage by checking memory bus utilization to optimize
power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
would use PPMU(Performance Profiling Monitoring Units).


> 
>> +- reg		: offset and length of the ppmudmc0/1
>> +		  - PPMU (Performance Profiling Monitoring Units)
> 
> You seem to require a particular order here. It would be good to be
> explicit about it.

OK, I'll modify it as following:
the offset and length of the PPMU_DMC0 / PPMU_DMC1
PPMU_DMC0/PPMU_DMC1 show memory buy utilization to exynos4_bus driver.

> 
>> +		  : It is to profile performance event of DMC(Dynamic Memory
>> +		  Controller) So, exynos4_bus.c can check memory bus utilization
>> +		  by using PPMU of Exynos4 SoC.
> 
> This is superfluous, and Linux-specific. The binding document shouldn't
> need to refer to drivers.

I'll remove this description.

> 
>> +- clocks	: clock number of ppmudmc0/1
>> +- clock-names	: clock name of ppmudmc0/1
> 
> Are these two clocks, or one clock with a slash in the name?
> 
> Please list each name separately.

I'll expalin clocks as following:
	"ppmudmc0", "ppmudmc1"

> 
>> +- vdd_int-supply: regulator for interface block of Exynos4
>> +
>> +Optional properties:
>> +- vdd_mif-supply: regulator for DMC block of Exynos4x12 if Exynos4x12 Soc
>> +- regs-name	: register name of ppmudmc0/1
> 
> This is completely nonstandard. Did you mean reg-names?

I'll remove it about 'regs-name'.

> 
> Please be explicit about the names you expect. Write them in full,
> quoted, and describe the relationship to the reg property.
> 
>> +
>> +All the required listed above must be defined under code busfreq with devfreq
> 
> I'm having some difficulty figuring out what exactly this is intended to
> mean.

I'll add detailed opeation method and role of exynos4_busfreq driver.

Thanks,
Chanwoo Choi

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Mark Rutland]

On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
> Hi Mark,
> 
> On 03/14/2014 02:53 AM, Mark Rutland wrote:
> > On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
> >> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
> >> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
> >> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
> >> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
> >> and then busfreq driver adjusts dynamically the operating frequency/voltage
> >> by using DEVFREQ Subsystem.
> >>
> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> >> ---
> >>  .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
> >>  1 file changed, 49 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> >> new file mode 100644
> >> index 0000000..2a83fcc
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> >> @@ -0,0 +1,49 @@
> >> +
> >> +Exynos4210/4x12 busfreq driver
> >> +-----------------------------
> >> +
> >> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
> >> +scaling according to PPMU counters of memory controllers
> >> +
> >> +Required properties:
> >> +- compatible	: should contain Exynos4 SoC type as follwoing:
> >> +		  - "samsung,exynos4x12-busfreq" for Exynos4x12
> >> +		  - "samsung,exynos4210-busfreq" for Exynos4210
> > 
> > Is there a device called "busfreq"? What device does this binding
> > describe?
> 
> I'll add detailed description of busfreq as following:
> 
> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
> memory bus frequency/voltage by checking memory bus utilization to optimize
> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
> would use PPMU(Performance Profiling Monitoring Units).

This still sounds like a description of the _driver_, not the _device_.
The binding should describe the hardware, now the high level abstraction
that software is going to build atop of it.

It sounds like this is a binding for the DMC PPMU?

Is the PPMU a component of the DMC, or is it bolted on the side?

> 
> 
> > 
> >> +- reg		: offset and length of the ppmudmc0/1
> >> +		  - PPMU (Performance Profiling Monitoring Units)
> > 
> > You seem to require a particular order here. It would be good to be
> > explicit about it.
> 
> OK, I'll modify it as following:
> the offset and length of the PPMU_DMC0 / PPMU_DMC1
> PPMU_DMC0/PPMU_DMC1 show memory buy utilization to exynos4_bus driver.
> 
> > 
> >> +		  : It is to profile performance event of DMC(Dynamic Memory
> >> +		  Controller) So, exynos4_bus.c can check memory bus utilization
> >> +		  by using PPMU of Exynos4 SoC.
> > 
> > This is superfluous, and Linux-specific. The binding document shouldn't
> > need to refer to drivers.
> 
> I'll remove this description.
> 
> > 
> >> +- clocks	: clock number of ppmudmc0/1
> >> +- clock-names	: clock name of ppmudmc0/1
> > 
> > Are these two clocks, or one clock with a slash in the name?
> > 
> > Please list each name separately.
> 
> I'll expalin clocks as following:
> 	"ppmudmc0", "ppmudmc1"
> 
> > 
> >> +- vdd_int-supply: regulator for interface block of Exynos4

How does the interface block relate to the DMC / PPMU?

Cheers,
Mark.

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Chanwoo Choi]

Hi Mark,

On 03/14/2014 07:35 PM, Mark Rutland wrote:
> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>> Hi Mark,
>>
>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>>>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>>>> by using DEVFREQ Subsystem.
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>>>  1 file changed, 49 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>> new file mode 100644
>>>> index 0000000..2a83fcc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>> @@ -0,0 +1,49 @@
>>>> +
>>>> +Exynos4210/4x12 busfreq driver
>>>> +-----------------------------
>>>> +
>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>>>> +scaling according to PPMU counters of memory controllers
>>>> +
>>>> +Required properties:
>>>> +- compatible	: should contain Exynos4 SoC type as follwoing:
>>>> +		  - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>> +		  - "samsung,exynos4210-busfreq" for Exynos4210
>>>
>>> Is there a device called "busfreq"? What device does this binding
>>> describe?
>>
>> I'll add detailed description of busfreq as following:
>>
>> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
>> memory bus frequency/voltage by checking memory bus utilization to optimize
>> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
>> would use PPMU(Performance Profiling Monitoring Units).
> 
> This still sounds like a description of the _driver_, not the _device_.
> The binding should describe the hardware, now the high level abstraction
> that software is going to build atop of it.
> 
> It sounds like this is a binding for the DMC PPMU?
> 
> Is the PPMU a component of the DMC, or is it bolted on the side?

PPMU(Performance Profiling Monitoring Unit) is to profile performance event of
various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
We can check various PPMU as following:

PPMU_3D
PPMU_ACP
PPMU_CAMIF
PPMU_CPU
PPMU_DMC0
PPMU_DMC1
PPMU_FSYS
PPMU_IMAGE
PPMU_LCD0
PPMU_LCD1
PPMU_MFC_L
PPMU_MFC_R
PPMU_TV
PPMU_LEFT_BUS
PPMU_RIGHT_BUS

DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC.
If we need to get memory bust utilization of DMC, we can get memory bus utilization
from PPMU_DMC0/PPMU_DMC1.

So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list.

> 
>>
>>
>>>
>>>> +- reg		: offset and length of the ppmudmc0/1
>>>> +		  - PPMU (Performance Profiling Monitoring Units)
>>>
>>> You seem to require a particular order here. It would be good to be
>>> explicit about it.
>>
>> OK, I'll modify it as following:
>> the offset and length of the PPMU_DMC0 / PPMU_DMC1
>> PPMU_DMC0/PPMU_DMC1 show memory buy utilization to exynos4_bus driver.
>>
>>>
>>>> +		  : It is to profile performance event of DMC(Dynamic Memory
>>>> +		  Controller) So, exynos4_bus.c can check memory bus utilization
>>>> +		  by using PPMU of Exynos4 SoC.
>>>
>>> This is superfluous, and Linux-specific. The binding document shouldn't
>>> need to refer to drivers.
>>
>> I'll remove this description.
>>
>>>
>>>> +- clocks	: clock number of ppmudmc0/1
>>>> +- clock-names	: clock name of ppmudmc0/1
>>>
>>> Are these two clocks, or one clock with a slash in the name?
>>>
>>> Please list each name separately.
>>
>> I'll expalin clocks as following:
>> 	"ppmudmc0", "ppmudmc1"
>>
>>>
>>>> +- vdd_int-supply: regulator for interface block of Exynos4
> 
> How does the interface block relate to the DMC / PPMU?

If vdd_int is connected to buck3 of PMIC(Power Management IC),

We can see the flow of provided power as following:
: external power --> buck3 pin of PMIC --> vdd_int pin of Exynos4 SoC --> DMC IP and PPMU IP of Exynos4 SoC

Thanks,
Chanwoo Choi

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Tomasz Figa]

Hi Chanwoo, Mark,

On 14.03.2014 11:56, Chanwoo Choi wrote:
> Hi Mark,
>
> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>> Hi Mark,
>>>
>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>>>>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>>>>> by using DEVFREQ Subsystem.
>>>>>
>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> ---
>>>>>   .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>>>>   1 file changed, 49 insertions(+)
>>>>>   create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>> new file mode 100644
>>>>> index 0000000..2a83fcc
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>> @@ -0,0 +1,49 @@
>>>>> +
>>>>> +Exynos4210/4x12 busfreq driver
>>>>> +-----------------------------
>>>>> +
>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>>>>> +scaling according to PPMU counters of memory controllers
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible	: should contain Exynos4 SoC type as follwoing:
>>>>> +		  - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>> +		  - "samsung,exynos4210-busfreq" for Exynos4210
>>>>
>>>> Is there a device called "busfreq"? What device does this binding
>>>> describe?
>>>
>>> I'll add detailed description of busfreq as following:
>>>
>>> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
>>> memory bus frequency/voltage by checking memory bus utilization to optimize
>>> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
>>> would use PPMU(Performance Profiling Monitoring Units).
>>
>> This still sounds like a description of the _driver_, not the _device_.
>> The binding should describe the hardware, now the high level abstraction
>> that software is going to build atop of it.
>>
>> It sounds like this is a binding for the DMC PPMU?
>>
>> Is the PPMU a component of the DMC, or is it bolted on the side?
>
> PPMU(Performance Profiling Monitoring Unit) is to profile performance event of
> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
> We can check various PPMU as following:
>
> PPMU_3D
> PPMU_ACP
> PPMU_CAMIF
> PPMU_CPU
> PPMU_DMC0
> PPMU_DMC1
> PPMU_FSYS
> PPMU_IMAGE
> PPMU_LCD0
> PPMU_LCD1
> PPMU_MFC_L
> PPMU_MFC_R
> PPMU_TV
> PPMU_LEFT_BUS
> PPMU_RIGHT_BUS
>
> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC.
> If we need to get memory bust utilization of DMC, we can get memory bus utilization
> from PPMU_DMC0/PPMU_DMC1.
>
> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list.

Well, PPMUs and DMCs are separate hardware blocks found inside Exynos 
SoCs. Busfreq/devfreq is just a Linux-specific abstraction responsible 
for collecting data using PPMUs and controlling frequencies and voltages 
of appropriate power planes, vdd_int responsible for powering DMC0 and 
DMC1 blocks in this case.

I'm afraid that the binding you're proposing is unfortunately incorrect, 
because it represents the software abstraction, not the real hardware.

Instead, this should be separated into several independent bindings:

  - PPMU bindings to list all the PPMU instances present in the SoC and 
resources they need,

  - power plane bindings, which define a power plane in which multiple 
IP blocks might reside, can be monitored by one or more PPMU units and 
frequency and voltage of which can be configured according to determined 
performance level. Needed resources will be clocks and regulators to 
scale and probably also operating points.

Then, exynos-busfreq driver should bind to such power planes, parse 
necessary data from DT (list of PPMUs and IP blocks, clocks, regulators 
and operating points) and register a devfreq entity.

Best regards,
Tomasz

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Kyungmin Park]

On Sat, Mar 15, 2014 at 2:35 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chanwoo, Mark,
>
>
> On 14.03.2014 11:56, Chanwoo Choi wrote:
>>
>> Hi Mark,
>>
>> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>>>
>>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>>>
>>>> Hi Mark,
>>>>
>>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>>>
>>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>>>
>>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory
>>>>>> interface
>>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according
>>>>>> to PPMU
>>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4
>>>>>> SoC provides
>>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus
>>>>>> utilization
>>>>>> and then busfreq driver adjusts dynamically the operating
>>>>>> frequency/voltage
>>>>>> by using DEVFREQ Subsystem.
>>>>>>
>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>> ---
>>>>>>   .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49
>>>>>> ++++++++++++++++++++++
>>>>>>   1 file changed, 49 insertions(+)
>>>>>>   create mode 100644
>>>>>> Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>> b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..2a83fcc
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>> @@ -0,0 +1,49 @@
>>>>>> +
>>>>>> +Exynos4210/4x12 busfreq driver
>>>>>> +-----------------------------
>>>>>> +
>>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus
>>>>>> frequency/voltage
>>>>>> +scaling according to PPMU counters of memory controllers
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible   : should contain Exynos4 SoC type as follwoing:
>>>>>> +                 - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>>> +                 - "samsung,exynos4210-busfreq" for Exynos4210
>>>>>
>>>>>
>>>>> Is there a device called "busfreq"? What device does this binding
>>>>> describe?
>>>>
>>>>
>>>> I'll add detailed description of busfreq as following:
>>>>
>>>> "busfreq(bus frequendcy)" driver means that busfreq driver control
>>>> dynamically
>>>> memory bus frequency/voltage by checking memory bus utilization to
>>>> optimize
>>>> power-consumption. When checking memeory bus utilization,
>>>> exynos4_busfreq driver
>>>> would use PPMU(Performance Profiling Monitoring Units).
>>>
>>>
>>> This still sounds like a description of the _driver_, not the _device_.
>>> The binding should describe the hardware, now the high level abstraction
>>> that software is going to build atop of it.
>>>
>>> It sounds like this is a binding for the DMC PPMU?
>>>
>>> Is the PPMU a component of the DMC, or is it bolted on the side?
>>
>>
>> PPMU(Performance Profiling Monitoring Unit) is to profile performance
>> event of
>> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
>> We can check various PPMU as following:
>>
>> PPMU_3D
>> PPMU_ACP
>> PPMU_CAMIF
>> PPMU_CPU
>> PPMU_DMC0
>> PPMU_DMC1
>> PPMU_FSYS
>> PPMU_IMAGE
>> PPMU_LCD0
>> PPMU_LCD1
>> PPMU_MFC_L
>> PPMU_MFC_R
>> PPMU_TV
>> PPMU_LEFT_BUS
>> PPMU_RIGHT_BUS
>>
>> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4
>> SoC.
>> If we need to get memory bust utilization of DMC, we can get memory bus
>> utilization
>> from PPMU_DMC0/PPMU_DMC1.
>>
>> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various
>> PPMU list.
>
>
> Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs.
> Busfreq/devfreq is just a Linux-specific abstraction responsible for
> collecting data using PPMUs and controlling frequencies and voltages of
> appropriate power planes, vdd_int responsible for powering DMC0 and DMC1
> blocks in this case.
>
> I'm afraid that the binding you're proposing is unfortunately incorrect,
> because it represents the software abstraction, not the real hardware.
>
> Instead, this should be separated into several independent bindings:
>
>  - PPMU bindings to list all the PPMU instances present in the SoC and
> resources they need,
>
>  - power plane bindings, which define a power plane in which multiple IP
> blocks might reside, can be monitored by one or more PPMU units and
> frequency and voltage of which can be configured according to determined
> performance level. Needed resources will be clocks and regulators to scale
> and probably also operating points.
>
> Then, exynos-busfreq driver should bind to such power planes, parse
> necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and
> operating points) and register a devfreq entity.

How about to use component DT like DRM?
>
> Best regards,
> Tomasz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Tomasz Figa]

On 15.03.2014 12:36, Kyungmin Park wrote:
> On Sat, Mar 15, 2014 at 2:35 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Chanwoo, Mark,
>>
>>
>> On 14.03.2014 11:56, Chanwoo Choi wrote:
>>>
>>> Hi Mark,
>>>
>>> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>>>>
>>>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>>>>
>>>>> Hi Mark,
>>>>>
>>>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>>>>
>>>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>>>>
>>>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory
>>>>>>> interface
>>>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according
>>>>>>> to PPMU
>>>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4
>>>>>>> SoC provides
>>>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus
>>>>>>> utilization
>>>>>>> and then busfreq driver adjusts dynamically the operating
>>>>>>> frequency/voltage
>>>>>>> by using DEVFREQ Subsystem.
>>>>>>>
>>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49
>>>>>>> ++++++++++++++++++++++
>>>>>>>    1 file changed, 49 insertions(+)
>>>>>>>    create mode 100644
>>>>>>> Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>> b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..2a83fcc
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>> @@ -0,0 +1,49 @@
>>>>>>> +
>>>>>>> +Exynos4210/4x12 busfreq driver
>>>>>>> +-----------------------------
>>>>>>> +
>>>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus
>>>>>>> frequency/voltage
>>>>>>> +scaling according to PPMU counters of memory controllers
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible   : should contain Exynos4 SoC type as follwoing:
>>>>>>> +                 - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>>>> +                 - "samsung,exynos4210-busfreq" for Exynos4210
>>>>>>
>>>>>>
>>>>>> Is there a device called "busfreq"? What device does this binding
>>>>>> describe?
>>>>>
>>>>>
>>>>> I'll add detailed description of busfreq as following:
>>>>>
>>>>> "busfreq(bus frequendcy)" driver means that busfreq driver control
>>>>> dynamically
>>>>> memory bus frequency/voltage by checking memory bus utilization to
>>>>> optimize
>>>>> power-consumption. When checking memeory bus utilization,
>>>>> exynos4_busfreq driver
>>>>> would use PPMU(Performance Profiling Monitoring Units).
>>>>
>>>>
>>>> This still sounds like a description of the _driver_, not the _device_.
>>>> The binding should describe the hardware, now the high level abstraction
>>>> that software is going to build atop of it.
>>>>
>>>> It sounds like this is a binding for the DMC PPMU?
>>>>
>>>> Is the PPMU a component of the DMC, or is it bolted on the side?
>>>
>>>
>>> PPMU(Performance Profiling Monitoring Unit) is to profile performance
>>> event of
>>> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
>>> We can check various PPMU as following:
>>>
>>> PPMU_3D
>>> PPMU_ACP
>>> PPMU_CAMIF
>>> PPMU_CPU
>>> PPMU_DMC0
>>> PPMU_DMC1
>>> PPMU_FSYS
>>> PPMU_IMAGE
>>> PPMU_LCD0
>>> PPMU_LCD1
>>> PPMU_MFC_L
>>> PPMU_MFC_R
>>> PPMU_TV
>>> PPMU_LEFT_BUS
>>> PPMU_RIGHT_BUS
>>>
>>> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4
>>> SoC.
>>> If we need to get memory bust utilization of DMC, we can get memory bus
>>> utilization
>>> from PPMU_DMC0/PPMU_DMC1.
>>>
>>> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various
>>> PPMU list.
>>
>>
>> Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs.
>> Busfreq/devfreq is just a Linux-specific abstraction responsible for
>> collecting data using PPMUs and controlling frequencies and voltages of
>> appropriate power planes, vdd_int responsible for powering DMC0 and DMC1
>> blocks in this case.
>>
>> I'm afraid that the binding you're proposing is unfortunately incorrect,
>> because it represents the software abstraction, not the real hardware.
>>
>> Instead, this should be separated into several independent bindings:
>>
>>   - PPMU bindings to list all the PPMU instances present in the SoC and
>> resources they need,
>>
>>   - power plane bindings, which define a power plane in which multiple IP
>> blocks might reside, can be monitored by one or more PPMU units and
>> frequency and voltage of which can be configured according to determined
>> performance level. Needed resources will be clocks and regulators to scale
>> and probably also operating points.
>>
>> Then, exynos-busfreq driver should bind to such power planes, parse
>> necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and
>> operating points) and register a devfreq entity.
>
> How about to use component DT like DRM?

Well, basically this is what I proposed. Each "power plane" would be a 
"subsystem" built from components - PPMUs, IP blocks, clocks, 
regulators, etc, specified in DT by existing means, e.g.

ppmu_disp: ppmu@12340000 {
	/* Resources of PPMU */
}

fimd: fimd@23450000 {
	/* Resources of FIMD */
};

power-plane-display {
	compatible = "samsung,power-plane";
	samsung,ppmus = <&ppmu_disp>, ...;
	samsung,devices = <&fimd>, ...;
	clock-names = "aclk_xxx", "sclk_xxx", ...;
	clocks = ...;
	vdd_xxx-supply = ...;
};

However I'm still wondering whether there shouldn't be a relation 
between power planes and power domains and simply existing power domain 
nodes shouldn't be extended with the data I put above in 
power-plane-display node. I need to check this in the documentation back 
at work on Monday.

Best regards,
Tomasz

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Chanwoo Choi]

Hi Tomasz,

On 03/15/2014 02:35 AM, Tomasz Figa wrote:
> Hi Chanwoo, Mark,
> 
> On 14.03.2014 11:56, Chanwoo Choi wrote:
>> Hi Mark,
>>
>> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>>> Hi Mark,
>>>>
>>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>>>>>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>>>>>> by using DEVFREQ Subsystem.
>>>>>>
>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>> ---
>>>>>>   .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>>>>>   1 file changed, 49 insertions(+)
>>>>>>   create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..2a83fcc
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>> @@ -0,0 +1,49 @@
>>>>>> +
>>>>>> +Exynos4210/4x12 busfreq driver
>>>>>> +-----------------------------
>>>>>> +
>>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>>>>>> +scaling according to PPMU counters of memory controllers
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible    : should contain Exynos4 SoC type as follwoing:
>>>>>> +          - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>>> +          - "samsung,exynos4210-busfreq" for Exynos4210
>>>>>
>>>>> Is there a device called "busfreq"? What device does this binding
>>>>> describe?
>>>>
>>>> I'll add detailed description of busfreq as following:
>>>>
>>>> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
>>>> memory bus frequency/voltage by checking memory bus utilization to optimize
>>>> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
>>>> would use PPMU(Performance Profiling Monitoring Units).
>>>
>>> This still sounds like a description of the _driver_, not the _device_.
>>> The binding should describe the hardware, now the high level abstraction
>>> that software is going to build atop of it.
>>>
>>> It sounds like this is a binding for the DMC PPMU?
>>>
>>> Is the PPMU a component of the DMC, or is it bolted on the side?
>>
>> PPMU(Performance Profiling Monitoring Unit) is to profile performance event of
>> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
>> We can check various PPMU as following:
>>
>> PPMU_3D
>> PPMU_ACP
>> PPMU_CAMIF
>> PPMU_CPU
>> PPMU_DMC0
>> PPMU_DMC1
>> PPMU_FSYS
>> PPMU_IMAGE
>> PPMU_LCD0
>> PPMU_LCD1
>> PPMU_MFC_L
>> PPMU_MFC_R
>> PPMU_TV
>> PPMU_LEFT_BUS
>> PPMU_RIGHT_BUS
>>
>> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC.
>> If we need to get memory bust utilization of DMC, we can get memory bus utilization
>> from PPMU_DMC0/PPMU_DMC1.
>>
>> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list.
> 
> Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs. Busfreq/devfreq is just a Linux-specific abstraction responsible for collecting data using PPMUs and controlling frequencies and voltages of appropriate power planes, vdd_int responsible for powering DMC0 and DMC1 blocks in this case.
> 

I knew already.

> I'm afraid that the binding you're proposing is unfortunately incorrect, because it represents the software abstraction, not the real hardware.

What is exactly incorrect part of this patch?

> 
> Instead, this should be separated into several independent bindings:
> 
>  - PPMU bindings to list all the PPMU instances present in the SoC and resources they need,
> 
>  - power plane bindings, which define a power plane in which multiple IP blocks might reside, can be monitored by one or more PPMU units and frequency and voltage of which can be configured according to determined performance level. Needed resources will be clocks and regulators to scale and probably also operating points.
> 
> Then, exynos-busfreq driver should bind to such power planes, parse necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and operating points) and register a devfreq entity.

What is 'power plane'? I don't know 'power plane'.
If you suggest 'power plane' concept and then merge this concept to mainline,
After merged 'power plane' concept, I will apply 'power plane' concept to Exynos4's busfreq driver.

I prefer to handle 'power plane' concept on other patchset for Exynos4's busfreq driver.

Best Regards,
Chanwoo Choi

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Tomasz Figa]

On 17.03.2014 06:19, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 03/15/2014 02:35 AM, Tomasz Figa wrote:
>> Hi Chanwoo, Mark,
>>
>> On 14.03.2014 11:56, Chanwoo Choi wrote:
>>> Hi Mark,
>>>
>>> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>>>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>>>> Hi Mark,
>>>>>
>>>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>>>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>>>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>>>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>>>>>>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>>>>>>> by using DEVFREQ Subsystem.
>>>>>>>
>>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>> ---
>>>>>>>    .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>>>>>>    1 file changed, 49 insertions(+)
>>>>>>>    create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..2a83fcc
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>> @@ -0,0 +1,49 @@
>>>>>>> +
>>>>>>> +Exynos4210/4x12 busfreq driver
>>>>>>> +-----------------------------
>>>>>>> +
>>>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>>>>>>> +scaling according to PPMU counters of memory controllers
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible    : should contain Exynos4 SoC type as follwoing:
>>>>>>> +          - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>>>> +          - "samsung,exynos4210-busfreq" for Exynos4210
>>>>>>
>>>>>> Is there a device called "busfreq"? What device does this binding
>>>>>> describe?
>>>>>
>>>>> I'll add detailed description of busfreq as following:
>>>>>
>>>>> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
>>>>> memory bus frequency/voltage by checking memory bus utilization to optimize
>>>>> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
>>>>> would use PPMU(Performance Profiling Monitoring Units).
>>>>
>>>> This still sounds like a description of the _driver_, not the _device_.
>>>> The binding should describe the hardware, now the high level abstraction
>>>> that software is going to build atop of it.
>>>>
>>>> It sounds like this is a binding for the DMC PPMU?
>>>>
>>>> Is the PPMU a component of the DMC, or is it bolted on the side?
>>>
>>> PPMU(Performance Profiling Monitoring Unit) is to profile performance event of
>>> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
>>> We can check various PPMU as following:
>>>
>>> PPMU_3D
>>> PPMU_ACP
>>> PPMU_CAMIF
>>> PPMU_CPU
>>> PPMU_DMC0
>>> PPMU_DMC1
>>> PPMU_FSYS
>>> PPMU_IMAGE
>>> PPMU_LCD0
>>> PPMU_LCD1
>>> PPMU_MFC_L
>>> PPMU_MFC_R
>>> PPMU_TV
>>> PPMU_LEFT_BUS
>>> PPMU_RIGHT_BUS
>>>
>>> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC.
>>> If we need to get memory bust utilization of DMC, we can get memory bus utilization
>>> from PPMU_DMC0/PPMU_DMC1.
>>>
>>> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list.
>>
>> Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs. Busfreq/devfreq is just a Linux-specific abstraction responsible for collecting data using PPMUs and controlling frequencies and voltages of appropriate power planes, vdd_int responsible for powering DMC0 and DMC1 blocks in this case.
>>
>
> I knew already.
>
>> I'm afraid that the binding you're proposing is unfortunately incorrect, because it represents the software abstraction, not the real hardware.
>
> What is exactly incorrect part of this patch?
>

Device tree contains information about hardware, not about OS-specific 
drivers or subsystems. Busfreq/devfreq is not a hardware block, but a 
Linux-specific driver, so it's not suitable to be described by DT 
directly, especially considering the fact that in future it might be 
replaced by or merged with another subsystem.

Only PPMUs are real hardware blocks present in the SoC. In addition, a 
SoC-level aspect of hardware description may be added, such as a list of 
power planes. See below for explanation.

>>
>> Instead, this should be separated into several independent bindings:
>>
>>   - PPMU bindings to list all the PPMU instances present in the SoC and resources they need,
>>
>>   - power plane bindings, which define a power plane in which multiple IP blocks might reside, can be monitored by one or more PPMU units and frequency and voltage of which can be configured according to determined performance level. Needed resources will be clocks and regulators to scale and probably also operating points.
>>
>> Then, exynos-busfreq driver should bind to such power planes, parse necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and operating points) and register a devfreq entity.
>
> What is 'power plane'? I don't know 'power plane'.

Power plane is a part of the SoC (set of IP blocks and buses) that is 
powered by the same power source. For example in Exynos4412 a power 
plane would be VDD_ARM, containing 4 Cortex A9 cores, two L2 cache 
blocks, L2 cache controller, SCU, and several other blocks. Another 
example in Exynos4412 would be VDD_INT plane, which contains MFC, 
AudioSS, LCD0, ISP, TV, CAM and several other blocks.

In addition, such power plane might contain certain facilities, such as 
PPMU blocks, that let you estimate bus bandwidth utilization and scale 
certain performance and power parameters of such power plane, e.g. 
frequencies of internal buses and voltage of external regulator used to 
power it.

> If you suggest 'power plane' concept and then merge this concept to mainline,
> After merged 'power plane' concept, I will apply 'power plane' concept to Exynos4's busfreq driver.
>
> I prefer to handle 'power plane' concept on other patchset for Exynos4's busfreq driver.

This is a necessary prerequisite for having Exynos busfreq facilities 
described properly in device tree. This is not a concept of high level 
generic framework, but just a concept of hardware description that can 
be parsed by Linux drivers, such as Exynos busfreq.

As I said above, a requirement is that DT does not describe 
Linux-specific internals, but the hardware that is used by them.

Best regards,
Tomasz

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Chanwoo Choi]

Hi Tomasz,

On 03/19/2014 12:46 AM, Tomasz Figa wrote:
> On 17.03.2014 06:19, Chanwoo Choi wrote:
>> Hi Tomasz,
>>
>> On 03/15/2014 02:35 AM, Tomasz Figa wrote:
>>> Hi Chanwoo, Mark,
>>>
>>> On 14.03.2014 11:56, Chanwoo Choi wrote:
>>>> Hi Mark,
>>>>
>>>> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>>>>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>>>>> Hi Mark,
>>>>>>
>>>>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>>>>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>>>>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>>>>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>>>>>>>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>>>>>>>> by using DEVFREQ Subsystem.
>>>>>>>>
>>>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>>> ---
>>>>>>>>    .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>>>>>>>    1 file changed, 49 insertions(+)
>>>>>>>>    create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..2a83fcc
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>> @@ -0,0 +1,49 @@
>>>>>>>> +
>>>>>>>> +Exynos4210/4x12 busfreq driver
>>>>>>>> +-----------------------------
>>>>>>>> +
>>>>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>>>>>>>> +scaling according to PPMU counters of memory controllers
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- compatible    : should contain Exynos4 SoC type as follwoing:
>>>>>>>> +          - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>>>>> +          - "samsung,exynos4210-busfreq" for Exynos4210
>>>>>>>
>>>>>>> Is there a device called "busfreq"? What device does this binding
>>>>>>> describe?
>>>>>>
>>>>>> I'll add detailed description of busfreq as following:
>>>>>>
>>>>>> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
>>>>>> memory bus frequency/voltage by checking memory bus utilization to optimize
>>>>>> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
>>>>>> would use PPMU(Performance Profiling Monitoring Units).
>>>>>
>>>>> This still sounds like a description of the _driver_, not the _device_.
>>>>> The binding should describe the hardware, now the high level abstraction
>>>>> that software is going to build atop of it.
>>>>>
>>>>> It sounds like this is a binding for the DMC PPMU?
>>>>>
>>>>> Is the PPMU a component of the DMC, or is it bolted on the side?
>>>>
>>>> PPMU(Performance Profiling Monitoring Unit) is to profile performance event of
>>>> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
>>>> We can check various PPMU as following:
>>>>
>>>> PPMU_3D
>>>> PPMU_ACP
>>>> PPMU_CAMIF
>>>> PPMU_CPU
>>>> PPMU_DMC0
>>>> PPMU_DMC1
>>>> PPMU_FSYS
>>>> PPMU_IMAGE
>>>> PPMU_LCD0
>>>> PPMU_LCD1
>>>> PPMU_MFC_L
>>>> PPMU_MFC_R
>>>> PPMU_TV
>>>> PPMU_LEFT_BUS
>>>> PPMU_RIGHT_BUS
>>>>
>>>> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC.
>>>> If we need to get memory bust utilization of DMC, we can get memory bus utilization
>>>> from PPMU_DMC0/PPMU_DMC1.
>>>>
>>>> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list.
>>>
>>> Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs. Busfreq/devfreq is just a Linux-specific abstraction responsible for collecting data using PPMUs and controlling frequencies and voltages of appropriate power planes, vdd_int responsible for powering DMC0 and DMC1 blocks in this case.
>>>
>>
>> I knew already.
>>
>>> I'm afraid that the binding you're proposing is unfortunately incorrect, because it represents the software abstraction, not the real hardware.
>>
>> What is exactly incorrect part of this patch?
>>
> 
> Device tree contains information about hardware, not about OS-specific drivers or subsystems. Busfreq/devfreq is not a hardware block, but a Linux-specific driver, so it's not suitable to be described by DT directly, especially considering the fact that in future it might be replaced by or merged with another subsystem.
> 
> Only PPMUs are real hardware blocks present in the SoC. In addition, a SoC-level aspect of hardware description may be added, such as a list of power planes. See below for explanation.
> 

You means that PPMU must need separate framework from devfreq subsystem?
If PPMU framework will be implemented, PPMU framework provides API to external device driver
as common clk framework, regulator framework.
And then,exynos4 busfreq with devfreq have to use PPMU framework to monitor memory utilization.

Is this right?

>>>
>>> Instead, this should be separated into several independent bindings:
>>>
>>>   - PPMU bindings to list all the PPMU instances present in the SoC and resources they need,
>>>
>>>   - power plane bindings, which define a power plane in which multiple IP blocks might reside, can be monitored by one or more PPMU units and frequency and voltage of which can be configured according to determined performance level. Needed resources will be clocks and regulators to scale and probably also operating points.
>>>
>>> Then, exynos-busfreq driver should bind to such power planes, parse necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and operating points) and register a devfreq entity.
>>
>> What is 'power plane'? I don't know 'power plane'.
> 
> Power plane is a part of the SoC (set of IP blocks and buses) that is powered by the same power source. For example in Exynos4412 a power plane would be VDD_ARM, containing 4 Cortex A9 cores, two L2 cache blocks, L2 cache controller, SCU, and several other blocks. Another example in Exynos4412 would be VDD_INT plane, which contains MFC, AudioSS, LCD0, ISP, TV, CAM and several other blocks.
> 
> In addition, such power plane might contain certain facilities, such as PPMU blocks, that let you estimate bus bandwidth utilization and scale certain performance and power parameters of such power plane, e.g. frequencies of internal buses and voltage of external regulator used to power it.
> 
>> If you suggest 'power plane' concept and then merge this concept to mainline,
>> After merged 'power plane' concept, I will apply 'power plane' concept to Exynos4's busfreq driver.
>>
>> I prefer to handle 'power plane' concept on other patchset for Exynos4's busfreq driver.
> 
> This is a necessary prerequisite for having Exynos busfreq facilities described properly in device tree. This is not a concept of high level generic framework, but just a concept of hardware description that can be parsed by Linux drivers, such as Exynos busfreq.
> 
> As I said above, a requirement is that DT does not describe Linux-specific internals, but the hardware that is used by them.
> 

exynos4 busfreq with devfreq must need to execute .probe().
So, exynos4 busfreq with devfreq should add dt node for exynos4 busfreq.
But,
You means that exynos4 busfreq can't add dt node of exynos4 busfreq
because this driver has not the hardware block. ?

Best Regards,
Chanwoo Choi

Re: [PATCHv2 8/8] devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12

[Tomasz Figa]

Hi Chanwoo,

On 19.03.2014 10:47, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 03/19/2014 12:46 AM, Tomasz Figa wrote:
>> On 17.03.2014 06:19, Chanwoo Choi wrote:
>>> Hi Tomasz,
>>>
>>> On 03/15/2014 02:35 AM, Tomasz Figa wrote:
>>>> Hi Chanwoo, Mark,
>>>>
>>>> On 14.03.2014 11:56, Chanwoo Choi wrote:
>>>>> Hi Mark,
>>>>>
>>>>> On 03/14/2014 07:35 PM, Mark Rutland wrote:
>>>>>> On Fri, Mar 14, 2014 at 07:14:37AM +0000, Chanwoo Choi wrote:
>>>>>>> Hi Mark,
>>>>>>>
>>>>>>> On 03/14/2014 02:53 AM, Mark Rutland wrote:
>>>>>>>> On Thu, Mar 13, 2014 at 08:17:29AM +0000, Chanwoo Choi wrote:
>>>>>>>>> This patch add busfreq driver for Exynos4210/Exynos4x12 memory interface
>>>>>>>>> and bus to support DVFS(Dynamic Voltage Frequency Scaling) according to PPMU
>>>>>>>>> counters. PPMU (Performance Profiling Monitorings Units) of Exynos4 SoC provides
>>>>>>>>> PPMU counters for DMC(Dynamic Memory Controller) to check memory bus utilization
>>>>>>>>> and then busfreq driver adjusts dynamically the operating frequency/voltage
>>>>>>>>> by using DEVFREQ Subsystem.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>>>> ---
>>>>>>>>>     .../devicetree/bindings/devfreq/exynos4_bus.txt    | 49 ++++++++++++++++++++++
>>>>>>>>>     1 file changed, 49 insertions(+)
>>>>>>>>>     create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..2a83fcc
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>>>>>>> @@ -0,0 +1,49 @@
>>>>>>>>> +
>>>>>>>>> +Exynos4210/4x12 busfreq driver
>>>>>>>>> +-----------------------------
>>>>>>>>> +
>>>>>>>>> +Exynos4210/4x12 Soc busfreq driver with devfreq for Memory bus frequency/voltage
>>>>>>>>> +scaling according to PPMU counters of memory controllers
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible    : should contain Exynos4 SoC type as follwoing:
>>>>>>>>> +          - "samsung,exynos4x12-busfreq" for Exynos4x12
>>>>>>>>> +          - "samsung,exynos4210-busfreq" for Exynos4210
>>>>>>>>
>>>>>>>> Is there a device called "busfreq"? What device does this binding
>>>>>>>> describe?
>>>>>>>
>>>>>>> I'll add detailed description of busfreq as following:
>>>>>>>
>>>>>>> "busfreq(bus frequendcy)" driver means that busfreq driver control dynamically
>>>>>>> memory bus frequency/voltage by checking memory bus utilization to optimize
>>>>>>> power-consumption. When checking memeory bus utilization, exynos4_busfreq driver
>>>>>>> would use PPMU(Performance Profiling Monitoring Units).
>>>>>>
>>>>>> This still sounds like a description of the _driver_, not the _device_.
>>>>>> The binding should describe the hardware, now the high level abstraction
>>>>>> that software is going to build atop of it.
>>>>>>
>>>>>> It sounds like this is a binding for the DMC PPMU?
>>>>>>
>>>>>> Is the PPMU a component of the DMC, or is it bolted on the side?
>>>>>
>>>>> PPMU(Performance Profiling Monitoring Unit) is to profile performance event of
>>>>> various IP on Exynos4. Each PPMU provide perforamnce event for each IP.
>>>>> We can check various PPMU as following:
>>>>>
>>>>> PPMU_3D
>>>>> PPMU_ACP
>>>>> PPMU_CAMIF
>>>>> PPMU_CPU
>>>>> PPMU_DMC0
>>>>> PPMU_DMC1
>>>>> PPMU_FSYS
>>>>> PPMU_IMAGE
>>>>> PPMU_LCD0
>>>>> PPMU_LCD1
>>>>> PPMU_MFC_L
>>>>> PPMU_MFC_R
>>>>> PPMU_TV
>>>>> PPMU_LEFT_BUS
>>>>> PPMU_RIGHT_BUS
>>>>>
>>>>> DMC (Dynamic Memory Controller) control the operation of DRAM in Exynos4 SoC.
>>>>> If we need to get memory bust utilization of DMC, we can get memory bus utilization
>>>>> from PPMU_DMC0/PPMU_DMC1.
>>>>>
>>>>> So, Exynos4's busfreq used two(PPMU_DMC0/PPMU_DMC1) among upper various PPMU list.
>>>>
>>>> Well, PPMUs and DMCs are separate hardware blocks found inside Exynos SoCs. Busfreq/devfreq is just a Linux-specific abstraction responsible for collecting data using PPMUs and controlling frequencies and voltages of appropriate power planes, vdd_int responsible for powering DMC0 and DMC1 blocks in this case.
>>>>
>>>
>>> I knew already.
>>>
>>>> I'm afraid that the binding you're proposing is unfortunately incorrect, because it represents the software abstraction, not the real hardware.
>>>
>>> What is exactly incorrect part of this patch?
>>>
>>
>> Device tree contains information about hardware, not about OS-specific drivers or subsystems. Busfreq/devfreq is not a hardware block, but a Linux-specific driver, so it's not suitable to be described by DT directly, especially considering the fact that in future it might be replaced by or merged with another subsystem.
>>
>> Only PPMUs are real hardware blocks present in the SoC. In addition, a SoC-level aspect of hardware description may be added, such as a list of power planes. See below for explanation.
>>
>
> You means that PPMU must need separate framework from devfreq subsystem?
> If PPMU framework will be implemented, PPMU framework provides API to external device driver
> as common clk framework, regulator framework.
> And then,exynos4 busfreq with devfreq have to use PPMU framework to monitor memory utilization.
>
> Is this right?
>

Not exactly. I'm just saying that in device tree, particular hardware 
blocks (such as PPMU) must be represented by their own nodes. This 
doesn't imply any particular implementation in code. DT bindings should 
be considered without any particular driver code in mind.

The principle here is that with device tree describing all the 
information needed for Exynos bus frequency and power plane voltage 
scaling, you might later rewrite completely the code, without changing 
the DT.

Of course this also gives some hints about how a good implementation can 
look, which would be as you wrote a separate framework for PPMUs. 
However calling this a framework would be a bit exaggerated, because it 
can be as simple as:

  - PPMU being separate platform devices, tracked by a minimal framework 
(e.g. a linked list and add(), remove(), find_by_node() functions),

  - devfreq for given power plane also being a platform device, binding 
directly to power plane node, i.e. matching the compatible string of 
Exynos power plane,

  - devfreq reading the list of PPMUs by phandles provided by its power 
plane node and looking up in the linked list mentioned above by 
find_by_node() function. Here deferred probe should happen if requested 
PPMU is not probed yet.

Basically that's all, the rest of the driver (except other issues 
mentioned in review) could stay as is.

>>>>
>>>> Instead, this should be separated into several independent bindings:
>>>>
>>>>    - PPMU bindings to list all the PPMU instances present in the SoC and resources they need,
>>>>
>>>>    - power plane bindings, which define a power plane in which multiple IP blocks might reside, can be monitored by one or more PPMU units and frequency and voltage of which can be configured according to determined performance level. Needed resources will be clocks and regulators to scale and probably also operating points.
>>>>
>>>> Then, exynos-busfreq driver should bind to such power planes, parse necessary data from DT (list of PPMUs and IP blocks, clocks, regulators and operating points) and register a devfreq entity.
>>>
>>> What is 'power plane'? I don't know 'power plane'.
>>
>> Power plane is a part of the SoC (set of IP blocks and buses) that is powered by the same power source. For example in Exynos4412 a power plane would be VDD_ARM, containing 4 Cortex A9 cores, two L2 cache blocks, L2 cache controller, SCU, and several other blocks. Another example in Exynos4412 would be VDD_INT plane, which contains MFC, AudioSS, LCD0, ISP, TV, CAM and several other blocks.
>>
>> In addition, such power plane might contain certain facilities, such as PPMU blocks, that let you estimate bus bandwidth utilization and scale certain performance and power parameters of such power plane, e.g. frequencies of internal buses and voltage of external regulator used to power it.
>>
>>> If you suggest 'power plane' concept and then merge this concept to mainline,
>>> After merged 'power plane' concept, I will apply 'power plane' concept to Exynos4's busfreq driver.
>>>
>>> I prefer to handle 'power plane' concept on other patchset for Exynos4's busfreq driver.
>>
>> This is a necessary prerequisite for having Exynos busfreq facilities described properly in device tree. This is not a concept of high level generic framework, but just a concept of hardware description that can be parsed by Linux drivers, such as Exynos busfreq.
>>
>> As I said above, a requirement is that DT does not describe Linux-specific internals, but the hardware that is used by them.
>>
>
> exynos4 busfreq with devfreq must need to execute .probe().
> So, exynos4 busfreq with devfreq should add dt node for exynos4 busfreq.
> But,
> You means that exynos4 busfreq can't add dt node of exynos4 busfreq
> because this driver has not the hardware block. ?

Yes, this is what I mean. DT should not contain virtual nor 
implementation-specific data, but rather mostly generic hardware 
description that can be reused by any implementation of given functionality.

However in this case, what is currently a single busfreq instance would 
correspond to a single power plane, which is a physical hardware entity 
and to which the exynos4 busfreq driver can bind.

Best regards,
Tomasz

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday, March 13, 2014 05:17:21 PM Chanwoo Choi wrote:
> This patchset support devicetree and use common ppmu driver instead of
> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
> get the resources for busfreq from dt data by using DT helper function.
> - PPMU register address
> - PPMU clock
> - Regulator for INT/MIF block
> 
> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
> To remove power-leakage in suspend state, before entering suspend state,
> disable ppmu clocks.
> 
> Changes from v1:
> - Add exynos4_bus.txt documentation for devicetree guide
> - Fix probe failure if CONFIG_PM_OPP is disabled
> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
> - Add additionally comment for PPMU usage instead of previous PPC
> - Split separate patch to remove ambiguous of patch
> 
> Chanwoo Choi (8):
>   devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
>   devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
>   devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
>   devfreq: exynos4: Fix bug of resource leak and code clean on probe()
>   devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
>   devfreq: exynos4: Fix power-leakage of clock on suspend state
>   devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
>   devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
> 
>  .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
>  drivers/devfreq/Kconfig                            |   1 +
>  drivers/devfreq/exynos/Makefile                    |   2 +-
>  drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
>  4 files changed, 341 insertions(+), 126 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt

Thanks for updating this patchset.  There are still some minor issues
left though:

- patch #4 should be at beginning of the patch series

- moving of devfreq_unregister_opp_notifier(dev, data->devfreq) from
  exynos4_bus_exit() to exynos4_busfreq_remove() should be in patch #4
  (which should really be at the beggining of patch series) not #3

- handling of iounmap(data->ppmu[i].hw_base) should be added to
  exynos4_bus_exit() in patch #2 not #3

- patch #8 summary and description should mention fact that it adds DT
  binding documentation (not the driver itself) and the patch itself
  can be slighlty polished

One important note about this patchset not mentioned in the cover
letter is that it is improving currently unused driver (because of
DT-only mach-exynos conversion the only user was removed in June 2013
and from the reading the code I suspect that even that user hadn't
worked previously).  As such this patch series should not cause any
regressions.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Chanwoo Choi]

Hi,

On 03/14/2014 01:43 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Thursday, March 13, 2014 05:17:21 PM Chanwoo Choi wrote:
>> This patchset support devicetree and use common ppmu driver instead of
>> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
>> get the resources for busfreq from dt data by using DT helper function.
>> - PPMU register address
>> - PPMU clock
>> - Regulator for INT/MIF block
>>
>> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
>> To remove power-leakage in suspend state, before entering suspend state,
>> disable ppmu clocks.
>>
>> Changes from v1:
>> - Add exynos4_bus.txt documentation for devicetree guide
>> - Fix probe failure if CONFIG_PM_OPP is disabled
>> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
>> - Add additionally comment for PPMU usage instead of previous PPC
>> - Split separate patch to remove ambiguous of patch
>>
>> Chanwoo Choi (8):
>>   devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
>>   devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
>>   devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
>>   devfreq: exynos4: Fix bug of resource leak and code clean on probe()
>>   devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
>>   devfreq: exynos4: Fix power-leakage of clock on suspend state
>>   devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
>>   devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
>>
>>  .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
>>  drivers/devfreq/Kconfig                            |   1 +
>>  drivers/devfreq/exynos/Makefile                    |   2 +-
>>  drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
>>  4 files changed, 341 insertions(+), 126 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> 
> Thanks for updating this patchset.  There are still some minor issues
> left though:
> 
> - patch #4 should be at beginning of the patch series
> 
> - moving of devfreq_unregister_opp_notifier(dev, data->devfreq) from
>   exynos4_bus_exit() to exynos4_busfreq_remove() should be in patch #4
>   (which should really be at the beggining of patch series) not #3
> 
> - handling of iounmap(data->ppmu[i].hw_base) should be added to
>   exynos4_bus_exit() in patch #2 not #3
> 
> - patch #8 summary and description should mention fact that it adds DT
>   binding documentation (not the driver itself) and the patch itself
>   can be slighlty polished

OK, I'll re-order the sequence of patchset and modify minior issues about your comment.
Also, I'll modify the patch description for patch8.

> 
> One important note about this patchset not mentioned in the cover
> letter is that it is improving currently unused driver (because of
> DT-only mach-exynos conversion the only user was removed in June 2013
> and from the reading the code I suspect that even that user hadn't
> worked previously).  As such this patch series should not cause any
> regressions.

I don't understand correct your meaning.I explained DT support on upper
patchset description by using DT helper function and I added PPMU descritpion.
Also, Each patch include detailed description of patch content.

What is more needed?

Best Regards,
Chanwoo Choi

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Bartlomiej Zolnierkiewicz]

On Friday, March 14, 2014 12:14:03 PM Chanwoo Choi wrote:
> Hi,
> 
> On 03/14/2014 01:43 AM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Thursday, March 13, 2014 05:17:21 PM Chanwoo Choi wrote:
> >> This patchset support devicetree and use common ppmu driver instead of
> >> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
> >> get the resources for busfreq from dt data by using DT helper function.
> >> - PPMU register address
> >> - PPMU clock
> >> - Regulator for INT/MIF block
> >>
> >> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
> >> To remove power-leakage in suspend state, before entering suspend state,
> >> disable ppmu clocks.
> >>
> >> Changes from v1:
> >> - Add exynos4_bus.txt documentation for devicetree guide
> >> - Fix probe failure if CONFIG_PM_OPP is disabled
> >> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
> >> - Add additionally comment for PPMU usage instead of previous PPC
> >> - Split separate patch to remove ambiguous of patch
> >>
> >> Chanwoo Choi (8):
> >>   devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
> >>   devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
> >>   devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
> >>   devfreq: exynos4: Fix bug of resource leak and code clean on probe()
> >>   devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
> >>   devfreq: exynos4: Fix power-leakage of clock on suspend state
> >>   devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
> >>   devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
> >>
> >>  .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
> >>  drivers/devfreq/Kconfig                            |   1 +
> >>  drivers/devfreq/exynos/Makefile                    |   2 +-
> >>  drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
> >>  4 files changed, 341 insertions(+), 126 deletions(-)
> >>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
> > 
> > Thanks for updating this patchset.  There are still some minor issues
> > left though:
> > 
> > - patch #4 should be at beginning of the patch series
> > 
> > - moving of devfreq_unregister_opp_notifier(dev, data->devfreq) from
> >   exynos4_bus_exit() to exynos4_busfreq_remove() should be in patch #4
> >   (which should really be at the beggining of patch series) not #3
> > 
> > - handling of iounmap(data->ppmu[i].hw_base) should be added to
> >   exynos4_bus_exit() in patch #2 not #3
> > 
> > - patch #8 summary and description should mention fact that it adds DT
> >   binding documentation (not the driver itself) and the patch itself
> >   can be slighlty polished
> 
> OK, I'll re-order the sequence of patchset and modify minior issues about your comment.
> Also, I'll modify the patch description for patch8.
> 
> > 
> > One important note about this patchset not mentioned in the cover
> > letter is that it is improving currently unused driver (because of
> > DT-only mach-exynos conversion the only user was removed in June 2013
> > and from the reading the code I suspect that even that user hadn't
> > worked previously).  As such this patch series should not cause any
> > regressions.
> 
> I don't understand correct your meaning.I explained DT support on upper
> patchset description by using DT helper function and I added PPMU descritpion.
> Also, Each patch include detailed description of patch content.

Everything is okay, I just noted that since there are no users of this
driver currently (the only user was NURI and it was removed by DT
conversion of mach-exynos) it should be okay to merge the patch series
quickly once reviewed and acked by the respective maintainers.

> What is more needed?

Users of the driver? ;)

Your patchset adds DT support and fixes to the driver but it doesn't
add actual users of the driver to arch/arm/boot/dts/ files.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Chanwoo Choi]

Hi,
On 03/14/2014 07:47 PM, Bartlomiej Zolnierkiewicz wrote:
> On Friday, March 14, 2014 12:14:03 PM Chanwoo Choi wrote:
>> Hi,
>>
>> On 03/14/2014 01:43 AM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> On Thursday, March 13, 2014 05:17:21 PM Chanwoo Choi wrote:
>>>> This patchset support devicetree and use common ppmu driver instead of
>>>> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
>>>> get the resources for busfreq from dt data by using DT helper function.
>>>> - PPMU register address
>>>> - PPMU clock
>>>> - Regulator for INT/MIF block
>>>>
>>>> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
>>>> To remove power-leakage in suspend state, before entering suspend state,
>>>> disable ppmu clocks.
>>>>
>>>> Changes from v1:
>>>> - Add exynos4_bus.txt documentation for devicetree guide
>>>> - Fix probe failure if CONFIG_PM_OPP is disabled
>>>> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
>>>> - Add additionally comment for PPMU usage instead of previous PPC
>>>> - Split separate patch to remove ambiguous of patch
>>>>
>>>> Chanwoo Choi (8):
>>>>   devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
>>>>   devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
>>>>   devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
>>>>   devfreq: exynos4: Fix bug of resource leak and code clean on probe()
>>>>   devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
>>>>   devfreq: exynos4: Fix power-leakage of clock on suspend state
>>>>   devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
>>>>   devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
>>>>
>>>>  .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
>>>>  drivers/devfreq/Kconfig                            |   1 +
>>>>  drivers/devfreq/exynos/Makefile                    |   2 +-
>>>>  drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
>>>>  4 files changed, 341 insertions(+), 126 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>
>>> Thanks for updating this patchset.  There are still some minor issues
>>> left though:
>>>
>>> - patch #4 should be at beginning of the patch series
>>>
>>> - moving of devfreq_unregister_opp_notifier(dev, data->devfreq) from
>>>   exynos4_bus_exit() to exynos4_busfreq_remove() should be in patch #4
>>>   (which should really be at the beggining of patch series) not #3
>>>
>>> - handling of iounmap(data->ppmu[i].hw_base) should be added to
>>>   exynos4_bus_exit() in patch #2 not #3
>>>
>>> - patch #8 summary and description should mention fact that it adds DT
>>>   binding documentation (not the driver itself) and the patch itself
>>>   can be slighlty polished
>>
>> OK, I'll re-order the sequence of patchset and modify minior issues about your comment.
>> Also, I'll modify the patch description for patch8.
>>
>>>
>>> One important note about this patchset not mentioned in the cover
>>> letter is that it is improving currently unused driver (because of
>>> DT-only mach-exynos conversion the only user was removed in June 2013
>>> and from the reading the code I suspect that even that user hadn't
>>> worked previously).  As such this patch series should not cause any
>>> regressions.
>>
>> I don't understand correct your meaning.I explained DT support on upper
>> patchset description by using DT helper function and I added PPMU descritpion.
>> Also, Each patch include detailed description of patch content.
> 
> Everything is okay, I just noted that since there are no users of this
> driver currently (the only user was NURI and it was removed by DT
> conversion of mach-exynos) it should be okay to merge the patch series
> quickly once reviewed and acked by the respective maintainers.
> 
>> What is more needed?
> 
> Users of the driver? ;)
> 
> Your patchset adds DT support and fixes to the driver but it doesn't
> add actual users of the driver to arch/arm/boot/dts/ files.

Ah, I didn't understand 'users'  meanings.

Now, clk-exynos4.c driver in mainline don't provide the clocks for PPMU IP. So, I can't add dt node of exynos4_busfreq to exynos4210.dtsi/exynos4x12.dtsi/exynos4210-trats.dts/exynos4412-trats2.dts.

First of all, I will add the ppmu clocks to clk-exynos4.c driver and then modify dts file for exynos4_busfreq as your comment. That which add the ppmu clocks is apart from this patch set.

Thanks for your comment.

Best Regards,
Chanwoo Choi

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Tomasz Figa]

Hi Chanwoo,

On 13.03.2014 09:17, Chanwoo Choi wrote:
> This patchset support devicetree and use common ppmu driver instead of
> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
> get the resources for busfreq from dt data by using DT helper function.
> - PPMU register address
> - PPMU clock
> - Regulator for INT/MIF block
>
> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
> To remove power-leakage in suspend state, before entering suspend state,
> disable ppmu clocks.
>
> Changes from v1:
> - Add exynos4_bus.txt documentation for devicetree guide
> - Fix probe failure if CONFIG_PM_OPP is disabled
> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
> - Add additionally comment for PPMU usage instead of previous PPC
> - Split separate patch to remove ambiguous of patch
>
> Chanwoo Choi (8):
>    devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
>    devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
>    devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
>    devfreq: exynos4: Fix bug of resource leak and code clean on probe()
>    devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
>    devfreq: exynos4: Fix power-leakage of clock on suspend state
>    devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
>    devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
>
>   .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
>   drivers/devfreq/Kconfig                            |   1 +
>   drivers/devfreq/exynos/Makefile                    |   2 +-
>   drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
>   4 files changed, 341 insertions(+), 126 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>

I have reviewed this series and there are several comments that I'd like 
to ask you to address. Please see my replies to particular patches.

However, this driver, even after applying your series, is still far from 
a state that would allow it to be enabled. The most important issue is 
direct access to CMU registers, based on static mapping, which is not 
allowed on multiplatform kernels and multiplatform-awareness for drivers 
is currently a must.

To allow this driver to be enabled, it needs to be converted to use 
common clock framework functions to configure all clocks, e.g. 
clk_set_rate(), clk_set_parent(), etc., without accessing CMU registers 
directly.

Of course as long as the driver is effectively unusable, to keep 
development, we can proceed with refactoring it step-by-step and your 
series would be basically the first step, after addressing the review 
comments.

Best regards,
Tomasz

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Chanwoo Choi]

Hi Tomasz,

On 03/15/2014 02:58 AM, Tomasz Figa wrote:
> Hi Chanwoo,
> 
> On 13.03.2014 09:17, Chanwoo Choi wrote:
>> This patchset support devicetree and use common ppmu driver instead of
>> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
>> get the resources for busfreq from dt data by using DT helper function.
>> - PPMU register address
>> - PPMU clock
>> - Regulator for INT/MIF block
>>
>> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
>> To remove power-leakage in suspend state, before entering suspend state,
>> disable ppmu clocks.
>>
>> Changes from v1:
>> - Add exynos4_bus.txt documentation for devicetree guide
>> - Fix probe failure if CONFIG_PM_OPP is disabled
>> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
>> - Add additionally comment for PPMU usage instead of previous PPC
>> - Split separate patch to remove ambiguous of patch
>>
>> Chanwoo Choi (8):
>>    devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
>>    devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
>>    devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
>>    devfreq: exynos4: Fix bug of resource leak and code clean on probe()
>>    devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
>>    devfreq: exynos4: Fix power-leakage of clock on suspend state
>>    devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
>>    devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
>>
>>   .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
>>   drivers/devfreq/Kconfig                            |   1 +
>>   drivers/devfreq/exynos/Makefile                    |   2 +-
>>   drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
>>   4 files changed, 341 insertions(+), 126 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>
> 
> I have reviewed this series and there are several comments that I'd like to ask you to address. Please see my replies to particular patches.

OK, I'll fix it about your comment.

> 
> However, this driver, even after applying your series, is still far from a state that would allow it to be enabled. The most important issue is direct access to CMU registers, based on static mapping, which is not allowed on multiplatform kernels and multiplatform-awareness for drivers is currently a must.
> 
> To allow this driver to be enabled, it needs to be converted to use common clock framework functions to configure all clocks, e.g. clk_set_rate(), clk_set_parent(), etc., without accessing CMU registers directly.
> 
> Of course as long as the driver is effectively unusable, to keep development, we can proceed with refactoring it step-by-step and your series would be basically the first step, after addressing the review comments.
> 

I agree your opinion. When setting frequency of memory bus, this driver access
directly to CMU registers. I know it should be modified by using common clk
framework as your comment.

I'll send patch set about using common clk framework instead of CMU register
based on static mapping after finished the review and apply of this patch set as next step.

Thanks for your review.

Best Regards,
Chanwoo Choi

Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Tomasz Figa]

On 17.03.2014 02:58, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 03/15/2014 02:58 AM, Tomasz Figa wrote:
>> Hi Chanwoo,
>>
>> On 13.03.2014 09:17, Chanwoo Choi wrote:
>>> This patchset support devicetree and use common ppmu driver instead of
>>> individual code of exynos4_bus.c to remove duplicate code. Also this patchset
>>> get the resources for busfreq from dt data by using DT helper function.
>>> - PPMU register address
>>> - PPMU clock
>>> - Regulator for INT/MIF block
>>>
>>> This patchset use SET_SYSTEM_SLEEP_PM_OPS macro intead of legacy method.
>>> To remove power-leakage in suspend state, before entering suspend state,
>>> disable ppmu clocks.
>>>
>>> Changes from v1:
>>> - Add exynos4_bus.txt documentation for devicetree guide
>>> - Fix probe failure if CONFIG_PM_OPP is disabled
>>> - Fix typo and resource leak(regulator/clock/memory) when happening probe failure
>>> - Add additionally comment for PPMU usage instead of previous PPC
>>> - Split separate patch to remove ambiguous of patch
>>>
>>> Chanwoo Choi (8):
>>>     devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC
>>>     devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data
>>>     devfreq: exynos4: Add ppmu's clock control and code clean about regulator control
>>>     devfreq: exynos4: Fix bug of resource leak and code clean on probe()
>>>     devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro
>>>     devfreq: exynos4: Fix power-leakage of clock on suspend state
>>>     devfreq: exynos4: Add CONFIG_PM_OPP dependency to fix probe fail
>>>     devfreq: exynos4: Add busfreq driver for exynos4210/exynos4x12
>>>
>>>    .../devicetree/bindings/devfreq/exynos4_bus.txt    |  49 +++
>>>    drivers/devfreq/Kconfig                            |   1 +
>>>    drivers/devfreq/exynos/Makefile                    |   2 +-
>>>    drivers/devfreq/exynos/exynos4_bus.c               | 415 ++++++++++++++-------
>>>    4 files changed, 341 insertions(+), 126 deletions(-)
>>>    create mode 100644 Documentation/devicetree/bindings/devfreq/exynos4_bus.txt
>>>
>>
>> I have reviewed this series and there are several comments that I'd like to ask you to address. Please see my replies to particular patches.
>
> OK, I'll fix it about your comment.
>
>>
>> However, this driver, even after applying your series, is still far from a state that would allow it to be enabled. The most important issue is direct access to CMU registers, based on static mapping, which is not allowed on multiplatform kernels and multiplatform-awareness for drivers is currently a must.
>>
>> To allow this driver to be enabled, it needs to be converted to use common clock framework functions to configure all clocks, e.g. clk_set_rate(), clk_set_parent(), etc., without accessing CMU registers directly.
>>
>> Of course as long as the driver is effectively unusable, to keep development, we can proceed with refactoring it step-by-step and your series would be basically the first step, after addressing the review comments.
>>
>
> I agree your opinion. When setting frequency of memory bus, this driver access
> directly to CMU registers. I know it should be modified by using common clk
> framework as your comment.
>
> I'll send patch set about using common clk framework instead of CMU register
> based on static mapping after finished the review and apply of this patch set as next step.

OK. I'm looking forward for patches sorting this out.

Best regards,
Tomasz

Re: Re: [PATCHv2 0/8] devfreq: exynos4: Support dt and use common ppmu driver

[Tomeu Vizoso]

On 03/17/2014 02:58 AM, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On 03/15/2014 02:58 AM, Tomasz Figa wrote:
>>
>> However, this driver, even after applying your series, is still far
>> from a state that would allow it to be enabled. The most important
>> issue is direct access to CMU registers, based on static mapping,
>> which is not allowed on multiplatform kernels and
>> multiplatform-awareness for drivers is currently a must.
>>
>> To allow this driver to be enabled, it needs to be converted to use
>> common clock framework functions to configure all clocks, e.g.
>> clk_set_rate(), clk_set_parent(), etc., without accessing CMU
>> registers directly.
>>
>> Of course as long as the driver is effectively unusable, to keep
>> development, we can proceed with refactoring it step-by-step and
>> your series would be basically the first step, after addressing the
>> review comments.
>>
>
> I agree your opinion. When setting frequency of memory bus, this
> driver access directly to CMU registers. I know it should be modified
> by using common clk framework as your comment.
>
> I'll send patch set about using common clk framework instead of CMU
> register based on static mapping after finished the review and apply
> of this patch set as next step.

Hi Chanwoo,

any news on this?

I would love to test this devfreq driver with the clk_set_floor_rate API 
I'm adding to the common clk framework.

Regards,

Tomeu