| Message ID | 1437549069-29655-4-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 07/22/2015 12:10 AM, Rajendra Nayak wrote: > @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain *domain) > return gdsc_toggle_logic(sc, false); > } > > +static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev) > +{ > + int ret; > + struct gdsc *sc = domain_to_gdsc(domain); > + char **con_id, *con_ids[] = { "core", "iface", NULL }; const? This is where I get scared of sniffing too much SoC glue. What's to enforce the "core", and "iface" naming scheme? What's to enforce there being two clocks vs. one? Maybe a better approach would be to use of_clk_get() and iterate through all clocks of the device, or to encode the clock names in the gdsc structure. The problem I'm getting at is that we're going through the consumer struct device's mapping of names to clks when we're inside this SoC glue code that has to know about what the consumer has decided to do. This code becomes tightly coupled with that decision that doesn't seem to be under the glue code's control. Using of_clk_get() sidesteps that problem, with the loss of flexibility of deciding which clock does what so at least it's a step in the right direction. But if we want to control individual clocks then we have to know which clock is which. Maybe we should associate clk_hw pointers with the gdscs and then export __clk_hw_create_clk() so that drivers can turn clk_hw pointers into clocks? > + > + ret = pm_clk_create(dev); > + if (ret) { > + dev_err(dev, "pm_clk_create failed %d\n", ret); Who cares? debug? > + return ret; > + } > + > + for (con_id = con_ids; *con_id; con_id++) { > + ret = pm_clk_add(dev, *con_id); > + if (ret) { > + dev_err(dev, "pm_clk_add failed %d\n", ret); Who cares? debug? > + goto fail; > + } > + } > + return 0; > +fail: > + pm_clk_destroy(dev); > + return ret; > +}; > + > +static void gdsc_detach(struct generic_pm_domain *domain, struct device *dev) > +{ > + pm_clk_destroy(dev); > + return; useless return
On 07/23/2015 06:31 AM, Stephen Boyd wrote: > On 07/22/2015 12:10 AM, Rajendra Nayak wrote: >> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain >> *domain) >> return gdsc_toggle_logic(sc, false); >> } >> +static int gdsc_attach(struct generic_pm_domain *domain, struct >> device *dev) >> +{ >> + int ret; >> + struct gdsc *sc = domain_to_gdsc(domain); >> + char **con_id, *con_ids[] = { "core", "iface", NULL }; > > const? > > This is where I get scared of sniffing too much SoC glue. What's to > enforce the "core", and "iface" naming scheme? What's to enforce there > being two clocks vs. one? Maybe a better approach would be to use > of_clk_get() and iterate through all clocks of the device, or to encode > the clock names in the gdsc structure. I had the clock names in the gdsc structure in v5. I should probably go back to having it that way. -- 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/23/2015 11:34 AM, Rajendra Nayak wrote: > On 07/23/2015 06:31 AM, Stephen Boyd wrote: >> On 07/22/2015 12:10 AM, Rajendra Nayak wrote: >>> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain >>> *domain) >>> return gdsc_toggle_logic(sc, false); >>> } >>> +static int gdsc_attach(struct generic_pm_domain *domain, struct >>> device *dev) >>> +{ >>> + int ret; >>> + struct gdsc *sc = domain_to_gdsc(domain); >>> + char **con_id, *con_ids[] = { "core", "iface", NULL }; >> >> const? >> >> This is where I get scared of sniffing too much SoC glue. What's to >> enforce the "core", and "iface" naming scheme? What's to enforce there >> being two clocks vs. one? Maybe a better approach would be to use >> of_clk_get() and iterate through all clocks of the device, or to encode >> the clock names in the gdsc structure. > > I had the clock names in the gdsc structure in v5. I should probably go > back to having it that way. Rajendra, If you decide to go back please look at that comment [1], as well.
On 07/23/2015 02:52 PM, Stanimir Varbanov wrote: > On 07/23/2015 11:34 AM, Rajendra Nayak wrote: >> On 07/23/2015 06:31 AM, Stephen Boyd wrote: >>> On 07/22/2015 12:10 AM, Rajendra Nayak wrote: >>>> @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain >>>> *domain) >>>> return gdsc_toggle_logic(sc, false); >>>> } >>>> +static int gdsc_attach(struct generic_pm_domain *domain, struct >>>> device *dev) >>>> +{ >>>> + int ret; >>>> + struct gdsc *sc = domain_to_gdsc(domain); >>>> + char **con_id, *con_ids[] = { "core", "iface", NULL }; >>> >>> const? >>> >>> This is where I get scared of sniffing too much SoC glue. What's to >>> enforce the "core", and "iface" naming scheme? What's to enforce there >>> being two clocks vs. one? Maybe a better approach would be to use >>> of_clk_get() and iterate through all clocks of the device, or to encode >>> the clock names in the gdsc structure. >> >> I had the clock names in the gdsc structure in v5. I should probably go >> back to having it that way. > > Rajendra, If you decide to go back please look at that comment [1], as well. yes, thanks, I will. -- 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/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index a59655b..3125809 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -14,6 +14,7 @@ #include <linux/delay.h> #include <linux/err.h> #include <linux/jiffies.h> +#include <linux/pm_clock.h> #include <linux/slab.h> #include "gdsc.h" @@ -104,6 +105,37 @@ static int gdsc_disable(struct generic_pm_domain *domain) return gdsc_toggle_logic(sc, false); } +static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev) +{ + int ret; + struct gdsc *sc = domain_to_gdsc(domain); + char **con_id, *con_ids[] = { "core", "iface", NULL }; + + ret = pm_clk_create(dev); + if (ret) { + dev_err(dev, "pm_clk_create failed %d\n", ret); + return ret; + } + + for (con_id = con_ids; *con_id; con_id++) { + ret = pm_clk_add(dev, *con_id); + if (ret) { + dev_err(dev, "pm_clk_add failed %d\n", ret); + goto fail; + } + } + return 0; +fail: + pm_clk_destroy(dev); + return ret; +}; + +static void gdsc_detach(struct generic_pm_domain *domain, struct device *dev) +{ + pm_clk_destroy(dev); + return; +}; + static int gdsc_init(struct gdsc *sc) { u32 mask, val; @@ -127,6 +159,9 @@ static int gdsc_init(struct gdsc *sc) sc->pd.power_off = gdsc_disable; sc->pd.power_on = gdsc_enable; + sc->pd.attach_dev = gdsc_attach; + sc->pd.detach_dev = gdsc_detach; + sc->pd.flags = GENPD_FLAG_PM_CLK; pm_genpd_init(&sc->pd, NULL, !on); return 0;
The devices within a gdsc power domain, quite often have additional clocks to be turned on/off along with the power domain itself. Once the drivers for these devices are converted to use runtime PM, it would be possible to remove all clock handling from the drivers if the gdsc driver can handle it. Use PM clocks to add support for this. A list of con_ids[] specified per gdsc would be the clocks turned on/off on every device start/stop callbacks. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- drivers/clk/qcom/gdsc.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)