[PATCH] clk: provide public clk_is_enabled function

[PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

To determine if a clk has been previously enabled, provide a public
clk_is_enabled function. This is especially helpful to check the state
of clk-gate without actually changing the state of the gate.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/clk.c   |   18 ++++++++++++++++++
 include/linux/clk.h |   13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a004769..26698d3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -929,6 +929,24 @@ int clk_enable(struct clk *clk)
 EXPORT_SYMBOL_GPL(clk_enable);
 
 /**
+ * clk_is_enabled - return the enabled state of a clk
+ * @clk: the clk whose enabled state gets returned
+ *
+ * Returns true if the clock is enabled, false otherwise.
+ */
+bool clk_is_enabled(struct clk *clk)
+{
+	bool is_en;
+
+	clk_prepare_lock();
+	is_en = __clk_is_enabled(clk);
+	clk_prepare_unlock();
+
+	return is_en;
+}
+EXPORT_SYMBOL_GPL(clk_is_enabled);
+
+/**
  * __clk_round_rate - round the given rate for a clk
  * @clk: round the rate of this clock
  * @rate: the rate which is to be rounded
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..8cbf2f7 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -187,6 +187,14 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_is_enabled - return the enabled state of a clk
+ * @clk: the clk whose enabled state gets returned
+ *
+ * Returns true if the clock is enabled, false otherwise.
+ */
+bool clk_is_enabled(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
@@ -299,6 +307,11 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline bool clk_is_enabled(struct clk *clk)
+{
+	return false;
+}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;
-- 
1.7.10.4

[PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

To determine if a clk has been previously enabled, provide a public
clk_is_enabled function. This is especially helpful to check the state
of clk-gate without actually changing the state of the gate.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/clk/clk.c   |   18 ++++++++++++++++++
 include/linux/clk.h |   13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a004769..26698d3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -929,6 +929,24 @@ int clk_enable(struct clk *clk)
 EXPORT_SYMBOL_GPL(clk_enable);
 
 /**
+ * clk_is_enabled - return the enabled state of a clk
+ * @clk: the clk whose enabled state gets returned
+ *
+ * Returns true if the clock is enabled, false otherwise.
+ */
+bool clk_is_enabled(struct clk *clk)
+{
+	bool is_en;
+
+	clk_prepare_lock();
+	is_en = __clk_is_enabled(clk);
+	clk_prepare_unlock();
+
+	return is_en;
+}
+EXPORT_SYMBOL_GPL(clk_is_enabled);
+
+/**
  * __clk_round_rate - round the given rate for a clk
  * @clk: round the rate of this clock
  * @rate: the rate which is to be rounded
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..8cbf2f7 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -187,6 +187,14 @@ int clk_enable(struct clk *clk);
 void clk_disable(struct clk *clk);
 
 /**
+ * clk_is_enabled - return the enabled state of a clk
+ * @clk: the clk whose enabled state gets returned
+ *
+ * Returns true if the clock is enabled, false otherwise.
+ */
+bool clk_is_enabled(struct clk *clk);
+
+/**
  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
  *		  This is only valid once the clock source has been enabled.
  * @clk: clock source
@@ -299,6 +307,11 @@ static inline int clk_enable(struct clk *clk)
 
 static inline void clk_disable(struct clk *clk) {}
 
+static inline bool clk_is_enabled(struct clk *clk)
+{
+	return false;
+}
+
 static inline unsigned long clk_get_rate(struct clk *clk)
 {
 	return 0;
-- 
1.7.10.4

Re: [PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> To determine if a clk has been previously enabled, provide a public
> clk_is_enabled function. This is especially helpful to check the state
> of clk-gate without actually changing the state of the gate.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Tested-by: Andrew Lunn <andrew@lunn.ch>

	   Andrew


> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/clk/clk.c   |   18 ++++++++++++++++++
>  include/linux/clk.h |   13 +++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a004769..26698d3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -929,6 +929,24 @@ int clk_enable(struct clk *clk)
>  EXPORT_SYMBOL_GPL(clk_enable);
>  
>  /**
> + * clk_is_enabled - return the enabled state of a clk
> + * @clk: the clk whose enabled state gets returned
> + *
> + * Returns true if the clock is enabled, false otherwise.
> + */
> +bool clk_is_enabled(struct clk *clk)
> +{
> +	bool is_en;
> +
> +	clk_prepare_lock();
> +	is_en = __clk_is_enabled(clk);
> +	clk_prepare_unlock();
> +
> +	return is_en;
> +}
> +EXPORT_SYMBOL_GPL(clk_is_enabled);
> +
> +/**
>   * __clk_round_rate - round the given rate for a clk
>   * @clk: round the rate of this clock
>   * @rate: the rate which is to be rounded
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 9a6d045..8cbf2f7 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -187,6 +187,14 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_is_enabled - return the enabled state of a clk
> + * @clk: the clk whose enabled state gets returned
> + *
> + * Returns true if the clock is enabled, false otherwise.
> + */
> +bool clk_is_enabled(struct clk *clk);
> +
> +/**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
>   * @clk: clock source
> @@ -299,6 +307,11 @@ static inline int clk_enable(struct clk *clk)
>  
>  static inline void clk_disable(struct clk *clk) {}
>  
> +static inline bool clk_is_enabled(struct clk *clk)
> +{
> +	return false;
> +}
> +
>  static inline unsigned long clk_get_rate(struct clk *clk)
>  {
>  	return 0;
> -- 
> 1.7.10.4
> 

[PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> To determine if a clk has been previously enabled, provide a public
> clk_is_enabled function. This is especially helpful to check the state
> of clk-gate without actually changing the state of the gate.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Tested-by: Andrew Lunn <andrew@lunn.ch>

	   Andrew


> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/clk/clk.c   |   18 ++++++++++++++++++
>  include/linux/clk.h |   13 +++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a004769..26698d3 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -929,6 +929,24 @@ int clk_enable(struct clk *clk)
>  EXPORT_SYMBOL_GPL(clk_enable);
>  
>  /**
> + * clk_is_enabled - return the enabled state of a clk
> + * @clk: the clk whose enabled state gets returned
> + *
> + * Returns true if the clock is enabled, false otherwise.
> + */
> +bool clk_is_enabled(struct clk *clk)
> +{
> +	bool is_en;
> +
> +	clk_prepare_lock();
> +	is_en = __clk_is_enabled(clk);
> +	clk_prepare_unlock();
> +
> +	return is_en;
> +}
> +EXPORT_SYMBOL_GPL(clk_is_enabled);
> +
> +/**
>   * __clk_round_rate - round the given rate for a clk
>   * @clk: round the rate of this clock
>   * @rate: the rate which is to be rounded
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 9a6d045..8cbf2f7 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -187,6 +187,14 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_is_enabled - return the enabled state of a clk
> + * @clk: the clk whose enabled state gets returned
> + *
> + * Returns true if the clock is enabled, false otherwise.
> + */
> +bool clk_is_enabled(struct clk *clk);
> +
> +/**
>   * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>   *		  This is only valid once the clock source has been enabled.
>   * @clk: clock source
> @@ -299,6 +307,11 @@ static inline int clk_enable(struct clk *clk)
>  
>  static inline void clk_disable(struct clk *clk) {}
>  
> +static inline bool clk_is_enabled(struct clk *clk)
> +{
> +	return false;
> +}
> +
>  static inline unsigned long clk_get_rate(struct clk *clk)
>  {
>  	return 0;
> -- 
> 1.7.10.4
> 

Re: [PATCH] clk: provide public clk_is_enabled function

[Uwe Kleine-König]

On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> To determine if a clk has been previously enabled, provide a public
> clk_is_enabled function. This is especially helpful to check the state
> of clk-gate without actually changing the state of the gate.
I wonder what you want to do with the return value.

When doing

	if (clk_is_enabled(someclk))
		do_something();

you cannot in general know if the clock is still on when you start to
do_something.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] clk: provide public clk_is_enabled function

[Uwe Kleine-König]

On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> To determine if a clk has been previously enabled, provide a public
> clk_is_enabled function. This is especially helpful to check the state
> of clk-gate without actually changing the state of the gate.
I wonder what you want to do with the return value.

When doing

	if (clk_is_enabled(someclk))
		do_something();

you cannot in general know if the clock is still on when you start to
do_something.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> > To determine if a clk has been previously enabled, provide a public
> > clk_is_enabled function. This is especially helpful to check the state
> > of clk-gate without actually changing the state of the gate.
> I wonder what you want to do with the return value.
> 
> When doing
> 
> 	if (clk_is_enabled(someclk))
> 		do_something();
> 
> you cannot in general know if the clock is still on when you start to
> do_something.

Hi Uwe

At least in the use case Sebastian needs it for, we don't need an "in
general" solution. It is used early boot time to see if the boot
loader left the clock running. The other user of the clock is the
ethernet driver, which we know cannot change it yet, because driver
probing has not started yet.

	Andrew

[PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-K?nig wrote:
> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> > To determine if a clk has been previously enabled, provide a public
> > clk_is_enabled function. This is especially helpful to check the state
> > of clk-gate without actually changing the state of the gate.
> I wonder what you want to do with the return value.
> 
> When doing
> 
> 	if (clk_is_enabled(someclk))
> 		do_something();
> 
> you cannot in general know if the clock is still on when you start to
> do_something.

Hi Uwe

At least in the use case Sebastian needs it for, we don't need an "in
general" solution. It is used early boot time to see if the boot
loader left the clock running. The other user of the clock is the
ethernet driver, which we know cannot change it yet, because driver
probing has not started yet.

	Andrew

Re: [PATCH] clk: provide public clk_is_enabled function

[Gerhard Sittig]

On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
> 
> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote:
> > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> > > To determine if a clk has been previously enabled, provide a public
> > > clk_is_enabled function. This is especially helpful to check the state
> > > of clk-gate without actually changing the state of the gate.
> > I wonder what you want to do with the return value.
> > 
> > When doing
> > 
> > 	if (clk_is_enabled(someclk))
> > 		do_something();
> > 
> > you cannot in general know if the clock is still on when you start to
> > do_something.
> 
> Hi Uwe
> 
> At least in the use case Sebastian needs it for, we don't need an "in
> general" solution. It is used early boot time to see if the boot
> loader left the clock running.

Wait, unless I'm missing something, the clk_is_enabled() call
_won't_ determine whether the clock is enabled in hardware
(whether the boot loader created or left this condition), instead
it only determines whether clk_enable() was called previously and
thus the clock _shall_ be enabled.

AFAIK the kernel's CCF support is "self contained" and does not
consider any data or state that was "inherited" from boot staged
before the kernel.  That's why the "disable unused" step disables
everything that wasn't acquired _in the kernel_ regardless of
what the boot loader may have done or what is enabled at reset.

> The other user of the clock is the
> ethernet driver, which we know cannot change it yet, because driver
> probing has not started yet.

I understand that the situation here is, that the ethernet driver
hasn't probed yet, but the clock driver did.  You are in early
setup code and want to (check and) fetch data from the hardware
which the ethernet driver later needs.

What's wrong with an explicit enable/disable around the data
acquisition?  This allows to reliably fetch and store the
information (into the device tree data?) for later use, and is
neutral to the enable counter.  If the clock was enabled before,
it remains enabled.  If the clock was disabled before, it will
get disabled again.  In any case clock only may be turned off
after you have grabbed the data, the data is only taken when it's
available and valid.

This approach of explicit enable/disable around data access only
breaks if the ethernet driver won't acquire its related clock
appropriately, which would be a bug in the ethernet driver that
should be easy to fix.  (Strictly speaking it's not the data
gathering that breaks, but the already broken network driver
which accesses the hardware without acquiring the clock, and not
finds the clock disabled.)


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

[PATCH] clk: provide public clk_is_enabled function

[Gerhard Sittig]

On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
> 
> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> > > To determine if a clk has been previously enabled, provide a public
> > > clk_is_enabled function. This is especially helpful to check the state
> > > of clk-gate without actually changing the state of the gate.
> > I wonder what you want to do with the return value.
> > 
> > When doing
> > 
> > 	if (clk_is_enabled(someclk))
> > 		do_something();
> > 
> > you cannot in general know if the clock is still on when you start to
> > do_something.
> 
> Hi Uwe
> 
> At least in the use case Sebastian needs it for, we don't need an "in
> general" solution. It is used early boot time to see if the boot
> loader left the clock running.

Wait, unless I'm missing something, the clk_is_enabled() call
_won't_ determine whether the clock is enabled in hardware
(whether the boot loader created or left this condition), instead
it only determines whether clk_enable() was called previously and
thus the clock _shall_ be enabled.

AFAIK the kernel's CCF support is "self contained" and does not
consider any data or state that was "inherited" from boot staged
before the kernel.  That's why the "disable unused" step disables
everything that wasn't acquired _in the kernel_ regardless of
what the boot loader may have done or what is enabled at reset.

> The other user of the clock is the
> ethernet driver, which we know cannot change it yet, because driver
> probing has not started yet.

I understand that the situation here is, that the ethernet driver
hasn't probed yet, but the clock driver did.  You are in early
setup code and want to (check and) fetch data from the hardware
which the ethernet driver later needs.

What's wrong with an explicit enable/disable around the data
acquisition?  This allows to reliably fetch and store the
information (into the device tree data?) for later use, and is
neutral to the enable counter.  If the clock was enabled before,
it remains enabled.  If the clock was disabled before, it will
get disabled again.  In any case clock only may be turned off
after you have grabbed the data, the data is only taken when it's
available and valid.

This approach of explicit enable/disable around data access only
breaks if the ethernet driver won't acquire its related clock
appropriately, which would be a bug in the ethernet driver that
should be easy to fix.  (Strictly speaking it's not the data
gathering that breaks, but the already broken network driver
which accesses the hardware without acquiring the clock, and not
finds the clock disabled.)


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

Re: [PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
> > 
> > On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote:
> > > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> > > > To determine if a clk has been previously enabled, provide a public
> > > > clk_is_enabled function. This is especially helpful to check the state
> > > > of clk-gate without actually changing the state of the gate.
> > > I wonder what you want to do with the return value.
> > > 
> > > When doing
> > > 
> > > 	if (clk_is_enabled(someclk))
> > > 		do_something();
> > > 
> > > you cannot in general know if the clock is still on when you start to
> > > do_something.
> > 
> > Hi Uwe
> > 
> > At least in the use case Sebastian needs it for, we don't need an "in
> > general" solution. It is used early boot time to see if the boot
> > loader left the clock running.
> 
> Wait, unless I'm missing something, the clk_is_enabled() call
> _won't_ determine whether the clock is enabled in hardware
> (whether the boot loader created or left this condition), instead
> it only determines whether clk_enable() was called previously and
> thus the clock _shall_ be enabled.

Nope, you are wrong.

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);

	/* if a set bit disables this clk, flip it before masking */
	if (gate->flags & CLK_GATE_SET_TO_DISABLE)
	   reg ^= BIT(gate->bit_idx);

	   reg &= BIT(gate->bit_idx);

	   return reg ? 1 : 0;
}

It reads the hardware state.

> AFAIK the kernel's CCF support is "self contained" and does not
> consider any data or state that was "inherited" from boot staged
> before the kernel.  That's why the "disable unused" step disables
> everything that wasn't acquired _in the kernel_ regardless of
> what the boot loader may have done or what is enabled at reset.

Not quite true. It uses the is_enabled(), which gets the real hardware
state, to turn off clocks which are unused but on. It will not turn
off clock which are already off. So it is inheriting some state from
the boot loader, in that it knows if the bootloader turned it
on. However this is not propagated into prepare/enable status.

> > The other user of the clock is the
> > ethernet driver, which we know cannot change it yet, because driver
> > probing has not started yet.
> 
> I understand that the situation here is, that the ethernet driver
> hasn't probed yet, but the clock driver did.  You are in early setup
> code and want to (check and) fetch data from the hardware which the
> ethernet driver later needs.

Nearly, but not quite. If there is an enabled DT node for the device,
and that node does not have a valid local-mac-address property in the
node, the bootloader should of programmed the MAC address into the
device. If it has done that, the clock should be running, because as
soon as you turn the clock off, it forgets the MAC address. Thus, if
we find an enabled device in DT, without a valid local-mac-address,
and the clock is off, we have a bootloader bug, which we want to
report.

> What's wrong with an explicit enable/disable around the data
> acquisition?

It avoids the CPU locking hard, but will not get us a valid MAC
address, which is the point of the exercise.

	 Andrew

[PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
> > 
> > On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-K?nig wrote:
> > > On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> > > > To determine if a clk has been previously enabled, provide a public
> > > > clk_is_enabled function. This is especially helpful to check the state
> > > > of clk-gate without actually changing the state of the gate.
> > > I wonder what you want to do with the return value.
> > > 
> > > When doing
> > > 
> > > 	if (clk_is_enabled(someclk))
> > > 		do_something();
> > > 
> > > you cannot in general know if the clock is still on when you start to
> > > do_something.
> > 
> > Hi Uwe
> > 
> > At least in the use case Sebastian needs it for, we don't need an "in
> > general" solution. It is used early boot time to see if the boot
> > loader left the clock running.
> 
> Wait, unless I'm missing something, the clk_is_enabled() call
> _won't_ determine whether the clock is enabled in hardware
> (whether the boot loader created or left this condition), instead
> it only determines whether clk_enable() was called previously and
> thus the clock _shall_ be enabled.

Nope, you are wrong.

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);

	/* if a set bit disables this clk, flip it before masking */
	if (gate->flags & CLK_GATE_SET_TO_DISABLE)
	   reg ^= BIT(gate->bit_idx);

	   reg &= BIT(gate->bit_idx);

	   return reg ? 1 : 0;
}

It reads the hardware state.

> AFAIK the kernel's CCF support is "self contained" and does not
> consider any data or state that was "inherited" from boot staged
> before the kernel.  That's why the "disable unused" step disables
> everything that wasn't acquired _in the kernel_ regardless of
> what the boot loader may have done or what is enabled at reset.

Not quite true. It uses the is_enabled(), which gets the real hardware
state, to turn off clocks which are unused but on. It will not turn
off clock which are already off. So it is inheriting some state from
the boot loader, in that it knows if the bootloader turned it
on. However this is not propagated into prepare/enable status.

> > The other user of the clock is the
> > ethernet driver, which we know cannot change it yet, because driver
> > probing has not started yet.
> 
> I understand that the situation here is, that the ethernet driver
> hasn't probed yet, but the clock driver did.  You are in early setup
> code and want to (check and) fetch data from the hardware which the
> ethernet driver later needs.

Nearly, but not quite. If there is an enabled DT node for the device,
and that node does not have a valid local-mac-address property in the
node, the bootloader should of programmed the MAC address into the
device. If it has done that, the clock should be running, because as
soon as you turn the clock off, it forgets the MAC address. Thus, if
we find an enabled device in DT, without a valid local-mac-address,
and the clock is off, we have a bootloader bug, which we want to
report.

> What's wrong with an explicit enable/disable around the data
> acquisition?

It avoids the CPU locking hard, but will not get us a valid MAC
address, which is the point of the exercise.

	 Andrew

Re: [PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

On 10/06/2013 06:30 PM, Andrew Lunn wrote:
> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
>>>
>>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote:
>>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
>>>>> To determine if a clk has been previously enabled, provide a public
>>>>> clk_is_enabled function. This is especially helpful to check the state
>>>>> of clk-gate without actually changing the state of the gate.
>>>> I wonder what you want to do with the return value.
>>>>
>>>> When doing
>>>>
>>>> 	if (clk_is_enabled(someclk))
>>>> 		do_something();
>>>>
>>>> you cannot in general know if the clock is still on when you start to
>>>> do_something.
>>>
>>> Hi Uwe
>>>
>>> At least in the use case Sebastian needs it for, we don't need an "in
>>> general" solution. It is used early boot time to see if the boot
>>> loader left the clock running.
>>
>> Wait, unless I'm missing something, the clk_is_enabled() call
>> _won't_ determine whether the clock is enabled in hardware
>> (whether the boot loader created or left this condition), instead
>> it only determines whether clk_enable() was called previously and
>> thus the clock _shall_ be enabled.
>
> Nope, you are wrong.
>
> 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);
>
> 	/* if a set bit disables this clk, flip it before masking */
> 	if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> 	   reg ^= BIT(gate->bit_idx);
>
> 	   reg &= BIT(gate->bit_idx);
>
> 	   return reg ? 1 : 0;
> }
>
> It reads the hardware state.
>
>> AFAIK the kernel's CCF support is "self contained" and does not
>> consider any data or state that was "inherited" from boot staged
>> before the kernel.  That's why the "disable unused" step disables
>> everything that wasn't acquired _in the kernel_ regardless of
>> what the boot loader may have done or what is enabled at reset.
>
> Not quite true. It uses the is_enabled(), which gets the real hardware
> state, to turn off clocks which are unused but on. It will not turn
> off clock which are already off. So it is inheriting some state from
> the boot loader, in that it knows if the bootloader turned it
> on. However this is not propagated into prepare/enable status.
>
>>> The other user of the clock is the
>>> ethernet driver, which we know cannot change it yet, because driver
>>> probing has not started yet.
>>
>> I understand that the situation here is, that the ethernet driver
>> hasn't probed yet, but the clock driver did.  You are in early setup
>> code and want to (check and) fetch data from the hardware which the
>> ethernet driver later needs.
>
> Nearly, but not quite. If there is an enabled DT node for the device,
> and that node does not have a valid local-mac-address property in the
> node, the bootloader should of programmed the MAC address into the
> device. If it has done that, the clock should be running, because as
> soon as you turn the clock off, it forgets the MAC address. Thus, if
> we find an enabled device in DT, without a valid local-mac-address,
> and the clock is off, we have a bootloader bug, which we want to
> report.
>
>> What's wrong with an explicit enable/disable around the data
>> acquisition?
>
> It avoids the CPU locking hard, but will not get us a valid MAC
> address, which is the point of the exercise.

While I agree to all Andew explained above, I somehow have the strong
feeling that an clk_is_enabled will just be abused where possible. We
already have two ppl complaining about it - even though a quick look at
clk/core.c should have cleared out most of it.

Maybe we should just enable the clock, get the (possibly bogus) MAC
and disable it again. We loose one possible FW_BUG report and overwrite
an invalid local-mac-address property with another invalid one.

Sebastian

[PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

On 10/06/2013 06:30 PM, Andrew Lunn wrote:
> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
>>>
>>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-K?nig wrote:
>>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
>>>>> To determine if a clk has been previously enabled, provide a public
>>>>> clk_is_enabled function. This is especially helpful to check the state
>>>>> of clk-gate without actually changing the state of the gate.
>>>> I wonder what you want to do with the return value.
>>>>
>>>> When doing
>>>>
>>>> 	if (clk_is_enabled(someclk))
>>>> 		do_something();
>>>>
>>>> you cannot in general know if the clock is still on when you start to
>>>> do_something.
>>>
>>> Hi Uwe
>>>
>>> At least in the use case Sebastian needs it for, we don't need an "in
>>> general" solution. It is used early boot time to see if the boot
>>> loader left the clock running.
>>
>> Wait, unless I'm missing something, the clk_is_enabled() call
>> _won't_ determine whether the clock is enabled in hardware
>> (whether the boot loader created or left this condition), instead
>> it only determines whether clk_enable() was called previously and
>> thus the clock _shall_ be enabled.
>
> Nope, you are wrong.
>
> 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);
>
> 	/* if a set bit disables this clk, flip it before masking */
> 	if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> 	   reg ^= BIT(gate->bit_idx);
>
> 	   reg &= BIT(gate->bit_idx);
>
> 	   return reg ? 1 : 0;
> }
>
> It reads the hardware state.
>
>> AFAIK the kernel's CCF support is "self contained" and does not
>> consider any data or state that was "inherited" from boot staged
>> before the kernel.  That's why the "disable unused" step disables
>> everything that wasn't acquired _in the kernel_ regardless of
>> what the boot loader may have done or what is enabled at reset.
>
> Not quite true. It uses the is_enabled(), which gets the real hardware
> state, to turn off clocks which are unused but on. It will not turn
> off clock which are already off. So it is inheriting some state from
> the boot loader, in that it knows if the bootloader turned it
> on. However this is not propagated into prepare/enable status.
>
>>> The other user of the clock is the
>>> ethernet driver, which we know cannot change it yet, because driver
>>> probing has not started yet.
>>
>> I understand that the situation here is, that the ethernet driver
>> hasn't probed yet, but the clock driver did.  You are in early setup
>> code and want to (check and) fetch data from the hardware which the
>> ethernet driver later needs.
>
> Nearly, but not quite. If there is an enabled DT node for the device,
> and that node does not have a valid local-mac-address property in the
> node, the bootloader should of programmed the MAC address into the
> device. If it has done that, the clock should be running, because as
> soon as you turn the clock off, it forgets the MAC address. Thus, if
> we find an enabled device in DT, without a valid local-mac-address,
> and the clock is off, we have a bootloader bug, which we want to
> report.
>
>> What's wrong with an explicit enable/disable around the data
>> acquisition?
>
> It avoids the CPU locking hard, but will not get us a valid MAC
> address, which is the point of the exercise.

While I agree to all Andew explained above, I somehow have the strong
feeling that an clk_is_enabled will just be abused where possible. We
already have two ppl complaining about it - even though a quick look at
clk/core.c should have cleared out most of it.

Maybe we should just enable the clock, get the (possibly bogus) MAC
and disable it again. We loose one possible FW_BUG report and overwrite
an invalid local-mac-address property with another invalid one.

Sebastian

Re: [PATCH] clk: provide public clk_is_enabled function

[Mike Turquette]

Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
> > On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
> >> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
> >>>
> >>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-König wrote:
> >>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> >>>>> To determine if a clk has been previously enabled, provide a public
> >>>>> clk_is_enabled function. This is especially helpful to check the state
> >>>>> of clk-gate without actually changing the state of the gate.
> >>>> I wonder what you want to do with the return value.
> >>>>
> >>>> When doing
> >>>>
> >>>>    if (clk_is_enabled(someclk))
> >>>>            do_something();
> >>>>
> >>>> you cannot in general know if the clock is still on when you start to
> >>>> do_something.
> >>>
> >>> Hi Uwe
> >>>
> >>> At least in the use case Sebastian needs it for, we don't need an "in
> >>> general" solution. It is used early boot time to see if the boot
> >>> loader left the clock running.
> >>
> >> Wait, unless I'm missing something, the clk_is_enabled() call
> >> _won't_ determine whether the clock is enabled in hardware
> >> (whether the boot loader created or left this condition), instead
> >> it only determines whether clk_enable() was called previously and
> >> thus the clock _shall_ be enabled.
> >
> > Nope, you are wrong.
> >
> > 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);
> >
> >       /* if a set bit disables this clk, flip it before masking */
> >       if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> >          reg ^= BIT(gate->bit_idx);
> >
> >          reg &= BIT(gate->bit_idx);
> >
> >          return reg ? 1 : 0;
> > }
> >
> > It reads the hardware state.
> >
> >> AFAIK the kernel's CCF support is "self contained" and does not
> >> consider any data or state that was "inherited" from boot staged
> >> before the kernel.  That's why the "disable unused" step disables
> >> everything that wasn't acquired _in the kernel_ regardless of
> >> what the boot loader may have done or what is enabled at reset.
> >
> > Not quite true. It uses the is_enabled(), which gets the real hardware
> > state, to turn off clocks which are unused but on. It will not turn
> > off clock which are already off. So it is inheriting some state from
> > the boot loader, in that it knows if the bootloader turned it
> > on. However this is not propagated into prepare/enable status.
> >
> >>> The other user of the clock is the
> >>> ethernet driver, which we know cannot change it yet, because driver
> >>> probing has not started yet.
> >>
> >> I understand that the situation here is, that the ethernet driver
> >> hasn't probed yet, but the clock driver did.  You are in early setup
> >> code and want to (check and) fetch data from the hardware which the
> >> ethernet driver later needs.
> >
> > Nearly, but not quite. If there is an enabled DT node for the device,
> > and that node does not have a valid local-mac-address property in the
> > node, the bootloader should of programmed the MAC address into the
> > device. If it has done that, the clock should be running, because as
> > soon as you turn the clock off, it forgets the MAC address. Thus, if
> > we find an enabled device in DT, without a valid local-mac-address,
> > and the clock is off, we have a bootloader bug, which we want to
> > report.
> >
> >> What's wrong with an explicit enable/disable around the data
> >> acquisition?
> >
> > It avoids the CPU locking hard, but will not get us a valid MAC
> > address, which is the point of the exercise.
> 
> While I agree to all Andew explained above, I somehow have the strong
> feeling that an clk_is_enabled will just be abused where possible. We
> already have two ppl complaining about it - even though a quick look at
> clk/core.c should have cleared out most of it.
> 
> Maybe we should just enable the clock, get the (possibly bogus) MAC
> and disable it again. We loose one possible FW_BUG report and overwrite
> an invalid local-mac-address property with another invalid one.

Firstly, I'm OK with adding a new clk_is_enabled API for The Right
Reasons, if we can figure out what those reasons are. A major concern is
the lack of locks/barriers around that call that create a critical
section where the enabled state is guaranteed not to change. Those locks
are not exported to drivers nor do I want them to be.

Secondly, this specific Ethernet/MAC address issue seems like another
case of "the driver should be calling clk_get and clk_prepare_enable but
for some reason it doesn't". This seems to be a common pattern and I'm
not sure why. Does calling clk_enable on your clock which has been
already enabled by the bootloader cause issues for you? If not then it
is better to just call clk_enable and have the clock framework book
keeping in-sync with the hardware state.

Is it possible in your case to only detect the invalid MAC address
without sniffing the state of the clock hardware? Isn't it valid to
report a bootloader bug after only looking at the MAC address and not
the clock enabled state?

Regards,
Mike

> 
> Sebastian

[PATCH] clk: provide public clk_is_enabled function

[Mike Turquette]

Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
> > On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
> >> On Sat, Oct 05, 2013 at 22:42 +0200, Andrew Lunn wrote:
> >>>
> >>> On Sat, Oct 05, 2013 at 10:24:30PM +0200, Uwe Kleine-K?nig wrote:
> >>>> On Fri, Oct 04, 2013 at 12:08:30PM +0200, Sebastian Hesselbarth wrote:
> >>>>> To determine if a clk has been previously enabled, provide a public
> >>>>> clk_is_enabled function. This is especially helpful to check the state
> >>>>> of clk-gate without actually changing the state of the gate.
> >>>> I wonder what you want to do with the return value.
> >>>>
> >>>> When doing
> >>>>
> >>>>    if (clk_is_enabled(someclk))
> >>>>            do_something();
> >>>>
> >>>> you cannot in general know if the clock is still on when you start to
> >>>> do_something.
> >>>
> >>> Hi Uwe
> >>>
> >>> At least in the use case Sebastian needs it for, we don't need an "in
> >>> general" solution. It is used early boot time to see if the boot
> >>> loader left the clock running.
> >>
> >> Wait, unless I'm missing something, the clk_is_enabled() call
> >> _won't_ determine whether the clock is enabled in hardware
> >> (whether the boot loader created or left this condition), instead
> >> it only determines whether clk_enable() was called previously and
> >> thus the clock _shall_ be enabled.
> >
> > Nope, you are wrong.
> >
> > 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);
> >
> >       /* if a set bit disables this clk, flip it before masking */
> >       if (gate->flags & CLK_GATE_SET_TO_DISABLE)
> >          reg ^= BIT(gate->bit_idx);
> >
> >          reg &= BIT(gate->bit_idx);
> >
> >          return reg ? 1 : 0;
> > }
> >
> > It reads the hardware state.
> >
> >> AFAIK the kernel's CCF support is "self contained" and does not
> >> consider any data or state that was "inherited" from boot staged
> >> before the kernel.  That's why the "disable unused" step disables
> >> everything that wasn't acquired _in the kernel_ regardless of
> >> what the boot loader may have done or what is enabled at reset.
> >
> > Not quite true. It uses the is_enabled(), which gets the real hardware
> > state, to turn off clocks which are unused but on. It will not turn
> > off clock which are already off. So it is inheriting some state from
> > the boot loader, in that it knows if the bootloader turned it
> > on. However this is not propagated into prepare/enable status.
> >
> >>> The other user of the clock is the
> >>> ethernet driver, which we know cannot change it yet, because driver
> >>> probing has not started yet.
> >>
> >> I understand that the situation here is, that the ethernet driver
> >> hasn't probed yet, but the clock driver did.  You are in early setup
> >> code and want to (check and) fetch data from the hardware which the
> >> ethernet driver later needs.
> >
> > Nearly, but not quite. If there is an enabled DT node for the device,
> > and that node does not have a valid local-mac-address property in the
> > node, the bootloader should of programmed the MAC address into the
> > device. If it has done that, the clock should be running, because as
> > soon as you turn the clock off, it forgets the MAC address. Thus, if
> > we find an enabled device in DT, without a valid local-mac-address,
> > and the clock is off, we have a bootloader bug, which we want to
> > report.
> >
> >> What's wrong with an explicit enable/disable around the data
> >> acquisition?
> >
> > It avoids the CPU locking hard, but will not get us a valid MAC
> > address, which is the point of the exercise.
> 
> While I agree to all Andew explained above, I somehow have the strong
> feeling that an clk_is_enabled will just be abused where possible. We
> already have two ppl complaining about it - even though a quick look at
> clk/core.c should have cleared out most of it.
> 
> Maybe we should just enable the clock, get the (possibly bogus) MAC
> and disable it again. We loose one possible FW_BUG report and overwrite
> an invalid local-mac-address property with another invalid one.

Firstly, I'm OK with adding a new clk_is_enabled API for The Right
Reasons, if we can figure out what those reasons are. A major concern is
the lack of locks/barriers around that call that create a critical
section where the enabled state is guaranteed not to change. Those locks
are not exported to drivers nor do I want them to be.

Secondly, this specific Ethernet/MAC address issue seems like another
case of "the driver should be calling clk_get and clk_prepare_enable but
for some reason it doesn't". This seems to be a common pattern and I'm
not sure why. Does calling clk_enable on your clock which has been
already enabled by the bootloader cause issues for you? If not then it
is better to just call clk_enable and have the clock framework book
keeping in-sync with the hardware state.

Is it possible in your case to only detect the invalid MAC address
without sniffing the state of the clock hardware? Isn't it valid to
report a bootloader bug after only looking at the MAC address and not
the clock enabled state?

Regards,
Mike

> 
> Sebastian

Re: [PATCH] clk: provide public clk_is_enabled function

[Mike Turquette]

On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 10/06/2013 10:02 PM, Mike Turquette wrote:
>>
>> Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
>>>
>>> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
>>>>
>>>> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>>>>>
>>>>> What's wrong with an explicit enable/disable around the data
>>>>> acquisition?
>>>>
>>>>
>>>> It avoids the CPU locking hard, but will not get us a valid MAC
>>>> address, which is the point of the exercise.
>>>
>>>
>>> While I agree to all Andew explained above, I somehow have the strong
>>> feeling that an clk_is_enabled will just be abused where possible. We
>>> already have two ppl complaining about it - even though a quick look at
>>> clk/core.c should have cleared out most of it.
>>>
>>> Maybe we should just enable the clock, get the (possibly bogus) MAC
>>> and disable it again. We loose one possible FW_BUG report and overwrite
>>> an invalid local-mac-address property with another invalid one.
>>
>>
>> Firstly, I'm OK with adding a new clk_is_enabled API for The Right
>> Reasons, if we can figure out what those reasons are. A major concern is
>> the lack of locks/barriers around that call that create a critical
>> section where the enabled state is guaranteed not to change. Those locks
>> are not exported to drivers nor do I want them to be.
>>
>> Secondly, this specific Ethernet/MAC address issue seems like another
>> case of "the driver should be calling clk_get and clk_prepare_enable but
>> for some reason it doesn't". This seems to be a common pattern and I'm
>> not sure why. Does calling clk_enable on your clock which has been
>
>
> Ok, the driver is doing clk_prepare_enable/disable_unprepare just fine.
> It has nothing to do with the driver but the workaround for broken HW
> explained below.
>
>
>> already enabled by the bootloader cause issues for you? If not then it
>> is better to just call clk_enable and have the clock framework book
>> keeping in-sync with the hardware state.
>>
>> Is it possible in your case to only detect the invalid MAC address
>> without sniffing the state of the clock hardware? Isn't it valid to
>> report a bootloader bug after only looking at the MAC address and not
>> the clock enabled state?
>
>
> The ethernet IP used on Kirkwood (mv643xx_eth) is loosing the MAC
> address register contents on gated clocks. Everything was fine without
> DT and CCF, because clocks had to be enabled by bootloader. With DT and
> CCF, we gained clock gating but realized that IP has this flaw. For
> built-in ethernet driver usually everything is just fine, because driver
> picks up clock early and it never gets gated.
>
> As people were complaining about modular ethernet driver, we thought
> about how to (a) work around non-DT aware boot loaders, which we have
> a lot of and its not likely to change soon nor quick and (b) gate those
> clocks if possible.
>
> The workaround until now was to always enable the clocks in board init
> code, leaving clocks running. Now we want to switch to update the DT
> with the MAC address possibly found in those registers. It is also done
> on some other machs and archs, so I guess it is not uncommon do
> workaround broken/unaware bootloaders this way.
>
> The idea is to check nodes status and skip already disabled nodes. For
> enabled nodes, check the MAC address properties and skip those with
> valid MAC. Now, my initial workaround was to read registers for those
> left and copy the MAC address to local-mac-address property.
>
> Andrew has mentioned, that some bootloaders might disable clocks but
> leave the nodes enabled. Reading those registers would lock up
> the HW, of course. So we thought about to check clk gate status first,
> which this patch is about.
>
> Of course, we can do clk_enable, read, clk_disable as said before - and
> given the amount of questions and misinterpretation, I think it is the
> saner way.

Sorry for any misinterpretation on my end. I agree reading the
register(s) within a clk_enable/clk_disable-protected section is the
most sane option.

Regards,
Mike

>
> Sebastian

[PATCH] clk: provide public clk_is_enabled function

[Mike Turquette]

On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 10/06/2013 10:02 PM, Mike Turquette wrote:
>>
>> Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
>>>
>>> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
>>>>
>>>> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>>>>>
>>>>> What's wrong with an explicit enable/disable around the data
>>>>> acquisition?
>>>>
>>>>
>>>> It avoids the CPU locking hard, but will not get us a valid MAC
>>>> address, which is the point of the exercise.
>>>
>>>
>>> While I agree to all Andew explained above, I somehow have the strong
>>> feeling that an clk_is_enabled will just be abused where possible. We
>>> already have two ppl complaining about it - even though a quick look at
>>> clk/core.c should have cleared out most of it.
>>>
>>> Maybe we should just enable the clock, get the (possibly bogus) MAC
>>> and disable it again. We loose one possible FW_BUG report and overwrite
>>> an invalid local-mac-address property with another invalid one.
>>
>>
>> Firstly, I'm OK with adding a new clk_is_enabled API for The Right
>> Reasons, if we can figure out what those reasons are. A major concern is
>> the lack of locks/barriers around that call that create a critical
>> section where the enabled state is guaranteed not to change. Those locks
>> are not exported to drivers nor do I want them to be.
>>
>> Secondly, this specific Ethernet/MAC address issue seems like another
>> case of "the driver should be calling clk_get and clk_prepare_enable but
>> for some reason it doesn't". This seems to be a common pattern and I'm
>> not sure why. Does calling clk_enable on your clock which has been
>
>
> Ok, the driver is doing clk_prepare_enable/disable_unprepare just fine.
> It has nothing to do with the driver but the workaround for broken HW
> explained below.
>
>
>> already enabled by the bootloader cause issues for you? If not then it
>> is better to just call clk_enable and have the clock framework book
>> keeping in-sync with the hardware state.
>>
>> Is it possible in your case to only detect the invalid MAC address
>> without sniffing the state of the clock hardware? Isn't it valid to
>> report a bootloader bug after only looking at the MAC address and not
>> the clock enabled state?
>
>
> The ethernet IP used on Kirkwood (mv643xx_eth) is loosing the MAC
> address register contents on gated clocks. Everything was fine without
> DT and CCF, because clocks had to be enabled by bootloader. With DT and
> CCF, we gained clock gating but realized that IP has this flaw. For
> built-in ethernet driver usually everything is just fine, because driver
> picks up clock early and it never gets gated.
>
> As people were complaining about modular ethernet driver, we thought
> about how to (a) work around non-DT aware boot loaders, which we have
> a lot of and its not likely to change soon nor quick and (b) gate those
> clocks if possible.
>
> The workaround until now was to always enable the clocks in board init
> code, leaving clocks running. Now we want to switch to update the DT
> with the MAC address possibly found in those registers. It is also done
> on some other machs and archs, so I guess it is not uncommon do
> workaround broken/unaware bootloaders this way.
>
> The idea is to check nodes status and skip already disabled nodes. For
> enabled nodes, check the MAC address properties and skip those with
> valid MAC. Now, my initial workaround was to read registers for those
> left and copy the MAC address to local-mac-address property.
>
> Andrew has mentioned, that some bootloaders might disable clocks but
> leave the nodes enabled. Reading those registers would lock up
> the HW, of course. So we thought about to check clk gate status first,
> which this patch is about.
>
> Of course, we can do clk_enable, read, clk_disable as said before - and
> given the amount of questions and misinterpretation, I think it is the
> saner way.

Sorry for any misinterpretation on my end. I agree reading the
register(s) within a clk_enable/clk_disable-protected section is the
most sane option.

Regards,
Mike

>
> Sebastian

Re: [PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

> Andrew has mentioned, that some bootloaders might disable clocks but
> leave the nodes enabled. Reading those registers would lock up
> the HW, of course. So we thought about to check clk gate status first,
> which this patch is about.
> 
> Of course, we can do clk_enable, read, clk_disable as said before - and
> given the amount of questions and misinterpretation, I think it is the
> saner way.

Hi Sebastian

I agree. As you say, too many people have asked questions or
misinterpretation what is happening, so lets go for the simpler method
people can understand.

I would also suggest in the ethernet driver, maybe set_params(), check
if we have a valid MAC address, and if not, issue a warning and call
to eth_hw_addr_random(dev) to get a random one.

   Andrew

[PATCH] clk: provide public clk_is_enabled function

[Andrew Lunn]

> Andrew has mentioned, that some bootloaders might disable clocks but
> leave the nodes enabled. Reading those registers would lock up
> the HW, of course. So we thought about to check clk gate status first,
> which this patch is about.
> 
> Of course, we can do clk_enable, read, clk_disable as said before - and
> given the amount of questions and misinterpretation, I think it is the
> saner way.

Hi Sebastian

I agree. As you say, too many people have asked questions or
misinterpretation what is happening, so lets go for the simpler method
people can understand.

I would also suggest in the ethernet driver, maybe set_params(), check
if we have a valid MAC address, and if not, issue a warning and call
to eth_hw_addr_random(dev) to get a random one.

   Andrew

Re: [PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

On 10/06/2013 10:02 PM, Mike Turquette wrote:
> Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
>> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
>>> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>>>> What's wrong with an explicit enable/disable around the data
>>>> acquisition?
>>>
>>> It avoids the CPU locking hard, but will not get us a valid MAC
>>> address, which is the point of the exercise.
>>
>> While I agree to all Andew explained above, I somehow have the strong
>> feeling that an clk_is_enabled will just be abused where possible. We
>> already have two ppl complaining about it - even though a quick look at
>> clk/core.c should have cleared out most of it.
>>
>> Maybe we should just enable the clock, get the (possibly bogus) MAC
>> and disable it again. We loose one possible FW_BUG report and overwrite
>> an invalid local-mac-address property with another invalid one.
>
> Firstly, I'm OK with adding a new clk_is_enabled API for The Right
> Reasons, if we can figure out what those reasons are. A major concern is
> the lack of locks/barriers around that call that create a critical
> section where the enabled state is guaranteed not to change. Those locks
> are not exported to drivers nor do I want them to be.
>
> Secondly, this specific Ethernet/MAC address issue seems like another
> case of "the driver should be calling clk_get and clk_prepare_enable but
> for some reason it doesn't". This seems to be a common pattern and I'm
> not sure why. Does calling clk_enable on your clock which has been

Ok, the driver is doing clk_prepare_enable/disable_unprepare just fine.
It has nothing to do with the driver but the workaround for broken HW
explained below.

> already enabled by the bootloader cause issues for you? If not then it
> is better to just call clk_enable and have the clock framework book
> keeping in-sync with the hardware state.
>
> Is it possible in your case to only detect the invalid MAC address
> without sniffing the state of the clock hardware? Isn't it valid to
> report a bootloader bug after only looking at the MAC address and not
> the clock enabled state?

The ethernet IP used on Kirkwood (mv643xx_eth) is loosing the MAC
address register contents on gated clocks. Everything was fine without
DT and CCF, because clocks had to be enabled by bootloader. With DT and
CCF, we gained clock gating but realized that IP has this flaw. For
built-in ethernet driver usually everything is just fine, because driver
picks up clock early and it never gets gated.

As people were complaining about modular ethernet driver, we thought
about how to (a) work around non-DT aware boot loaders, which we have
a lot of and its not likely to change soon nor quick and (b) gate those
clocks if possible.

The workaround until now was to always enable the clocks in board init
code, leaving clocks running. Now we want to switch to update the DT
with the MAC address possibly found in those registers. It is also done
on some other machs and archs, so I guess it is not uncommon do
workaround broken/unaware bootloaders this way.

The idea is to check nodes status and skip already disabled nodes. For
enabled nodes, check the MAC address properties and skip those with
valid MAC. Now, my initial workaround was to read registers for those
left and copy the MAC address to local-mac-address property.

Andrew has mentioned, that some bootloaders might disable clocks but
leave the nodes enabled. Reading those registers would lock up
the HW, of course. So we thought about to check clk gate status first,
which this patch is about.

Of course, we can do clk_enable, read, clk_disable as said before - and
given the amount of questions and misinterpretation, I think it is the
saner way.

Sebastian

[PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

On 10/06/2013 10:02 PM, Mike Turquette wrote:
> Quoting Sebastian Hesselbarth (2013-10-06 12:42:01)
>> On 10/06/2013 06:30 PM, Andrew Lunn wrote:
>>> On Sun, Oct 06, 2013 at 11:06:09AM +0200, Gerhard Sittig wrote:
>>>> What's wrong with an explicit enable/disable around the data
>>>> acquisition?
>>>
>>> It avoids the CPU locking hard, but will not get us a valid MAC
>>> address, which is the point of the exercise.
>>
>> While I agree to all Andew explained above, I somehow have the strong
>> feeling that an clk_is_enabled will just be abused where possible. We
>> already have two ppl complaining about it - even though a quick look at
>> clk/core.c should have cleared out most of it.
>>
>> Maybe we should just enable the clock, get the (possibly bogus) MAC
>> and disable it again. We loose one possible FW_BUG report and overwrite
>> an invalid local-mac-address property with another invalid one.
>
> Firstly, I'm OK with adding a new clk_is_enabled API for The Right
> Reasons, if we can figure out what those reasons are. A major concern is
> the lack of locks/barriers around that call that create a critical
> section where the enabled state is guaranteed not to change. Those locks
> are not exported to drivers nor do I want them to be.
>
> Secondly, this specific Ethernet/MAC address issue seems like another
> case of "the driver should be calling clk_get and clk_prepare_enable but
> for some reason it doesn't". This seems to be a common pattern and I'm
> not sure why. Does calling clk_enable on your clock which has been

Ok, the driver is doing clk_prepare_enable/disable_unprepare just fine.
It has nothing to do with the driver but the workaround for broken HW
explained below.

> already enabled by the bootloader cause issues for you? If not then it
> is better to just call clk_enable and have the clock framework book
> keeping in-sync with the hardware state.
>
> Is it possible in your case to only detect the invalid MAC address
> without sniffing the state of the clock hardware? Isn't it valid to
> report a bootloader bug after only looking at the MAC address and not
> the clock enabled state?

The ethernet IP used on Kirkwood (mv643xx_eth) is loosing the MAC
address register contents on gated clocks. Everything was fine without
DT and CCF, because clocks had to be enabled by bootloader. With DT and
CCF, we gained clock gating but realized that IP has this flaw. For
built-in ethernet driver usually everything is just fine, because driver
picks up clock early and it never gets gated.

As people were complaining about modular ethernet driver, we thought
about how to (a) work around non-DT aware boot loaders, which we have
a lot of and its not likely to change soon nor quick and (b) gate those
clocks if possible.

The workaround until now was to always enable the clocks in board init
code, leaving clocks running. Now we want to switch to update the DT
with the MAC address possibly found in those registers. It is also done
on some other machs and archs, so I guess it is not uncommon do
workaround broken/unaware bootloaders this way.

The idea is to check nodes status and skip already disabled nodes. For
enabled nodes, check the MAC address properties and skip those with
valid MAC. Now, my initial workaround was to read registers for those
left and copy the MAC address to local-mac-address property.

Andrew has mentioned, that some bootloaders might disable clocks but
leave the nodes enabled. Reading those registers would lock up
the HW, of course. So we thought about to check clk gate status first,
which this patch is about.

Of course, we can do clk_enable, read, clk_disable as said before - and
given the amount of questions and misinterpretation, I think it is the
saner way.

Sebastian

Re: [PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

On 10/06/2013 11:04 PM, Mike Turquette wrote:
> On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com> wrote:
>> Of course, we can do clk_enable, read, clk_disable as said before - and
>> given the amount of questions and misinterpretation, I think it is the
>> saner way.
>
> Sorry for any misinterpretation on my end. I agree reading the
> register(s) within a clk_enable/clk_disable-protected section is the
> most sane option.

Well, as you are not the only one misinterpreting the purpose, I
guess it is more about the clk_is_enabled() function itself. Uwe was
very right, that it will lead to patches using it in a wrong way.

Using the common enable/disable functions does no harm to our workaround
and we will use it.

Thanks for taking the time to raise those questions and surface those
critical interpretations early!

Sebastian

[PATCH] clk: provide public clk_is_enabled function

[Sebastian Hesselbarth]

On 10/06/2013 11:04 PM, Mike Turquette wrote:
> On Sun, Oct 6, 2013 at 3:24 PM, Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com> wrote:
>> Of course, we can do clk_enable, read, clk_disable as said before - and
>> given the amount of questions and misinterpretation, I think it is the
>> saner way.
>
> Sorry for any misinterpretation on my end. I agree reading the
> register(s) within a clk_enable/clk_disable-protected section is the
> most sane option.

Well, as you are not the only one misinterpreting the purpose, I
guess it is more about the clk_is_enabled() function itself. Uwe was
very right, that it will lead to patches using it in a wrong way.

Using the common enable/disable functions does no harm to our workaround
and we will use it.

Thanks for taking the time to raise those questions and surface those
critical interpretations early!

Sebastian