[PATCH v3 0/8] PM / devfreq: Use OPP interface to handle the frequency

[PATCH v3 0/8] PM / devfreq: Use OPP interface to handle the frequency

[Chanwoo Choi]

These patches makes the devfreq to use the OPP interface and clean-up codes.
- patch 1~5 are related to te OPP interfaces. 
- patch 6 removes the unneeded code.
- patch 7 clean-up for the governor name.
- patch 8 registers the cooling device for exynos-bus.

[Detaild Descripion]
The commit a76caf55e5b3 ("thermal: Add devfreq cooling") provides
the devfreq cooling device by using the OPP interface such as
dev_pm_opp_disable() and dev_pm_opp_enable(). It means that
the OPP interface is able to change the available status of the frequency.

Firstly, the existing devfreq doesn't use the OPP interface when showing
the minimum and maximum frequency through the following sysfs nodes:
It shows the wrong frequency value because min_freq/max_freq don't
consider the frequency status by handling OPP interface
(opp_dev_pm_opp_{disable|add}()). So, these patches fix this issue.
- /sys/class/devfreq/devfreqX/min_freq
- /sys/class/devfreq/devfreqX/max_freq

Second, the 'available_frequencies' should show the all supported frequencis
even if the specific frequency is not available. It doesn't matter whether
frequneyc is available or not. Because the role of 'available_frequencies'
shows the all frequencies. Also, these patches fix this issue.
- /sys/class/devfreq/devfreqX/available_frequencies

Third, update_devfreq() get the available next frequency by using
the devfreq_recommended_opp() in order to consider the disabled OPP.

For example,
- devfreq's min_freq is 100Mhz and max_freq is 700Mhz.
- OPP disabled 500/600/700Mhz due to devfreq-cooling.c.
- simple_ondemand govenor decided the next target_freq (600Mhz)
|----------|-------------------------------------------------------------|
|Freq(MHz) |100     |200    |300    |400     |500     |600      |70 0    |
|Devfreq   |min_freq|       |       |        |        |         |max_freq|
|OPP avail |enabled |enabled|enabled|enabled |Disabled| Disabled|Disabled|
|Ondmenad  |        |       |       |        |        |next_freq|        |
|------------------------------------------------------------------------|

In result,
- Before this patch, target_freq is 600Mhz
  and TRANSITION_NOTIFIER sends the next_freq is 600Mhz to the notifiee.
- After this patch, target_freq is 400Mhz because 500/600 were disabled by OPP.
  and TRANSITION_NOTIFIER sends the next_freq is 400Mhz to the notifiee.

Lastly,
- patch6/7 fix the minor issue and cleanup codes.
- patch8 register the cooling device. It depends on opp patch[1].
[1] https://patchwork.kernel.org/patch/9962387/


Changes from v2:
(https://lkml.org/lkml/2017/9/20/886)
- Fix the exynos-bus.c for the cooling device on patch8

Changes from v1:
(https://lkml.org/lkml/2017/8/23/785)
- Show the available frequencies as an ascending order
- Change the author info from cwchoi00@gmail.com to cw00.choi@samsung.com
- Drop the patches related to opp_notifier
- Add new patch5/6/7/8

Chanwoo Choi (8):
  PM / devfreq: Set min/max_freq when adding the devfreq device
  Revert "PM / devfreq: Add show_one macro to delete the duplicate code"
  PM / devfreq: Show the available min/max frequency through sysfs node
  PM / devfreq: Show the all available frequencies
  PM / devfreq: Get the available next frequency on update_devfreq()
  PM / devfreq: Remove unneeded conditional statement
  PM / devfreq: Define the constant governor name
  PM / devfreq: exynos-bus: Register cooling device

 drivers/devfreq/devfreq.c                 | 121 +++++++++++++++++++++++-------
 drivers/devfreq/exynos-bus.c              |  25 +++++-
 drivers/devfreq/governor_passive.c        |   2 +-
 drivers/devfreq/governor_performance.c    |   2 +-
 drivers/devfreq/governor_powersave.c      |   2 +-
 drivers/devfreq/governor_simpleondemand.c |   2 +-
 drivers/devfreq/governor_userspace.c      |   2 +-
 drivers/devfreq/rk3399_dmc.c              |   2 +-
 include/linux/devfreq.h                   |   7 ++
 9 files changed, 131 insertions(+), 34 deletions(-)

-- 
1.9.1

[PATCH v3 1/8] PM / devfreq: Set min/max_freq when adding the devfreq device

[Chanwoo Choi]

Prior to that, the min/max_freq of the devfreq device are always zero
before the user changes the min/max_freq through sysfs entries.
It might make the confusion for the min/max_freq.

This patch initializes the available min/max_freq by using the OPP
during adding the devfreq device.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index a1c4ee818614..ae8717a6eee1 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -69,6 +69,34 @@ static struct devfreq *find_device_devfreq(struct device *dev)
 	return ERR_PTR(-ENODEV);
 }
 
+static unsigned long find_available_min_freq(struct devfreq *devfreq)
+{
+	struct dev_pm_opp *opp;
+	unsigned long min_freq = 0;
+
+	opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);
+	if (IS_ERR(opp))
+		min_freq = 0;
+	else
+		dev_pm_opp_put(opp);
+
+	return min_freq;
+}
+
+static unsigned long find_available_max_freq(struct devfreq *devfreq)
+{
+	struct dev_pm_opp *opp;
+	unsigned long max_freq = ULONG_MAX;
+
+	opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
+	if (IS_ERR(opp))
+		max_freq = 0;
+	else
+		dev_pm_opp_put(opp);
+
+	return max_freq;
+}
+
 /**
  * devfreq_get_freq_level() - Lookup freq_table for the frequency
  * @devfreq:	the devfreq instance
@@ -559,6 +587,21 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		mutex_lock(&devfreq->lock);
 	}
 
+	/* Set the scaling available min_freq and max_freq */
+	devfreq->min_freq = find_available_min_freq(devfreq);
+	if (!devfreq->min_freq) {
+		mutex_unlock(&devfreq->lock);
+		err = -EINVAL;
+		goto err_dev;
+	}
+
+	devfreq->max_freq = find_available_max_freq(devfreq);
+	if (!devfreq->max_freq) {
+		mutex_unlock(&devfreq->lock);
+		err = -EINVAL;
+		goto err_dev;
+	}
+
 	dev_set_name(&devfreq->dev, "devfreq%d",
 				atomic_inc_return(&devfreq_no));
 	err = device_register(&devfreq->dev);
-- 
1.9.1

[PATCH v3 2/8] Revert "PM / devfreq: Add show_one macro to delete the duplicate code"

[Chanwoo Choi]

This reverts commit 3104fa3081126c9bda35793af5f335d0ee0d5818.

The {min|max}_freq_show() show the stored value of the struct devfreq.
But, if the drivers/thermal/devfreq_cooling.c disables the specific
frequency value, {min|max}_freq_show() have to check this situation
before showing the stored value. So, this patch revert the macro
in order to add the additional codes.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ae8717a6eee1..2ce1fd0a1324 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1125,6 +1125,12 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 	return ret;
 }
 
+static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
+}
+
 static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -1151,17 +1157,13 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 	mutex_unlock(&df->lock);
 	return ret;
 }
+static DEVICE_ATTR_RW(min_freq);
 
-#define show_one(name)						\
-static ssize_t name##_show					\
-(struct device *dev, struct device_attribute *attr, char *buf)	\
-{								\
-	return sprintf(buf, "%lu\n", to_devfreq(dev)->name);	\
+static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
 }
-show_one(min_freq);
-show_one(max_freq);
-
-static DEVICE_ATTR_RW(min_freq);
 static DEVICE_ATTR_RW(max_freq);
 
 static ssize_t available_frequencies_show(struct device *d,
-- 
1.9.1

[PATCH v3 3/8] PM / devfreq: Show the available min/max frequency through sysfs node

[Chanwoo Choi]

The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq
are available or not. Those sysfs nodes show just the stored value
in the struct devfreq.

The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}()
might change the state of the device's supported frequency. This patch
shows the available minimum and maximum frequency through sysfs node.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2ce1fd0a1324..799a0cf75d39 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
+	struct devfreq *df = to_devfreq(dev);
+	unsigned long min_freq = to_devfreq(dev)->min_freq;
+	unsigned long available_min_freq = find_available_min_freq(df);
+
+	if (available_min_freq != 0 && min_freq < available_min_freq)
+		min_freq = available_min_freq;
+
+	return sprintf(buf, "%lu\n", min_freq);
 }
 
 static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
@@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
+	struct devfreq *df = to_devfreq(dev);
+	unsigned long max_freq = to_devfreq(dev)->max_freq;
+	unsigned long available_max_freq = find_available_max_freq(df);
+
+	if (available_max_freq != 0 && max_freq > available_max_freq)
+		max_freq = available_max_freq;
+
+	return sprintf(buf, "%lu\n", max_freq);
 }
 static DEVICE_ATTR_RW(max_freq);
 
-- 
1.9.1

[PATCH v3 4/8] PM / devfreq: Show the all available frequencies

[Chanwoo Choi]

The commit a76caf55e5b35 ("thermal: Add devfreq cooling") allows
the devfreq device to use the cooling device. When the cooling down
are required, the devfreq_cooling.c disables the OPP entry with
the dev_pm_opp_disable(). In result, 'available_frequencies'[1]
sysfs node never came to show the all available frequencies.
[1] /sys/class/devfreq/.../available_frequencies

So, this patch uses the 'freq_table' in the 'struct devfreq_dev_profile'
in order to show the all available frequencies.
- If 'freq_table' is NULL, devfreq core initializes them by using OPP values.
- If 'freq_table' is initialized, devfreq core just uses the 'freq_table'.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 799a0cf75d39..1c4b377cacfb 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1185,22 +1185,26 @@ static ssize_t available_frequencies_show(struct device *d,
 					  char *buf)
 {
 	struct devfreq *df = to_devfreq(d);
-	struct device *dev = df->dev.parent;
-	struct dev_pm_opp *opp;
 	ssize_t count = 0;
-	unsigned long freq = 0;
+	unsigned long *freq_table;
+	int i, max_state;
 
-	do {
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp))
-			break;
+	mutex_lock(&df->lock);
 
-		dev_pm_opp_put(opp);
-		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
-				   "%lu ", freq);
-		freq++;
-	} while (1);
+	freq_table = df->profile->freq_table;
+	max_state = df->profile->max_state;
 
+	if (freq_table[0] < freq_table[max_state - 1]) {
+		for (i = 0; i < max_state; i++)
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					"%lu ", freq_table[i]);
+	} else {
+		for (i = max_state - 1; i >= 0 ; i--)
+			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+					"%lu ", freq_table[i]);
+	}
+
+	mutex_unlock(&df->lock);
 	/* Truncate the trailing space */
 	if (count)
 		count--;
-- 
1.9.1

[PATCH v3 5/8] PM / devfreq: Get the available next frequency on update_devfreq()

[Chanwoo Choi]

The update_devfreq() considers only user frequency (min_freq/max_freq)
and the next target_freq provided by the governor. But, the commit
a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
OPP as a cooling device. In result, the update_devfreq() have to
consider the 'opp->available' status in order to decicde the next freq
by the devfreq_recommended_opp().

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1c4b377cacfb..3b9662ffe603 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
 int update_devfreq(struct devfreq *devfreq)
 {
 	struct devfreq_freqs freqs;
+	struct dev_pm_opp *opp;
 	unsigned long freq, cur_freq;
 	int err = 0;
 	u32 flags = 0;
@@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
 		return err;
 
 	/*
-	 * Adjust the frequency with user freq and QoS.
+	 * Adjust the frequency with user freq, QoS and available freq.
 	 *
 	 * List from the highest priority
 	 * max_freq
@@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
 		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
 	}
 
+	opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+	freq = dev_pm_opp_get_freq(opp);
+	dev_pm_opp_put(opp);
+
 	if (devfreq->profile->get_cur_freq)
 		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
 	else
-- 
1.9.1

[PATCH v3 6/8] PM / devfreq: Remove unneeded conditional statement

[Chanwoo Choi]

The commit 0ec09ac2cebe9 ("PM / devfreq: Set the freq_table of devfreq
device") initializes the freq_table array of each devfreq device always.
In result, it is unneeded to check whether profile->freq_table is NULL
or not.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 3b9662ffe603..b28810cae7ee 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -315,10 +315,9 @@ int update_devfreq(struct devfreq *devfreq)
 	freqs.new = freq;
 	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
 
-	if (devfreq->profile->freq_table)
-		if (devfreq_update_status(devfreq, freq))
-			dev_err(&devfreq->dev,
-				"Couldn't update frequency transition information.\n");
+	if (devfreq_update_status(devfreq, freq))
+		dev_err(&devfreq->dev,
+			"Couldn't update frequency transition information.\n");
 
 	devfreq->previous_freq = freq;
 	return err;
-- 
1.9.1

[PATCH v3 7/8] PM / devfreq: Define the constant governor name

[Chanwoo Choi]

Prior to that, the devfreq device uses the governor name when adding
the itself. In order to prevent the mistake used the wrong governor name,
this patch defines the governor name as a constant and then uses them
instead of using the string directly.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/devfreq/exynos-bus.c              | 5 +++--
 drivers/devfreq/governor_passive.c        | 2 +-
 drivers/devfreq/governor_performance.c    | 2 +-
 drivers/devfreq/governor_powersave.c      | 2 +-
 drivers/devfreq/governor_simpleondemand.c | 2 +-
 drivers/devfreq/governor_userspace.c      | 2 +-
 drivers/devfreq/rk3399_dmc.c              | 2 +-
 include/linux/devfreq.h                   | 7 +++++++
 8 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 49f68929e024..c25658b26598 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -436,7 +436,8 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	ondemand_data->downdifferential = 5;
 
 	/* Add devfreq device to monitor and handle the exynos bus */
-	bus->devfreq = devm_devfreq_add_device(dev, profile, "simple_ondemand",
+	bus->devfreq = devm_devfreq_add_device(dev, profile,
+						DEVFREQ_GOV_SIMPLE_ONDEMAND,
 						ondemand_data);
 	if (IS_ERR(bus->devfreq)) {
 		dev_err(dev, "failed to add devfreq device\n");
@@ -488,7 +489,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	passive_data->parent = parent_devfreq;
 
 	/* Add devfreq device for exynos bus with passive governor */
-	bus->devfreq = devm_devfreq_add_device(dev, profile, "passive",
+	bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
 						passive_data);
 	if (IS_ERR(bus->devfreq)) {
 		dev_err(dev,
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index 673ad8cc9a1d..3bc29acbd54e 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -183,7 +183,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
 }
 
 static struct devfreq_governor devfreq_passive = {
-	.name = "passive",
+	.name = DEVFREQ_GOV_PASSIVE,
 	.immutable = 1,
 	.get_target_freq = devfreq_passive_get_target_freq,
 	.event_handler = devfreq_passive_event_handler,
diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
index c72f942f30a8..4d23ecfbd948 100644
--- a/drivers/devfreq/governor_performance.c
+++ b/drivers/devfreq/governor_performance.c
@@ -42,7 +42,7 @@ static int devfreq_performance_handler(struct devfreq *devfreq,
 }
 
 static struct devfreq_governor devfreq_performance = {
-	.name = "performance",
+	.name = DEVFREQ_GOV_PERFORMANCE,
 	.get_target_freq = devfreq_performance_func,
 	.event_handler = devfreq_performance_handler,
 };
diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
index 0c6bed567e6d..0c42f23249ef 100644
--- a/drivers/devfreq/governor_powersave.c
+++ b/drivers/devfreq/governor_powersave.c
@@ -39,7 +39,7 @@ static int devfreq_powersave_handler(struct devfreq *devfreq,
 }
 
 static struct devfreq_governor devfreq_powersave = {
-	.name = "powersave",
+	.name = DEVFREQ_GOV_POWERSAVE,
 	.get_target_freq = devfreq_powersave_func,
 	.event_handler = devfreq_powersave_handler,
 };
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index ae72ba5e78df..28e0f2de7100 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -125,7 +125,7 @@ static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
 }
 
 static struct devfreq_governor devfreq_simple_ondemand = {
-	.name = "simple_ondemand",
+	.name = DEVFREQ_GOV_SIMPLE_ONDEMAND,
 	.get_target_freq = devfreq_simple_ondemand_func,
 	.event_handler = devfreq_simple_ondemand_handler,
 };
diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
index 77028c27593c..080607c3f34d 100644
--- a/drivers/devfreq/governor_userspace.c
+++ b/drivers/devfreq/governor_userspace.c
@@ -87,7 +87,7 @@ static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
 	NULL,
 };
 static const struct attribute_group dev_attr_group = {
-	.name	= "userspace",
+	.name	= DEVFREQ_GOV_USERSPACE,
 	.attrs	= dev_entries,
 };
 
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 1b89ebbad02c..5dfbfa3cc878 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -431,7 +431,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 
 	data->devfreq = devm_devfreq_add_device(dev,
 					   &rk3399_devfreq_dmc_profile,
-					   "simple_ondemand",
+					   DEVFREQ_GOV_SIMPLE_ONDEMAND,
 					   &data->ondemand_data);
 	if (IS_ERR(data->devfreq))
 		return PTR_ERR(data->devfreq);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 597294e0cc40..a3ad2074bebe 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -19,6 +19,13 @@
 
 #define DEVFREQ_NAME_LEN 16
 
+/* DEVFREQ governor name */
+#define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
+#define DEVFREQ_GOV_PERFORMANCE		"performance"
+#define DEVFREQ_GOV_POWERSAVE		"powersave"
+#define DEVFREQ_GOV_USERSPACE		"userspace"
+#define DEVFREQ_GOV_PASSIVE		"passive"
+
 /* DEVFREQ notifier interface */
 #define DEVFREQ_TRANSITION_NOTIFIER	(0)
 
-- 
1.9.1

[PATCH v3 8/8] PM / devfreq: exynos-bus: Register cooling device

[Chanwoo Choi]

This patch registers the Exynos Bus-Frequency scaling device
as a cooling device of thermal management.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Kukjin Kim <kgene@kernel.org>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/devfreq/exynos-bus.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index c25658b26598..fe9ef6a3238b 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -15,6 +15,7 @@
 #include <linux/clk.h>
 #include <linux/devfreq.h>
 #include <linux/devfreq-event.h>
+#include <linux/devfreq_cooling.h>
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/module.h>
@@ -41,6 +42,8 @@ struct exynos_bus {
 	struct clk *clk;
 	unsigned int voltage_tolerance;
 	unsigned int ratio;
+
+	struct thermal_cooling_device *cdev;
 };
 
 /*
@@ -468,6 +471,13 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	/* Register devfreq cooling device */
+	bus->cdev = of_devfreq_cooling_register(np, bus->devfreq);
+	if (IS_ERR(bus->cdev)) {
+		dev_err(dev, "running exynos-bus without cooling device\n");
+		bus->cdev = NULL;
+	}
+
 	goto out;
 passive:
 	/* Initialize the struct profile and governor data for passive device */
@@ -514,6 +524,15 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int exynos_bus_remove(struct platform_device *pdev)
+{
+	struct exynos_bus *bus = platform_get_drvdata(pdev);
+
+	devfreq_cooling_unregister(bus->cdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int exynos_bus_resume(struct device *dev)
 {
@@ -556,6 +575,7 @@ static int exynos_bus_suspend(struct device *dev)
 
 static struct platform_driver exynos_bus_platdrv = {
 	.probe		= exynos_bus_probe,
+	.remove		= exynos_bus_remove,
 	.driver = {
 		.name	= "exynos-bus",
 		.pm	= &exynos_bus_pm,
-- 
1.9.1

RE: [PATCH v3 1/8] PM / devfreq: Set min/max_freq when adding the devfreq device

[MyungJoo Ham]

> Prior to that, the min/max_freq of the devfreq device are always zero
> before the user changes the min/max_freq through sysfs entries.
> It might make the confusion for the min/max_freq.
> 
> This patch initializes the available min/max_freq by using the OPP
> during adding the devfreq device.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a1c4ee818614..ae8717a6eee1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index a1c4ee818614..ae8717a6eee1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -69,6 +69,34 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>  	return ERR_PTR(-ENODEV);
>  }
>  
> +static unsigned long find_available_min_freq(struct devfreq *devfreq)
> +{
> +	struct dev_pm_opp *opp;
> +	unsigned long min_freq = 0;
> +
> +	opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);

When min_freq is 0, dev_pm_opp_find_freq_ceil will return ERR_PTR(-EINVAL);

--- from drivers/base/power/opp/core.c ------ 
struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
                                             unsigned long *freq)
{
        struct opp_table *opp_table;
        struct dev_pm_opp *opp;

        if (!dev || !freq) {
                dev_err(dev, "%s: Invalid argument freq=%p\n", __func__, freq);
                return ERR_PTR(-EINVAL);
        }
---------------------------------------------

Cheers,
MyungJoo

RE: [PATCH v3 3/8] PM / devfreq: Show the available min/max frequency through sysfs node

[MyungJoo Ham]

> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq
> are available or not. Those sysfs nodes show just the stored value
> in the struct devfreq.
> 
> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}()
> might change the state of the device's supported frequency. This patch
> shows the available minimum and maximum frequency through sysfs node.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2ce1fd0a1324..799a0cf75d39 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
> -	return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
> +	struct devfreq *df = to_devfreq(dev);
> +	unsigned long min_freq = to_devfreq(dev)->min_freq;
> +	unsigned long available_min_freq = find_available_min_freq(df);
> +
> +	if (available_min_freq != 0 && min_freq < available_min_freq)

nitpick:

If available_min_freq == 0,
it can't be min_freq < available_min_freq anyway;
it's unsigned.

> +		min_freq = available_min_freq;
> +
> +	return sprintf(buf, "%lu\n", min_freq);
>  }
>  
>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
> -	return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
> +	struct devfreq *df = to_devfreq(dev);
> +	unsigned long max_freq = to_devfreq(dev)->max_freq;
> +	unsigned long available_max_freq = find_available_max_freq(df);
> +
> +	if (available_max_freq != 0 && max_freq > available_max_freq)
> +		max_freq = available_max_freq;

similar here.

> +
> +	return sprintf(buf, "%lu\n", max_freq);
>  }
>  static DEVICE_ATTR_RW(max_freq);

Re: [PATCH v3 3/8] PM / devfreq: Show the available min/max frequency through sysfs node

[Chanwoo Choi]

On Wed, Oct 11, 2017 at 8:15 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq
>> are available or not. Those sysfs nodes show just the stored value
>> in the struct devfreq.
>>
>> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}()
>> might change the state of the device's supported frequency. This patch
>> shows the available minimum and maximum frequency through sysfs node.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 2ce1fd0a1324..799a0cf75d39 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>>                            char *buf)
>>  {
>> -     return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
>> +     struct devfreq *df = to_devfreq(dev);
>> +     unsigned long min_freq = to_devfreq(dev)->min_freq;
>> +     unsigned long available_min_freq = find_available_min_freq(df);
>> +
>> +     if (available_min_freq != 0 && min_freq < available_min_freq)
>
> nitpick:
>
> If available_min_freq == 0,
> it can't be min_freq < available_min_freq anyway;
> it's unsigned.

If the dev_pm_opp_find_*() return the error in the find_available_min_freq(),
avaiable_min_freq is zero. So, if available_min_freq is zero,
min_freq_show doesn't need to compare 'min_freq < available_min_freq'.

In result, if 'available_min_freq' is zero, min_freq_show() only considers
the 'min_freq' variable.

>
>> +             min_freq = available_min_freq;
>> +
>> +     return sprintf(buf, "%lu\n", min_freq);
>>  }
>>
>>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>>                            char *buf)
>>  {
>> -     return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
>> +     struct devfreq *df = to_devfreq(dev);
>> +     unsigned long max_freq = to_devfreq(dev)->max_freq;
>> +     unsigned long available_max_freq = find_available_max_freq(df);
>> +
>> +     if (available_max_freq != 0 && max_freq > available_max_freq)
>> +             max_freq = available_max_freq;
>
> similar here.

ditto.

>
>> +
>> +     return sprintf(buf, "%lu\n", max_freq);
>>  }
>>  static DEVICE_ATTR_RW(max_freq);


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 3/8] PM / devfreq: Show the available min/max frequency through sysfs node

[Chanwoo Choi]

On 2017년 10월 11일 21:57, Chanwoo Choi wrote:
> On Wed, Oct 11, 2017 at 8:15 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq
>>> are available or not. Those sysfs nodes show just the stored value
>>> in the struct devfreq.
>>>
>>> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}()
>>> might change the state of the device's supported frequency. This patch
>>> shows the available minimum and maximum frequency through sysfs node.
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/devfreq.c | 18 ++++++++++++++++--
>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 2ce1fd0a1324..799a0cf75d39 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>>>                            char *buf)
>>>  {
>>> -     return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
>>> +     struct devfreq *df = to_devfreq(dev);
>>> +     unsigned long min_freq = to_devfreq(dev)->min_freq;
>>> +     unsigned long available_min_freq = find_available_min_freq(df);
>>> +
>>> +     if (available_min_freq != 0 && min_freq < available_min_freq)
>>
>> nitpick:
>>
>> If available_min_freq == 0,
>> it can't be min_freq < available_min_freq anyway;
>> it's unsigned.
> 
> If the dev_pm_opp_find_*() return the error in the find_available_min_freq(),
> avaiable_min_freq is zero. So, if available_min_freq is zero,
> min_freq_show doesn't need to compare 'min_freq < available_min_freq'.
> 
> In result, if 'available_min_freq' is zero, min_freq_show() only considers
> the 'min_freq' variable.

I'll modify this patch as following: How about that?

+       if (available_min_freq == 0)
+               goto out;
+       else if (min_freq < available_min_freq)
+               min_freq = available_min_freq;
+
+out:
+       return sprintf(buf, "%lu\n", min_freq);


>>
>>> +             min_freq = available_min_freq;
>>> +
>>> +     return sprintf(buf, "%lu\n", min_freq);
>>>  }
>>>
>>>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>>>                            char *buf)
>>>  {
>>> -     return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
>>> +     struct devfreq *df = to_devfreq(dev);
>>> +     unsigned long max_freq = to_devfreq(dev)->max_freq;
>>> +     unsigned long available_max_freq = find_available_max_freq(df);
>>> +
>>> +     if (available_max_freq != 0 && max_freq > available_max_freq)
>>> +             max_freq = available_max_freq;
>>
>> similar here.
> 
> ditto.
> 
>>
>>> +
>>> +     return sprintf(buf, "%lu\n", max_freq);
>>>  }
>>>  static DEVICE_ATTR_RW(max_freq);
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 3/8] PM / devfreq: Show the available min/max frequency through sysfs node

[Chanwoo Choi]

Hi,

On 2017년 10월 12일 13:08, Chanwoo Choi wrote:
> On 2017년 10월 11일 21:57, Chanwoo Choi wrote:
>> On Wed, Oct 11, 2017 at 8:15 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>>> The existing {min|max}_freq sysfs nodes don't consider whether min/max_freq
>>>> are available or not. Those sysfs nodes show just the stored value
>>>> in the struct devfreq.
>>>>
>>>> The devfreq uses the OPP interface and then dev_pm_opp_{disable|add}()
>>>> might change the state of the device's supported frequency. This patch
>>>> shows the available minimum and maximum frequency through sysfs node.
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>>  drivers/devfreq/devfreq.c | 18 ++++++++++++++++--
>>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 2ce1fd0a1324..799a0cf75d39 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -1128,7 +1128,14 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>>>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>>>>                            char *buf)
>>>>  {
>>>> -     return sprintf(buf, "%lu\n", to_devfreq(dev)->min_freq);
>>>> +     struct devfreq *df = to_devfreq(dev);
>>>> +     unsigned long min_freq = to_devfreq(dev)->min_freq;
>>>> +     unsigned long available_min_freq = find_available_min_freq(df);
>>>> +
>>>> +     if (available_min_freq != 0 && min_freq < available_min_freq)
>>>
>>> nitpick:
>>>
>>> If available_min_freq == 0,
>>> it can't be min_freq < available_min_freq anyway;
>>> it's unsigned.
>>
>> If the dev_pm_opp_find_*() return the error in the find_available_min_freq(),
>> avaiable_min_freq is zero. So, if available_min_freq is zero,
>> min_freq_show doesn't need to compare 'min_freq < available_min_freq'.
>>
>> In result, if 'available_min_freq' is zero, min_freq_show() only considers
>> the 'min_freq' variable.
> 
> I'll modify this patch as following: How about that?
> 
> +       if (available_min_freq == 0)
> +               goto out;
> +       else if (min_freq < available_min_freq)
> +               min_freq = available_min_freq;
> +
> +out:
> +       return sprintf(buf, "%lu\n", min_freq);
> 

Please ignore this approach because
I'll use the new 'scaling_min/max_freq' variables on v4.


> 
>>>
>>>> +             min_freq = available_min_freq;
>>>> +
>>>> +     return sprintf(buf, "%lu\n", min_freq);
>>>>  }
>>>>
>>>>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>> @@ -1162,7 +1169,14 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>>  static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
>>>>                            char *buf)
>>>>  {
>>>> -     return sprintf(buf, "%lu\n", to_devfreq(dev)->max_freq);
>>>> +     struct devfreq *df = to_devfreq(dev);
>>>> +     unsigned long max_freq = to_devfreq(dev)->max_freq;
>>>> +     unsigned long available_max_freq = find_available_max_freq(df);
>>>> +
>>>> +     if (available_max_freq != 0 && max_freq > available_max_freq)
>>>> +             max_freq = available_max_freq;
>>>
>>> similar here.
>>
>> ditto.
>>
>>>
>>>> +
>>>> +     return sprintf(buf, "%lu\n", max_freq);
>>>>  }
>>>>  static DEVICE_ATTR_RW(max_freq);
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

FW: RE: [PATCH v3 1/8] PM / devfreq: Set min/max_freq when adding the devfreq device

[MyungJoo Ham]

> > Prior to that, the min/max_freq of the devfreq device are always zero
> > before the user changes the min/max_freq through sysfs entries.
> > It might make the confusion for the min/max_freq.
> > 
> > This patch initializes the available min/max_freq by using the OPP
> > during adding the devfreq device.
> > 
> > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> > ---
> >  drivers/devfreq/devfreq.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index a1c4ee818614..ae8717a6eee1 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index a1c4ee818614..ae8717a6eee1 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -69,6 +69,34 @@ static struct devfreq *find_device_devfreq(struct device *dev)
> >  	return ERR_PTR(-ENODEV);
> >  }
> >  
> > +static unsigned long find_available_min_freq(struct devfreq *devfreq)
> > +{
> > +	struct dev_pm_opp *opp;
> > +	unsigned long min_freq = 0;
> > +
> > +	opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);
> 
> When min_freq is 0, dev_pm_opp_find_freq_ceil will return ERR_PTR(-EINVAL);
> 
> --- from drivers/base/power/opp/core.c ------ 
> struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>                                              unsigned long *freq)
> {
>         struct opp_table *opp_table;
>         struct dev_pm_opp *opp;
> 
>         if (!dev || !freq) {
>                 dev_err(dev, "%s: Invalid argument freq=%p\n", __func__, freq);
>                 return ERR_PTR(-EINVAL);
>         }
> ---------------------------------------------

Sorry, please never mind. My Bad. Your code had no such issues.

So, I'm adding:
Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

> 
> Cheers,
> MyungJoo
> 

RE: [PATCH v3 4/8] PM / devfreq: Show the all available frequencies

[MyungJoo Ham]

> The commit a76caf55e5b35 ("thermal: Add devfreq cooling") allows
> the devfreq device to use the cooling device. When the cooling down
> are required, the devfreq_cooling.c disables the OPP entry with
> the dev_pm_opp_disable(). In result, 'available_frequencies'[1]
> sysfs node never came to show the all available frequencies.
> [1] /sys/class/devfreq/.../available_frequencies
> 
> So, this patch uses the 'freq_table' in the 'struct devfreq_dev_profile'
> in order to show the all available frequencies.
> - If 'freq_table' is NULL, devfreq core initializes them by using OPP values.
> - If 'freq_table' is initialized, devfreq core just uses the 'freq_table'.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 799a0cf75d39..1c4b377cacfb 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1185,22 +1185,26 @@ static ssize_t available_frequencies_show(struct device *d,
>  					  char *buf)
>  {
>  	struct devfreq *df = to_devfreq(d);
> -	struct device *dev = df->dev.parent;
> -	struct dev_pm_opp *opp;
>  	ssize_t count = 0;
> -	unsigned long freq = 0;
> +	unsigned long *freq_table;
> +	int i, max_state;
>  
> -	do {
> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> -		if (IS_ERR(opp))
> -			break;
> +	mutex_lock(&df->lock);
>  
> -		dev_pm_opp_put(opp);
> -		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> -				   "%lu ", freq);
> -		freq++;
> -	} while (1);
> +	freq_table = df->profile->freq_table;
> +	max_state = df->profile->max_state;
>  
> +	if (freq_table[0] < freq_table[max_state - 1]) {
> +		for (i = 0; i < max_state; i++)
> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +					"%lu ", freq_table[i]);
> +	} else {
> +		for (i = max_state - 1; i >= 0 ; i--)
> +			count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
> +					"%lu ", freq_table[i]);

Is there a possibilty that PC will hit here?
(If so, don't you need to worry about not being sorted at all?)

At least, devfreq_set_freq_table sorts it in ascending order.
And if you are worried about someone not using devfreq_set_freq_table
and set freq_table on their own, we'd better simple put some comments
to mandate it to be sorted in ascending order or not allowing to do it
anyway.

Cheers,
MyungJoo.

> +	}
> +
> +	mutex_unlock(&df->lock);
>  	/* Truncate the trailing space */
>  	if (count)
>  		count--;

Re: [PATCH v3 4/8] PM / devfreq: Show the all available frequencies

[Chanwoo Choi]

On Wed, Oct 11, 2017 at 8:26 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> The commit a76caf55e5b35 ("thermal: Add devfreq cooling") allows
>> the devfreq device to use the cooling device. When the cooling down
>> are required, the devfreq_cooling.c disables the OPP entry with
>> the dev_pm_opp_disable(). In result, 'available_frequencies'[1]
>> sysfs node never came to show the all available frequencies.
>> [1] /sys/class/devfreq/.../available_frequencies
>>
>> So, this patch uses the 'freq_table' in the 'struct devfreq_dev_profile'
>> in order to show the all available frequencies.
>> - If 'freq_table' is NULL, devfreq core initializes them by using OPP values.
>> - If 'freq_table' is initialized, devfreq core just uses the 'freq_table'.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 28 ++++++++++++++++------------
>>  1 file changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 799a0cf75d39..1c4b377cacfb 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -1185,22 +1185,26 @@ static ssize_t available_frequencies_show(struct device *d,
>>                                         char *buf)
>>  {
>>       struct devfreq *df = to_devfreq(d);
>> -     struct device *dev = df->dev.parent;
>> -     struct dev_pm_opp *opp;
>>       ssize_t count = 0;
>> -     unsigned long freq = 0;
>> +     unsigned long *freq_table;
>> +     int i, max_state;
>>
>> -     do {
>> -             opp = dev_pm_opp_find_freq_ceil(dev, &freq);
>> -             if (IS_ERR(opp))
>> -                     break;
>> +     mutex_lock(&df->lock);
>>
>> -             dev_pm_opp_put(opp);
>> -             count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> -                                "%lu ", freq);
>> -             freq++;
>> -     } while (1);
>> +     freq_table = df->profile->freq_table;
>> +     max_state = df->profile->max_state;
>>
>> +     if (freq_table[0] < freq_table[max_state - 1]) {
>> +             for (i = 0; i < max_state; i++)
>> +                     count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> +                                     "%lu ", freq_table[i]);
>> +     } else {
>> +             for (i = max_state - 1; i >= 0 ; i--)
>> +                     count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>> +                                     "%lu ", freq_table[i]);
>
> Is there a possibilty that PC will hit here?

As I commented on patch description, the 'freq_table' is always filled
with frequency as following description: There is no issue on both
embedded and PC.
--------
- If 'freq_table' is NULL, devfreq core initializes them by using OPP values.
- If 'freq_table' is initialized, devfreq core just uses the 'freq_table'.
--------

> (If so, don't you need to worry about not being sorted at all?)
>
> At least, devfreq_set_freq_table sorts it in ascending order.
> And if you are worried about someone not using devfreq_set_freq_table
> and set freq_table on their own, we'd better simple put some comments
> to mandate it to be sorted in ascending order or not allowing to do it
> anyway.

OK. I'll just consider the ascending order and then I'll add some comments
for that the ascending order is mandatory for 'freq_table'.

(Actually. the ARM Mali kernel driver makes the 'freq_table'
in the descending order. It doesn't matter. As you commented,
I'll add the comment about the ordering way.)

>
> Cheers,
> MyungJoo.
>
>> +     }
>> +
>> +     mutex_unlock(&df->lock);
>>       /* Truncate the trailing space */
>>       if (count)
>>               count--;

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

RE: [PATCH v3 5/8] PM / devfreq: Get the available next frequency on update_devfreq()

[MyungJoo Ham]

> The update_devfreq() considers only user frequency (min_freq/max_freq)
> and the next target_freq provided by the governor. But, the commit
> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
> OPP as a cooling device. In result, the update_devfreq() have to
> consider the 'opp->available' status in order to decicde the next freq
> by the devfreq_recommended_opp().
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1c4b377cacfb..3b9662ffe603 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>  int update_devfreq(struct devfreq *devfreq)
>  {
>  	struct devfreq_freqs freqs;
> +	struct dev_pm_opp *opp;
>  	unsigned long freq, cur_freq;
>  	int err = 0;
>  	u32 flags = 0;
> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>  		return err;
>  
>  	/*
> -	 * Adjust the frequency with user freq and QoS.
> +	 * Adjust the frequency with user freq, QoS and available freq.
>  	 *
>  	 * List from the highest priority
>  	 * max_freq
> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>  		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>  	}
>  
> +	opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +	freq = dev_pm_opp_get_freq(opp);
> +	dev_pm_opp_put(opp);
> +

Is this really necessary?

devfreq_recommended_opp is going to be called by the device driver
invoked by devfreq->profile->target() function anyway.

We are now going to call devfreq_recommended_opp twice in this context.

>  	if (devfreq->profile->get_cur_freq)
>  		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>  	else
> -- 

Re: [PATCH v3 5/8] PM / devfreq: Get the available next frequency on update_devfreq()

[Chanwoo Choi]

On Wed, Oct 11, 2017 at 8:30 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> The update_devfreq() considers only user frequency (min_freq/max_freq)
>> and the next target_freq provided by the governor. But, the commit
>> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
>> OPP as a cooling device. In result, the update_devfreq() have to
>> consider the 'opp->available' status in order to decicde the next freq
>> by the devfreq_recommended_opp().
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 1c4b377cacfb..3b9662ffe603 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>  int update_devfreq(struct devfreq *devfreq)
>>  {
>>       struct devfreq_freqs freqs;
>> +     struct dev_pm_opp *opp;
>>       unsigned long freq, cur_freq;
>>       int err = 0;
>>       u32 flags = 0;
>> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>>               return err;
>>
>>       /*
>> -      * Adjust the frequency with user freq and QoS.
>> +      * Adjust the frequency with user freq, QoS and available freq.
>>        *
>>        * List from the highest priority
>>        * max_freq
>> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>>               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>       }
>>
>> +     opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
>> +     if (IS_ERR(opp))
>> +             return PTR_ERR(opp);
>> +     freq = dev_pm_opp_get_freq(opp);
>> +     dev_pm_opp_put(opp);
>> +
>
> Is this really necessary?

The requirement is due to devfreq_cooling device using
dev_pm_opp_disable/enable().

I added the detailed explanation on cover letter as following:
If this code is not included, the notifiee using TRANSITION_NOTIFIER
receives the wrong next target_freq. On the cpufreq, cpufreq doesn't
use the 'dev_pm_opp_disable/enable()' function and then there is no
the same issue on cpufreq.

[Cover letter's description about this patch]
For example,
- devfreq's min_freq is 100Mhz and max_freq is 700Mhz.
- OPP disabled 500/600/700Mhz due to devfreq-cooling.c.
- simple_ondemand govenor decided the next target_freq (600Mhz)
|----------|-------------------------------------------------------------|
|Freq(MHz) |100     |200    |300    |400     |500     |600      |70 0    |
|Devfreq   |min_freq|       |       |        |        |         |max_freq|
|OPP avail |enabled |enabled|enabled|enabled |Disabled| Disabled|Disabled|
|Ondmenad  |        |       |       |        |        |next_freq|        |
|------------------------------------------------------------------------|

In result,
- Before this patch, target_freq is 600Mhz
  and TRANSITION_NOTIFIER sends the next_freq is 600Mhz to the notifiee.
- After this patch, target_freq is 400Mhz because 500/600 were disabled by OPP.
  and TRANSITION_NOTIFIER sends the next_freq is 400Mhz to the notifiee.
--------------

>
> devfreq_recommended_opp is going to be called by the device driver
> invoked by devfreq->profile->target() function anyway.
>
> We are now going to call devfreq_recommended_opp twice in this context.
>
>>       if (devfreq->profile->get_cur_freq)
>>               devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>>       else
>> --

Right. The devfreq_recommended_opp() is called twice.
I wish there was a better way.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 5/8] PM / devfreq: Get the available next frequency on update_devfreq()

[Chanwoo Choi]

Hi,

On 2017년 10월 11일 22:33, Chanwoo Choi wrote:
> On Wed, Oct 11, 2017 at 8:30 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>> The update_devfreq() considers only user frequency (min_freq/max_freq)
>>> and the next target_freq provided by the governor. But, the commit
>>> a76caf55e5b35 ("thermal: Add devfreq cooling") is able to disable
>>> OPP as a cooling device. In result, the update_devfreq() have to
>>> consider the 'opp->available' status in order to decicde the next freq
>>> by the devfreq_recommended_opp().
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>>  drivers/devfreq/devfreq.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 1c4b377cacfb..3b9662ffe603 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -255,6 +255,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>>  int update_devfreq(struct devfreq *devfreq)
>>>  {
>>>       struct devfreq_freqs freqs;
>>> +     struct dev_pm_opp *opp;
>>>       unsigned long freq, cur_freq;
>>>       int err = 0;
>>>       u32 flags = 0;
>>> @@ -273,7 +274,7 @@ int update_devfreq(struct devfreq *devfreq)
>>>               return err;
>>>
>>>       /*
>>> -      * Adjust the frequency with user freq and QoS.
>>> +      * Adjust the frequency with user freq, QoS and available freq.
>>>        *
>>>        * List from the highest priority
>>>        * max_freq
>>> @@ -289,6 +290,12 @@ int update_devfreq(struct devfreq *devfreq)
>>>               flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>>       }
>>>
>>> +     opp = devfreq_recommended_opp(devfreq->dev.parent, &freq, flags);
>>> +     if (IS_ERR(opp))
>>> +             return PTR_ERR(opp);
>>> +     freq = dev_pm_opp_get_freq(opp);
>>> +     dev_pm_opp_put(opp);
>>> +
>>
>> Is this really necessary?
> 
> The requirement is due to devfreq_cooling device using
> dev_pm_opp_disable/enable().

I got the better solution. If struct devfreq contains the 'scaling_min/max_freq'
variable, this issue could be fixed. I'll update it with scaling_min/max_freq'
variables on v4.

> 
> I added the detailed explanation on cover letter as following:
> If this code is not included, the notifiee using TRANSITION_NOTIFIER
> receives the wrong next target_freq. On the cpufreq, cpufreq doesn't
> use the 'dev_pm_opp_disable/enable()' function and then there is no
> the same issue on cpufreq.
> 
> [Cover letter's description about this patch]
> For example,
> - devfreq's min_freq is 100Mhz and max_freq is 700Mhz.
> - OPP disabled 500/600/700Mhz due to devfreq-cooling.c.
> - simple_ondemand govenor decided the next target_freq (600Mhz)
> |----------|-------------------------------------------------------------|
> |Freq(MHz) |100     |200    |300    |400     |500     |600      |70 0    |
> |Devfreq   |min_freq|       |       |        |        |         |max_freq|
> |OPP avail |enabled |enabled|enabled|enabled |Disabled| Disabled|Disabled|
> |Ondmenad  |        |       |       |        |        |next_freq|        |
> |------------------------------------------------------------------------|
> 
> In result,
> - Before this patch, target_freq is 600Mhz
>   and TRANSITION_NOTIFIER sends the next_freq is 600Mhz to the notifiee.
> - After this patch, target_freq is 400Mhz because 500/600 were disabled by OPP.
>   and TRANSITION_NOTIFIER sends the next_freq is 400Mhz to the notifiee.
> --------------
> 
>>
>> devfreq_recommended_opp is going to be called by the device driver
>> invoked by devfreq->profile->target() function anyway.
>>
>> We are now going to call devfreq_recommended_opp twice in this context.
>>
>>>       if (devfreq->profile->get_cur_freq)
>>>               devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
>>>       else
>>> --
> 
> Right. The devfreq_recommended_opp() is called twice.
> I wish there was a better way.
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

RE: [PATCH v3 6/8] PM / devfreq: Remove unneeded conditional statement

[MyungJoo Ham]

> The commit 0ec09ac2cebe9 ("PM / devfreq: Set the freq_table of devfreq
> device") initializes the freq_table array of each devfreq device always.
> In result, it is unneeded to check whether profile->freq_table is NULL
> or not.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 3b9662ffe603..b28810cae7ee 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -315,10 +315,9 @@ int update_devfreq(struct devfreq *devfreq)
>  	freqs.new = freq;
>  	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>  
> -	if (devfreq->profile->freq_table)
> -		if (devfreq_update_status(devfreq, freq))
> -			dev_err(&devfreq->dev,
> -				"Couldn't update frequency transition information.\n");
> +	if (devfreq_update_status(devfreq, freq))
> +		dev_err(&devfreq->dev,
> +			"Couldn't update frequency transition information.\n");

To make this commit really safe,
in this commit, please make devfreq_set_freq_table to return error
and let add_device fails when it returns error.

In the current code, we cannot guarantee that there will be
freq_table and trans_table available when it hits this line.

Cheers,
MyungJoo

>  
>  	devfreq->previous_freq = freq;
>  	return err;
> -- 
> 1.9.1

Re: [PATCH v3 6/8] PM / devfreq: Remove unneeded conditional statement

[Chanwoo Choi]

On Wed, Oct 11, 2017 at 8:36 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> The commit 0ec09ac2cebe9 ("PM / devfreq: Set the freq_table of devfreq
>> device") initializes the freq_table array of each devfreq device always.
>> In result, it is unneeded to check whether profile->freq_table is NULL
>> or not.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 3b9662ffe603..b28810cae7ee 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -315,10 +315,9 @@ int update_devfreq(struct devfreq *devfreq)
>>       freqs.new = freq;
>>       devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>
>> -     if (devfreq->profile->freq_table)
>> -             if (devfreq_update_status(devfreq, freq))
>> -                     dev_err(&devfreq->dev,
>> -                             "Couldn't update frequency transition information.\n");
>> +     if (devfreq_update_status(devfreq, freq))
>> +             dev_err(&devfreq->dev,
>> +                     "Couldn't update frequency transition information.\n");
>
> To make this commit really safe,
> in this commit, please make devfreq_set_freq_table to return error
> and let add_device fails when it returns error.
>
> In the current code, we cannot guarantee that there will be
> freq_table and trans_table available when it hits this line.

You're right. The devfreq_set_freq_table() should return error if fail.
I'll modify them on v4.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics