Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Tero Kristo]

On 01/09/16 01:31, Stephen Boyd wrote:

On 08/31, Tero Kristo wrote:

On 24/08/16 11:34, Stephen Boyd wrote:

On 08/19, Nishanth Menon wrote:

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index ..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c
@@ -0,0 +1,539 @@
+   return (int)new_rate;


determine rate should return a negative number on failure and 0
on success. The actual rate that was found should go into
req->rate. This looks broken.


Yea it seems broken, I wonder how we haven't seen any issues with
this in testing Apparently positive return values from this are
interpreted as success. Having a quick look at clk.c seems to
confirm this.

Anyway, will fix.


True, perhaps we should fix that so we don't use a long to hold
the int return value either.


+ *
+ * Gets a handle to an existing TI SCI clock, or builds a new clock
+ * entry and registers it with the common clock framework. Called from
+ * the common clock framework, when a corresponding of_clk_get call is
+ * executed, or recursively from itself when parsing parent clocks.
+ * Returns a pointer to the clock struct, or ERR_PTR value in failure.
+ */


Please move this driver to clk_hw_register() and friends. This on
the fly clk generation is scary considering how we hold locks
while the provider is asked to give us the pointer, so allocating
and registering clks (basically reentering the CCF again) could
lead to a locking nightmare. Best to avoid that.


Ok, so just converting the driver to use provider->get_hw should be
enough? This seems to be a relatively new API in the CCF. Will look
at that.


Hopefully it will simplify things greatly.


Well, it didn't simplify things greatly, but somewhat. I still need to 
use of_parse_phandle_with_args with one of the helpers for example. Will 
send out v2 in a bit.







+   }
+
+   snprintf(name, 20, "%s:%d:%d", dev_name(provider->dev), sci_clk->dev_id,
+sci_clk->clk_id);


I hope we don't make dev_name() longer than 20 characters


Shall I just increase the size of the buffer and add a length check?
Using kmalloc or something here seems overkill, as the name gets
copied by CCF anyway.


There's kasprintf() which would always make it long enough. I
don't know if it really matters though.


Ok, I will use this one.

-Tero

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Tero Kristo]

On 01/09/16 01:31, Stephen Boyd wrote:

On 08/31, Tero Kristo wrote:

On 24/08/16 11:34, Stephen Boyd wrote:

On 08/19, Nishanth Menon wrote:

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index ..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c
@@ -0,0 +1,539 @@
+   return (int)new_rate;


determine rate should return a negative number on failure and 0
on success. The actual rate that was found should go into
req->rate. This looks broken.


Yea it seems broken, I wonder how we haven't seen any issues with
this in testing Apparently positive return values from this are
interpreted as success. Having a quick look at clk.c seems to
confirm this.

Anyway, will fix.


True, perhaps we should fix that so we don't use a long to hold
the int return value either.


+ *
+ * Gets a handle to an existing TI SCI clock, or builds a new clock
+ * entry and registers it with the common clock framework. Called from
+ * the common clock framework, when a corresponding of_clk_get call is
+ * executed, or recursively from itself when parsing parent clocks.
+ * Returns a pointer to the clock struct, or ERR_PTR value in failure.
+ */


Please move this driver to clk_hw_register() and friends. This on
the fly clk generation is scary considering how we hold locks
while the provider is asked to give us the pointer, so allocating
and registering clks (basically reentering the CCF again) could
lead to a locking nightmare. Best to avoid that.


Ok, so just converting the driver to use provider->get_hw should be
enough? This seems to be a relatively new API in the CCF. Will look
at that.


Hopefully it will simplify things greatly.


Well, it didn't simplify things greatly, but somewhat. I still need to 
use of_parse_phandle_with_args with one of the helpers for example. Will 
send out v2 in a bit.







+   }
+
+   snprintf(name, 20, "%s:%d:%d", dev_name(provider->dev), sci_clk->dev_id,
+sci_clk->clk_id);


I hope we don't make dev_name() longer than 20 characters


Shall I just increase the size of the buffer and add a length check?
Using kmalloc or something here seems overkill, as the name gets
copied by CCF anyway.


There's kasprintf() which would always make it long enough. I
don't know if it really matters though.


Ok, I will use this one.

-Tero

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Stephen Boyd]

On 08/31, Tero Kristo wrote:
> On 24/08/16 11:34, Stephen Boyd wrote:
> >On 08/19, Nishanth Menon wrote:
> >>diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> >>new file mode 100644
> >>index ..6c43e097e6d6
> >>--- /dev/null
> >>+++ b/drivers/clk/keystone/sci-clk.c
> >>@@ -0,0 +1,539 @@
> >>+   return (int)new_rate;
> >
> >determine rate should return a negative number on failure and 0
> >on success. The actual rate that was found should go into
> >req->rate. This looks broken.
> 
> Yea it seems broken, I wonder how we haven't seen any issues with
> this in testing Apparently positive return values from this are
> interpreted as success. Having a quick look at clk.c seems to
> confirm this.
> 
> Anyway, will fix.

True, perhaps we should fix that so we don't use a long to hold
the int return value either.

> >>+ *
> >>+ * Gets a handle to an existing TI SCI clock, or builds a new clock
> >>+ * entry and registers it with the common clock framework. Called from
> >>+ * the common clock framework, when a corresponding of_clk_get call is
> >>+ * executed, or recursively from itself when parsing parent clocks.
> >>+ * Returns a pointer to the clock struct, or ERR_PTR value in failure.
> >>+ */
> >
> >Please move this driver to clk_hw_register() and friends. This on
> >the fly clk generation is scary considering how we hold locks
> >while the provider is asked to give us the pointer, so allocating
> >and registering clks (basically reentering the CCF again) could
> >lead to a locking nightmare. Best to avoid that.
> 
> Ok, so just converting the driver to use provider->get_hw should be
> enough? This seems to be a relatively new API in the CCF. Will look
> at that.

Hopefully it will simplify things greatly.

> 
> >>+   }
> >>+
> >>+   snprintf(name, 20, "%s:%d:%d", dev_name(provider->dev), sci_clk->dev_id,
> >>+sci_clk->clk_id);
> >
> >I hope we don't make dev_name() longer than 20 characters
> 
> Shall I just increase the size of the buffer and add a length check?
> Using kmalloc or something here seems overkill, as the name gets
> copied by CCF anyway.

There's kasprintf() which would always make it long enough. I
don't know if it really matters though.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Stephen Boyd]

On 08/31, Tero Kristo wrote:
> On 24/08/16 11:34, Stephen Boyd wrote:
> >On 08/19, Nishanth Menon wrote:
> >>diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> >>new file mode 100644
> >>index ..6c43e097e6d6
> >>--- /dev/null
> >>+++ b/drivers/clk/keystone/sci-clk.c
> >>@@ -0,0 +1,539 @@
> >>+   return (int)new_rate;
> >
> >determine rate should return a negative number on failure and 0
> >on success. The actual rate that was found should go into
> >req->rate. This looks broken.
> 
> Yea it seems broken, I wonder how we haven't seen any issues with
> this in testing Apparently positive return values from this are
> interpreted as success. Having a quick look at clk.c seems to
> confirm this.
> 
> Anyway, will fix.

True, perhaps we should fix that so we don't use a long to hold
the int return value either.

> >>+ *
> >>+ * Gets a handle to an existing TI SCI clock, or builds a new clock
> >>+ * entry and registers it with the common clock framework. Called from
> >>+ * the common clock framework, when a corresponding of_clk_get call is
> >>+ * executed, or recursively from itself when parsing parent clocks.
> >>+ * Returns a pointer to the clock struct, or ERR_PTR value in failure.
> >>+ */
> >
> >Please move this driver to clk_hw_register() and friends. This on
> >the fly clk generation is scary considering how we hold locks
> >while the provider is asked to give us the pointer, so allocating
> >and registering clks (basically reentering the CCF again) could
> >lead to a locking nightmare. Best to avoid that.
> 
> Ok, so just converting the driver to use provider->get_hw should be
> enough? This seems to be a relatively new API in the CCF. Will look
> at that.

Hopefully it will simplify things greatly.

> 
> >>+   }
> >>+
> >>+   snprintf(name, 20, "%s:%d:%d", dev_name(provider->dev), sci_clk->dev_id,
> >>+sci_clk->clk_id);
> >
> >I hope we don't make dev_name() longer than 20 characters
> 
> Shall I just increase the size of the buffer and add a length check?
> Using kmalloc or something here seems overkill, as the name gets
> copied by CCF anyway.

There's kasprintf() which would always make it long enough. I
don't know if it really matters though.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Tero Kristo]

Hi Stephen,

Thanks for comments, inlined replies below.

On 24/08/16 11:34, Stephen Boyd wrote:

On 08/19, Nishanth Menon wrote:

 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M: Hans Verkuil 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e2d9bd760c84..d1724999be78 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -179,6 +179,19 @@ config COMMON_CLK_NXP
---help---
  Support for clock providers on NXP platforms.

+if COMMON_CLK_KEYSTONE


Why not depends on COMMON_CLK_KEYSTONE? Or make the tristate have
an if COMMON_CLK_KEYSTONE after it? So far an if  hasn't
happened in this file. And is anything inside
drivers/clk/keystone used by this driver? Maybe this if statement
can just be dropped entirely and then the keystone/Makefile
updated?


Yea will fix that.




+
+config TI_SCI_CLK
+   tristate "TI System Control Interface clock drivers"
+   depends on TI_SCI_PROTOCOL || COMPILE_TEST
+   default y


default TI_SCI_PROTOCOL?


That too.




+   help
+ This adds the clock driver support over TI System Control Interface.
+ If you wish to use clock resources from the PMMC firmware, say Y.
+ Otherwise, say N.
+
+endif # COMMON_CLK_KEYSTONE
+
 config COMMON_CLK_PALMAS
tristate "Clock driver for TI Palmas devices"
depends on MFD_PALMAS
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index ..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c
@@ -0,0 +1,539 @@
+/*
+ * SCI Clock driver for keystone based devices
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ * Tero Kristo 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 


Is this include used?


Yes, it seems I get a compile fail if this is not included:

  CC  drivers/clk/keystone/sci-clk.o
drivers/clk/keystone/sci-clk.c: In function 'ti_sci_clk_parse_flags':
drivers/clk/keystone/sci-clk.c:444:3: error: implicit declaration of 
function 'of_clk_get_from_provider' [-Werror=implicit-function-declaration]

   clk = of_clk_get_from_provider();

However, as there is the new API for clk_hw xlate, not sure if this is 
needed once I convert the driver to use that. Will check.





+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SCI_CLK_SSC_ENABLE BIT(0)
+#define SCI_CLK_ALLOW_FREQ_CHANGE  BIT(1)
+#define SCI_CLK_INPUT_TERMINATION  BIT(2)
+
+/**
+ * struct sci_clk_provider - TI SCI clock provider representation
+ * @sci:Handle to the System Control Interface protocol handler
+ * @ops:Pointer to the SCI ops to be used by the clocks
+ * @dev:Device pointer for the clock provider
+ * @clocks: List of all registered clocks
+ * @lock:   Mutex for locking access to the @clocks list
+ */
+struct sci_clk_provider {
+   const struct ti_sci_handle *sci;
+   const struct ti_sci_clk_ops *ops;
+   struct device *dev;
+   struct list_head clocks;
+   struct mutex lock; /* Protects access to the @clocks list */
+};
+
+/**
+ * struct sci_clk - TI SCI clock representation
+ * @hw: Hardware clock cookie for common clock framework
+ * @dev_id: Device index
+ * @clk_id: Clock index
+ * @node:   Clocks list link
+ * @provider:   Master clock provider
+ * @flags:  Flags for the clock
+ */
+struct sci_clk {
+   struct clk_hw hw;
+   u16 dev_id;
+   u8 clk_id;
+   struct list_head node;
+   struct sci_clk_provider *provider;
+   u8 flags;
+};
+
+#define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
+
+/**
+ * sci_clk_prepare - Prepare (enable) a TI SCI clock
+ * @hw: clock to prepare
+ *
+ * Prepares a clock to be actively used. Returns the SCI protocol status.
+ */
+static int sci_clk_prepare(struct clk_hw *hw)
+{
+   struct sci_clk *clk = to_sci_clk(hw);
+   bool enable_ssc = clk->flags & SCI_CLK_SSC_ENABLE;
+   bool allow_freq_change = clk->flags & SCI_CLK_ALLOW_FREQ_CHANGE;
+   bool input_termination = clk->flags & SCI_CLK_INPUT_TERMINATION;
+
+   return clk->provider->ops->get_clock(clk->provider->sci, clk->dev_id,
+clk->clk_id, enable_ssc,
+allow_freq_change,
+input_termination);
+}
+
+/**
+ * sci_clk_unprepare - Un-prepares (disables) a TI SCI clock
+ * @hw: clock to unprepare
+ *
+ 

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Tero Kristo]

Hi Stephen,

Thanks for comments, inlined replies below.

On 24/08/16 11:34, Stephen Boyd wrote:

On 08/19, Nishanth Menon wrote:

 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M: Hans Verkuil 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e2d9bd760c84..d1724999be78 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -179,6 +179,19 @@ config COMMON_CLK_NXP
---help---
  Support for clock providers on NXP platforms.

+if COMMON_CLK_KEYSTONE


Why not depends on COMMON_CLK_KEYSTONE? Or make the tristate have
an if COMMON_CLK_KEYSTONE after it? So far an if  hasn't
happened in this file. And is anything inside
drivers/clk/keystone used by this driver? Maybe this if statement
can just be dropped entirely and then the keystone/Makefile
updated?


Yea will fix that.




+
+config TI_SCI_CLK
+   tristate "TI System Control Interface clock drivers"
+   depends on TI_SCI_PROTOCOL || COMPILE_TEST
+   default y


default TI_SCI_PROTOCOL?


That too.




+   help
+ This adds the clock driver support over TI System Control Interface.
+ If you wish to use clock resources from the PMMC firmware, say Y.
+ Otherwise, say N.
+
+endif # COMMON_CLK_KEYSTONE
+
 config COMMON_CLK_PALMAS
tristate "Clock driver for TI Palmas devices"
depends on MFD_PALMAS
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index ..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c
@@ -0,0 +1,539 @@
+/*
+ * SCI Clock driver for keystone based devices
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ * Tero Kristo 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 


Is this include used?


Yes, it seems I get a compile fail if this is not included:

  CC  drivers/clk/keystone/sci-clk.o
drivers/clk/keystone/sci-clk.c: In function 'ti_sci_clk_parse_flags':
drivers/clk/keystone/sci-clk.c:444:3: error: implicit declaration of 
function 'of_clk_get_from_provider' [-Werror=implicit-function-declaration]

   clk = of_clk_get_from_provider();

However, as there is the new API for clk_hw xlate, not sure if this is 
needed once I convert the driver to use that. Will check.





+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SCI_CLK_SSC_ENABLE BIT(0)
+#define SCI_CLK_ALLOW_FREQ_CHANGE  BIT(1)
+#define SCI_CLK_INPUT_TERMINATION  BIT(2)
+
+/**
+ * struct sci_clk_provider - TI SCI clock provider representation
+ * @sci:Handle to the System Control Interface protocol handler
+ * @ops:Pointer to the SCI ops to be used by the clocks
+ * @dev:Device pointer for the clock provider
+ * @clocks: List of all registered clocks
+ * @lock:   Mutex for locking access to the @clocks list
+ */
+struct sci_clk_provider {
+   const struct ti_sci_handle *sci;
+   const struct ti_sci_clk_ops *ops;
+   struct device *dev;
+   struct list_head clocks;
+   struct mutex lock; /* Protects access to the @clocks list */
+};
+
+/**
+ * struct sci_clk - TI SCI clock representation
+ * @hw: Hardware clock cookie for common clock framework
+ * @dev_id: Device index
+ * @clk_id: Clock index
+ * @node:   Clocks list link
+ * @provider:   Master clock provider
+ * @flags:  Flags for the clock
+ */
+struct sci_clk {
+   struct clk_hw hw;
+   u16 dev_id;
+   u8 clk_id;
+   struct list_head node;
+   struct sci_clk_provider *provider;
+   u8 flags;
+};
+
+#define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
+
+/**
+ * sci_clk_prepare - Prepare (enable) a TI SCI clock
+ * @hw: clock to prepare
+ *
+ * Prepares a clock to be actively used. Returns the SCI protocol status.
+ */
+static int sci_clk_prepare(struct clk_hw *hw)
+{
+   struct sci_clk *clk = to_sci_clk(hw);
+   bool enable_ssc = clk->flags & SCI_CLK_SSC_ENABLE;
+   bool allow_freq_change = clk->flags & SCI_CLK_ALLOW_FREQ_CHANGE;
+   bool input_termination = clk->flags & SCI_CLK_INPUT_TERMINATION;
+
+   return clk->provider->ops->get_clock(clk->provider->sci, clk->dev_id,
+clk->clk_id, enable_ssc,
+allow_freq_change,
+input_termination);
+}
+
+/**
+ * sci_clk_unprepare - Un-prepares (disables) a TI SCI clock
+ * @hw: clock to unprepare
+ *
+ * Un-prepares a clock from active 

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Stephen Boyd]

On 08/19, Nishanth Menon wrote:
>  THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>  M:   Hans Verkuil 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index e2d9bd760c84..d1724999be78 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -179,6 +179,19 @@ config COMMON_CLK_NXP
>   ---help---
> Support for clock providers on NXP platforms.
>  
> +if COMMON_CLK_KEYSTONE

Why not depends on COMMON_CLK_KEYSTONE? Or make the tristate have
an if COMMON_CLK_KEYSTONE after it? So far an if  hasn't
happened in this file. And is anything inside
drivers/clk/keystone used by this driver? Maybe this if statement
can just be dropped entirely and then the keystone/Makefile
updated?

> +
> +config TI_SCI_CLK
> + tristate "TI System Control Interface clock drivers"
> + depends on TI_SCI_PROTOCOL || COMPILE_TEST
> + default y

default TI_SCI_PROTOCOL?

> + help
> +   This adds the clock driver support over TI System Control Interface.
> +   If you wish to use clock resources from the PMMC firmware, say Y.
> +   Otherwise, say N.
> +
> +endif # COMMON_CLK_KEYSTONE
> +
>  config COMMON_CLK_PALMAS
>   tristate "Clock driver for TI Palmas devices"
>   depends on MFD_PALMAS
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> new file mode 100644
> index ..6c43e097e6d6
> --- /dev/null
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -0,0 +1,539 @@
> +/*
> + * SCI Clock driver for keystone based devices
> + *
> + * Copyright (C) 2015-2016 Texas Instruments Incorporated - 
> http://www.ti.com/
> + *   Tero Kristo 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 

Is this include used?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SCI_CLK_SSC_ENABLE   BIT(0)
> +#define SCI_CLK_ALLOW_FREQ_CHANGEBIT(1)
> +#define SCI_CLK_INPUT_TERMINATIONBIT(2)
> +
> +/**
> + * struct sci_clk_provider - TI SCI clock provider representation
> + * @sci:Handle to the System Control Interface protocol handler
> + * @ops:Pointer to the SCI ops to be used by the clocks
> + * @dev:Device pointer for the clock provider
> + * @clocks: List of all registered clocks
> + * @lock:   Mutex for locking access to the @clocks list
> + */
> +struct sci_clk_provider {
> + const struct ti_sci_handle *sci;
> + const struct ti_sci_clk_ops *ops;
> + struct device *dev;
> + struct list_head clocks;
> + struct mutex lock; /* Protects access to the @clocks list */
> +};
> +
> +/**
> + * struct sci_clk - TI SCI clock representation
> + * @hw:   Hardware clock cookie for common clock framework
> + * @dev_id:   Device index
> + * @clk_id:   Clock index
> + * @node: Clocks list link
> + * @provider: Master clock provider
> + * @flags:Flags for the clock
> + */
> +struct sci_clk {
> + struct clk_hw hw;
> + u16 dev_id;
> + u8 clk_id;
> + struct list_head node;
> + struct sci_clk_provider *provider;
> + u8 flags;
> +};
> +
> +#define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
> +
> +/**
> + * sci_clk_prepare - Prepare (enable) a TI SCI clock
> + * @hw: clock to prepare
> + *
> + * Prepares a clock to be actively used. Returns the SCI protocol status.
> + */
> +static int sci_clk_prepare(struct clk_hw *hw)
> +{
> + struct sci_clk *clk = to_sci_clk(hw);
> + bool enable_ssc = clk->flags & SCI_CLK_SSC_ENABLE;
> + bool allow_freq_change = clk->flags & SCI_CLK_ALLOW_FREQ_CHANGE;
> + bool input_termination = clk->flags & SCI_CLK_INPUT_TERMINATION;
> +
> + return clk->provider->ops->get_clock(clk->provider->sci, clk->dev_id,
> +  clk->clk_id, enable_ssc,
> +  allow_freq_change,
> +  input_termination);
> +}
> +
> +/**
> + * sci_clk_unprepare - Un-prepares (disables) a TI SCI clock
> + * @hw: clock to unprepare
> + *
> + * Un-prepares a clock from active state.
> + */
> +static void sci_clk_unprepare(struct clk_hw *hw)
> +{
> + struct sci_clk *clk = to_sci_clk(hw);
> + int ret;
> +
> + ret = clk->provider->ops->put_clock(clk->provider->sci, clk->dev_id,
> + clk->clk_id);
> + if (ret)
> + dev_err(clk->provider->dev,
> + "unprepare failed for dev=%d, clk=%d, ret=%d\n",
> +  

Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Stephen Boyd]

On 08/19, Nishanth Menon wrote:
>  THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
>  M:   Hans Verkuil 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index e2d9bd760c84..d1724999be78 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -179,6 +179,19 @@ config COMMON_CLK_NXP
>   ---help---
> Support for clock providers on NXP platforms.
>  
> +if COMMON_CLK_KEYSTONE

Why not depends on COMMON_CLK_KEYSTONE? Or make the tristate have
an if COMMON_CLK_KEYSTONE after it? So far an if  hasn't
happened in this file. And is anything inside
drivers/clk/keystone used by this driver? Maybe this if statement
can just be dropped entirely and then the keystone/Makefile
updated?

> +
> +config TI_SCI_CLK
> + tristate "TI System Control Interface clock drivers"
> + depends on TI_SCI_PROTOCOL || COMPILE_TEST
> + default y

default TI_SCI_PROTOCOL?

> + help
> +   This adds the clock driver support over TI System Control Interface.
> +   If you wish to use clock resources from the PMMC firmware, say Y.
> +   Otherwise, say N.
> +
> +endif # COMMON_CLK_KEYSTONE
> +
>  config COMMON_CLK_PALMAS
>   tristate "Clock driver for TI Palmas devices"
>   depends on MFD_PALMAS
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> new file mode 100644
> index ..6c43e097e6d6
> --- /dev/null
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -0,0 +1,539 @@
> +/*
> + * SCI Clock driver for keystone based devices
> + *
> + * Copyright (C) 2015-2016 Texas Instruments Incorporated - 
> http://www.ti.com/
> + *   Tero Kristo 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +#include 

Is this include used?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SCI_CLK_SSC_ENABLE   BIT(0)
> +#define SCI_CLK_ALLOW_FREQ_CHANGEBIT(1)
> +#define SCI_CLK_INPUT_TERMINATIONBIT(2)
> +
> +/**
> + * struct sci_clk_provider - TI SCI clock provider representation
> + * @sci:Handle to the System Control Interface protocol handler
> + * @ops:Pointer to the SCI ops to be used by the clocks
> + * @dev:Device pointer for the clock provider
> + * @clocks: List of all registered clocks
> + * @lock:   Mutex for locking access to the @clocks list
> + */
> +struct sci_clk_provider {
> + const struct ti_sci_handle *sci;
> + const struct ti_sci_clk_ops *ops;
> + struct device *dev;
> + struct list_head clocks;
> + struct mutex lock; /* Protects access to the @clocks list */
> +};
> +
> +/**
> + * struct sci_clk - TI SCI clock representation
> + * @hw:   Hardware clock cookie for common clock framework
> + * @dev_id:   Device index
> + * @clk_id:   Clock index
> + * @node: Clocks list link
> + * @provider: Master clock provider
> + * @flags:Flags for the clock
> + */
> +struct sci_clk {
> + struct clk_hw hw;
> + u16 dev_id;
> + u8 clk_id;
> + struct list_head node;
> + struct sci_clk_provider *provider;
> + u8 flags;
> +};
> +
> +#define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
> +
> +/**
> + * sci_clk_prepare - Prepare (enable) a TI SCI clock
> + * @hw: clock to prepare
> + *
> + * Prepares a clock to be actively used. Returns the SCI protocol status.
> + */
> +static int sci_clk_prepare(struct clk_hw *hw)
> +{
> + struct sci_clk *clk = to_sci_clk(hw);
> + bool enable_ssc = clk->flags & SCI_CLK_SSC_ENABLE;
> + bool allow_freq_change = clk->flags & SCI_CLK_ALLOW_FREQ_CHANGE;
> + bool input_termination = clk->flags & SCI_CLK_INPUT_TERMINATION;
> +
> + return clk->provider->ops->get_clock(clk->provider->sci, clk->dev_id,
> +  clk->clk_id, enable_ssc,
> +  allow_freq_change,
> +  input_termination);
> +}
> +
> +/**
> + * sci_clk_unprepare - Un-prepares (disables) a TI SCI clock
> + * @hw: clock to unprepare
> + *
> + * Un-prepares a clock from active state.
> + */
> +static void sci_clk_unprepare(struct clk_hw *hw)
> +{
> + struct sci_clk *clk = to_sci_clk(hw);
> + int ret;
> +
> + ret = clk->provider->ops->put_clock(clk->provider->sci, clk->dev_id,
> + clk->clk_id);
> + if (ret)
> + dev_err(clk->provider->dev,
> + "unprepare failed for dev=%d, clk=%d, ret=%d\n",
> + clk->dev_id, clk->clk_id, ret);
> 

[PATCH 3/3] clk: keystone: Add sci-clk driver support

[Nishanth Menon]

From: Tero Kristo 

In K2G, the clock handling is done through firmware executing on a
separate core. Linux kernel needs to communicate to the firmware
through TI system control interface to access any power management
related resources, including clocks.

The keystone sci-clk driver does this, by communicating to the
firmware through the TI SCI driver. The driver adds support for
registering clocks through DT, and basic required clock operations
like prepare/get_rate, etc.

Signed-off-by: Tero Kristo 
Tested-by: Dave Gerlach 
Signed-off-by: Nishanth Menon 
---
 MAINTAINERS|   1 +
 drivers/clk/Kconfig|  13 +
 drivers/clk/keystone/Makefile  |   1 +
 drivers/clk/keystone/sci-clk.c | 539 +
 4 files changed, 554 insertions(+)
 create mode 100644 drivers/clk/keystone/sci-clk.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f24aa97f845c..af06f022b725 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11578,6 +11578,7 @@ F:  include/dt-bindings/genpd/k2g.h
 F: drivers/soc/ti/ti_sci_pm_domains.c
 F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F: include/dt-bindings/clock/k2g.h
+F: drivers/clk/keystone/sci-clk.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M: Hans Verkuil 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e2d9bd760c84..d1724999be78 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -179,6 +179,19 @@ config COMMON_CLK_NXP
---help---
  Support for clock providers on NXP platforms.
 
+if COMMON_CLK_KEYSTONE
+
+config TI_SCI_CLK
+   tristate "TI System Control Interface clock drivers"
+   depends on TI_SCI_PROTOCOL || COMPILE_TEST
+   default y
+   help
+ This adds the clock driver support over TI System Control Interface.
+ If you wish to use clock resources from the PMMC firmware, say Y.
+ Otherwise, say N.
+
+endif # COMMON_CLK_KEYSTONE
+
 config COMMON_CLK_PALMAS
tristate "Clock driver for TI Palmas devices"
depends on MFD_PALMAS
diff --git a/drivers/clk/keystone/Makefile b/drivers/clk/keystone/Makefile
index 0477cf63f132..0e7993d2c09a 100644
--- a/drivers/clk/keystone/Makefile
+++ b/drivers/clk/keystone/Makefile
@@ -1 +1,2 @@
 obj-y  += pll.o gate.o
+obj-$(CONFIG_TI_SCI_CLK)   += sci-clk.o
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index ..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c
@@ -0,0 +1,539 @@
+/*
+ * SCI Clock driver for keystone based devices
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ * Tero Kristo 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SCI_CLK_SSC_ENABLE BIT(0)
+#define SCI_CLK_ALLOW_FREQ_CHANGE  BIT(1)
+#define SCI_CLK_INPUT_TERMINATION  BIT(2)
+
+/**
+ * struct sci_clk_provider - TI SCI clock provider representation
+ * @sci:Handle to the System Control Interface protocol handler
+ * @ops:Pointer to the SCI ops to be used by the clocks
+ * @dev:Device pointer for the clock provider
+ * @clocks: List of all registered clocks
+ * @lock:   Mutex for locking access to the @clocks list
+ */
+struct sci_clk_provider {
+   const struct ti_sci_handle *sci;
+   const struct ti_sci_clk_ops *ops;
+   struct device *dev;
+   struct list_head clocks;
+   struct mutex lock; /* Protects access to the @clocks list */
+};
+
+/**
+ * struct sci_clk - TI SCI clock representation
+ * @hw: Hardware clock cookie for common clock framework
+ * @dev_id: Device index
+ * @clk_id: Clock index
+ * @node:   Clocks list link
+ * @provider:   Master clock provider
+ * @flags:  Flags for the clock
+ */
+struct sci_clk {
+   struct clk_hw hw;
+   u16 dev_id;
+   u8 clk_id;
+   struct list_head node;
+   struct sci_clk_provider *provider;
+   u8 flags;
+};
+
+#define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
+
+/**
+ * sci_clk_prepare - Prepare (enable) a TI SCI clock
+ * @hw: clock to prepare
+ *
+ * Prepares a clock to be actively used. Returns the SCI protocol status.
+ */
+static int sci_clk_prepare(struct clk_hw *hw)
+{
+   struct sci_clk *clk = to_sci_clk(hw);
+   bool enable_ssc = clk->flags & 

[PATCH 3/3] clk: keystone: Add sci-clk driver support

[Nishanth Menon]

From: Tero Kristo 

In K2G, the clock handling is done through firmware executing on a
separate core. Linux kernel needs to communicate to the firmware
through TI system control interface to access any power management
related resources, including clocks.

The keystone sci-clk driver does this, by communicating to the
firmware through the TI SCI driver. The driver adds support for
registering clocks through DT, and basic required clock operations
like prepare/get_rate, etc.

Signed-off-by: Tero Kristo 
Tested-by: Dave Gerlach 
Signed-off-by: Nishanth Menon 
---
 MAINTAINERS|   1 +
 drivers/clk/Kconfig|  13 +
 drivers/clk/keystone/Makefile  |   1 +
 drivers/clk/keystone/sci-clk.c | 539 +
 4 files changed, 554 insertions(+)
 create mode 100644 drivers/clk/keystone/sci-clk.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f24aa97f845c..af06f022b725 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11578,6 +11578,7 @@ F:  include/dt-bindings/genpd/k2g.h
 F: drivers/soc/ti/ti_sci_pm_domains.c
 F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt
 F: include/dt-bindings/clock/k2g.h
+F: drivers/clk/keystone/sci-clk.c
 
 THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
 M: Hans Verkuil 
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e2d9bd760c84..d1724999be78 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -179,6 +179,19 @@ config COMMON_CLK_NXP
---help---
  Support for clock providers on NXP platforms.
 
+if COMMON_CLK_KEYSTONE
+
+config TI_SCI_CLK
+   tristate "TI System Control Interface clock drivers"
+   depends on TI_SCI_PROTOCOL || COMPILE_TEST
+   default y
+   help
+ This adds the clock driver support over TI System Control Interface.
+ If you wish to use clock resources from the PMMC firmware, say Y.
+ Otherwise, say N.
+
+endif # COMMON_CLK_KEYSTONE
+
 config COMMON_CLK_PALMAS
tristate "Clock driver for TI Palmas devices"
depends on MFD_PALMAS
diff --git a/drivers/clk/keystone/Makefile b/drivers/clk/keystone/Makefile
index 0477cf63f132..0e7993d2c09a 100644
--- a/drivers/clk/keystone/Makefile
+++ b/drivers/clk/keystone/Makefile
@@ -1 +1,2 @@
 obj-y  += pll.o gate.o
+obj-$(CONFIG_TI_SCI_CLK)   += sci-clk.o
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
new file mode 100644
index ..6c43e097e6d6
--- /dev/null
+++ b/drivers/clk/keystone/sci-clk.c
@@ -0,0 +1,539 @@
+/*
+ * SCI Clock driver for keystone based devices
+ *
+ * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/
+ * Tero Kristo 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SCI_CLK_SSC_ENABLE BIT(0)
+#define SCI_CLK_ALLOW_FREQ_CHANGE  BIT(1)
+#define SCI_CLK_INPUT_TERMINATION  BIT(2)
+
+/**
+ * struct sci_clk_provider - TI SCI clock provider representation
+ * @sci:Handle to the System Control Interface protocol handler
+ * @ops:Pointer to the SCI ops to be used by the clocks
+ * @dev:Device pointer for the clock provider
+ * @clocks: List of all registered clocks
+ * @lock:   Mutex for locking access to the @clocks list
+ */
+struct sci_clk_provider {
+   const struct ti_sci_handle *sci;
+   const struct ti_sci_clk_ops *ops;
+   struct device *dev;
+   struct list_head clocks;
+   struct mutex lock; /* Protects access to the @clocks list */
+};
+
+/**
+ * struct sci_clk - TI SCI clock representation
+ * @hw: Hardware clock cookie for common clock framework
+ * @dev_id: Device index
+ * @clk_id: Clock index
+ * @node:   Clocks list link
+ * @provider:   Master clock provider
+ * @flags:  Flags for the clock
+ */
+struct sci_clk {
+   struct clk_hw hw;
+   u16 dev_id;
+   u8 clk_id;
+   struct list_head node;
+   struct sci_clk_provider *provider;
+   u8 flags;
+};
+
+#define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
+
+/**
+ * sci_clk_prepare - Prepare (enable) a TI SCI clock
+ * @hw: clock to prepare
+ *
+ * Prepares a clock to be actively used. Returns the SCI protocol status.
+ */
+static int sci_clk_prepare(struct clk_hw *hw)
+{
+   struct sci_clk *clk = to_sci_clk(hw);
+   bool enable_ssc = clk->flags & SCI_CLK_SSC_ENABLE;
+   bool allow_freq_change = clk->flags & SCI_CLK_ALLOW_FREQ_CHANGE;
+