[3/3] irqchip/gic-pm: use devm_clk_*() helpers

[3/3] irqchip/gic-pm: use devm_clk_*() helpers

[Jon Hunter]

On 06/03/2019 11:31, Marc Zyngier wrote:
> Hi Sameer,
> 
> [unrelated to this email: anything that comes from NVIDIA reaches me
>  encrypted with my public key. Not a big deal, but it'd be good if
>  someone could fix that.]
> 
> On 06/03/2019 10:41, Sameer Pujar wrote:
>> With pm_clk_*() usage, it is seen that clocks always remain ON. This
>> happens because clocks are managed by BPMP on Tegra devices and clock
>> enable/disable happens during prepare/unprepare phase. This patch
>> avoids use of pm_clk_*() and replaces it with devm_clk_*() helpers.
>>
>> Suggested-by: Mohan Kumar D <mkumard@nvidia.com>
>> Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com>
>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
> 
> On its own, I'm not opposed to that patch.
> 
> But given that there is no in-tree platform using this, despite the code
> sitting here for more than 2.5 years, this is just updating dead code.
> 
> Am I missing anything?

Nope, but we are working to fix that at long last. I hope in the next
few months it will not longer be dormant! This driver is still very much
important to our audio support for newer Tegra devices.

Cheers
Jon

[3/3] irqchip/gic-pm: use devm_clk_*() helpers

[Marc Zyngier]

On 06/03/2019 12:00, Jon Hunter wrote:
> 
> On 06/03/2019 11:31, Marc Zyngier wrote:
>> Hi Sameer,
>> 
>> [unrelated to this email: anything that comes from NVIDIA reaches me
>>  encrypted with my public key. Not a big deal, but it'd be good if
>>  someone could fix that.]
>> 
>> On 06/03/2019 10:41, Sameer Pujar wrote:
>>> With pm_clk_*() usage, it is seen that clocks always remain ON. This
>>> happens because clocks are managed by BPMP on Tegra devices and clock
>>> enable/disable happens during prepare/unprepare phase. This patch
>>> avoids use of pm_clk_*() and replaces it with devm_clk_*() helpers.
>>>
>>> Suggested-by: Mohan Kumar D <mkumard@nvidia.com>
>>> Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com>
>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com>
>> 
>> On its own, I'm not opposed to that patch.
>> 
>> But given that there is no in-tree platform using this, despite the code
>> sitting here for more than 2.5 years, this is just updating dead code.
>> 
>> Am I missing anything?
> 
> Nope, but we are working to fix that at long last. I hope in the next
> few months it will not longer be dormant! This driver is still very much
> important to our audio support for newer Tegra devices.

Can we at least make sure it gets compiled when an NVIDIA platform
(ARCH_TEGRA?) is selected? And if this isn't relevant to the current
platforms, then hold off until it actually makes sense.

Thanks,

	M.

[3/3] irqchip/gic-pm: use devm_clk_*() helpers

[Marc Zyngier]

Hi Sameer,

[unrelated to this email: anything that comes from NVIDIA reaches me
 encrypted with my public key. Not a big deal, but it'd be good if
 someone could fix that.]

On 06/03/2019 10:41, Sameer Pujar wrote:
> With pm_clk_*() usage, it is seen that clocks always remain ON. This
> happens because clocks are managed by BPMP on Tegra devices and clock
> enable/disable happens during prepare/unprepare phase. This patch
> avoids use of pm_clk_*() and replaces it with devm_clk_*() helpers.
> 
> Suggested-by: Mohan Kumar D <mkumard@nvidia.com>
> Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com>
> Signed-off-by: Sameer Pujar <spujar@nvidia.com>

On its own, I'm not opposed to that patch.

But given that there is no in-tree platform using this, despite the code
sitting here for more than 2.5 years, this is just updating dead code.

Am I missing anything?

Thanks,

	M.

[3/3] irqchip/gic-pm: use devm_clk_*() helpers

[Sameer Pujar]

With pm_clk_*() usage, it is seen that clocks always remain ON. This
happens because clocks are managed by BPMP on Tegra devices and clock
enable/disable happens during prepare/unprepare phase. This patch
avoids use of pm_clk_*() and replaces it with devm_clk_*() helpers.

Suggested-by: Mohan Kumar D <mkumard@nvidia.com>
Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com>
Signed-off-by: Sameer Pujar <spujar@nvidia.com>
---
 drivers/irqchip/irq-gic-pm.c | 69 +++++++++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
index ecafd29..1690939 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,29 @@ 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 **clk_handle;
+};
+
 static int gic_runtime_resume(struct device *dev)
 {
-	struct gic_chip_data *gic = dev_get_drvdata(dev);
-	int ret;
+	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, i;
 
-	ret = pm_clk_resume(dev);
-	if (ret)
-		return ret;
+	for (i = 0; i < data->num_clocks; i++) {
+		ret = clk_prepare_enable(chip_pm->clk_handle[i]);
+		if (ret) {
+			while (--i >= 0)
+				clk_disable_unprepare(chip_pm->clk_handle[i]);
+
+			dev_err(dev, " clk_enable failed: %d\n", ret);
+			return ret;
+		}
+	}
 
 	/*
 	 * On the very first resume, the pointer to the driver data
@@ -54,33 +68,39 @@ 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;
+	int i;
 
 	gic_dist_save(gic);
 	gic_cpu_save(gic);
 
-	return pm_clk_suspend(dev);
+	for (i = 0; i < data->num_clocks; i++)
+		clk_disable_unprepare(chip_pm->clk_handle[i]);
+
+	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)
 {
 	unsigned int i;
-	int ret;
+	const struct gic_clk_data *data = chip_pm->clk_data;
 
 	if (!dev || !data)
 		return -EINVAL;
 
-	ret = pm_clk_create(dev);
-	if (ret)
-		return ret;
+	chip_pm->clk_handle = devm_kzalloc(dev, data->num_clocks *
+					   sizeof(struct clk *), GFP_KERNEL);
+	if (!chip_pm->clk_handle)
+		return -ENOMEM;
 
 	for (i = 0; i < data->num_clocks; i++) {
-		ret = of_pm_clk_add_clk(dev, data->clocks[i]);
-		if (ret) {
+		chip_pm->clk_handle[i] = devm_clk_get(dev, data->clocks[i]);
+		if (IS_ERR(chip_pm->clk_handle[i])) {
 			dev_err(dev, "failed to add clock %s\n",
 				data->clocks[i]);
-			pm_clk_destroy(dev);
-			return ret;
+			return PTR_ERR(chip_pm->clk_handle[i]);
 		}
 	}
 
@@ -91,14 +111,20 @@ 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 *gic_chip_pm;
 	int ret, irq;
 
+	gic_chip_pm = devm_kzalloc(dev, sizeof(*gic_chip_pm), GFP_KERNEL);
+	if (!gic_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;
 	}
+	gic_chip_pm->clk_data = data;
+	platform_set_drvdata(pdev, gic_chip_pm);
 
 	irq = irq_of_parse_and_map(dev->of_node, 0);
 	if (!irq) {
@@ -106,7 +132,7 @@ static int gic_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = gic_get_clocks(dev, data);
+	ret = gic_get_clocks(dev, gic_chip_pm);
 	if (ret)
 		goto irq_dispose;
 
@@ -116,12 +142,10 @@ static int gic_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto rpm_disable;
 
-	ret = gic_of_init_child(dev, &gic, irq);
+	ret = gic_of_init_child(dev, &gic_chip_pm->chip_data, irq);
 	if (ret)
 		goto rpm_put;
 
-	platform_set_drvdata(pdev, gic);
-
 	pm_runtime_put(dev);
 
 	dev_info(dev, "GIC IRQ controller registered\n");
@@ -132,7 +156,6 @@ static int gic_probe(struct platform_device *pdev)
 	pm_runtime_put_sync(dev);
 rpm_disable:
 	pm_runtime_disable(dev);
-	pm_clk_destroy(dev);
 irq_dispose:
 	irq_dispose_mapping(irq);