[PATCH v1 05/24] clk: wrap I/O access for improved portability
the common clock drivers were motivated/initiated by ARM development
and apparently assume little endian peripherals
wrap register/peripherals access in the common code (div, gate, mux)
in preparation of adding COMMON_CLK support for other platforms
Signed-off-by: Gerhard Sittig
---
drivers/clk/clk-divider.c|6 +++---
drivers/clk/clk-gate.c |6 +++---
drivers/clk/clk-mux.c|6 +++---
include/linux/clk-provider.h | 17 +
4 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 6d55eb2..2c07061 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw
*hw,
struct clk_divider *divider = to_clk_divider(hw);
unsigned int div, val;
- val = readl(divider->reg) >> divider->shift;
+ val = clk_readl(divider->reg) >> divider->shift;
val &= div_mask(divider);
div = _get_div(divider, val);
@@ -230,11 +230,11 @@ static int clk_divider_set_rate(struct clk_hw *hw,
unsigned long rate,
if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
val = div_mask(divider) << (divider->shift + 16);
} else {
- val = readl(divider->reg);
+ val = clk_readl(divider->reg);
val &= ~(div_mask(divider) << divider->shift);
}
val |= value << divider->shift;
- writel(val, divider->reg);
+ clk_writel(val, divider->reg);
if (divider->lock)
spin_unlock_irqrestore(divider->lock, flags);
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 790306e..b7fbd96 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -58,7 +58,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
if (set)
reg |= BIT(gate->bit_idx);
} else {
- reg = readl(gate->reg);
+ reg = clk_readl(gate->reg);
if (set)
reg |= BIT(gate->bit_idx);
@@ -66,7 +66,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
reg &= ~BIT(gate->bit_idx);
}
- writel(reg, gate->reg);
+ clk_writel(reg, gate->reg);
if (gate->lock)
spin_unlock_irqrestore(gate->lock, flags);
@@ -89,7 +89,7 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
u32 reg;
struct clk_gate *gate = to_clk_gate(hw);
- reg = readl(gate->reg);
+ reg = clk_readl(gate->reg);
/* if a set bit disables this clk, flip it before masking */
if (gate->flags & CLK_GATE_SET_TO_DISABLE)
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 61c..02ef506 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -42,7 +42,7 @@ static u8 clk_mux_get_parent(struct clk_hw *hw)
* OTOH, pmd_trace_clk_mux_ck uses a separate bit for each clock, so
* val = 0x4 really means "bit 2, index starts at bit 0"
*/
- val = readl(mux->reg) >> mux->shift;
+ val = clk_readl(mux->reg) >> mux->shift;
val &= mux->mask;
if (mux->table) {
@@ -89,11 +89,11 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
if (mux->flags & CLK_MUX_HIWORD_MASK) {
val = mux->mask << (mux->shift + 16);
} else {
- val = readl(mux->reg);
+ val = clk_readl(mux->reg);
val &= ~(mux->mask << mux->shift);
}
val |= index << mux->shift;
- writel(val, mux->reg);
+ clk_writel(val, mux->reg);
if (mux->lock)
spin_unlock_irqrestore(mux->lock, flags);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 1ec14a7..c4f7799 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -12,6 +12,7 @@
#define __LINUX_CLK_PROVIDER_H
#include
+#include
#ifdef CONFIG_COMMON_CLK
@@ -490,5 +491,21 @@ static inline const char *of_clk_get_parent_name(struct
device_node *np,
#define of_clk_init(matches) \
{ while (0); }
#endif /* CONFIG_OF */
+
+/*
+ * wrap access to peripherals in accessor routines
+ * for improved portability across platforms
+ */
+
+static inline u32 clk_readl(u32 __iomem *reg)
+{
+ return readl(reg);
+}
+
+static inline void clk_writel(u32 val, u32 __iomem *reg)
+{
+ writel(val, reg);
+}
+
#endif /* CONFIG_COMMON_CLK */
#endif /* CLK_PROVIDER_H */
--
1.7.10.4
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Mon, Jul 15, 2013 at 08:47:34PM +0200, Gerhard Sittig wrote: > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 6d55eb2..2c07061 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct > clk_hw *hw, > struct clk_divider *divider = to_clk_divider(hw); > unsigned int div, val; > > - val = readl(divider->reg) >> divider->shift; > + val = clk_readl(divider->reg) >> divider->shift; > val &= div_mask(divider); Would it be an option to use regmap for the generic dividers/muxes instead? This should be suitable for ppc and also for people who want to use the generic clocks on i2c devices. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Mon, Jul 15, 2013 at 21:38 +0200, Sascha Hauer wrote:
>
> On Mon, Jul 15, 2013 at 08:47:34PM +0200, Gerhard Sittig wrote:
> > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> > index 6d55eb2..2c07061 100644
> > --- a/drivers/clk/clk-divider.c
> > +++ b/drivers/clk/clk-divider.c
> > @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct
> > clk_hw *hw,
> > struct clk_divider *divider = to_clk_divider(hw);
> > unsigned int div, val;
> >
> > - val = readl(divider->reg) >> divider->shift;
> > + val = clk_readl(divider->reg) >> divider->shift;
> > val &= div_mask(divider);
>
> Would it be an option to use regmap for the generic dividers/muxes
> instead? This should be suitable for ppc and also for people who want to
> use the generic clocks on i2c devices.
Does regmap assume that those registers form a (dense) array of
equal sized items? Does it introduce unconditional indirection
and requirement for explicit setup beyond mere mapping? What's
the overhead compared to the readl/inbe32 approach that's
currently resolved at compile time?
Neither of the above needs to be a blocker, just needs to be
acceptable after consideration. So far nobody appears to have
felt pain with the LE32 register assumption.
BTW does the common clock support that I introduce for MPC512x
only cover those parts from crystal over internal busses to
internal peripherals (register files).
It does not include bitrate generation within the peripherals --
this appears to remain the domain of individual drivers, which
keep manipulating register fields to derive wire bitrates from
internal references. Sharing drivers between platforms with and
without common clock support forbids the switch anyway, or both
CCF abstraction as well as local register manipulation need to
get implemented in parallel, as I did for the mscan(4) driver.
Some of these bitrate related registers aren't 32bit entities and
thus could not get managed by the common clock primitives in
their current form. Some of the IP blocks may even spread
integer values across several non-adjacent bit fields for legacy
reasons, but I guess that these aren't in the scope of the shared
primitives either (and they may not be popular either).
While we are at improvements for the common clock primitives:
What I missed was support for fractional dividers, i.e. dividers
with a register bitfield backed divider part but a fixed factor
multiplier part. Currently there's only dividers (bitfield
factor or bitfield with table lookup for the factor) and
fixed-factor (multiplier and divider, but both of them fixed and
not adjustable by register manipulation). This was addressed by
"intermediate" clock items ("ungated", "x4")
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Mon, Jul 15, 2013 at 21:38 +0200, Sascha Hauer wrote: > > On Mon, Jul 15, 2013 at 08:47:34PM +0200, Gerhard Sittig wrote: > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > > index 6d55eb2..2c07061 100644 > > --- a/drivers/clk/clk-divider.c > > +++ b/drivers/clk/clk-divider.c > > @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct > > clk_hw *hw, > > struct clk_divider *divider = to_clk_divider(hw); > > unsigned int div, val; > > > > - val = readl(divider->reg) >> divider->shift; > > + val = clk_readl(divider->reg) >> divider->shift; > > val &= div_mask(divider); > > Would it be an option to use regmap for the generic dividers/muxes > instead? This should be suitable for ppc and also for people who want to > use the generic clocks on i2c devices. Some other thought crossed my mind regarding access to clock control registers that reside behind some communication channel like I2C: The common clock API assumes (it's part of the contract) that there are potentially expensive operations like get, put, prepare and unprepare, as well as swift and non-blocking operations like enable and disable. Would the regmap abstraction hide the potentially blocking nature of a register access (I understand that you can implement "local" as well as "remote" register sets by this mechanism)? Or could you still meet the assumptions or requirements of the common clock API? It might as well be the responsibility of the clock driver's implementor to arrange for the availability of non-blocking enable/disable operations, just as it is today. Such that expensive register access need not be forbidden in general. virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Thu, Jul 18, 2013 at 09:04:02AM +0200, Gerhard Sittig wrote: > On Mon, Jul 15, 2013 at 21:38 +0200, Sascha Hauer wrote: > > > > On Mon, Jul 15, 2013 at 08:47:34PM +0200, Gerhard Sittig wrote: > > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > > > index 6d55eb2..2c07061 100644 > > > --- a/drivers/clk/clk-divider.c > > > +++ b/drivers/clk/clk-divider.c > > > @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct > > > clk_hw *hw, > > > struct clk_divider *divider = to_clk_divider(hw); > > > unsigned int div, val; > > > > > > - val = readl(divider->reg) >> divider->shift; > > > + val = clk_readl(divider->reg) >> divider->shift; > > > val &= div_mask(divider); > > > > Would it be an option to use regmap for the generic dividers/muxes > > instead? This should be suitable for ppc and also for people who want to > > use the generic clocks on i2c devices. > > Some other thought crossed my mind regarding access to clock > control registers that reside behind some communication channel > like I2C: > > The common clock API assumes (it's part of the contract) that > there are potentially expensive operations like get, put, prepare > and unprepare, as well as swift and non-blocking operations like > enable and disable. > > Would the regmap abstraction hide the potentially blocking nature > of a register access (I understand that you can implement "local" > as well as "remote" register sets by this mechanism)? Or could > you still meet the assumptions or requirements of the common > clock API? > > It might as well be the responsibility of the clock driver's > implementor to arrange for the availability of non-blocking > enable/disable operations, just as it is today. Such that > expensive register access need not be forbidden in general. regmap for mmio uses a spinlock for read/modify/write operations, just like you have to use a spinlock in the common clk dividers/gates. This part wouldn't change with regmap. For i2c connected clocks where a spinlock doesn't work due to the nonatomic nature of i2c devices we would have to move the enable/disble stuff to prepare/unprepare in the common gate code. This can be left for someone who works on i2c clocks though. I think regmap has the potential to solve a number of issues like the hardcoded readl/writel in the common clock blocks, issues with i2c clocks and your endianess issue. The biggest question probably is how to get there without putting too much of a burden on you. It's probably not an option to convert all users to regmap, so it seems additional functions like clk_register_gate_regmap are better to handle. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Thu, Jul 18, 2013 at 09:04:02AM +0200, Gerhard Sittig wrote: > The common clock API assumes (it's part of the contract) that > there are potentially expensive operations like get, put, prepare > and unprepare, as well as swift and non-blocking operations like > enable and disable. Let's get something straight here, because what you've said above is wrong. 1. clk_get() and clk_put() are NOT part of the common clock API. They're separate - they're part of the clk API, and the infrastructure behind that is clkdev, which is a separately owned thing (by me.) 2. The "contract" of the clk API is defined by the clk API, not by some random implementation like the common clock API. The clk API is maintained by myself, and is described in include/linux/clk.h 3. clk_prepare() and clk_unprepare() are functions MUST only be called from contexts where sleeping is permitted. These functions MAY sleep for whatever reason they require to, and as long as they require to. (This is the whole reason these two functions were created in the first place.) 4. clk_enable() and clk_disable() MAY be called from any context, but MUST never sleep. If you need to talk over a non-atomic bus for these, then these functions should be no-ops, and the code which does that must be executed from the clk_prepare()/clk_unprepare() operations. That is the "clk API" contract. The CCF has no bearing on this; if it disagrees, then the CCF is buggy and is non-conformant. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Thu, Jul 18, 2013 at 10:06:57AM +0200, Sascha Hauer wrote: > I think regmap has the potential to solve a number of issues like the > hardcoded readl/writel in the common clock blocks, issues with i2c > clocks and your endianess issue. The biggest question probably is how > to get there without putting too much of a burden on you. It's probably > not an option to convert all users to regmap, so it seems additional > functions like clk_register_gate_regmap are better to handle. That's basically what regulator does. There's versions of the ops that all the drivers can use. signature.asc Description: Digital signature ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Thu, 18 Jul 2013, Russell King - ARM Linux wrote: > 1. clk_get() and clk_put() are NOT part of the common clock API. >They're separate - they're part of the clk API, and the infrastructure >behind that is clkdev, which is a separately owned thing (by me.) > > 2. The "contract" of the clk API is defined by the clk API, not by some >random implementation like the common clock API. The clk API is >maintained by myself, and is described in include/linux/clk.h > > 3. clk_prepare() and clk_unprepare() are functions MUST only be called >from contexts where sleeping is permitted. These functions MAY sleep >for whatever reason they require to, and as long as they require to. >(This is the whole reason these two functions were created in the >first place.) > > 4. clk_enable() and clk_disable() MAY be called from any context, but >MUST never sleep. If you need to talk over a non-atomic bus for these, >then these functions should be no-ops, and the code which does that >must be executed from the clk_prepare()/clk_unprepare() operations. Could the above be included in some form in Documentation/clk.txt (this is likely one of the first location people look for information) and elsewhere if appropriate please? A *lot* of people are confused by the prepare-enable-disable-unprepare sequence and when I try to find some rational for the prepare/enable split I can only direct them to mail archive posts since this is nowhere to be found in the kernel. The comments in include/linux/clk.h, while correct, are very terse and don't provide any insight to the reason why there is a split in the API. The content of Documentation/clk.txt does refer to prepare and enable (and their counterparts) but again doesn't provide any clue about the reason for their existence. Since there've been several good posts with usage example now buried into list archives, I think this would go a long way helping people get it right if those were part of the kernel documentation as well. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
