| Message ID | 1457105585-25157-2-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
Hi Jon, On Fri, Mar 4, 2016 at 4:33 PM, Jon Hunter <jonathanh@nvidia.com> wrote: > The PM clocks framework requires clients to pass either a con-id or a > valid clk pointer in order to add a clock to a device. Add a new > function pm_clk_add_clks_of() to allows device clocks to be retrieved > from device-tree and populated for a given device. Note that there > should be no need to add a stub function for this new function when > CONFIG_OF is not selected because the OF functions used already have > stubs functions. > > In order to handle errors encountered when adding clocks from > device-tree, add a function pm_clk_remove_clk() to remove any clocks > (using a pointer to the clk structure) that have been added > successfully before the error occurred. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/base/power/clock_ops.c | 85 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/pm_clock.h | 9 +++++ > 2 files changed, 94 insertions(+) > > diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c > index 272a52ebafc0..5aa7365699c1 100644 > --- a/drivers/base/power/clock_ops.c > +++ b/drivers/base/power/clock_ops.c > @@ -138,6 +138,58 @@ int pm_clk_add_clk(struct device *dev, struct clk *clk) > } > > /** > + * pm_clk_add_clks_of - Start using device clock(s) for power management. > + * @dev: Device whose clock(s) is going to be used for power management. > + * > + * Add a series of clocks described in the 'clocks' device-tree node for > + * a device to the list of clocks used for the power management of @dev. This adds all clocks referenced by the device node, while not all clocks may be suitable for power management, and some of them may be under explicit driver control. Cfr. drivers/clk/shmobile/renesas-cpg-mssr.c:cpg_mssr_attach_dev(), which looks for module clocks, or core clocks explicitly listed in core_pm_clks[]. What about adding an optional filter function to filter for suitable clocks? > + */ > +int pm_clk_add_clks_of(struct device *dev) > +{ > + struct clk **clks; > + unsigned int i, count; > + int ret; > + > + if (!dev || !dev->of_node) > + return -EINVAL; > + > + count = of_count_phandle_with_args(dev->of_node, "clocks", > + "#clock-cells"); > + if (count == 0) > + return -ENODEV; > + > + clks = kcalloc(count, sizeof(*clks), GFP_KERNEL); > + if (!clks) > + return -ENOMEM; > + Don't you have to call pm_clk_create() here, to allocate a pm_subsys_data structure that pm_clk_add_clk() can populate? > + for (i = 0; i < count; i++) { > + clks[i] = of_clk_get(dev->of_node, i); > + if (IS_ERR(clks[i])) { > + ret = PTR_ERR(clks[i]); > + goto error; > + } > + > + ret = pm_clk_add_clk(dev, clks[i]); > + if (ret) { > + clk_put(clks[i]); > + goto error; > + } > + } > + > + kfree(clks); > + > + return 0; > + > +error: > + while (i--) > + pm_clk_remove_clk(dev, clks[i]); Just pm_clk_destroy(dev) to destroy all at once? > + > + kfree(clks); > + > + return ret; > +} Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/03/16 08:48, Geert Uytterhoeven wrote: > Hi Jon, > > On Fri, Mar 4, 2016 at 4:33 PM, Jon Hunter <jonathanh@nvidia.com> wrote: >> The PM clocks framework requires clients to pass either a con-id or a >> valid clk pointer in order to add a clock to a device. Add a new >> function pm_clk_add_clks_of() to allows device clocks to be retrieved >> from device-tree and populated for a given device. Note that there >> should be no need to add a stub function for this new function when >> CONFIG_OF is not selected because the OF functions used already have >> stubs functions. >> >> In order to handle errors encountered when adding clocks from >> device-tree, add a function pm_clk_remove_clk() to remove any clocks >> (using a pointer to the clk structure) that have been added >> successfully before the error occurred. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/base/power/clock_ops.c | 85 ++++++++++++++++++++++++++++++++++++++++++ >> include/linux/pm_clock.h | 9 +++++ >> 2 files changed, 94 insertions(+) >> >> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c >> index 272a52ebafc0..5aa7365699c1 100644 >> --- a/drivers/base/power/clock_ops.c >> +++ b/drivers/base/power/clock_ops.c >> @@ -138,6 +138,58 @@ int pm_clk_add_clk(struct device *dev, struct clk *clk) >> } >> >> /** >> + * pm_clk_add_clks_of - Start using device clock(s) for power management. >> + * @dev: Device whose clock(s) is going to be used for power management. >> + * >> + * Add a series of clocks described in the 'clocks' device-tree node for >> + * a device to the list of clocks used for the power management of @dev. > > This adds all clocks referenced by the device node, while not all clocks may > be suitable for power management, and some of them may be under explicit > driver control. > Cfr. drivers/clk/shmobile/renesas-cpg-mssr.c:cpg_mssr_attach_dev(), which > looks for module clocks, or core clocks explicitly listed in core_pm_clks[]. > > What about adding an optional filter function to filter for suitable clocks? Yes that sounds reasonable. Are you filtering on clock ID? Would it work to allow the caller to pass a filter function which takes a struct *clk as it's only argument? Or do you prefer we call of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, &clkspec) and return the clkspec info? >> + */ >> +int pm_clk_add_clks_of(struct device *dev) >> +{ >> + struct clk **clks; >> + unsigned int i, count; >> + int ret; >> + >> + if (!dev || !dev->of_node) >> + return -EINVAL; >> + >> + count = of_count_phandle_with_args(dev->of_node, "clocks", >> + "#clock-cells"); >> + if (count == 0) >> + return -ENODEV; >> + >> + clks = kcalloc(count, sizeof(*clks), GFP_KERNEL); >> + if (!clks) >> + return -ENOMEM; >> + > > Don't you have to call pm_clk_create() here, to allocate a pm_subsys_data > structure that pm_clk_add_clk() can populate? This function will return an error when pm_clk_add_clk() is called if the user has not called pm_clk_create() first. It seems more natural to me that the user should call pm_clk_create() before calling pm_clk_add_clks_of() which is the same convention used for adding clocks by the other APIs to add clocks. >> + for (i = 0; i < count; i++) { >> + clks[i] = of_clk_get(dev->of_node, i); >> + if (IS_ERR(clks[i])) { >> + ret = PTR_ERR(clks[i]); >> + goto error; >> + } >> + >> + ret = pm_clk_add_clk(dev, clks[i]); >> + if (ret) { >> + clk_put(clks[i]); >> + goto error; >> + } >> + } >> + >> + kfree(clks); >> + >> + return 0; >> + >> +error: >> + while (i--) >> + pm_clk_remove_clk(dev, clks[i]); > > Just pm_clk_destroy(dev) to destroy all at once? Right, however, this will remove all clocks associated with the device and not just those we have added. It is probably very unlikely that someone would call pm_clk_add_clks_of() and had previously called pm_clk_add(), but it seems best to not make any assumptions and only remove the clocks that were actually added. I guess that it would be possible to check to see if there were any clocks already added when this function is called and return an error in that case. All of that said, I do agree that it may be simpler, if we do call pm_clk_create() from within pm_clk_add_clks_of() and this would allow us to pmc_clk_destroy() in the error path. So if this is preferred I could do that, but I think that I would not allow clocks to be added if clock_list is not empty when this function is called. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jon, On Mon, Mar 7, 2016 at 11:45 AM, Jon Hunter <jonathanh@nvidia.com> wrote: > On 07/03/16 08:48, Geert Uytterhoeven wrote: >> On Fri, Mar 4, 2016 at 4:33 PM, Jon Hunter <jonathanh@nvidia.com> wrote: >>> The PM clocks framework requires clients to pass either a con-id or a >>> valid clk pointer in order to add a clock to a device. Add a new >>> function pm_clk_add_clks_of() to allows device clocks to be retrieved >>> from device-tree and populated for a given device. Note that there >>> should be no need to add a stub function for this new function when >>> CONFIG_OF is not selected because the OF functions used already have >>> stubs functions. >>> >>> In order to handle errors encountered when adding clocks from >>> device-tree, add a function pm_clk_remove_clk() to remove any clocks >>> (using a pointer to the clk structure) that have been added >>> successfully before the error occurred. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> --- >>> drivers/base/power/clock_ops.c | 85 ++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/pm_clock.h | 9 +++++ >>> 2 files changed, 94 insertions(+) >>> >>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c >>> index 272a52ebafc0..5aa7365699c1 100644 >>> --- a/drivers/base/power/clock_ops.c >>> +++ b/drivers/base/power/clock_ops.c >>> @@ -138,6 +138,58 @@ int pm_clk_add_clk(struct device *dev, struct clk *clk) >>> } >>> >>> /** >>> + * pm_clk_add_clks_of - Start using device clock(s) for power management. >>> + * @dev: Device whose clock(s) is going to be used for power management. >>> + * >>> + * Add a series of clocks described in the 'clocks' device-tree node for >>> + * a device to the list of clocks used for the power management of @dev. >> >> This adds all clocks referenced by the device node, while not all clocks may >> be suitable for power management, and some of them may be under explicit >> driver control. >> Cfr. drivers/clk/shmobile/renesas-cpg-mssr.c:cpg_mssr_attach_dev(), which >> looks for module clocks, or core clocks explicitly listed in core_pm_clks[]. >> >> What about adding an optional filter function to filter for suitable clocks? > > Yes that sounds reasonable. Are you filtering on clock ID? Would it work > to allow the caller to pass a filter function which takes a struct *clk > as it's only argument? Or do you prefer we call > of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, &clkspec) > and return the clkspec info? cpg_mssr_attach_dev() filters on clkspec. As pm_clk_add_clks_of() has "_of" in the name (BTW, should it be called of_pm_clk_add_clks()), I think passing the clkspec makes more sense than passing a struct clk *. >>> +int pm_clk_add_clks_of(struct device *dev) >>> +{ >>> + struct clk **clks; >>> + unsigned int i, count; >>> + int ret; >>> + >>> + if (!dev || !dev->of_node) >>> + return -EINVAL; >>> + >>> + count = of_count_phandle_with_args(dev->of_node, "clocks", >>> + "#clock-cells"); >>> + if (count == 0) >>> + return -ENODEV; >>> + >>> + clks = kcalloc(count, sizeof(*clks), GFP_KERNEL); >>> + if (!clks) >>> + return -ENOMEM; >>> + >> >> Don't you have to call pm_clk_create() here, to allocate a pm_subsys_data >> structure that pm_clk_add_clk() can populate? > > This function will return an error when pm_clk_add_clk() is called if > the user has not called pm_clk_create() first. It seems more natural to > me that the user should call pm_clk_create() before calling > pm_clk_add_clks_of() which is the same convention used for adding clocks > by the other APIs to add clocks. OK. That way the caller can add more clocks if needed. I missed that case. >>> + for (i = 0; i < count; i++) { >>> + clks[i] = of_clk_get(dev->of_node, i); >>> + if (IS_ERR(clks[i])) { >>> + ret = PTR_ERR(clks[i]); >>> + goto error; >>> + } >>> + >>> + ret = pm_clk_add_clk(dev, clks[i]); >>> + if (ret) { >>> + clk_put(clks[i]); >>> + goto error; >>> + } >>> + } >>> + >>> + kfree(clks); >>> + >>> + return 0; >>> + >>> +error: >>> + while (i--) >>> + pm_clk_remove_clk(dev, clks[i]); >> >> Just pm_clk_destroy(dev) to destroy all at once? > > Right, however, this will remove all clocks associated with the device > and not just those we have added. It is probably very unlikely that > someone would call pm_clk_add_clks_of() and had previously called > pm_clk_add(), but it seems best to not make any assumptions and only > remove the clocks that were actually added. I guess that it would be > possible to check to see if there were any clocks already added when > this function is called and return an error in that case. > > All of that said, I do agree that it may be simpler, if we do call > pm_clk_create() from within pm_clk_add_clks_of() and this would allow us > to pmc_clk_destroy() in the error path. So if this is preferred I could > do that, but I think that I would not allow clocks to be added if > clock_list is not empty when this function is called. Given the above, your code is fine. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index 272a52ebafc0..5aa7365699c1 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -138,6 +138,58 @@ int pm_clk_add_clk(struct device *dev, struct clk *clk) } /** + * pm_clk_add_clks_of - Start using device clock(s) for power management. + * @dev: Device whose clock(s) is going to be used for power management. + * + * Add a series of clocks described in the 'clocks' device-tree node for + * a device to the list of clocks used for the power management of @dev. + */ +int pm_clk_add_clks_of(struct device *dev) +{ + struct clk **clks; + unsigned int i, count; + int ret; + + if (!dev || !dev->of_node) + return -EINVAL; + + count = of_count_phandle_with_args(dev->of_node, "clocks", + "#clock-cells"); + if (count == 0) + return -ENODEV; + + clks = kcalloc(count, sizeof(*clks), GFP_KERNEL); + if (!clks) + return -ENOMEM; + + for (i = 0; i < count; i++) { + clks[i] = of_clk_get(dev->of_node, i); + if (IS_ERR(clks[i])) { + ret = PTR_ERR(clks[i]); + goto error; + } + + ret = pm_clk_add_clk(dev, clks[i]); + if (ret) { + clk_put(clks[i]); + goto error; + } + } + + kfree(clks); + + return 0; + +error: + while (i--) + pm_clk_remove_clk(dev, clks[i]); + + kfree(clks); + + return ret; +} + +/** * __pm_clk_remove - Destroy PM clock entry. * @ce: PM clock entry to destroy. */ @@ -198,6 +250,39 @@ void pm_clk_remove(struct device *dev, const char *con_id) } /** + * pm_clk_remove_clk - Stop using a device clock for power management. + * @dev: Device whose clock should not be used for PM any more. + * @clk: Clock pointer + * + * Remove the clock pointed to by @clk from the list of clocks used for + * the power management of @dev. + */ +void pm_clk_remove_clk(struct device *dev, struct clk *clk) +{ + struct pm_subsys_data *psd = dev_to_psd(dev); + struct pm_clock_entry *ce; + + if (!psd || !clk) + return; + + spin_lock_irq(&psd->lock); + + list_for_each_entry(ce, &psd->clock_list, node) { + if (clk == ce->clk) + goto remove; + } + + spin_unlock_irq(&psd->lock); + return; + + remove: + list_del(&ce->node); + spin_unlock_irq(&psd->lock); + + __pm_clk_remove(ce); +} + +/** * pm_clk_init - Initialize a device's list of power management clocks. * @dev: Device to initialize the list of PM clocks for. * diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h index ffefbf2df1aa..6f609226bd85 100644 --- a/include/linux/pm_clock.h +++ b/include/linux/pm_clock.h @@ -42,7 +42,9 @@ extern int pm_clk_create(struct device *dev); extern void pm_clk_destroy(struct device *dev); extern int pm_clk_add(struct device *dev, const char *con_id); extern int pm_clk_add_clk(struct device *dev, struct clk *clk); +extern int pm_clk_add_clks_of(struct device *dev); extern void pm_clk_remove(struct device *dev, const char *con_id); +extern void pm_clk_remove_clk(struct device *dev, struct clk *clk); extern int pm_clk_suspend(struct device *dev); extern int pm_clk_resume(struct device *dev); #else @@ -69,9 +71,16 @@ static inline int pm_clk_add_clk(struct device *dev, struct clk *clk) { return -EINVAL; } +static inline int pm_clk_add_clks_of(struct device *dev) +{ + return -EINVAL; +} static inline void pm_clk_remove(struct device *dev, const char *con_id) { } +static inline void pm_clk_remove_clk(struct device *dev, struct clk *clk) +{ +} static inline int pm_clk_suspend(struct device *dev) { return -EINVAL;
The PM clocks framework requires clients to pass either a con-id or a valid clk pointer in order to add a clock to a device. Add a new function pm_clk_add_clks_of() to allows device clocks to be retrieved from device-tree and populated for a given device. Note that there should be no need to add a stub function for this new function when CONFIG_OF is not selected because the OF functions used already have stubs functions. In order to handle errors encountered when adding clocks from device-tree, add a function pm_clk_remove_clk() to remove any clocks (using a pointer to the clk structure) that have been added successfully before the error occurred. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/base/power/clock_ops.c | 85 ++++++++++++++++++++++++++++++++++++++++++ include/linux/pm_clock.h | 9 +++++ 2 files changed, 94 insertions(+)