[PATCH 0/4] devfreq: exynos4: Support dt and use common ppmu driver

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

Chanwoo Choi (4):
  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: Use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method

 drivers/devfreq/exynos/exynos4_bus.c | 396 ++++++++++++++++++++++++-----------
 1 file changed, 271 insertions(+), 125 deletions(-)

-- 
1.8.0

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

Re: [PATCH 1/4] devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC

[Bartlomiej Zolnierkiewicz]

Hi Chanwoo,

On Wednesday, March 12, 2014 08:47:59 PM Chanwoo Choi wrote:
> 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,
>  	},
>  };

It looks OK but it would be good to also add bindings documentation file,
i.e. Documentation/devicetree/bindings/devfreq/exynos4_bus.txt.

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

Re: [PATCH 1/4] devfreq: exynos4: Support devicetree to get device id of Exynos4 SoC

[Chanwoo Choi]

Hi Batlomiej,

On 03/12/2014 11:37 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi Chanwoo,
> 
> On Wednesday, March 12, 2014 08:47:59 PM Chanwoo Choi wrote:
>> 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,
>>  	},
>>  };
> 
> It looks OK but it would be good to also add bindings documentation file,
> i.e. Documentation/devicetree/bindings/devfreq/exynos4_bus.txt.

OK I'll add documentation for exynos4_bus.c

Best Regards,
Chanwoo Choi

[PATCH 2/4] 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 block 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,a
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/exynos4_bus.c | 237 +++++++++++++++++++----------------
 1 file changed, 128 insertions(+), 109 deletions(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 168a7c6..16fb3cb 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, "Faild 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,23 +1096,48 @@ 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;
+	}
 
-	devfreq_register_opp_notifier(dev, data->devfreq);
+	/*
+	 * Start PPMU(Performance Profiling Monitoring Unit) to check
+	 * utilization of each IP in the Exynos4 SoC.
+	 */
+	busfreq_mon_reset(data);
 
+	/* 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_notifier_pm:
+	devfreq_unregister_opp_notifier(dev, data->devfreq);
+err_notifier_opp:
+	devfreq_remove_device(data->devfreq);
+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

Re: [PATCH 2/4] devfreq: exynos4: Use common ppmu driver and get ppmu address from dt data

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, March 12, 2014 08:48:00 PM Chanwoo Choi wrote:
> This patch use common ppmu driver of exynos_ppmu.c driver instead of individual
> function related to PPC block 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,a
> busfreq@106A0000 {
> 	compatible = "samsung,exynos4x12-busfreq";
> 	reg = <0x106A0000 0x2000>, <0x106B0000 0x2000>;
> 	regs-name = "PPMU_DMC0", "PPMU_DMC1";
> };

DT bindings need to be documented in Documentation/devicetree/bindings/.

> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/exynos/exynos4_bus.c | 237 +++++++++++++++++++----------------
>  1 file changed, 128 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
> index 168a7c6..16fb3cb 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];

I believe that above change should be mentioned in the patch description.

>  	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, "Faild to find devicetree node\n");

Failed

> +		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,23 +1096,48 @@ 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;
> +	}
>  
> -	devfreq_register_opp_notifier(dev, data->devfreq);
> +	/*
> +	 * Start PPMU(Performance Profiling Monitoring Unit) to check
> +	 * utilization of each IP in the Exynos4 SoC.
> +	 */
> +	busfreq_mon_reset(data);
>  
> +	/* Register opp_notifier for Exynos4 busfreq */
> +	err = devfreq_register_opp_notifier(dev, data->devfreq);
> +	if (err < 0) {

This patch adds devfreq_register_opp_notifier() return value checking,
this would be best done in a separate pre-patch.

> +		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);

It also adds devfreq_unregister_opp_notifier() call on error which
is (again) best to be handled in separate pre-patch (together with
devfreq_register_opp_notifier() return value checking).

There is also another change in patch #3 which would be best to be
moved to afore-mentioned pre-patch:

- bugfix moving devfreq_unregister_opp_notifier() call from
  exynos4_bus_exit() to exynos4_busfreq_remove()

> -		return err;
> +		goto err_notifier_pm;
>  	}
>  
>  	return 0;
> +
> +err_notifier_pm:
> +	devfreq_unregister_opp_notifier(dev, data->devfreq);
> +err_notifier_opp:
> +	devfreq_remove_device(data->devfreq);
> +err_opp:
> +	for (i = 0; i < PPMU_END; i++) {
> +		if (data->ppmu[i].hw_base)
> +			iounmap(data->ppmu[i].hw_base);
> +	}

This covers iounmap in the failure path but for handling remove
handler (exynos4_busfreq_remove()) ioumap needs to be done also in
exynos4_bus_exit().  This is currently done in patch #3 and should
be done in this patch instead.

> +	return err;
>  }
>  
>  static int exynos4_busfreq_remove(struct platform_device *pdev)

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

[PATCH 3/4] 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 | 107 ++++++++++++++++++++++++++++++-----
 1 file changed, 93 insertions(+), 14 deletions(-)

diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
index 16fb3cb..0c5b99e 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;
 
-	devfreq_unregister_opp_notifier(dev, data->devfreq);
+	/*
+	 * Un-map memory man 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]);
+	}
+
+	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,67 @@ 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 regulators to control voltage of int/mif 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_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_clocks;
+		}
+		ret = regulator_enable(data->vdd_mif);
+		if (ret < 0) {
+			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
+			goto err_clocks;
+		}
+		break;
+	case TYPE_BUSF_EXYNOS4210:
+	default:
+		dev_err(data->dev, "Unknown device type\n");
+		return -EINVAL;
+	};
+
 	return 0;
 
+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 +1152,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);
@@ -1133,6 +1204,11 @@ err_notifier_opp:
 	devfreq_remove_device(data->devfreq);
 err_opp:
 	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);
 	}
@@ -1144,7 +1220,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: [PATCH 3/4] devfreq: exynos4: Add ppmu's clock control and code clean about regulator control

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, March 12, 2014 08:48:01 PM 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>;
> };

This should be in Documentation/devicetree/bindings/ documentation.

> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/exynos/exynos4_bus.c | 107 ++++++++++++++++++++++++++++++-----
>  1 file changed, 93 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
> index 16fb3cb..0c5b99e 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;
>  
> -	devfreq_unregister_opp_notifier(dev, data->devfreq);
> +	/*
> +	 * Un-map memory man 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]);
> +	}
> +
> +	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,67 @@ 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 regulators to control voltage of int/mif 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_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_clocks;

This won't disable vdd_int regulator.

> +		}
> +		ret = regulator_enable(data->vdd_mif);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
> +			goto err_clocks;

ditto

> +		}
> +		break;
> +	case TYPE_BUSF_EXYNOS4210:
> +	default:
> +		dev_err(data->dev, "Unknown device type\n");
> +		return -EINVAL;

This looks very wrong for Exynos4210.

> +	};
> +
>  	return 0;
>  
> +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 +1152,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);
> @@ -1133,6 +1204,11 @@ err_notifier_opp:
>  	devfreq_remove_device(data->devfreq);
>  err_opp:

Disabling of regulators on failure is missing here.

>  	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);
>  	}
> @@ -1144,7 +1220,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;

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

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

[Chanwoo Choi]

Hi Batlomiej,

On 03/13/2014 12:17 AM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Wednesday, March 12, 2014 08:48:01 PM 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>;
>> };
> 
> This should be in Documentation/devicetree/bindings/ documentation.

OK, I will add documentation about it.

> 
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/exynos/exynos4_bus.c | 107 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 93 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>> index 16fb3cb..0c5b99e 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;
>>  
>> -	devfreq_unregister_opp_notifier(dev, data->devfreq);
>> +	/*
>> +	 * Un-map memory man 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]);
>> +	}
>> +
>> +	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,67 @@ 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 regulators to control voltage of int/mif 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_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_clocks;
> 
> This won't disable vdd_int regulator.

I don't understand. This patch contro/enable always vdd_int regualor as following:
	>> +	data->vdd_int = devm_regulator_get(dev, "vdd_int");
	>> +	ret = regulator_enable(data->vdd_int);
You can check this code on this patch. upper

> 
>> +		}
>> +		ret = regulator_enable(data->vdd_mif);
>> +		if (ret < 0) {
>> +			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
>> +			goto err_clocks;
> 
> ditto

> 
>> +		}
>> +		break;
>> +	case TYPE_BUSF_EXYNOS4210:
>> +	default:
>> +		dev_err(data->dev, "Unknown device type\n");
>> +		return -EINVAL;
> 
> This looks very wrong for Exynos4210.

What is wrong for Exynos4210?
Always, exynos4210/exynos4x12 control vdd_int regulator uppper this patch.

> 
>> +	};
>> +
>>  	return 0;
>>  
>> +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 +1152,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);
>> @@ -1133,6 +1204,11 @@ err_notifier_opp:
>>  	devfreq_remove_device(data->devfreq);
>>  err_opp:
> 
> Disabling of regulators on failure is missing here.

OK, I'll control regulator.

Best Regard,
Chanwoo Choi

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

[Chanwoo Choi]

Hi Bartlomiej,

On 03/13/2014 11:15 AM, Chanwoo Choi wrote:
> Hi Batlomiej,
> 
> On 03/13/2014 12:17 AM, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi,
>>
>> On Wednesday, March 12, 2014 08:48:01 PM 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>;
>>> };
>>
>> This should be in Documentation/devicetree/bindings/ documentation.
> 
> OK, I will add documentation about it.
> 
>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/exynos/exynos4_bus.c | 107 ++++++++++++++++++++++++++++++-----
>>>  1 file changed, 93 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c
>>> index 16fb3cb..0c5b99e 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;
>>>  
>>> -	devfreq_unregister_opp_notifier(dev, data->devfreq);
>>> +	/*
>>> +	 * Un-map memory man 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]);
>>> +	}
>>> +
>>> +	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,67 @@ 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 regulators to control voltage of int/mif 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_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_clocks;
>>
>> This won't disable vdd_int regulator.
> 
> I don't understand. This patch contro/enable always vdd_int regualor as following:
> 	>> +	data->vdd_int = devm_regulator_get(dev, "vdd_int");
> 	>> +	ret = regulator_enable(data->vdd_int);
> You can check this code on this patch. upper
> 
>>
>>> +		}
>>> +		ret = regulator_enable(data->vdd_mif);
>>> +		if (ret < 0) {
>>> +			dev_err(dev, "Failed to enable regulator of vdd_mif\n");
>>> +			goto err_clocks;
>>
>> ditto
> 
>>
>>> +		}
>>> +		break;
>>> +	case TYPE_BUSF_EXYNOS4210:
>>> +	default:
>>> +		dev_err(data->dev, "Unknown device type\n");
>>> +		return -EINVAL;
>>
>> This looks very wrong for Exynos4210.
> 
> What is wrong for Exynos4210?
> Always, exynos4210/exynos4x12 control vdd_int regulator uppper this patch.

It is my mistake. I'll modify it.

Best Regards,
Chanwoo Choi

[PATCH 4/4] devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method

[Chanwoo Choi]

This patch use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method. Also,
Before entering suspend state, disable ppmu's clock to remove power-leakage
in suspend state.

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 0c5b99e..7e1540a 100644
--- a/drivers/devfreq/exynos/exynos4_bus.c
+++ b/drivers/devfreq/exynos/exynos4_bus.c
@@ -1229,16 +1229,40 @@ 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);
+	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 = {
-	.resume	= 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: [PATCH 4/4] devfreq: exynos4: Use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, March 12, 2014 08:48:02 PM Chanwoo Choi wrote:
> This patch use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method. Also,
> Before entering suspend state, disable ppmu's clock to remove power-leakage
> in suspend state.

The main thing that this patch does is adding ->suspend method and PPMU
clocks handling and this should be reflected in the patch summary (i.e.
"devfreq: exynos4: fix PM suspend/resume handling for PPMU clocks") and in
the patch description. The fact that the patch also converts code to use
SET_SYSTEM_SLEEP_PM_OPS() macro is secondary.

> 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 0c5b99e..7e1540a 100644
> --- a/drivers/devfreq/exynos/exynos4_bus.c
> +++ b/drivers/devfreq/exynos/exynos4_bus.c
> @@ -1229,16 +1229,40 @@ 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);
> +	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 = {
> -	.resume	= exynos4_busfreq_resume,
> +	SET_SYSTEM_SLEEP_PM_OPS(exynos4_busfreq_suspend, exynos4_busfreq_resume)
>  };
>  
>  static const struct platform_device_id exynos4_busfreq_id[] = {

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

Re: [PATCH 0/4] devfreq: exynos4: Support dt and use common ppmu driver

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday, March 12, 2014 08:47:58 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.
> 
> Chanwoo Choi (4):
>   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: Use SET_SYSTEM_SLEEP_PM_OPS macro instead of legacy method
> 
>  drivers/devfreq/exynos/exynos4_bus.c | 396 ++++++++++++++++++++++++-----------
>  1 file changed, 271 insertions(+), 125 deletions(-)

The patchset generally looks OK, for my review comments please see separate
mails.  One thing I forgot to mention yet: please also fix DTS files so
exynos4_bus driver is finally functional (currently even with this patchset
it is not).

PS Please also add linux-samsung-soc@vger.kernel.org to Cc: when sending
the next revision of this patchset.

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