On 21/03/2019 15:33, Sameer Pujar wrote:
gic-pm driver is using pm_clk_*() interface to manage clock resources. With
this, clocks always remain ON. This happens on Tegra devices which use BPMP
co-processor to manage the clocks, where clocks are enabled during prepare
phase. This is necessary because calls to BPMP are always blocking. When
pm_clk_*() interface is used on such devices, clock prepare count is not
balanced till driver remove() gets executed and hence clocks are seen ON
always. This patch helps to keep clock ref counts balanced and thus clocks
are off, when device not in use.
Please note that this works for any device and the fix is not specific to
Tegra devices.
Suggested-by: Mohan Kumar D <mkumard@xxxxxxxxxx>
Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx>
Reviewed-by: Jonathan Hunter <jonathanh@xxxxxxxxxx>
Not yet!
---
drivers/irqchip/irq-gic-pm.c | 68 ++++++++++++++++++++++++++------------------
1 file changed, 41 insertions(+), 27 deletions(-)
diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index ecafd29..b557a53 100644
--- a/drivers/irqchip/irq-gic-pm.c
+++ b/drivers/irqchip/irq-gic-pm.c
@@ -19,7 +19,6 @@
#include <linux/of_irq.h>
#include <linux/irqchip/arm-gic.h>
#include <linux/platform_device.h>
-#include <linux/pm_clock.h>
#include <linux/pm_runtime.h>
#include <linux/slab.h>
@@ -28,14 +27,24 @@ struct gic_clk_data {
const char *const *clocks;
};
+struct gic_chip_pm {
+ struct gic_chip_data *chip_data;
+ const struct gic_clk_data *clk_data;
+ struct clk_bulk_data *clks;
+};
+
static int gic_runtime_resume(struct device *dev)
{
- struct gic_chip_data *gic = dev_get_drvdata(dev);
+ struct gic_chip_pm *chip_pm = dev_get_drvdata(dev);
+ struct gic_chip_data *gic = chip_pm->chip_data;
+ const struct gic_clk_data *data = chip_pm->clk_data;
int ret;
- ret = pm_clk_resume(dev);
- if (ret)
+ ret = clk_bulk_prepare_enable(data->num_clocks, chip_pm->clks);
+ if (ret) {
+ dev_err(dev, " clk_enable failed: %d\n", ret);
Unnecessary space here.
return ret;
+ }
/*
* On the very first resume, the pointer to the driver data
@@ -54,51 +63,59 @@ static int gic_runtime_resume(struct device *dev)
static int gic_runtime_suspend(struct device *dev)
{
- struct gic_chip_data *gic = dev_get_drvdata(dev);
+ struct gic_chip_pm *chip_pm = dev_get_drvdata(dev);
+ struct gic_chip_data *gic = chip_pm->chip_data;
+ const struct gic_clk_data *data = chip_pm->clk_data;
gic_dist_save(gic);
gic_cpu_save(gic);
- return pm_clk_suspend(dev);
+ clk_bulk_disable_unprepare(data->num_clocks, chip_pm->clks);
+
+ return 0;
}
-static int gic_get_clocks(struct device *dev, const struct gic_clk_data *data)
+static int gic_get_clocks(struct device *dev, struct gic_chip_pm *chip_pm)
{
+ const struct gic_clk_data *data = chip_pm->clk_data;
unsigned int i;
- int ret;
if (!dev || !data)
return -EINVAL;
- ret = pm_clk_create(dev);
- if (ret)
- return ret;
+ chip_pm->clks =
+ devm_kzalloc(dev,
+ data->num_clocks * sizeof(struct clk_bulk_data),
+ GFP_KERNEL);
Ugh, please sort out the formatting here. No reason why devm_kzalloc
cannot start on the same line as chip_pm->clks. You should use
devm_kcalloc here and for sizeof use sizeof(*chip_pm->clks).
- for (i = 0; i < data->num_clocks; i++) {
- ret = of_pm_clk_add_clk(dev, data->clocks[i]);
- if (ret) {
- dev_err(dev, "failed to add clock %s\n",
- data->clocks[i]);
- pm_clk_destroy(dev);
- return ret;
- }
- }
+ if (!chip_pm->clks)
+ return -ENOMEM;
- return 0;
+ for (i = 0; i < data->num_clocks; i++)
+ chip_pm->clks[i].id = data->clocks[i];
Hmm ... that's unfortunate but I don't have a better idea to avoid this :-(
+
+ return devm_clk_bulk_get(dev, data->num_clocks, chip_pm->clks);
}
static int gic_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
const struct gic_clk_data *data;
- struct gic_chip_data *gic;
+ struct gic_chip_pm *chip_pm;
int ret, irq;
+ chip_pm = devm_kzalloc(dev, sizeof(struct gic_chip_pm),
sizeof(*chip_pm)
+ GFP_KERNEL);
+ if (!chip_pm)
+ return -ENOMEM;
+
data = of_device_get_match_data(&pdev->dev);
if (!data) {
dev_err(&pdev->dev, "no device match found\n");
return -ENODEV;
}
+ chip_pm->clk_data = data;
+ platform_set_drvdata(pdev, chip_pm);
So why has this been moved? There is a comment in gic_runtime_resume()
on why this was set after pm_runtime_get_sync() in probe.
