| Message ID | 1438857474-20262-12-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/06, Rajendra Nayak wrote: > + > +static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev) > +{ > + int ret, i = 0, j = 0; > + struct gdsc *sc = domain_to_gdsc(domain); > + struct of_phandle_args clkspec; > + struct device_node *np = dev->of_node; > + > + if (!sc->clock_count) > + return 0; > + > + ret = pm_clk_create(dev); > + if (ret) { > + dev_dbg(dev, "pm_clk_create failed %d\n", ret); > + return ret; > + } > + > + sc->clks = devm_kcalloc(dev, sc->clock_count, sizeof(sc->clks), > + GFP_KERNEL); > + if (!sc->clks) > + return -ENOMEM; > + > + while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, > + &clkspec)) { > + if (match(clkspec.args[0], sc->clocks, sc->clock_count)) { I'm lost. I was hoping we could just make up a clkspec on the stack and pass it over to of_clk_get_from_provider() without having to go through the np of the client device. The point being to avoid forcing this code from knowing about the consumer binding or connection name choice for each device. Instead, it assumes that it's a #clock-cells=<1> binding and gets the clocks by passing the 1 cell data without calling of_parse_phandle_with_args(). Now, one downside of that approach is that it's DT centric (also of_clk_get_from_provider() is not an exported symbol yet). So I'm really starting to lean towards exposing __clk_create_clk() (or some better named "provider" function) that will allow clk providers to turn their clk_hw structure into a struct clk pointer. That avoids the DT centric design, and avoids binding the provider to the connection ids too. > + sc->clks[j] = of_clk_get_from_provider(&clkspec); > + pm_clk_add_clk(dev, sc->clks[j]); > + j++; > + } > + i++; > + } > + return 0; > +};
On 08/11/2015 12:22 PM, Stephen Boyd wrote: > On 08/06, Rajendra Nayak wrote: >> + >> +static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev) >> +{ >> + int ret, i = 0, j = 0; >> + struct gdsc *sc = domain_to_gdsc(domain); >> + struct of_phandle_args clkspec; >> + struct device_node *np = dev->of_node; >> + >> + if (!sc->clock_count) >> + return 0; >> + >> + ret = pm_clk_create(dev); >> + if (ret) { >> + dev_dbg(dev, "pm_clk_create failed %d\n", ret); >> + return ret; >> + } >> + >> + sc->clks = devm_kcalloc(dev, sc->clock_count, sizeof(sc->clks), >> + GFP_KERNEL); >> + if (!sc->clks) >> + return -ENOMEM; >> + >> + while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, >> + &clkspec)) { >> + if (match(clkspec.args[0], sc->clocks, sc->clock_count)) { > > I'm lost. I was hoping we could just make up a clkspec on the > stack and pass it over to of_clk_get_from_provider() without > having to go through the np of the client device. Sorry, can you elaborate a little more on 'make up a clkspec on the stack'? I don't seem to be able to figure out. The point being > to avoid forcing this code from knowing about the consumer > binding or connection name choice for each device. Instead, it I am not associating connection names but internal clock ids with the corresponding gdsc. I don;t see how this is forcing the code to know consumer DT bindings. However, it is forcing the client device to have all clocks that need to be controlled listed in DT, which I assume should be the case. > assumes that it's a #clock-cells=<1> binding and gets the clocks > by passing the 1 cell data without calling > of_parse_phandle_with_args(). Yes, the code did assume #clock-cells=<1>, which is what all of the existing clock controllers used in QCOM SoCs use. > > Now, one downside of that approach is that it's DT centric (also > of_clk_get_from_provider() is not an exported symbol yet). So I'm > really starting to lean towards exposing __clk_create_clk() (or > some better named "provider" function) that will allow clk > providers to turn their clk_hw structure into a struct clk > pointer. That avoids the DT centric design, and avoids binding > the provider to the connection ids too. Again, there are no connection ids used here. I can explore the approach of associating clk_hw structs to gdscs and avoiding anything to do with a DT lookup. I seem to however fail to understand why we need this to have nothing to do with DT though. Do you think the clocks for a given device which need to be controlled along with the power switch (gdsc) should *not* be associated/listed within its DT node?
Hi Stephen, >> +static int gdsc_attach(struct generic_pm_domain *domain, struct device >> *dev) >> +{ >> + int ret, i = 0, j = 0; >> + struct gdsc *sc = domain_to_gdsc(domain); >> + struct of_phandle_args clkspec; >> + struct device_node *np = dev->of_node; >> + >> + if (!sc->clock_count) >> + return 0; >> + >> + ret = pm_clk_create(dev); >> + if (ret) { >> + dev_dbg(dev, "pm_clk_create failed %d\n", ret); >> + return ret; >> + } >> + >> + sc->clks = devm_kcalloc(dev, sc->clock_count, sizeof(sc->clks), >> + GFP_KERNEL); >> + if (!sc->clks) >> + return -ENOMEM; >> + >> + while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, >> + &clkspec)) { >> + if (match(clkspec.args[0], sc->clocks, sc->clock_count)) { > > I'm lost. I was hoping we could just make up a clkspec on the > stack and pass it over to of_clk_get_from_provider() without > having to go through the np of the client device. The point being > to avoid forcing this code from knowing about the consumer > binding or connection name choice for each device. Instead, it > assumes that it's a #clock-cells=<1> binding and gets the clocks > by passing the 1 cell data without calling > of_parse_phandle_with_args(). > > Now, one downside of that approach is that it's DT centric (also > of_clk_get_from_provider() is not an exported symbol yet). So I'm > really starting to lean towards exposing __clk_create_clk() (or > some better named "provider" function) that will allow clk > providers to turn their clk_hw structure into a struct clk > pointer. That avoids the DT centric design, and avoids binding > the provider to the connection ids too. Stephen, I started to relook at these patches, avoiding the DT centric design and implementing a clk helper API as you suggested above. While this would work for GDSCs with just one device, its hard to scale if we ever run into GDSCs with multiple devices (In which case you need to know which device within the GDSC needs which clocks) Do you think its a fair limitation (one device per gdsc) to live with? because I can't seem to figure how a non DT centric design would otherwise work. regards, Rajendra
On 11/27, Rajendra Nayak wrote: > > > > I'm lost. I was hoping we could just make up a clkspec on the > > stack and pass it over to of_clk_get_from_provider() without > > having to go through the np of the client device. The point being > > to avoid forcing this code from knowing about the consumer > > binding or connection name choice for each device. Instead, it > > assumes that it's a #clock-cells=<1> binding and gets the clocks > > by passing the 1 cell data without calling > > of_parse_phandle_with_args(). > > > > Now, one downside of that approach is that it's DT centric (also > > of_clk_get_from_provider() is not an exported symbol yet). So I'm > > really starting to lean towards exposing __clk_create_clk() (or > > some better named "provider" function) that will allow clk > > providers to turn their clk_hw structure into a struct clk > > pointer. That avoids the DT centric design, and avoids binding > > the provider to the connection ids too. > > Stephen, I started to relook at these patches, avoiding the DT centric > design and implementing a clk helper API as you suggested above. > > While this would work for GDSCs with just one device, its hard to scale > if we ever run into GDSCs with multiple devices (In which case you > need to know which device within the GDSC needs which clocks) Why? The GDSC should know which clocks it's forcing on and off and it shouldn't need to care which devices are consuming those clocks. The GDSC structure would have a bunch of clk_hw pointers that it could convert into struct clk pointer to do clk consumer API things as needed. Maybe I'm missing something? If we're trying to take away clock control from our device drivers and hide that in SoC glue code (something we almost have to do so that the ordering of clocks vs. GDSC enable isn't wrong), then that isn't a GDSC, but some linux genpd concept. I imagine it would be a genpd per hw block (or device driver really) and then those genpds would be children of a larger GDSC genpd. For example, two hw blocks could have separate genpds for their clock control so that we can hide the clock on/off from the drivers, and then those are children of a single GDSC backed genpd for the physical domain they both reside in. If the GDSC domain and the sw domain need to control the same clocks, everything will work because we reference count appropriately. The downside of this all is that we're putting a bunch of knowledge about which drivers are using which clocks into the SoC clock driver. But I don't know where else we would put this if we want to hide these details from the driver authors. The only other option is to do it with DT, and the whole DT ABI there scares me enough to want to try and make it work in the SoC clock driver for now. > > Do you think its a fair limitation (one device per gdsc) to live with? > because I can't seem to figure how a non DT centric design would otherwise > work. No. The SMMU use case on a-family has SMMU and the hw block (video, gpu, display) that uses it within the same GDSC power domain.
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index da9fad8..ec1dfb5 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -12,10 +12,12 @@ */ #include <linux/bitops.h> +#include <linux/clk.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/jiffies.h> #include <linux/kernel.h> +#include <linux/pm_clock.h> #include <linux/pm_domain.h> #include <linux/regmap.h> #include <linux/reset-controller.h> @@ -161,6 +163,59 @@ static int gdsc_disable(struct generic_pm_domain *domain) return gdsc_toggle_logic(sc, false); } +static inline bool match(unsigned int id, unsigned int *ids, unsigned int count) +{ + int i; + + for (i = 0; i < count; i++) + if (id == ids[i]) + return true; + return false; +} + +static int gdsc_attach(struct generic_pm_domain *domain, struct device *dev) +{ + int ret, i = 0, j = 0; + struct gdsc *sc = domain_to_gdsc(domain); + struct of_phandle_args clkspec; + struct device_node *np = dev->of_node; + + if (!sc->clock_count) + return 0; + + ret = pm_clk_create(dev); + if (ret) { + dev_dbg(dev, "pm_clk_create failed %d\n", ret); + return ret; + } + + sc->clks = devm_kcalloc(dev, sc->clock_count, sizeof(sc->clks), + GFP_KERNEL); + if (!sc->clks) + return -ENOMEM; + + while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, + &clkspec)) { + if (match(clkspec.args[0], sc->clocks, sc->clock_count)) { + sc->clks[j] = of_clk_get_from_provider(&clkspec); + pm_clk_add_clk(dev, sc->clks[j]); + j++; + } + i++; + } + return 0; +}; + +static void gdsc_detach(struct generic_pm_domain *domain, struct device *dev) +{ + struct gdsc *sc = domain_to_gdsc(domain); + + if (!sc->clock_count) + return; + + pm_clk_destroy(dev); +}; + static int gdsc_init(struct gdsc *sc) { u32 mask, val; @@ -196,6 +251,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; diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h index 5ded268..2fdb332 100644 --- a/drivers/clk/qcom/gdsc.h +++ b/drivers/clk/qcom/gdsc.h @@ -17,6 +17,7 @@ #include <linux/err.h> #include <linux/pm_domain.h> +struct clk; struct regmap; struct reset_controller_dev; @@ -38,6 +39,9 @@ struct reset_controller_dev; * @resets: ids of resets associated with this gdsc * @reset_count: number of @resets * @rcdev: reset controller + * @clocks: ids of clocks associated with the gdsc + * @clock_count: number of @clocks + * @clks: clock pointers to gdsc clocks */ struct gdsc { struct generic_pm_domain pd; @@ -49,6 +53,9 @@ struct gdsc { struct reset_controller_dev *rcdev; unsigned int *resets; unsigned int reset_count; + unsigned int *clocks; + unsigned int clock_count; + struct clk **clks; }; #ifdef CONFIG_QCOM_GDSC
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 clock 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/qcom/gdsc.h | 7 ++++++ 2 files changed, 65 insertions(+)