| Message ID | 1553182415-11269-3-git-send-email-spujar@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | build support and fixes for gic-pm | expand |
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@nvidia.com> > Signed-off-by: Sameer Pujar <spujar@nvidia.com> > Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com> 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. Cheers Jon
On 3/21/2019 10:01 PM, Jon Hunter wrote: > 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@nvidia.com> >> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >> Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com> > 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. This is moved up to make sure clk_data in chip_pm is populated by the time gic_get_clocks() is called. The comment in runtime_resume() would still hold good, gic would be NULL till gic_of_init_child() is called. May be the comment needs to be slightly updated, because chip_pm is the new driver data now. > > Cheers > Jon >
On 21/03/2019 17:23, Sameer Pujar wrote: ... >>> 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. > This is moved up to make sure clk_data in chip_pm is populated by the time > gic_get_clocks() is called. The comment in runtime_resume() would still > hold > good, gic would be NULL till gic_of_init_child() is called. May be the > comment > needs to be slightly updated, because chip_pm is the new driver data now. I see, but I am not sure it is necessary as you are directly passing chip_pm to gic_get_clocks(). Cheers Jon
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@nvidia.com> > Signed-off-by: Sameer Pujar <spujar@nvidia.com> > Reviewed-by: Jonathan Hunter <jonathanh@nvidia.com> The $subject is only relevant to Tegra. For other devices this may not be a problem. However, the $subject implies there is a bug in the driver where really there is not. Furthermore, devm_clk is only used for allocating clocks and has nothing to do with clock management and so it is still a bit confusing. I would suggest that you change the $subject to simply 'update driver to use clk_bulk APIs' or 'update driver not to use pm-clk APIs' and let the changelog describe why we are doing this. Cheers Jon
On 3/21/2019 11:03 PM, Jon Hunter wrote: > On 21/03/2019 17:23, Sameer Pujar wrote: > > ... > > >>> 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. >> This is moved up to make sure clk_data in chip_pm is populated by the time >> gic_get_clocks() is called. The comment in runtime_resume() would still >> hold >> good, gic would be NULL till gic_of_init_child() is called. May be the >> comment >> needs to be slightly updated, because chip_pm is the new driver data now. > I see, but I am not sure it is necessary as you are directly passing > chip_pm to gic_get_clocks(). Ah yes, chip_pm need not be passed for gic_get_clocks(). But driver data needs to be set before runtime_resume() happens. > Cheers > Jon >
On 22/03/2019 04:37, Sameer Pujar wrote: > > On 3/21/2019 11:03 PM, Jon Hunter wrote: >> On 21/03/2019 17:23, Sameer Pujar wrote: >> >> ... >> >> >>> 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. >>> This is moved up to make sure clk_data in chip_pm is populated by the >>> time >>> gic_get_clocks() is called. The comment in runtime_resume() would still >>> hold >>> good, gic would be NULL till gic_of_init_child() is called. May be the >>> comment >>> needs to be slightly updated, because chip_pm is the new driver data >>> now. >> I see, but I am not sure it is necessary as you are directly passing >> chip_pm to gic_get_clocks(). > Ah yes, chip_pm need not be passed for gic_get_clocks(). But driver data > needs > to be set before runtime_resume() happens. OK yes indeed. Then that should be fine. Thanks Jon
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); 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); - 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]; + + 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), + 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 +123,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 +133,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 +147,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);