| Message ID | 1382716658-6964-2-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/25/2013 10:56 AM, Tero Kristo wrote: [...] > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 7e59253..63ff78c 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h [...] > -static inline u32 clk_readl(u32 __iomem *reg) > +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) > { > - return readl(reg); > + u32 val; > + > + if (regmap) > + regmap_read(regmap, (u32)reg, &val); > + else > + val = readl(reg); > + return val; > } > > -static inline void clk_writel(u32 val, u32 __iomem *reg) > +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) > { > - writel(val, reg); > + if (regmap) > + regmap_write(regmap, (u32)reg, val); > + else > + writel(val, reg); > } > > #endif /* CONFIG_COMMON_CLK */ > Might it not be better to introduce regmap variants? static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap *regmap) and corresponding readl? that allows cleaner readability for clk drivers that use regmap and those that dont. regmap can also return error value that could also be handled as a result.
On 10/31/2013 04:03 PM, Nishanth Menon wrote: > On 10/25/2013 10:56 AM, Tero Kristo wrote: > [...] >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index 7e59253..63ff78c 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h > > [...] >> -static inline u32 clk_readl(u32 __iomem *reg) >> +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) >> { >> - return readl(reg); >> + u32 val; >> + >> + if (regmap) >> + regmap_read(regmap, (u32)reg, &val); >> + else >> + val = readl(reg); >> + return val; >> } >> >> -static inline void clk_writel(u32 val, u32 __iomem *reg) >> +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) >> { >> - writel(val, reg); >> + if (regmap) >> + regmap_write(regmap, (u32)reg, val); >> + else >> + writel(val, reg); >> } >> >> #endif /* CONFIG_COMMON_CLK */ >> > > Might it not be better to introduce regmap variants? > static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap > *regmap) > and corresponding readl? that allows cleaner readability for clk > drivers that use regmap and those that dont. Well, doing that will introduce a lot of redundant code, as the checks for the presence of regmap must be copied all over the place. With this patch, all the generic clock drivers support internally both regmap or non-regmap register accesses. > regmap can also return error value that could also be handled as a result. True, however if the regmap fails for the clock code, not sure what we can do but panic. If this code is expanded, it is probably better to not inline it anymore. -Tero
On 10/31/2013 09:40 AM, Tero Kristo wrote: > On 10/31/2013 04:03 PM, Nishanth Menon wrote: >> On 10/25/2013 10:56 AM, Tero Kristo wrote: >> [...] >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >>> index 7e59253..63ff78c 100644 >>> --- a/include/linux/clk-provider.h >>> +++ b/include/linux/clk-provider.h >> >> [...] >>> -static inline u32 clk_readl(u32 __iomem *reg) >>> +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) >>> { >>> - return readl(reg); >>> + u32 val; >>> + >>> + if (regmap) >>> + regmap_read(regmap, (u32)reg, &val); >>> + else >>> + val = readl(reg); >>> + return val; >>> } >>> >>> -static inline void clk_writel(u32 val, u32 __iomem *reg) >>> +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) >>> { >>> - writel(val, reg); >>> + if (regmap) >>> + regmap_write(regmap, (u32)reg, val); >>> + else >>> + writel(val, reg); >>> } >>> >>> #endif /* CONFIG_COMMON_CLK */ >>> >> >> Might it not be better to introduce regmap variants? >> static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap >> *regmap) >> and corresponding readl? that allows cleaner readability for clk >> drivers that use regmap and those that dont. > > Well, doing that will introduce a lot of redundant code, as the checks > for the presence of regmap must be copied all over the place. With this > patch, all the generic clock drivers support internally both regmap or > non-regmap register accesses. > using function pointers might be an appropriate solution. in general a low level reg access api that uses two different approaches sounds a little.. umm.. fishy.. >> regmap can also return error value that could also be handled as a result. > > True, however if the regmap fails for the clock code, not sure what we > can do but panic. If this code is expanded, it is probably better to not > inline it anymore. > let the compiler deal with that decision. regmap operation fail should be percollated back to initiator of the request. in some cases that will be ir-recoverable, but on other cases panic might be the right answer - at the low level we are in no position to make that distinction.
On 10/31/2013 05:46 PM, Nishanth Menon wrote: > On 10/31/2013 09:40 AM, Tero Kristo wrote: >> On 10/31/2013 04:03 PM, Nishanth Menon wrote: >>> On 10/25/2013 10:56 AM, Tero Kristo wrote: >>> [...] >>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >>>> index 7e59253..63ff78c 100644 >>>> --- a/include/linux/clk-provider.h >>>> +++ b/include/linux/clk-provider.h >>> >>> [...] >>>> -static inline u32 clk_readl(u32 __iomem *reg) >>>> +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) >>>> { >>>> - return readl(reg); >>>> + u32 val; >>>> + >>>> + if (regmap) >>>> + regmap_read(regmap, (u32)reg, &val); >>>> + else >>>> + val = readl(reg); >>>> + return val; >>>> } >>>> >>>> -static inline void clk_writel(u32 val, u32 __iomem *reg) >>>> +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) >>>> { >>>> - writel(val, reg); >>>> + if (regmap) >>>> + regmap_write(regmap, (u32)reg, val); >>>> + else >>>> + writel(val, reg); >>>> } >>>> >>>> #endif /* CONFIG_COMMON_CLK */ >>>> >>> >>> Might it not be better to introduce regmap variants? >>> static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap >>> *regmap) >>> and corresponding readl? that allows cleaner readability for clk >>> drivers that use regmap and those that dont. >> >> Well, doing that will introduce a lot of redundant code, as the checks >> for the presence of regmap must be copied all over the place. With this >> patch, all the generic clock drivers support internally both regmap or >> non-regmap register accesses. >> > > using function pointers might be an appropriate solution. in general a > low level reg access api that uses two different approaches sounds a > little.. umm.. fishy.. Initial work I did for v9 had clk_reg_ops struct in place which pretty much did this, however Mike recommended looking at the regmap so I did. I could put the reg_ops struct back and just have it use internally regmap if that would be better, but I guess we need comment from Mike how he wants this to be done. We could have: struct clk_reg_ops { int (*clk_readl)(u32 addr, u32 *val); int (*clk_writel)(u32 addr, u32 val); }; struct clk_foo { ... void __iomem *reg; struct clk_reg_ops *regops; }; > >>> regmap can also return error value that could also be handled as a result. >> >> True, however if the regmap fails for the clock code, not sure what we >> can do but panic. If this code is expanded, it is probably better to not >> inline it anymore. >> > let the compiler deal with that decision. regmap operation fail should > be percollated back to initiator of the request. in some cases that > will be ir-recoverable, but on other cases panic might be the right > answer - at the low level we are in no position to make that distinction. Currently, none of the clock drivers handle failures for regmap operations, I can of course add some sort of support for this given what we do with above point and what the API for the register accesses ends up like. -Tero
Hi Tero, On Friday 25 of October 2013 18:56:55 Tero Kristo wrote: > Previously, only direct register read/write was supported. Now a > per-clock regmap can be provided for same purpose, which allows the > clock drivers to access clock registers behind e.g. I2C bus. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/clk-divider.c | 6 +++--- > drivers/clk/clk-gate.c | 6 +++--- > drivers/clk/clk-mux.c | 6 +++--- > include/linux/clk-provider.h | 23 +++++++++++++++++++---- > 4 files changed, 28 insertions(+), 13 deletions(-) The idea itself sounds not bad, but patch description fails to specify what is the need for this. I mean, if you know that this will have some actual users, it is nice to specify them. Also IMHO the variant with clk_reg_ops (but with regmap used) would look a bit better in terms of readability and extensibility, but I'm leaving this completely to Mike's decision. (OR maybe you could migrate this fully to regmap, which can do MMIO accesses as well, without the need of having separate code paths for both in clock code?) Best regards, Tomasz
On Thu, Oct 31, 2013 at 16:40 +0200, Tero Kristo wrote: > > On 10/31/2013 04:03 PM, Nishanth Menon wrote: > >On 10/25/2013 10:56 AM, Tero Kristo wrote: > >[...] > >>diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > >>index 7e59253..63ff78c 100644 > >>--- a/include/linux/clk-provider.h > >>+++ b/include/linux/clk-provider.h > > > >[...] > >>-static inline u32 clk_readl(u32 __iomem *reg) > >>+static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) > >> { > >>- return readl(reg); > >>+ u32 val; > >>+ > >>+ if (regmap) > >>+ regmap_read(regmap, (u32)reg, &val); > >>+ else > >>+ val = readl(reg); > >>+ return val; > >> } > >> > >>-static inline void clk_writel(u32 val, u32 __iomem *reg) > >>+static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) > >> { > >>- writel(val, reg); > >>+ if (regmap) > >>+ regmap_write(regmap, (u32)reg, val); > >>+ else > >>+ writel(val, reg); > >> } > >> > >> #endif /* CONFIG_COMMON_CLK */ > >> > > > >Might it not be better to introduce regmap variants? > >static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap > >*regmap) > >and corresponding readl? that allows cleaner readability for clk > >drivers that use regmap and those that dont. > > Well, doing that will introduce a lot of redundant code, as the > checks for the presence of regmap must be copied all over the place. > With this patch, all the generic clock drivers support internally > both regmap or non-regmap register accesses. Please keep in mind that the "identity" of clk_readl() and readl() only applies in the current source code (ARM only use of common CCF primitives), while patches are pending (currently under review and receiving further improvement) which introduce several alternative implementations of clk_readl() depending on the platform. Making all of them duplicate the "regmap vs direct register access" branch would be as bad. Keeping one set of clk_readl() and clk_writel() routines and adding #ifdefs around the direct register access would be rather ugly, and I understand that preprocessor ifdef counts should get reduced instead of introduced. virtually yours Gerhard Sittig
On 11/05/2013 11:43 PM, Gerhard Sittig wrote: > On Thu, Oct 31, 2013 at 16:40 +0200, Tero Kristo wrote: >> >> On 10/31/2013 04:03 PM, Nishanth Menon wrote: >>> On 10/25/2013 10:56 AM, Tero Kristo wrote: >>> [...] >>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >>>> index 7e59253..63ff78c 100644 >>>> --- a/include/linux/clk-provider.h >>>> +++ b/include/linux/clk-provider.h >>> >>> [...] >>>> -static inline u32 clk_readl(u32 __iomem *reg) >>>> +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) >>>> { >>>> - return readl(reg); >>>> + u32 val; >>>> + >>>> + if (regmap) >>>> + regmap_read(regmap, (u32)reg, &val); >>>> + else >>>> + val = readl(reg); >>>> + return val; >>>> } >>>> >>>> -static inline void clk_writel(u32 val, u32 __iomem *reg) >>>> +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) >>>> { >>>> - writel(val, reg); >>>> + if (regmap) >>>> + regmap_write(regmap, (u32)reg, val); >>>> + else >>>> + writel(val, reg); >>>> } >>>> >>>> #endif /* CONFIG_COMMON_CLK */ >>>> >>> >>> Might it not be better to introduce regmap variants? >>> static inline void clk_regmap_writel(u32 val, u32 reg, struct regmap >>> *regmap) >>> and corresponding readl? that allows cleaner readability for clk >>> drivers that use regmap and those that dont. >> >> Well, doing that will introduce a lot of redundant code, as the >> checks for the presence of regmap must be copied all over the place. >> With this patch, all the generic clock drivers support internally >> both regmap or non-regmap register accesses. > > Please keep in mind that the "identity" of clk_readl() and > readl() only applies in the current source code (ARM only use of > common CCF primitives), while patches are pending (currently > under review and receiving further improvement) which introduce > several alternative implementations of clk_readl() depending on > the platform. Making all of them duplicate the "regmap vs direct > register access" branch would be as bad. Keeping one set of > clk_readl() and clk_writel() routines and adding #ifdefs around > the direct register access would be rather ugly, and I understand > that preprocessor ifdef counts should get reduced instead of > introduced. So, it might be better to provide platform / or clock specific clk_reg_ops or something, so we can cover all the possible cases easily. The requirement from TI SoC point of view is that: 1) need to be able to specify custom register read/write ops 2) need to be able to provide an index parameter to the read/write ops 3) need to be able to provide a register offset to the read/write ops This could be done in following way for example: clk_readl / clk_writel would be accessed through a single platform specific struct which provides function pointers to both. The content of 'reg' provided to the clk_readl / clk_writel would be internally interpreted as a struct omap_clk_reg_internal { u16 offset; u16 index; }; ... the index part would be used to select the appropriate regmap to use, and TI SoC:s would be using 3...4 regmaps for actually accessing the physical registers themselves. -Tero > > > virtually yours > Gerhard Sittig >
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 8d3009e..9c17b1a 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 = clk_readl(divider->reg) >> divider->shift; + val = clk_readl(divider->reg, divider->regmap) >> 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 = clk_readl(divider->reg); + val = clk_readl(divider->reg, divider->regmap); val &= ~(div_mask(divider) << divider->shift); } val |= value << divider->shift; - clk_writel(val, divider->reg); + clk_writel(val, divider->reg, divider->regmap); if (divider->lock) spin_unlock_irqrestore(divider->lock, flags); diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index 4a58c55..3c7f686 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 = clk_readl(gate->reg); + reg = clk_readl(gate->reg, gate->regmap); 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); } - clk_writel(reg, gate->reg); + clk_writel(reg, gate->reg, gate->regmap); 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 = clk_readl(gate->reg); + reg = clk_readl(gate->reg, gate->regmap); /* 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 4f96ff3..68eb8c2 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 = clk_readl(mux->reg) >> mux->shift; + val = clk_readl(mux->reg, mux->regmap) >> 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 = clk_readl(mux->reg); + val = clk_readl(mux->reg, mux->regmap); val &= ~(mux->mask << mux->shift); } val |= index << mux->shift; - clk_writel(val, mux->reg); + clk_writel(val, mux->reg, mux->regmap); if (mux->lock) spin_unlock_irqrestore(mux->lock, flags); diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 7e59253..63ff78c 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -13,6 +13,7 @@ #include <linux/clk.h> #include <linux/io.h> +#include <linux/regmap.h> #ifdef CONFIG_COMMON_CLK @@ -209,6 +210,7 @@ void of_fixed_clk_setup(struct device_node *np); * * @hw: handle between common and hardware-specific interfaces * @reg: register controlling gate + * @regmap: regmap for accessing the gate register (if any) * @bit_idx: single bit controlling gate * @flags: hardware-specific flags * @lock: register lock @@ -227,6 +229,7 @@ void of_fixed_clk_setup(struct device_node *np); struct clk_gate { struct clk_hw hw; void __iomem *reg; + struct regmap *regmap; u8 bit_idx; u8 flags; spinlock_t *lock; @@ -251,6 +254,7 @@ struct clk_div_table { * * @hw: handle between common and hardware-specific interfaces * @reg: register containing the divider + * @regmap: regmap for accessing the divider register (if any) * @shift: shift to the divider bit field * @width: width of the divider bit field * @table: array of value/divider pairs, last entry should have div = 0 @@ -279,6 +283,7 @@ struct clk_div_table { struct clk_divider { struct clk_hw hw; void __iomem *reg; + struct regmap *regmap; u8 shift; u8 width; u8 flags; @@ -326,6 +331,7 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name, struct clk_mux { struct clk_hw hw; void __iomem *reg; + struct regmap *regmap; u32 *table; u32 mask; u8 shift; @@ -512,14 +518,23 @@ static inline const char *of_clk_get_parent_name(struct device_node *np, * for improved portability across platforms */ -static inline u32 clk_readl(u32 __iomem *reg) +static inline u32 clk_readl(u32 __iomem *reg, struct regmap *regmap) { - return readl(reg); + u32 val; + + if (regmap) + regmap_read(regmap, (u32)reg, &val); + else + val = readl(reg); + return val; } -static inline void clk_writel(u32 val, u32 __iomem *reg) +static inline void clk_writel(u32 val, u32 __iomem *reg, struct regmap *regmap) { - writel(val, reg); + if (regmap) + regmap_write(regmap, (u32)reg, val); + else + writel(val, reg); } #endif /* CONFIG_COMMON_CLK */
Previously, only direct register read/write was supported. Now a per-clock regmap can be provided for same purpose, which allows the clock drivers to access clock registers behind e.g. I2C bus. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/clk-divider.c | 6 +++--- drivers/clk/clk-gate.c | 6 +++--- drivers/clk/clk-mux.c | 6 +++--- include/linux/clk-provider.h | 23 +++++++++++++++++++---- 4 files changed, 28 insertions(+), 13 deletions(-)