| Message ID | 1387557274-22583-7-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[ dropped devicetree, we're clock specific here ] On Fri, Dec 20, 2013 at 18:34 +0200, Tero Kristo wrote: > > Divider clock can now be registered to use low level register access ops. > Preferred initialization method is via clock description. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/clk-divider.c | 22 +++++++++++++++++++--- > include/linux/clk-provider.h | 4 ++++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 8cfed5c..887e2d8 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -108,7 +108,12 @@ 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; > + if (divider->ll_ops) > + val = divider->ll_ops->clk_readl(divider->reg); > + else > + val = clk_readl(divider->reg); Should this not better always use an ll_ops structure, which either is individual to the clock item, or is "global" for a platform, yet can get re-registered at runtime (see the comment on 06/49)? And why are you referencing clk_readl() instead of clk_readl_default() which you specifically have introduced in the previous patch? Adding a copy of the routine and using both the copy and the original doesn't look right. virtually yours Gerhard Sittig
On 12/22/2013 07:52 PM, Gerhard Sittig wrote: > [ dropped devicetree, we're clock specific here ] > > On Fri, Dec 20, 2013 at 18:34 +0200, Tero Kristo wrote: >> >> Divider clock can now be registered to use low level register access ops. >> Preferred initialization method is via clock description. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/clk-divider.c | 22 +++++++++++++++++++--- >> include/linux/clk-provider.h | 4 ++++ >> 2 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c >> index 8cfed5c..887e2d8 100644 >> --- a/drivers/clk/clk-divider.c >> +++ b/drivers/clk/clk-divider.c >> @@ -108,7 +108,12 @@ 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; >> + if (divider->ll_ops) >> + val = divider->ll_ops->clk_readl(divider->reg); >> + else >> + val = clk_readl(divider->reg); > > Should this not better always use an ll_ops structure, which > either is individual to the clock item, or is "global" for a > platform, yet can get re-registered at runtime (see the comment > on 06/49)? And why are you referencing clk_readl() instead of > clk_readl_default() which you specifically have introduced in the > previous patch? Adding a copy of the routine and using both the > copy and the original doesn't look right. In some cases, the clock data is defined statically during compile time and here, ll_ops can be (and for OMAP cases at least is) NULL. I had kind of a global ll_ops definition in some of the earlier versions of this series, but it was frowned upon by some of the maintainers thus I dropped it. -Tero -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 03, 2014 at 11:17 +0200, Tero Kristo wrote: > > On 12/22/2013 07:52 PM, Gerhard Sittig wrote: > >[ dropped devicetree, we're clock specific here ] > > > >On Fri, Dec 20, 2013 at 18:34 +0200, Tero Kristo wrote: > >> > >>Divider clock can now be registered to use low level register access ops. > >>Preferred initialization method is via clock description. > >> > >>Signed-off-by: Tero Kristo <t-kristo@ti.com> > >>--- > >> drivers/clk/clk-divider.c | 22 +++++++++++++++++++--- > >> include/linux/clk-provider.h | 4 ++++ > >> 2 files changed, 23 insertions(+), 3 deletions(-) > >> > >>diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > >>index 8cfed5c..887e2d8 100644 > >>--- a/drivers/clk/clk-divider.c > >>+++ b/drivers/clk/clk-divider.c > >>@@ -108,7 +108,12 @@ 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; > >>+ if (divider->ll_ops) > >>+ val = divider->ll_ops->clk_readl(divider->reg); > >>+ else > >>+ val = clk_readl(divider->reg); > > > >Should this not better always use an ll_ops structure, which > >either is individual to the clock item, or is "global" for a > >platform, yet can get re-registered at runtime (see the comment > >on 06/49)? And why are you referencing clk_readl() instead of > >clk_readl_default() which you specifically have introduced in the > >previous patch? Adding a copy of the routine and using both the > >copy and the original doesn't look right. > > In some cases, the clock data is defined statically during compile > time and here, ll_ops can be (and for OMAP cases at least is) NULL. > I had kind of a global ll_ops definition in some of the earlier > versions of this series, but it was frowned upon by some of the > maintainers thus I dropped it. Well, admittedly v11 was the first version that I have seen/noticed. But the static declaration you talk about and the ll_ops potentially being NULL was discussed there. My response was about replacing the above clk_readl() call with some ll_ops dereference while the ll_ops to use gets determined before, as 'divider' need not have one. Consider something like this - have default routines which do readl() and writel(), and have a default ll_ops struct which references the default routines - have a default ll_ops struct _pointer_ that by default references the default readl/writel struct, yet can get changed to something else for a whole platform or architecture - never use routines directly upon hardware access, but instead always dereference an ll_ops struct that gets determined so: - start with the clock item's ll_ops reference - use the global ll_ops pointer if the above is NULL - use the pointer to the default ll_ops struct if still NULL (this protects against NULL platform specs, which should not happen yet are possible, and the test is cheap) In pseudo code, this would then be init: default_struct = { default_read, default_write, NULL, }; default_ops = &default_struct; setup: /* allow platforms to optionally change default_ops */ /* allow clk items to have individual clk->ll_ops */ use: ops = divider->ll_ops; if (!ops) ops = default_ops; if (!ops) ops = &default_struct; val = ops->clk_readl(ops, ...); This way - you always end up with an ll_ops reference which leads to routines (and potential additional data that can be specific to a clock item, like regmap data), for both "dynamically" registered clock items as well as "static" declarations - one initialization step can apply different accessors for a whole platform, while the default behaviour equals the current implementation - individual clock items can specify their accessor routine and data, or may go with the builtin or the preset defaults The biggest change in cost here is the indirect routine call in contrast to the static inline of the current implementation, while this additional cost of calling a real wrapper routine can't get avoided in the first place. Comparing pointers and falling back in the absence of individual overrides should not outweight the above additional cost of calling the routine. The always identical logic to determine which routine to call can get implemented in a static inline and then won't clutter the actual clock handling logic. If regmap access is rather popular a method, and requires only little additional information, it might receive more explicit support such that users need not hand-roll the same logic over and over again, yet not too much waste occurs to non-regmap items. But available information suggests that currently there is only one user for regmap style access to clock related hardware (Steven's). This could change in the future. virtually yours Gerhard Sittig
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 8cfed5c..887e2d8 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -108,7 +108,12 @@ 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; + if (divider->ll_ops) + val = divider->ll_ops->clk_readl(divider->reg); + else + val = clk_readl(divider->reg); + + val >>= divider->shift; val &= div_mask(divider); div = _get_div(divider, val); @@ -234,11 +239,17 @@ 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); + if (divider->ll_ops) + val = divider->ll_ops->clk_readl(divider->reg); + else + val = clk_readl(divider->reg); val &= ~(div_mask(divider) << divider->shift); } val |= value << divider->shift; - clk_writel(val, divider->reg); + if (divider->ll_ops) + divider->ll_ops->clk_writel(val, divider->reg); + else + clk_writel(val, divider->reg); if (divider->lock) spin_unlock_irqrestore(divider->lock, flags); @@ -291,6 +302,7 @@ static struct clk *_register_divider(struct device *dev, const char *name, div->lock = lock; div->hw.init = &init; div->table = table; + div->ll_ops = &clk_ll_ops_default; /* register the clock */ clk = clk_register(dev, &div->hw); @@ -368,6 +380,10 @@ struct clk_hw *clk_register_divider_desc(struct device *dev, divider->flags = hw_desc->flags; divider->table = hw_desc->table; divider->lock = hw_desc->lock; + divider->ll_ops = hw_desc->ll_ops; + + if (!divider->ll_ops) + divider->ll_ops = &clk_ll_ops_default; if (!desc->ops) desc->ops = &clk_divider_ops; diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 671dff4..f082a89 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -322,6 +322,7 @@ struct clk_div_table { * * @hw: handle between common and hardware-specific interfaces * @reg: register containing the divider + * @ll_ops: low-level ops for accessing the register * @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 @@ -350,6 +351,7 @@ struct clk_div_table { struct clk_divider { struct clk_hw hw; void __iomem *reg; + struct clk_ll_ops *ll_ops; u8 shift; u8 width; u8 flags; @@ -361,6 +363,7 @@ struct clk_divider { * struct clk_divider_desc - init descriptor for divider clock * @desc: handle between common and hardware-specific interfaces * @reg: register containing the divider + * @ll_ops: low-level ops for accessing the register * @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 @@ -369,6 +372,7 @@ struct clk_divider { struct clk_divider_desc { struct clk_desc desc; void __iomem *reg; + struct clk_ll_ops *ll_ops; u8 shift; u8 width; u8 flags;
Divider clock can now be registered to use low level register access ops. Preferred initialization method is via clock description. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/clk-divider.c | 22 +++++++++++++++++++--- include/linux/clk-provider.h | 4 ++++ 2 files changed, 23 insertions(+), 3 deletions(-)