| Message ID | 1387847559-18330-4-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Dec 23, 2013 at 05:12:27PM -0800, Stephen Boyd wrote: > The clock framework already has support for simple gate clocks > but if drivers want to use the gate clock functionality they need > to wrap the gate clock in another struct and chain the ops by > calling the gate ops from their own custom ops. Plus the gate Reviewed-by: Mark Brown <broonie@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 23, 2013 at 17:12 -0800, Stephen Boyd wrote: > > The clock framework already has support for simple gate clocks > but if drivers want to use the gate clock functionality they need > to wrap the gate clock in another struct and chain the ops by > calling the gate ops from their own custom ops. Plus the gate > clock implementation only supports MMIO accessors so other bus > type clocks don't benefit from the potential code reuse. Add some > simple regmap helpers for enable/disable/is_enabled that drivers > can use as drop in replacements for their clock ops or as simple > functions they call from their own custom ops. This is based on > similar helps in the regulator framework. The same comment applies as to the previous version. Is it useful to introduce copies of the gate handling while the difference in only in how the hardware registers get accessed? > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -177,11 +177,21 @@ struct clk_init_data { > [ ... ] > @@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name); > long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate, > unsigned long *best_parent_rate, > struct clk **best_parent_p); > +int clk_is_enabled_regmap(struct clk_hw *hw); > +int clk_enable_regmap(struct clk_hw *hw); > +void clk_disable_regmap(struct clk_hw *hw); Looking at the patch: Do you expect callers to remember whether a clock gate is backed by mmio or by regmap access, to call a different set of routines? Should this not be hidden behind the API and be transparent after clock registration? I'd suggest to fold regmap support into Tero Kristo's ll_ops approach, and to discuss this in his v12 thread. virtually yours Gerhard Sittig
On 12/24, Gerhard Sittig wrote: > On Mon, Dec 23, 2013 at 17:12 -0800, Stephen Boyd wrote: > > > > The clock framework already has support for simple gate clocks > > but if drivers want to use the gate clock functionality they need > > to wrap the gate clock in another struct and chain the ops by > > calling the gate ops from their own custom ops. Plus the gate > > clock implementation only supports MMIO accessors so other bus > > type clocks don't benefit from the potential code reuse. Add some > > simple regmap helpers for enable/disable/is_enabled that drivers > > can use as drop in replacements for their clock ops or as simple > > functions they call from their own custom ops. This is based on > > similar helps in the regulator framework. > > The same comment applies as to the previous version. Is it > useful to introduce copies of the gate handling while the > difference in only in how the hardware registers get accessed? > I don't plan to use the clk-gate.c implementation because I need more than just a bit toggling clock. We can easily make clk-gate.c use these helpers if you're worried about the very small amount of code duplication between the two. I'd be glad to do that, I just didn't include it here because I don't have a use for it. > > --- a/include/linux/clk-provider.h > > +++ b/include/linux/clk-provider.h > > @@ -177,11 +177,21 @@ struct clk_init_data { > > [ ... ] > > @@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name); > > long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate, > > unsigned long *best_parent_rate, > > struct clk **best_parent_p); > > +int clk_is_enabled_regmap(struct clk_hw *hw); > > +int clk_enable_regmap(struct clk_hw *hw); > > +void clk_disable_regmap(struct clk_hw *hw); > > Looking at the patch: Do you expect callers to remember whether > a clock gate is backed by mmio or by regmap access, to call a > different set of routines? There are only regmap functions. I'm not sure where the choice is, but I expect the callers to know what they're doing. If you look at the rest of this series you'll see that I assign these functions directly to the clk_ops, or I call them from the enable/disable functions that need to do some status bit polling after the clock is enabled or disabled. > Should this not be hidden behind the > API and be transparent after clock registration? I don't really understand what you mean by hiding it behind the API? What API? If we're talking about clk_register_gate() I think we would need to add a clk_register_regmap_gate() function because the reg argument is an __iomem pointer. It doesn't look like it can be transparent unless that pointer is reused as an offset. I don't attempt to do anything about that here though because I don't use the clk-gate.c code. > > I'd suggest to fold regmap support into Tero Kristo's ll_ops > approach, and to discuss this in his v12 thread. Sure, I'll go look at and reply to that thread. How do you think I can benefit from Tero's patch series? From what I can tell ll_ops are a simplified version of regmap. Was regmap dismissed because the omap clock driver is not actually a platform driver? There doesn't seem to be any details in the thread(s) about why the ll_ops were proposed over regmap. From my perspective, using a regmap like is proposed in my patches is the better way to do this and it doesn't require any thing like ll_ops or clk_readl() to do it.
On Thu, Dec 26, 2013 at 11:31 -0800, Stephen Boyd wrote: > > On 12/24, Gerhard Sittig wrote: > > On Mon, Dec 23, 2013 at 17:12 -0800, Stephen Boyd wrote: > > > > > > The clock framework already has support for simple gate clocks > > > but if drivers want to use the gate clock functionality they need > > > to wrap the gate clock in another struct and chain the ops by > > > calling the gate ops from their own custom ops. Plus the gate > > > clock implementation only supports MMIO accessors so other bus > > > type clocks don't benefit from the potential code reuse. Add some > > > simple regmap helpers for enable/disable/is_enabled that drivers > > > can use as drop in replacements for their clock ops or as simple > > > functions they call from their own custom ops. This is based on > > > similar helps in the regulator framework. > > > > The same comment applies as to the previous version. Is it > > useful to introduce copies of the gate handling while the > > difference in only in how the hardware registers get accessed? > > > > I don't plan to use the clk-gate.c implementation because I need > more than just a bit toggling clock. We can easily make > clk-gate.c use these helpers if you're worried about the very > small amount of code duplication between the two. I'd be glad to > do that, I just didn't include it here because I don't have a use > for it. OK, then I simply misunderstood. From past threads I got the impression that your clock item was "a gate with regmap instead of mmio for hardware access, everything else being the same". The concerns were not so much about the size of duplicated code, but the "quality step" in starting duplication at all. But since I was wrong, nevermind. > > > --- a/include/linux/clk-provider.h > > > +++ b/include/linux/clk-provider.h > > > @@ -177,11 +177,21 @@ struct clk_init_data { > > > [ ... ] > > > @@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name); > > > long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate, > > > unsigned long *best_parent_rate, > > > struct clk **best_parent_p); > > > +int clk_is_enabled_regmap(struct clk_hw *hw); > > > +int clk_enable_regmap(struct clk_hw *hw); > > > +void clk_disable_regmap(struct clk_hw *hw); > > > > Looking at the patch: Do you expect callers to remember whether > > a clock gate is backed by mmio or by regmap access, to call a > > different set of routines? > > There are only regmap functions. I'm not sure where the choice > is, but I expect the callers to know what they're doing. If you > look at the rest of this series you'll see that I assign these > functions directly to the clk_ops, or I call them from the > enable/disable functions that need to do some status bit polling > after the clock is enabled or disabled. > > > Should this not be hidden behind the > > API and be transparent after clock registration? > > I don't really understand what you mean by hiding it behind the > API? What API? If we're talking about clk_register_gate() I think > we would need to add a clk_register_regmap_gate() function > because the reg argument is an __iomem pointer. It doesn't look > like it can be transparent unless that pointer is reused as an > offset. I don't attempt to do anything about that here though > because I don't use the clk-gate.c code. See above, it's simple. I misunderstood, asked (in a previous thread), got no response, saw another submission, asked again. Now that you told me, it's clear I got something wrong. Thank you for telling me what I missed before. virtually yours Gerhard Sittig
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 5e71f5c..8b40170 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -742,6 +742,76 @@ out: return best; } +/** + * clk_is_enabled_regmap - standard is_enabled() for regmap users + * + * @hw: clk to operate on + * + * Clocks that use regmap for their register I/O can set the + * enable_reg and enable_mask fields in their struct clk_hw and then use + * this as their is_enabled operation, saving some code. + */ +int clk_is_enabled_regmap(struct clk_hw *hw) +{ + unsigned int val; + int ret; + + ret = regmap_read(hw->regmap, hw->enable_reg, &val); + if (ret != 0) + return ret; + + if (hw->enable_is_inverted) + return (val & hw->enable_mask) == 0; + else + return (val & hw->enable_mask) != 0; +} +EXPORT_SYMBOL_GPL(clk_is_enabled_regmap); + +/** + * clk_enable_regmap - standard enable() for regmap users + * + * @hw: clk to operate on + * + * Clocks that use regmap for their register I/O can set the + * enable_reg and enable_mask fields in their struct clk_hw and then use + * this as their enable() operation, saving some code. + */ +int clk_enable_regmap(struct clk_hw *hw) +{ + unsigned int val; + + if (hw->enable_is_inverted) + val = 0; + else + val = hw->enable_mask; + + return regmap_update_bits(hw->regmap, hw->enable_reg, + hw->enable_mask, val); +} +EXPORT_SYMBOL_GPL(clk_enable_regmap); + +/** + * clk_disable_regmap - standard disable() for regmap users + * + * @hw: clk to operate on + * + * Clocks that use regmap for their register I/O can set the + * enable_reg and enable_mask fields in their struct clk_hw and then use + * this as their disable() operation, saving some code. + */ +void clk_disable_regmap(struct clk_hw *hw) +{ + unsigned int val; + + if (hw->enable_is_inverted) + val = hw->enable_mask; + else + val = 0; + + regmap_update_bits(hw->regmap, hw->enable_reg, hw->enable_mask, val); +} +EXPORT_SYMBOL_GPL(clk_disable_regmap); + /*** clk api ***/ void __clk_unprepare(struct clk *clk) diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 31f2890..61507fb 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -177,11 +177,21 @@ struct clk_init_data { * with the common clock framework. * * @regmap: regmap to use for regmap helpers and/or by providers + * + * @enable_reg: register when using regmap enable/disable ops + * + * @enable_mask: mask when using regmap enable/disable ops + * + * @enable_is_inverted: flag to indicate set enable_mask bits to disable + * when using clock_enable_regmap and friends APIs. */ struct clk_hw { struct clk *clk; const struct clk_init_data *init; struct regmap *regmap; + unsigned int enable_reg; + unsigned int enable_mask; + bool enable_is_inverted; }; /* @@ -447,6 +457,9 @@ struct clk *__clk_lookup(const char *name); long __clk_mux_determine_rate(struct clk_hw *hw, unsigned long rate, unsigned long *best_parent_rate, struct clk **best_parent_p); +int clk_is_enabled_regmap(struct clk_hw *hw); +int clk_enable_regmap(struct clk_hw *hw); +void clk_disable_regmap(struct clk_hw *hw); /* * FIXME clock api without lock protection
The clock framework already has support for simple gate clocks but if drivers want to use the gate clock functionality they need to wrap the gate clock in another struct and chain the ops by calling the gate ops from their own custom ops. Plus the gate clock implementation only supports MMIO accessors so other bus type clocks don't benefit from the potential code reuse. Add some simple regmap helpers for enable/disable/is_enabled that drivers can use as drop in replacements for their clock ops or as simple functions they call from their own custom ops. This is based on similar helps in the regulator framework. Cc: Mark Brown <broonie@kernel.org> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/clk/clk.c | 70 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk-provider.h | 13 ++++++++ 2 files changed, 83 insertions(+)