diff mbox

[PATCHv12,06/49] clk: add support for low level register ops

Message ID 1387557274-22583-6-git-send-email-t-kristo@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tero Kristo Dec. 20, 2013, 4:34 p.m. UTC
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(+)

Comments

Gerhard Sittig Dec. 22, 2013, 5:39 p.m. UTC | #1
[ 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
Tero Kristo Jan. 3, 2014, 9:13 a.m. UTC | #2
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
Stephen Boyd Jan. 3, 2014, 7:48 p.m. UTC | #3
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
Tero Kristo Jan. 7, 2014, 7:44 a.m. UTC | #4
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
diff mbox

Patch

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
  *