| Message ID | 1387557274-22583-6-git-send-email-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[ added agust@ for mpc5xxx, dropped devicetree ] On Fri, Dec 20, 2013 at 18:34 +0200, Tero Kristo wrote: > > Low level register ops are needed for providing SoC or IP block specific > access routines to clock registers. Subsequent patches add support for > the low level ops for the individual clock drivers. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/clk.c | 28 ++++++++++++++++++++++++++++ > include/linux/clk-provider.h | 17 +++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 29281f6..8bcd1e0 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -34,6 +34,34 @@ static HLIST_HEAD(clk_root_list); > static HLIST_HEAD(clk_orphan_list); > static LIST_HEAD(clk_notifier_list); > > +/** > + * clk_readl_default - default clock register read support function > + * @reg: register to read > + * > + * Default implementation for reading a clock register. > + */ > +static u32 clk_readl_default(void __iomem *reg) > +{ > + return readl(reg); > +} > + > +/** > + * clk_writel_default - default clock register write support function > + * @val: value to write > + * @reg: register to write to > + * > + * Default implementation for writing a clock register. > + */ > +static void clk_writel_default(u32 val, void __iomem *reg) > +{ > + writel(val, reg); > +} > + > +struct clk_ll_ops clk_ll_ops_default = { > + .clk_readl = clk_readl_default, > + .clk_writel = clk_writel_default > +}; > + > /*** locking ***/ > static void clk_prepare_lock(void) > { Mike, Anatolij, this is a HEADS UP for the clock and mpc5xxx maintainers: This patch will break the recently introduced CCF support for MPC512x (currently sitting in -next, to get merged for 3.14), and requires some adjustment. Either the clk_{read,write}l_default() routines' bodies need to depend on the architecture, or the clk_ll_ops_default structure needs to get preset differently depending on the architecture. For least intrusive changes in future use, adding a routine which allows to register a different ll_ops structure could be most appropriate. That would allow to pre-set the ll_ops structure with the readl()/writel() implementation (current state of mainline code, working for the ARM architecture and maybe other little endian peripherals), and allows to register the ioread32be()/iowrite32be() routines for PowerPC, or whatever other platforms or architectures will require. Expecting each individual clock item registration to specify a differing set of ll_ops if readl()/writel() are not appropriate would look weird to me. Further I'd suggest to split this register access aspect out of the TI clock series, and to prepare it already for regmap style access to the hardware registers. See the next comment below. > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index a4f14ae..671dff4 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -198,6 +198,23 @@ struct clk_hw { > const struct clk_init_data *init; > }; > > +/** > + * struct clk_ll_ops - low-level register access ops for a clock > + * @clk_readl: pointer to register read function > + * @clk_writel: pointer to register write function > + * > + * Low-level register access ops are generally used by the basic clock types > + * (clk-gate, clk-mux, clk-divider etc.) to provide support for various > + * low-level hardware interfaces (direct MMIO, regmap etc.), but can also be > + * used by other hardware-specific clock drivers if needed. > + */ > +struct clk_ll_ops { > + u32 (*clk_readl)(void __iomem *reg); > + void (*clk_writel)(u32 val, void __iomem *reg); > +}; > + > +extern struct clk_ll_ops clk_ll_ops_default; > + I'd suggest to add a "strct clk_ll_ops" pointer to the routines' list of arguments, and to add some "user data" pointer to the struct. This would provide more than the "reg" pointer to the routine, e.g. to determine an offset within a register set, and/or to hold a regmap handle. Not adding this extension now would lead to our queueing several patches which will result in potential conflicts, or requiring more cycles than necessary to achieve a working state for currently pending changes. There is another issue with this series: It introduces "alternative" _default() routines (which are verbatim copies of the static inlines from the header), then adjusts the basic clock types (div, gate, mux), but does not remove the then obsolete static inlines from the header. Tero, can you please verify whether the clk_readl() and clk_writel() routines from <linux/clock-provider.h> become obsolete with your patch, or whether any unchanged users remain? Are other parts of v12 still stuck? I only saw 15 of 49 patches. virtually yours Gerhard Sittig
On 12/22/2013 07:39 PM, Gerhard Sittig wrote: > [ added agust@ for mpc5xxx, dropped devicetree ] > > On Fri, Dec 20, 2013 at 18:34 +0200, Tero Kristo wrote: >> >> Low level register ops are needed for providing SoC or IP block specific >> access routines to clock registers. Subsequent patches add support for >> the low level ops for the individual clock drivers. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/clk/clk.c | 28 ++++++++++++++++++++++++++++ >> include/linux/clk-provider.h | 17 +++++++++++++++++ >> 2 files changed, 45 insertions(+) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 29281f6..8bcd1e0 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -34,6 +34,34 @@ static HLIST_HEAD(clk_root_list); >> static HLIST_HEAD(clk_orphan_list); >> static LIST_HEAD(clk_notifier_list); >> >> +/** >> + * clk_readl_default - default clock register read support function >> + * @reg: register to read >> + * >> + * Default implementation for reading a clock register. >> + */ >> +static u32 clk_readl_default(void __iomem *reg) >> +{ >> + return readl(reg); >> +} >> + >> +/** >> + * clk_writel_default - default clock register write support function >> + * @val: value to write >> + * @reg: register to write to >> + * >> + * Default implementation for writing a clock register. >> + */ >> +static void clk_writel_default(u32 val, void __iomem *reg) >> +{ >> + writel(val, reg); >> +} >> + >> +struct clk_ll_ops clk_ll_ops_default = { >> + .clk_readl = clk_readl_default, >> + .clk_writel = clk_writel_default >> +}; >> + >> /*** locking ***/ >> static void clk_prepare_lock(void) >> { > > Mike, Anatolij, this is a HEADS UP for the clock and mpc5xxx > maintainers: This patch will break the recently introduced CCF > support for MPC512x (currently sitting in -next, to get merged > for 3.14), and requires some adjustment. > > Either the clk_{read,write}l_default() routines' bodies need to > depend on the architecture, or the clk_ll_ops_default structure > needs to get preset differently depending on the architecture. > > For least intrusive changes in future use, adding a routine which > allows to register a different ll_ops structure could be most > appropriate. That would allow to pre-set the ll_ops structure > with the readl()/writel() implementation (current state of > mainline code, working for the ARM architecture and maybe other > little endian peripherals), and allows to register the > ioread32be()/iowrite32be() routines for PowerPC, or whatever > other platforms or architectures will require. > > Expecting each individual clock item registration to specify a > differing set of ll_ops if readl()/writel() are not appropriate > would look weird to me. > > > Further I'd suggest to split this register access aspect out of > the TI clock series, and to prepare it already for regmap style > access to the hardware registers. See the next comment below. This sounds like a good idea to me, seeing it is blocking lots of other things. >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index a4f14ae..671dff4 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h >> @@ -198,6 +198,23 @@ struct clk_hw { >> const struct clk_init_data *init; >> }; >> >> +/** >> + * struct clk_ll_ops - low-level register access ops for a clock >> + * @clk_readl: pointer to register read function >> + * @clk_writel: pointer to register write function >> + * >> + * Low-level register access ops are generally used by the basic clock types >> + * (clk-gate, clk-mux, clk-divider etc.) to provide support for various >> + * low-level hardware interfaces (direct MMIO, regmap etc.), but can also be >> + * used by other hardware-specific clock drivers if needed. >> + */ >> +struct clk_ll_ops { >> + u32 (*clk_readl)(void __iomem *reg); >> + void (*clk_writel)(u32 val, void __iomem *reg); >> +}; >> + >> +extern struct clk_ll_ops clk_ll_ops_default; >> + > > I'd suggest to add a "strct clk_ll_ops" pointer to the routines' > list of arguments, and to add some "user data" pointer to the > struct. > > This would provide more than the "reg" pointer to the routine, > e.g. to determine an offset within a register set, and/or to hold > a regmap handle. > > Not adding this extension now would lead to our queueing several > patches which will result in potential conflicts, or requiring > more cycles than necessary to achieve a working state for > currently pending changes. Yea I think this sounds like a good idea. > > > There is another issue with this series: It introduces > "alternative" _default() routines (which are verbatim copies of > the static inlines from the header), then adjusts the basic clock > types (div, gate, mux), but does not remove the then obsolete > static inlines from the header. > > Tero, can you please verify whether the clk_readl() and > clk_writel() routines from <linux/clock-provider.h> become > obsolete with your patch, or whether any unchanged users remain? Just did a quick grep and it seems you are right, the routines become obsolete and could be removed (or alternatively just rename the clk_readl/writel_default to clk_readl/writel.) > > > Are other parts of v12 still stuck? I only saw 15 of 49 patches. Rest of v12 was identical copy of v11, so I didn't repost them. -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 01/03/14 01:13, Tero Kristo wrote: > On 12/22/2013 07:39 PM, Gerhard Sittig wrote: >> >> >> Further I'd suggest to split this register access aspect out of >> the TI clock series, and to prepare it already for regmap style >> access to the hardware registers. See the next comment below. > > This sounds like a good idea to me, seeing it is blocking lots of > other things. This ll_ops struct looks like a simplified regmap. Have you seen my series that adds regmap support to the common clock framework[1]? Is there any reason why you can't use those patches and layer some patches on top to add support for regmap to the basic clock types? [1] https://lkml.org/lkml/2013/12/23/461
On 01/03/2014 09:48 PM, Stephen Boyd wrote: > On 01/03/14 01:13, Tero Kristo wrote: >> On 12/22/2013 07:39 PM, Gerhard Sittig wrote: >>> >>> >>> Further I'd suggest to split this register access aspect out of >>> the TI clock series, and to prepare it already for regmap style >>> access to the hardware registers. See the next comment below. >> >> This sounds like a good idea to me, seeing it is blocking lots of >> other things. > > This ll_ops struct looks like a simplified regmap. Have you seen my > series that adds regmap support to the common clock framework[1]? Is > there any reason why you can't use those patches and layer some patches > on top to add support for regmap to the basic clock types? > > [1] https://lkml.org/lkml/2013/12/23/461 Yea I've seen that. I also used regmap myself on some earlier rev, but it is rather an overkill solution to my purposes, and it also adds additional latencies to the register access. -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
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 29281f6..8bcd1e0 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -34,6 +34,34 @@ static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list); static LIST_HEAD(clk_notifier_list); +/** + * clk_readl_default - default clock register read support function + * @reg: register to read + * + * Default implementation for reading a clock register. + */ +static u32 clk_readl_default(void __iomem *reg) +{ + return readl(reg); +} + +/** + * clk_writel_default - default clock register write support function + * @val: value to write + * @reg: register to write to + * + * Default implementation for writing a clock register. + */ +static void clk_writel_default(u32 val, void __iomem *reg) +{ + writel(val, reg); +} + +struct clk_ll_ops clk_ll_ops_default = { + .clk_readl = clk_readl_default, + .clk_writel = clk_writel_default +}; + /*** locking ***/ static void clk_prepare_lock(void) { diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index a4f14ae..671dff4 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -198,6 +198,23 @@ struct clk_hw { const struct clk_init_data *init; }; +/** + * struct clk_ll_ops - low-level register access ops for a clock + * @clk_readl: pointer to register read function + * @clk_writel: pointer to register write function + * + * Low-level register access ops are generally used by the basic clock types + * (clk-gate, clk-mux, clk-divider etc.) to provide support for various + * low-level hardware interfaces (direct MMIO, regmap etc.), but can also be + * used by other hardware-specific clock drivers if needed. + */ +struct clk_ll_ops { + u32 (*clk_readl)(void __iomem *reg); + void (*clk_writel)(u32 val, void __iomem *reg); +}; + +extern struct clk_ll_ops clk_ll_ops_default; + /* * DOC: Basic clock implementations common to many platforms *
Low level register ops are needed for providing SoC or IP block specific access routines to clock registers. Subsequent patches add support for the low level ops for the individual clock drivers. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/clk/clk.c | 28 ++++++++++++++++++++++++++++ include/linux/clk-provider.h | 17 +++++++++++++++++ 2 files changed, 45 insertions(+)