On 20/03/2019 18:08, Marc Zyngier wrote:
> On Thu, 14 Mar 2019 13:23:13 +0530
> Sameer Pujar <spujar@xxxxxxxxxx> 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 replaces pm_clk_*() with devm_clk_*() framework.
>
> Does this mean this code effectively becomes Tegra specific, bypassing
> the existing PM framework to workaround implementation-specific issues?
> I don't really mind (Tegra is apparently the only user), but I'd rather
> make it sure I understand what is going on.
No not at all. It is still generic and not Tegra specific, but for newer
Tegra devices we cannot use the pm-clk framework and so use the clk
framework directly. So this still works for any device. Consider it a
lower common denominator.
> Also, I object to the subject line. This is not about switching to some
> fancy helpers. This is about changing the life-cycle of clocks, which
> is entirely different. Please make sure the subject reflects what the
> patch is doing.
>
> Otherwise, there isn't much to say about the code. A couple of very
> minor nits below:
>
>>
>> Suggested-by: Mohan Kumar D <mkumard@xxxxxxxxxx>
>> Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx>
>> Reviewed-by: Jonathan Hunter <jonathanh@xxxxxxxxxx>
>> ---
>> drivers/irqchip/irq-gic-pm.c | 70 +++++++++++++++++++++++++++++---------------
>> 1 file changed, 47 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
>> index ecafd29..ae6c8d6 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;
>
> It is really ugly that clk_handle cannot be bundled with clk_data.
> Oh well.
I wonder if we should use clk_bulk_data here and then use the clk_bulk
APIs below? It would simplify the code and given that the clocks are
well-defined for the GIC (ie. for a given version we know what clocks
are required) I don't see any harm in doing so.
>
>> +};
>> +
>> 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;
>> + }
>> + }
Using the clk_bulk APIs, this would become a single call to
clk_bulk_prepare_enable().
Cheers
Jon
--
nvpublic
