On 22/03/2019 10:53, Sameer Pujar wrote:
>
> On 3/22/2019 4:11 PM, Jon Hunter wrote:
>> On 22/03/2019 10:25, Sameer Pujar wrote:
>>> gic-pm driver is using pm-clk framework to manage clock resources, where
>>> clocks remain always ON. This happens on Tegra devices which use BPMP
>>> co-processor to manage the clocks. Calls to BPMP are always blocking and
>>> hence it is necessary to enable/disable clocks during prepare/unprepare
>>> phase respectively. When pm-clk is used, prepare count of clock is not
>>> balanced until pm_clk_remove() happens. Clock is prepared in the driver
>>> probe() and thus prepare count of clock remains non-zero, which in turn
>>> keeps clock ON always.
>>>
>>> Please note that above mentioned behavior is specific to Tegra devices
>>> using BPMP for clock management and this should not be seen on other
>>> devices. Though this patch uses clk_bulk APIs to address the mentioned
>>> behavior, this works fine for all devices.
>>>
>>> Suggested-by: Mohan Kumar D <mkumard@xxxxxxxxxx>
>>> Signed-off-by: Sameer Pujar <spujar@xxxxxxxxxx>
>>> ---
>>> drivers/irqchip/irq-gic-pm.c | 78
>>> +++++++++++++++++++++++++-------------------
>>> 1 file changed, 45 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
>>> index ecafd29..a2ef954 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,17 +27,27 @@ 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);
>>> return ret;
>>> + }
>>> /*
>>> - * On the very first resume, the pointer to the driver data
>>> + * On the very first resume, the pointer to chip_pm->chip_data
>>> * will be NULL and this is intentional, because we do not
>>> * want to restore the GIC on the very first resume. So if
>>> * the pointer is not valid just return.
>>> @@ -54,51 +63,55 @@ 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 = dev_get_drvdata(dev);
>>> + 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_kcalloc(dev, data->num_clocks,
>>> + sizeof(*chip_pm->clks), GFP_KERNEL);
>>> + if (!chip_pm->clks)
>>> + return -ENOMEM;
>>> - 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;
>>> - }
>>> - }
>>> + for (i = 0; i < data->num_clocks; i++)
>>> + chip_pm->clks[i].id = data->clocks[i];
>>> - return 0;
>>> + 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;
>>> - data = of_device_get_match_data(&pdev->dev);
>>> + if (!dev)
>>> + return -EINVAL;
>> This test is really not necessary.
> done
>>
>>> +
>>> + chip_pm = devm_kzalloc(dev, sizeof(*chip_pm), GFP_KERNEL);
>>> + if (!chip_pm)
>>> + return -ENOMEM;
>> Why not allocate this after the match? I don't think it is necessary to
>> change the sequence here and we should just allocate this once we have
>> found a match.
> done
>>> +
>>> + data = of_device_get_match_data(dev);
>>> if (!data) {
>>> - dev_err(&pdev->dev, "no device match found\n");
>>> + dev_err(dev, "no device match found\n");
>>> return -ENODEV;
>>> }
>>> + chip_pm->clk_data = data;
>>> irq = irq_of_parse_and_map(dev->of_node, 0);
>>> if (!irq) {
>>> @@ -106,7 +119,9 @@ static int gic_probe(struct platform_device *pdev)
>>> return -EINVAL;
>>> }
>>> - ret = gic_get_clocks(dev, data);
>>> + dev_set_drvdata(dev, chip_pm);
>>> +
>>> + ret = gic_get_clocks(dev);
>> Sorry to be pedantic, but I don't think that there is any value in
>> updating this function in terms of the number of parameters passed. We
>> have to the chip data so just pass it, it is simpler.
> I guess no need to pass a parameter, when it can be derived and for me it
> looked nicer this way. If you really don't like it, I can remove.
I would be tempted to get rid of gic_get_clocks altogether and just
place the code directly in probe seeing as it is simpler now.
Cheers
Jon
--
nvpublic
