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.
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.
> +};
> +
> 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;
Nit: please place this at the top (inverted pyramid).
>
> 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);
Don't break the line in the middle of an arithmetic operation. If you
really want to respect the 80-character rule, break the line on a comma
boundary.
> + 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,21 @@ 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),
> + 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);
>
> irq = irq_of_parse_and_map(dev->of_node, 0);
> if (!irq) {
> @@ -106,7 +133,7 @@ static int gic_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - ret = gic_get_clocks(dev, data);
> + ret = gic_get_clocks(dev, chip_pm);
> if (ret)
> goto irq_dispose;
>
> @@ -116,12 +143,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, &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 +157,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);
>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
