| Message ID | 1381909214-12693-5-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Stephen Boyd (2013-10-16 00:40:06) > 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. > > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> > --- > drivers/clk/clk.c | 70 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/clk-provider.h | 13 ++++++++ > 2 files changed, 83 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 8042c00..7cc8cb0 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 6ed62f1..4087a9b 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; Thanks for submitting this patch. Adding all of this stuff to struct clk_hw makes me a sad panda. You are essentially sharing a common set of ops for clocks that use regmap as their io operation back-end, and that is a good thing. However, why not just do this as drivers/clk/clk-regmap.c, or drivers/clk/clk-gate-regmap.c? Putting the clk_ops callback functions in drivers/clk/clk.c is a little weird and putting those struct members into struct clk_hw is definitely strange. Regards, Mike > }; > > /* > @@ -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 Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation >
On Wed, Dec 18, 2013 at 20:50 -0800, Mike Turquette wrote: > > Adding all of this stuff to struct clk_hw makes me a sad panda. You are > essentially sharing a common set of ops for clocks that use regmap as > their io operation back-end, and that is a good thing. > > However, why not just do this as drivers/clk/clk-regmap.c, or > drivers/clk/clk-gate-regmap.c? Putting the clk_ops callback functions in > drivers/clk/clk.c is a little weird and putting those struct members > into struct clk_hw is definitely strange. Wasn't the idea to extend the set of register accessor routines in <linux/clk-provider.h> such that memory mapped I/O as well as regmap style becomes possible? This is what I understood from past iterations of discussing this approach. I agree that duplicating the clock gate's implementation just to access the register in a different way feels strange and somehow unfortunate. There still may be the issue of expensive operations only being allowed within prepare and unprepare, while enable and disable are supposed to be "cheap and straight forward", and should not block and thus may not use external communication. But that appears to be orthogonal to the API which wraps register access. virtually yours Gerhard Sittig
On 12/18, Mike Turquette wrote: > Quoting Stephen Boyd (2013-10-16 00:40:06) > > 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; > > Thanks for submitting this patch. > > Adding all of this stuff to struct clk_hw makes me a sad panda. You are > essentially sharing a common set of ops for clocks that use regmap as > their io operation back-end, and that is a good thing. > > However, why not just do this as drivers/clk/clk-regmap.c, or > drivers/clk/clk-gate-regmap.c? These new members aren't exclusively for use by the regmap code. For example, we could extend the gate clock implementation to store the enable bit in the 'enable_mask' member. They also aren't used exclusively by simple gates. If you look at this patch series you'll see that I call the helpers from hardware specific ops when I want to do the gate operation in addition to something else like polling a status bit. What is the concern though? I can only guess that perhaps not every clock type will have a bit to toggle and so putting the information here would unnecessarily penalize those clocks? How many clocks in a particular system don't have an on/off capability? On MSM hardware I can only count 5 or so out of 200 that don't have on/off capabilities so it didn't seem worth the effort to have a struct inside a struct inside a struct just to save a 100 bytes of data. If I understand you, I could make clk-regmap.c and struct clk_regmap like so: struct clk_regmap { struct regmap *regmap; unsigned int enable_reg; unsigned int enable_mask; bool enable_is_inverted; struct clk_hw hw; }; int clk_is_enabled_regmap(struct clk_hw *hw) { ... That certainly avoids adding members to struct clk_hw, but it also means that if we want to use this structure and augment it with something like a status bit poll we need to make a third structure to wrap the clk_regmap struct. That makes me a sad panda. The reason is because we'll need to add a registration function like clk_regmap_register(). I would like to just have an array of clk_hw pointers that I iterate through and call clk_register() on, without having to think/worry that this clock is a basic clock that doesn't use a regmap so I should call clk_register() and this other clock is wrapping a clk_regmap struct so I need to call clk_regmap_register(). I guess I could have more arrays or pair the clk_hw pointers with some registration function pointer, but that doesn't make me feel much better. > Putting the clk_ops callback functions in > drivers/clk/clk.c is a little weird and putting those struct members > into struct clk_hw is definitely strange. In the regulator framework we have helpers.c. I could make a similar file for these very similar helpers.
On Thu, Dec 19, 2013 at 21:24 +0100, Gerhard Sittig wrote: > > Wasn't the idea to extend the set of register accessor routines > in <linux/clk-provider.h> such that memory mapped I/O as well as > regmap style becomes possible? This is what I understood from > past iterations of discussing this approach. Just FYI and to have the pointer in some archive: Had a look at recent threads because it appears that several pending changes have similar needs to adjust the kind of hardware access, while sharing common code to handle the actual clock item logic would be desirable. Please check "[PATCHv12 00/49] ARM: TI SoC clock DT conversion" from Tero Kristo as of 2013-12-20, which "in bypassing" introduces low level ops for register access (patches 06 and following). I suggested to separate this hardware access aspect out of the "TI SoC clock" series (almost missed the fact that it changes behaviour in common code), and to add regmap support or at least allow to do regmap on top of the ll_ops infrastructure. With the proposed generic approach all of us should be happy. The vast amount of ARM code should not experience any change, the queued PowerPC support can keep working, new TI SoC clocks can get implemented, Steven you can get regmap access, and more platforms may use the common code even if readl()/writel() won't fit them. Let's followup in Tero's v12 feedback thread to make that happen. virtually yours Gerhard Sittig
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8042c00..7cc8cb0 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 6ed62f1..4087a9b 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. Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- drivers/clk/clk.c | 70 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/clk-provider.h | 13 ++++++++ 2 files changed, 83 insertions(+)