| Message ID | 1553250323-15576-3-git-send-email-spujar@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | build support and fixes for gic-pm | expand |
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@nvidia.com> > Signed-off-by: Sameer Pujar <spujar@nvidia.com> > --- > 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. > + > + 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. > + > + 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. Cheers Jon
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@nvidia.com> >> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >> --- >> 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. > Cheers > Jon >
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@nvidia.com> >>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >>> --- >>> 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
On 3/22/2019 4:31 PM, Jon Hunter wrote: > 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@nvidia.com> >>>> Signed-off-by: Sameer Pujar <spujar@nvidia.com> >>>> --- >>>> 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. I think, It's fine to keep gic_get_clocks function, looks simpler this way. No harm in doing so I believe. > > Cheers > Jon >
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; + + chip_pm = devm_kzalloc(dev, sizeof(*chip_pm), GFP_KERNEL); + if (!chip_pm) + return -ENOMEM; + + 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); if (ret) goto irq_dispose; @@ -116,12 +131,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 +145,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);
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@nvidia.com> Signed-off-by: Sameer Pujar <spujar@nvidia.com> --- drivers/irqchip/irq-gic-pm.c | 78 +++++++++++++++++++++++++------------------- 1 file changed, 45 insertions(+), 33 deletions(-)