Re: [PATCH 05/13] OMAP: Introduce device scale implementation
Vishwanath Sripathy vishwanath...@ti.com writes:
-Original Message-
From: Kevin Hilman [mailto:khil...@ti.com]
Sent: Friday, February 04, 2011 9:35 PM
To: Vishwanath BS
Cc: linux-omap@vger.kernel.org; patc...@linaro.org; Thara Gopinath
Subject: Re: [PATCH 05/13] OMAP: Introduce device scale
implementation
Vishwanath BS vishwanath...@ti.com writes:
This patch adds omap_device_scale API which can be used to generic
device rate scaling.
I would've expected a new omap_device_* API to be part of the
omap_device layer, not added here.
Given that this API does not deal with any of the omap_device layer
functions or data structures, I am not sure if it logically belongs to
omap device layer. If you think it should belong to omap_device layer just
because the name starts with omap_device, I would rather rename this API.
That's up to you.
But if it's not an omap_device API, then it shouldn't be named
omap_device_*
Kevin
Based on original patch from Thara.
Signed-off-by: Vishwanath BS vishwanath...@ti.com
Cc: Thara Gopinath th...@ti.com
---
arch/arm/mach-omap2/dvfs.c | 116
arch/arm/plat-omap/include/plat/dvfs.h |7 ++
2 files changed, 123 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/dvfs.c b/arch/arm/mach-
omap2/dvfs.c
index c9d3894..05a9ce3 100755
--- a/arch/arm/mach-omap2/dvfs.c
+++ b/arch/arm/mach-omap2/dvfs.c
@@ -19,6 +19,7 @@
#include plat/common.h
#include plat/voltage.h
#include plat/omap_device.h
+#include plat/smartreflex.h
/**
* struct omap_dev_user_list - Structure maitain userlist per device
@@ -585,6 +586,121 @@ static int omap_dvfs_voltage_scale(struct
omap_vdd_dvfs_info *dvfs_info)
}
/**
+ * omap_device_scale() - Set a new rate at which the device is to
operate
+ * @req_dev: pointer to the device requesting the scaling.
+ * @target_dev: pointer to the device that is to be scaled
+ * @rate: the rnew rate for the device.
+ *
+ * This API gets the device opp table associated with this device and
+ * tries putting the device to the requested rate and the voltage
domain
+ * associated with the device to the voltage corresponding to the
+ * requested rate. Since multiple devices can be assocciated with a
+ * voltage domain this API finds out the possible voltage the
+ * voltage domain can enter and then decides on the final device
+ * rate. Return 0 on success else the error value
+ */
Here would be a good place to describe why both the requesting device
and the target device need to be tracked.
OK
+int omap_device_scale(struct device *req_dev, struct device
*target_dev,
+ unsigned long rate)
+{
+ struct opp *opp;
+ unsigned long volt, freq, min_freq, max_freq;
+ struct omap_vdd_dvfs_info *dvfs_info;
+ struct platform_device *pdev;
+ struct omap_device *od;
+ int ret = 0;
+
+ pdev = container_of(target_dev, struct platform_device, dev);
+ od = container_of(pdev, struct omap_device, pdev);
+
+ /*
+ * Figure out if the desired frquency lies between the
+ * maximum and minimum possible for the particular device
+ */
+ min_freq = 0;
+ if (IS_ERR(opp_find_freq_ceil(target_dev, min_freq))) {
+ dev_err(target_dev, %s: Unable to find lowest opp\n,
+ __func__);
+ return -ENODEV;
+ }
+
+ max_freq = ULONG_MAX;
+ if (IS_ERR(opp_find_freq_floor(target_dev, max_freq))) {
+ dev_err(target_dev, %s: Unable to find highest opp\n,
+ __func__);
+ return -ENODEV;
+ }
+
+ if (rate min_freq)
+ freq = min_freq;
+ else if (rate max_freq)
+ freq = max_freq;
+ else
+ freq = rate;
+
OK, frome here on, I would expect the adjusted value 'freq' to be used
instead of 'rate', but that is not the case below.
+ opp = opp_find_freq_ceil(target_dev, freq);
+ if (IS_ERR(opp)) {
+ dev_err(target_dev, %s: Unable to find OPP for
freq%ld\n,
+ __func__, rate);
+ return -ENODEV;
+ }
Freq is appropriate than rate here. Will fix it.
+
+ /* Get the voltage corresponding to the requested frequency */
+ volt = opp_get_voltage(opp);
+
+ /*
+ * Call into the voltage layer to get the final voltage possible
+ * for the voltage domain associated with the device.
+ */
This comment doesn't match the following code.
OK. Will fix it. Copy paste error.
+ if (rate) {
Why is rate used here, and not freq?
Freq can never be 0. If somebody wants to remove his DVFS request (he does
not really care about the device frequency), then he can pass rate as 0.
Hence rate is used.
+ dvfs_info = get_dvfs_info(od-hwmods[0]-voltdm);
+
+ ret = omap_dvfs_add_freq_request(dvfs_info, req_dev
RE: [PATCH 05/13] OMAP: Introduce device scale implementation
-Original Message-
From: Kevin Hilman [mailto:khil...@ti.com]
Sent: Friday, February 04, 2011 9:35 PM
To: Vishwanath BS
Cc: linux-omap@vger.kernel.org; patc...@linaro.org; Thara Gopinath
Subject: Re: [PATCH 05/13] OMAP: Introduce device scale
implementation
Vishwanath BS vishwanath...@ti.com writes:
This patch adds omap_device_scale API which can be used to generic
device rate scaling.
I would've expected a new omap_device_* API to be part of the
omap_device layer, not added here.
Given that this API does not deal with any of the omap_device layer
functions or data structures, I am not sure if it logically belongs to
omap device layer. If you think it should belong to omap_device layer just
because the name starts with omap_device, I would rather rename this API.
Based on original patch from Thara.
Signed-off-by: Vishwanath BS vishwanath...@ti.com
Cc: Thara Gopinath th...@ti.com
---
arch/arm/mach-omap2/dvfs.c | 116
arch/arm/plat-omap/include/plat/dvfs.h |7 ++
2 files changed, 123 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/dvfs.c b/arch/arm/mach-
omap2/dvfs.c
index c9d3894..05a9ce3 100755
--- a/arch/arm/mach-omap2/dvfs.c
+++ b/arch/arm/mach-omap2/dvfs.c
@@ -19,6 +19,7 @@
#include plat/common.h
#include plat/voltage.h
#include plat/omap_device.h
+#include plat/smartreflex.h
/**
* struct omap_dev_user_list - Structure maitain userlist per device
@@ -585,6 +586,121 @@ static int omap_dvfs_voltage_scale(struct
omap_vdd_dvfs_info *dvfs_info)
}
/**
+ * omap_device_scale() - Set a new rate at which the device is to
operate
+ * @req_dev: pointer to the device requesting the scaling.
+ * @target_dev:pointer to the device that is to be scaled
+ * @rate: the rnew rate for the device.
+ *
+ * This API gets the device opp table associated with this device and
+ * tries putting the device to the requested rate and the voltage
domain
+ * associated with the device to the voltage corresponding to the
+ * requested rate. Since multiple devices can be assocciated with a
+ * voltage domain this API finds out the possible voltage the
+ * voltage domain can enter and then decides on the final device
+ * rate. Return 0 on success else the error value
+ */
Here would be a good place to describe why both the requesting device
and the target device need to be tracked.
OK
+int omap_device_scale(struct device *req_dev, struct device
*target_dev,
+ unsigned long rate)
+{
+ struct opp *opp;
+ unsigned long volt, freq, min_freq, max_freq;
+ struct omap_vdd_dvfs_info *dvfs_info;
+ struct platform_device *pdev;
+ struct omap_device *od;
+ int ret = 0;
+
+ pdev = container_of(target_dev, struct platform_device, dev);
+ od = container_of(pdev, struct omap_device, pdev);
+
+ /*
+* Figure out if the desired frquency lies between the
+* maximum and minimum possible for the particular device
+*/
+ min_freq = 0;
+ if (IS_ERR(opp_find_freq_ceil(target_dev, min_freq))) {
+ dev_err(target_dev, %s: Unable to find lowest opp\n,
+ __func__);
+ return -ENODEV;
+ }
+
+ max_freq = ULONG_MAX;
+ if (IS_ERR(opp_find_freq_floor(target_dev, max_freq))) {
+ dev_err(target_dev, %s: Unable to find highest opp\n,
+ __func__);
+ return -ENODEV;
+ }
+
+ if (rate min_freq)
+ freq = min_freq;
+ else if (rate max_freq)
+ freq = max_freq;
+ else
+ freq = rate;
+
OK, frome here on, I would expect the adjusted value 'freq' to be used
instead of 'rate', but that is not the case below.
+ opp = opp_find_freq_ceil(target_dev, freq);
+ if (IS_ERR(opp)) {
+ dev_err(target_dev, %s: Unable to find OPP for
freq%ld\n,
+ __func__, rate);
+ return -ENODEV;
+ }
Freq is appropriate than rate here. Will fix it.
+
+ /* Get the voltage corresponding to the requested frequency */
+ volt = opp_get_voltage(opp);
+
+ /*
+* Call into the voltage layer to get the final voltage possible
+* for the voltage domain associated with the device.
+*/
This comment doesn't match the following code.
OK. Will fix it. Copy paste error.
+ if (rate) {
Why is rate used here, and not freq?
Freq can never be 0. If somebody wants to remove his DVFS request (he does
not really care about the device frequency), then he can pass rate as 0.
Hence rate is used.
+ dvfs_info = get_dvfs_info(od-hwmods[0]-voltdm);
+
+ ret = omap_dvfs_add_freq_request(dvfs_info, req_dev,
+ target_dev, freq);
+ if (ret) {
+ dev_err
Re: [PATCH 05/13] OMAP: Introduce device scale implementation
Vishwanath BS vishwanath...@ti.com writes:
This patch adds omap_device_scale API which can be used to generic
device rate scaling.
I would've expected a new omap_device_* API to be part of the
omap_device layer, not added here.
Based on original patch from Thara.
Signed-off-by: Vishwanath BS vishwanath...@ti.com
Cc: Thara Gopinath th...@ti.com
---
arch/arm/mach-omap2/dvfs.c | 116
arch/arm/plat-omap/include/plat/dvfs.h |7 ++
2 files changed, 123 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/dvfs.c b/arch/arm/mach-omap2/dvfs.c
index c9d3894..05a9ce3 100755
--- a/arch/arm/mach-omap2/dvfs.c
+++ b/arch/arm/mach-omap2/dvfs.c
@@ -19,6 +19,7 @@
#include plat/common.h
#include plat/voltage.h
#include plat/omap_device.h
+#include plat/smartreflex.h
/**
* struct omap_dev_user_list - Structure maitain userlist per device
@@ -585,6 +586,121 @@ static int omap_dvfs_voltage_scale(struct
omap_vdd_dvfs_info *dvfs_info)
}
/**
+ * omap_device_scale() - Set a new rate at which the device is to operate
+ * @req_dev: pointer to the device requesting the scaling.
+ * @target_dev: pointer to the device that is to be scaled
+ * @rate:the rnew rate for the device.
+ *
+ * This API gets the device opp table associated with this device and
+ * tries putting the device to the requested rate and the voltage domain
+ * associated with the device to the voltage corresponding to the
+ * requested rate. Since multiple devices can be assocciated with a
+ * voltage domain this API finds out the possible voltage the
+ * voltage domain can enter and then decides on the final device
+ * rate. Return 0 on success else the error value
+ */
Here would be a good place to describe why both the requesting device
and the target device need to be tracked.
+int omap_device_scale(struct device *req_dev, struct device *target_dev,
+ unsigned long rate)
+{
+ struct opp *opp;
+ unsigned long volt, freq, min_freq, max_freq;
+ struct omap_vdd_dvfs_info *dvfs_info;
+ struct platform_device *pdev;
+ struct omap_device *od;
+ int ret = 0;
+
+ pdev = container_of(target_dev, struct platform_device, dev);
+ od = container_of(pdev, struct omap_device, pdev);
+
+ /*
+ * Figure out if the desired frquency lies between the
+ * maximum and minimum possible for the particular device
+ */
+ min_freq = 0;
+ if (IS_ERR(opp_find_freq_ceil(target_dev, min_freq))) {
+ dev_err(target_dev, %s: Unable to find lowest opp\n,
+ __func__);
+ return -ENODEV;
+ }
+
+ max_freq = ULONG_MAX;
+ if (IS_ERR(opp_find_freq_floor(target_dev, max_freq))) {
+ dev_err(target_dev, %s: Unable to find highest opp\n,
+ __func__);
+ return -ENODEV;
+ }
+
+ if (rate min_freq)
+ freq = min_freq;
+ else if (rate max_freq)
+ freq = max_freq;
+ else
+ freq = rate;
+
OK, frome here on, I would expect the adjusted value 'freq' to be used
instead of 'rate', but that is not the case below.
+ opp = opp_find_freq_ceil(target_dev, freq);
+ if (IS_ERR(opp)) {
+ dev_err(target_dev, %s: Unable to find OPP for freq%ld\n,
+ __func__, rate);
+ return -ENODEV;
+ }
+
+ /* Get the voltage corresponding to the requested frequency */
+ volt = opp_get_voltage(opp);
+
+ /*
+ * Call into the voltage layer to get the final voltage possible
+ * for the voltage domain associated with the device.
+ */
This comment doesn't match the following code.
+ if (rate) {
Why is rate used here, and not freq?
+ dvfs_info = get_dvfs_info(od-hwmods[0]-voltdm);
+
+ ret = omap_dvfs_add_freq_request(dvfs_info, req_dev,
+ target_dev, freq);
+ if (ret) {
+ dev_err(target_dev, %s: Unable to add frequency
request\n,
+ __func__);
+ return ret;
+ }
+
+ ret = omap_dvfs_add_vdd_user(dvfs_info, req_dev, volt);
+ if (ret) {
+ dev_err(target_dev, %s: Unable to add voltage
request\n,
+ __func__);
+ omap_dvfs_remove_freq_request(dvfs_info, req_dev,
+ target_dev);
+ return ret;
+ }
+ } else {
The function comments don't describe this case. Namely, that if you
pass in rate = 0, it removes any previous requests for the requesting
device.
+ dvfs_info = get_dvfs_info(od-hwmods[0]-voltdm);
+
+ ret = omap_dvfs_remove_freq_request(dvfs_info,
