[patch 2.6.23-rc2 1/2] define clk_must_disable()

[patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

This patch adds a clk_must_disable() operation, exposing the clock constraints
which often distinguish system power states.  Systems with such constraints
include ones using ARM-based AT91, OMAP, and PXA chips.  The new operation
lets driver methods check those constraints.

A common benefit to leaving some device clocks enabled is that a suspended
device may then be able to issue system wakeup events.  RS232, USB, Ethernet,
and other drivers can use driver model wakeup flags as userspace directions
about how to trade off between the lowest power "full off" states and more
functional wakeup-enabled ones.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

--- at91.orig/include/linux/clk.h	2007-02-16 08:47:11.000000000 -0800
+++ at91/include/linux/clk.h	2007-02-16 08:47:17.000000000 -0800
@@ -121,4 +121,24 @@ int clk_set_parent(struct clk *clk, stru
  */
 struct clk *clk_get_parent(struct clk *clk);
 
+/**
+ * clk_must_disable - report whether a clock's users must disable it
+ * @clk: one node in the clock tree
+ *
+ * This routine returns true only if the upcoming system state requires
+ * disabling the specified clock.
+ *
+ * It's common for platform power states to constrain certain clocks (and
+ * their descendants) to be unavailable, while other states allow that
+ * clock to be active.  A platform's power states often include an "all on"
+ * mode; system wide sleep states like "standby" and "suspend-to-RAM"; and
+ * operating states which sacrifice functionality for lower power usage.
+ *
+ * The constraint value is commonly tested in device driver suspend(), to
+ * leave clocks active if they are needed for features like wakeup events.
+ * On platforms that support reduced functionality operating states, the
+ * constraint may also need to be tested during resume() and probe() calls.
+ */
+int clk_must_disable(struct clk *clk);
+
 #endif

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Russell King]

On Mon, Aug 06, 2007 at 11:11:03AM -0700, David Brownell wrote:
> This patch adds a clk_must_disable() operation, exposing the clock constraints
> which often distinguish system power states.  Systems with such constraints
> include ones using ARM-based AT91, OMAP, and PXA chips.  The new operation
> lets driver methods check those constraints.
> 
> A common benefit to leaving some device clocks enabled is that a suspended
> device may then be able to issue system wakeup events.  RS232, USB, Ethernet,
> and other drivers can use driver model wakeup flags as userspace directions
> about how to trade off between the lowest power "full off" states and more
> functional wakeup-enabled ones.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Andrew,

David sent these without giving me a chance to respond to them.  For
your record, below are my comments.  As you can see, I'm of the opinion
that they're not suitable at present.

Message 1:
On Mon, Aug 06, 2007 at 10:23:21AM -0700, David Brownell wrote:
> A better fix is to recognize that the clock API is missing PM hooks;
> some drivers should stay partially active in sleep states that don't
> actually require them to turn off their clocks, and that's not at all
> unique to AT91 hardware.
>
>   https://lists.linux-foundation.org/pipermail/linux-pm/2007-March/011495.html>   https://lists.linux-foundation.org/pipermail/linux-pm/2007-March/011496.html>
> Presumably I should just get those into the MM queue ... I've been
> scrubbing other patches out, it's time for those also.

+ * This routine returns true only if the upcoming system state requires
+ * disabling the specified clock.
...
+int clk_must_disable(struct clk *clk);

It isn't clear how such a function decides whether the clock is available
in this "upcoming system state".  Some SoCs have multiple power states
which they can enter, so without knowledge of what this "upcoming
system state" is, how can this function decide?

Shouldn't it be passed something representing this "upcoming system state" ?

Also "clk_must_disable" is a horrible function name - it seems far too
ambiguous.  "clk_must_suspend()" would be a better hint at what it's
trying to do.

Message 2:
On Mon, Aug 06, 2007 at 11:25:56AM -0700, David Brownell wrote:
> I've sent the two patches to Andrew, with a "please expedite,
> this is a build fix" comment.

Given that there's some concerns about it, please provide only a minimal
fix - in other words the function prototype necessary to fix the build
issue.

Please don't wrap up unreviewed patches as "important build fixes".


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Monday 06 August 2007, Russell King wrote:
> On Mon, Aug 06, 2007 at 11:11:03AM -0700, David Brownell wrote:
> > This patch adds a clk_must_disable() operation, exposing the clock constraints
> > which often distinguish system power states.  Systems with such constraints
> > include ones using ARM-based AT91, OMAP, and PXA chips.  The new operation
> > lets driver methods check those constraints.
> > 
> > A common benefit to leaving some device clocks enabled is that a suspended
> > device may then be able to issue system wakeup events.  RS232, USB, Ethernet,
> > and other drivers can use driver model wakeup flags as userspace directions
> > about how to trade off between the lowest power "full off" states and more
> > functional wakeup-enabled ones.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> 
> Andrew,
> 
> David sent these without giving me a chance to respond to them. 

Not at all true.  You've seen them at least two other times since they
were first posted last year against 2.6.17-rc4.  Let's see:

 16-May-2006	http://marc.info/?l=linux-kernel&m=114782690922167&w=2
 22-March-2007	http://marc.info/?l=linux-kernel&m=117458718812017&w=2

And you didn't say a word then.  That's well over a year that you
have been silent.


> For 
> your record, below are my comments.  As you can see, I'm of the opinion
> that they're not suitable at present.

I've responded to these on their original list, but to re-cap:


> Message 1:
> ...
> 
> + * This routine returns true only if the upcoming system state requires
> + * disabling the specified clock.
> ...
> +int clk_must_disable(struct clk *clk);
> 
> It isn't clear how such a function decides whether the clock is available
> in this "upcoming system state".  Some SoCs have multiple power states
> which they can enter, so without knowledge of what this "upcoming
> system state" is, how can this function decide?
> 
> Shouldn't it be passed something representing this "upcoming system state" ?

pm_ops.set_target_state() does that now; pm_ops.prepare() used to
do it before that was redefined to make ACPI happier.


> Also "clk_must_disable" is a horrible function name - it seems far too
> ambiguous.  "clk_must_suspend()" would be a better hint at what it's
> trying to do.

If the relevant action were the non-existent clk_suspend() call I'd
agree.  But instead, it's the same old clk_disable() call that's in
question.  So ... no, that response makes no sense.

 
> Message 2:
> On Mon, Aug 06, 2007 at 11:25:56AM -0700, David Brownell wrote:
> > I've sent the two patches to Andrew, with a "please expedite,
> > this is a build fix" comment.
> 
> Given that there's some concerns about it, please provide only a minimal
> fix - in other words the function prototype necessary to fix the build
> issue.

The preceding times this specific API has come up, *NOBODY* ever
mentioned even one concern beyond "when will this patch merge".


> Please don't wrap up unreviewed patches as "important build fixes".

>From my perspective, they've been reviewed for well over a year and
have just been waiting for me to get off my butt and be extremely
explicit that they need to merge.  (That is, not expect anyone to
just pick them up and merge.)

And since you asked for a build fix ... I have a hard time thinking
that anything else could really be the "right fix".

- Dave

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Monday 06 August 2007, David Brownell wrote:
> > Please don't wrap up unreviewed patches as "important build fixes".
> 
> From my perspective, they've been reviewed for well over a year and
> have just been waiting for me to get off my butt and be extremely
> explicit that they need to merge.  (That is, not expect anyone to
> just pick them up and merge.)

Oh, and one more point in favor of merging this:  they've been in
use in Andrew Victor's 2.6 AT91 patches since last November.

That would be yet another reason why nobody has noticed this build
problem before.  It's not just me; almost everyone doing AT91 work
is *already using* these two patches, and has been doing so for most
of the last year.


> And since you asked for a build fix ... I have a hard time thinking
> that anything else could really be the "right fix".
 

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Russell King]

On Mon, Aug 06, 2007 at 01:38:08PM -0700, David Brownell wrote:
> And since you asked for a build fix ... I have a hard time thinking
> that anything else could really be the "right fix".

Do you have to drag every discussion right down into the shit pile?
Your message isn't even worth this response.

Appologies for trying to do the right thing.  Appologies for missing
your patches earlier this year.  Appologies for even living.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Monday 06 August 2007, Russell King wrote:
> On Mon, Aug 06, 2007 at 01:38:08PM -0700, David Brownell wrote:
> > And since you asked for a build fix ... I have a hard time thinking
> > that anything else could really be the "right fix".
> 
> Do you have to drag every discussion right down into the shit pile?
> Your message isn't even worth this response.

Don't introduce four letter words then.  You know the drill:
good *technical* arguments are what should win.

I agree it would have been easier if I had asked to merge this
earlier; like for 2.6.18 as originally planned, or 2.6.22 ... sigh.
It never got to the top of the priority list until that Makefile
patch forced the issue; and merge windows are filled with stuff
that's higher priority.

Thing is, I've never been one to prefer short term hacks when
a good long-term solution is available.  In this case, that
solution has been used for over 9 months now, and was presented
more than once on LKML.  Even now, there are no good technical
counter-arguments.  So I still think this is the "right fix".

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Andrew Morton]

ookkkay.  I'm going to pretend that I had an email malfunction which
deleted all rmk and david-b email for the past 24 hours.  I'd suggest that
others adopt the same pretence.



Russell, I have received the below from David.  Could you please review it
for inclusion?

Thanks.


From: David Brownell <david-b@pacbell.net>

This supports the clk_must_disable() interface for AT91 systems:

 - Implement the call, replacing at91_suspend_entering_slow_clock();

 - Use it in three drivers: USB Host, USB Peripheral, and RS232 serial.

Briefly, those are three of the drivers that need to act differently
when going into deeper sleep states (suspend-to-RAM), where among other
things they can't act as wakeup event sources.

(The at91_ethernet driver would be another such driver, but it doesn't
currently implement wake-on-LAN even in the "standby" mode.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/arm/mach-at91/clock.c    |   18 ++++++++++++++++++
 arch/arm/mach-at91/generic.h  |    1 +
 arch/arm/mach-at91/pm.c       |    7 +------
 drivers/serial/atmel_serial.c |    3 ++-
 drivers/usb/gadget/at91_udc.c |    2 +-
 drivers/usb/host/ohci-at91.c  |    2 +-
 6 files changed, 24 insertions(+), 9 deletions(-)

diff -puN arch/arm/mach-at91/clock.c~at91-implements-clk_must_disable arch/arm/mach-at91/clock.c
--- a/arch/arm/mach-at91/clock.c~at91-implements-clk_must_disable
+++ a/arch/arm/mach-at91/clock.c
@@ -32,6 +32,7 @@
 #include <asm/arch/cpu.h>
 
 #include "clock.h"
+#include "generic.h"
 
 
 /*
@@ -254,6 +255,23 @@ EXPORT_SYMBOL(clk_get_rate);
 
 /*------------------------------------------------------------------------*/
 
+#ifdef CONFIG_PM
+
+int clk_must_disable(struct clk *clk)
+{
+	if (!at91_suspend_entering_slow_clock())
+		return 0;
+
+	while (clk->parent)
+		clk = clk->parent;
+	return clk != &clk32k;
+}
+EXPORT_SYMBOL(clk_must_disable);
+
+#endif
+
+/*------------------------------------------------------------------------*/
+
 #ifdef CONFIG_AT91_PROGRAMMABLE_CLOCKS
 
 /*
diff -puN arch/arm/mach-at91/generic.h~at91-implements-clk_must_disable arch/arm/mach-at91/generic.h
--- a/arch/arm/mach-at91/generic.h~at91-implements-clk_must_disable
+++ a/arch/arm/mach-at91/generic.h
@@ -36,6 +36,7 @@ extern void __init at91_clock_associate(
  /* Power Management */
 extern void at91_irq_suspend(void);
 extern void at91_irq_resume(void);
+extern int at91_suspend_entering_slow_clock(void);
 
  /* GPIO */
 #define AT91RM9200_PQFP		3	/* AT91RM9200 PQFP package has 3 banks */
diff -puN arch/arm/mach-at91/pm.c~at91-implements-clk_must_disable arch/arm/mach-at91/pm.c
--- a/arch/arm/mach-at91/pm.c~at91-implements-clk_must_disable
+++ a/arch/arm/mach-at91/pm.c
@@ -103,20 +103,15 @@ static int at91_pm_verify_clocks(void)
 }
 
 /*
- * Call this from platform driver suspend() to see how deeply to suspend.
+ * This is called from clk_must_disable(), to see how deeply to suspend.
  * For example, some controllers (like OHCI) need one of the PLL clocks
  * in order to act as a wakeup source, and those are not available when
  * going into slow clock mode.
- *
- * REVISIT: generalize as clk_will_be_available(clk)?  Other platforms have
- * the very same problem (but not using at91 main_clk), and it'd be better
- * to add one generic API rather than lots of platform-specific ones.
  */
 int at91_suspend_entering_slow_clock(void)
 {
 	return (target_state == PM_SUSPEND_MEM);
 }
-EXPORT_SYMBOL(at91_suspend_entering_slow_clock);
 
 
 static void (*slow_clock)(void);
diff -puN drivers/serial/atmel_serial.c~at91-implements-clk_must_disable drivers/serial/atmel_serial.c
--- a/drivers/serial/atmel_serial.c~at91-implements-clk_must_disable
+++ a/drivers/serial/atmel_serial.c
@@ -921,7 +921,8 @@ static int atmel_serial_suspend(struct p
 	struct uart_port *port = platform_get_drvdata(pdev);
 	struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
 
-	if (device_may_wakeup(&pdev->dev) && !at91_suspend_entering_slow_clock())
+	if (device_may_wakeup(&pdev->dev)
+			&& !clk_must_disable(atmel_port->clk))
 		enable_irq_wake(port->irq);
 	else {
 		uart_suspend_port(&atmel_uart, port);
diff -puN drivers/usb/gadget/at91_udc.c~at91-implements-clk_must_disable drivers/usb/gadget/at91_udc.c
--- a/drivers/usb/gadget/at91_udc.c~at91-implements-clk_must_disable
+++ a/drivers/usb/gadget/at91_udc.c
@@ -1782,7 +1782,7 @@ static int at91udc_suspend(struct platfo
 	 */
 	if ((!udc->suspended && udc->addr)
 			|| !wake
-			|| at91_suspend_entering_slow_clock()) {
+			|| clk_must_disable(udc->fclk)) {
 		pullup(udc, 0);
 		wake = 0;
 	} else
diff -puN drivers/usb/host/ohci-at91.c~at91-implements-clk_must_disable drivers/usb/host/ohci-at91.c
--- a/drivers/usb/host/ohci-at91.c~at91-implements-clk_must_disable
+++ a/drivers/usb/host/ohci-at91.c
@@ -299,7 +299,7 @@ ohci_hcd_at91_drv_suspend(struct platfor
 	 *
 	 * REVISIT: some boards will be able to turn VBUS off...
 	 */
-	if (at91_suspend_entering_slow_clock()) {
+	if (clk_must_disable(fclk)) {
 		ohci_usb_reset (ohci);
 		at91_stop_clock();
 	}
_

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Russell King]

I do not think that the clk_must_disable() API is well enough thought out
for the following reasons:

1. the name sucks - it tells you nothing about it's purpose, which as
   the name currently stands can be interpreted in as many ways as there
   are species of animals on this planet.

   While the comments around the prototype help interpret its semantics,
   it is no subsitute for having a good name for the function.

2. it's unclear how this function obtains information about the "upcoming
   system state" and therefore decides whether the particular clock may
   be available.

3. due to the negative semantics, code such as the following is difficult
   to interpret and work out whether it's correct due to the double
   negative:

  +     if (device_may_wakeup(&pdev->dev)
  +                     && !clk_must_disable(atmel_port->clk))
                enable_irq_wake(port->irq);

4. the description of the function implies that this function may be
   called when we are not suspending:

  + * On platforms that support reduced functionality operating states, the
  + * constraint may also need to be tested during resume() and probe() calls.

   With SoCs with multiple power states affecting which clocks are
   available, and the need in point (2) for the architecture code to
   record which PM mode we're entering via the pm_ops set_target method,
   calling clk_must_disable() outside of the suspend methods results in
   this function essentially returning undefined values at driver probe
   time.  Note: there is no locking between driver probing and the
   set_target method.

   Moreover, if used in a driver probe() path, the return value could
   well depend on the _last_ system suspend state entered, and would be
   undefined for a system which hasn't been suspended from boot.

Changing the function name to "clk_available_in_suspend()" addresses at
least two of these points.  The other two points are addressed by
providing a way for the method to be passed the desired system suspend
state, which may be resolved by expanding pm_message_t to contain that
information.

Finally, concerning merging this during the -rc phase, I'd much rather
see the one liner simple build fix of adding the missing function
prototype going into the -rc kernels, and then a similar patch to this
going in during the next merge window.

On Mon, Aug 06, 2007 at 10:23:51PM -0700, Andrew Morton wrote:
> diff -puN arch/arm/mach-at91/clock.c~at91-implements-clk_must_disable arch/arm/mach-at91/clock.c
> --- a/arch/arm/mach-at91/clock.c~at91-implements-clk_must_disable
> +++ a/arch/arm/mach-at91/clock.c
> @@ -32,6 +32,7 @@
>  #include <asm/arch/cpu.h>
>  
>  #include "clock.h"
> +#include "generic.h"
>  
>  
>  /*
> @@ -254,6 +255,23 @@ EXPORT_SYMBOL(clk_get_rate);
>  
>  /*------------------------------------------------------------------------*/
>  
> +#ifdef CONFIG_PM
> +
> +int clk_must_disable(struct clk *clk)
> +{
> +	if (!at91_suspend_entering_slow_clock())
> +		return 0;
> +
> +	while (clk->parent)
> +		clk = clk->parent;
> +	return clk != &clk32k;
> +}
> +EXPORT_SYMBOL(clk_must_disable);
> +
> +#endif
> +
> +/*------------------------------------------------------------------------*/
> +
>  #ifdef CONFIG_AT91_PROGRAMMABLE_CLOCKS
>  
>  /*
> diff -puN arch/arm/mach-at91/generic.h~at91-implements-clk_must_disable arch/arm/mach-at91/generic.h
> --- a/arch/arm/mach-at91/generic.h~at91-implements-clk_must_disable
> +++ a/arch/arm/mach-at91/generic.h
> @@ -36,6 +36,7 @@ extern void __init at91_clock_associate(
>   /* Power Management */
>  extern void at91_irq_suspend(void);
>  extern void at91_irq_resume(void);
> +extern int at91_suspend_entering_slow_clock(void);
>  
>   /* GPIO */
>  #define AT91RM9200_PQFP		3	/* AT91RM9200 PQFP package has 3 banks */
> diff -puN arch/arm/mach-at91/pm.c~at91-implements-clk_must_disable arch/arm/mach-at91/pm.c
> --- a/arch/arm/mach-at91/pm.c~at91-implements-clk_must_disable
> +++ a/arch/arm/mach-at91/pm.c
> @@ -103,20 +103,15 @@ static int at91_pm_verify_clocks(void)
>  }
>  
>  /*
> - * Call this from platform driver suspend() to see how deeply to suspend.
> + * This is called from clk_must_disable(), to see how deeply to suspend.
>   * For example, some controllers (like OHCI) need one of the PLL clocks
>   * in order to act as a wakeup source, and those are not available when
>   * going into slow clock mode.
> - *
> - * REVISIT: generalize as clk_will_be_available(clk)?  Other platforms have
> - * the very same problem (but not using at91 main_clk), and it'd be better
> - * to add one generic API rather than lots of platform-specific ones.
>   */
>  int at91_suspend_entering_slow_clock(void)
>  {
>  	return (target_state == PM_SUSPEND_MEM);
>  }
> -EXPORT_SYMBOL(at91_suspend_entering_slow_clock);
>  
>  
>  static void (*slow_clock)(void);
> diff -puN drivers/serial/atmel_serial.c~at91-implements-clk_must_disable drivers/serial/atmel_serial.c
> --- a/drivers/serial/atmel_serial.c~at91-implements-clk_must_disable
> +++ a/drivers/serial/atmel_serial.c
> @@ -921,7 +921,8 @@ static int atmel_serial_suspend(struct p
>  	struct uart_port *port = platform_get_drvdata(pdev);
>  	struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
>  
> -	if (device_may_wakeup(&pdev->dev) && !at91_suspend_entering_slow_clock())
> +	if (device_may_wakeup(&pdev->dev)
> +			&& !clk_must_disable(atmel_port->clk))
>  		enable_irq_wake(port->irq);
>  	else {
>  		uart_suspend_port(&atmel_uart, port);
> diff -puN drivers/usb/gadget/at91_udc.c~at91-implements-clk_must_disable drivers/usb/gadget/at91_udc.c
> --- a/drivers/usb/gadget/at91_udc.c~at91-implements-clk_must_disable
> +++ a/drivers/usb/gadget/at91_udc.c
> @@ -1782,7 +1782,7 @@ static int at91udc_suspend(struct platfo
>  	 */
>  	if ((!udc->suspended && udc->addr)
>  			|| !wake
> -			|| at91_suspend_entering_slow_clock()) {
> +			|| clk_must_disable(udc->fclk)) {
>  		pullup(udc, 0);
>  		wake = 0;
>  	} else
> diff -puN drivers/usb/host/ohci-at91.c~at91-implements-clk_must_disable drivers/usb/host/ohci-at91.c
> --- a/drivers/usb/host/ohci-at91.c~at91-implements-clk_must_disable
> +++ a/drivers/usb/host/ohci-at91.c
> @@ -299,7 +299,7 @@ ohci_hcd_at91_drv_suspend(struct platfor
>  	 *
>  	 * REVISIT: some boards will be able to turn VBUS off...
>  	 */
> -	if (at91_suspend_entering_slow_clock()) {
> +	if (clk_must_disable(fclk)) {
>  		ohci_usb_reset (ohci);
>  		at91_stop_clock();
>  	}
> _
> 

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Andrew Morton]

On Tue, 7 Aug 2007 13:50:54 +0100 Russell King <rmk@arm.linux.org.uk> wrote:

> I do not think that the clk_must_disable() API is well enough thought out
> for the following reasons:
> 
> 1. the name sucks - it tells you nothing about it's purpose, which as
>    the name currently stands can be interpreted in as many ways as there
>    are species of animals on this planet.
> 
>    While the comments around the prototype help interpret its semantics,
>    it is no subsitute for having a good name for the function.
> 
> 2. it's unclear how this function obtains information about the "upcoming
>    system state" and therefore decides whether the particular clock may
>    be available.
> 
> 3. due to the negative semantics, code such as the following is difficult
>    to interpret and work out whether it's correct due to the double
>    negative:
> 
>   +     if (device_may_wakeup(&pdev->dev)
>   +                     && !clk_must_disable(atmel_port->clk))
>                 enable_irq_wake(port->irq);
> 
> 4. the description of the function implies that this function may be
>    called when we are not suspending:
> 
>   + * On platforms that support reduced functionality operating states, the
>   + * constraint may also need to be tested during resume() and probe() calls.
> 
>    With SoCs with multiple power states affecting which clocks are
>    available, and the need in point (2) for the architecture code to
>    record which PM mode we're entering via the pm_ops set_target method,
>    calling clk_must_disable() outside of the suspend methods results in
>    this function essentially returning undefined values at driver probe
>    time.  Note: there is no locking between driver probing and the
>    set_target method.
> 
>    Moreover, if used in a driver probe() path, the return value could
>    well depend on the _last_ system suspend state entered, and would be
>    undefined for a system which hasn't been suspended from boot.
> 
> Changing the function name to "clk_available_in_suspend()" addresses at
> least two of these points.  The other two points are addressed by
> providing a way for the method to be passed the desired system suspend
> state, which may be resolved by expanding pm_message_t to contain that
> information.

I see, thanks.  clk_available_in_suspend() sure is a better name.

> Finally, concerning merging this during the -rc phase, I'd much rather
> see the one liner simple build fix of adding the missing function
> prototype going into the -rc kernels, and then a similar patch to this
> going in during the next merge window.

Here's where confusion sets in.  I have this:

--- at91.orig/include/linux/clk.h	2007-02-16 08:47:11.000000000 -0800
+++ at91/include/linux/clk.h	2007-02-16 08:47:17.000000000 -0800
@@ -121,4 +121,24 @@ int clk_set_parent(struct clk *clk, stru
  */
 struct clk *clk_get_parent(struct clk *clk);
 
+/**
+ * clk_must_disable - report whether a clock's users must disable it
+ * @clk: one node in the clock tree
+ *
+ * This routine returns true only if the upcoming system state requires
+ * disabling the specified clock.
+ *
+ * It's common for platform power states to constrain certain clocks (and
+ * their descendants) to be unavailable, while other states allow that
+ * clock to be active.  A platform's power states often include an "all on"
+ * mode; system wide sleep states like "standby" and "suspend-to-RAM"; and
+ * operating states which sacrifice functionality for lower power usage.
+ *
+ * The constraint value is commonly tested in device driver suspend(), to
+ * leave clocks active if they are needed for features like wakeup events.
+ * On platforms that support reduced functionality operating states, the
+ * constraint may also need to be tested during resume() and probe() calls.
+ */
+int clk_must_disable(struct clk *clk);
+
 #endif

but it's a prototype for a function which doesn't exist.  You must
be referring to a different patch.

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Russell King]

On Tue, Aug 07, 2007 at 10:21:06AM -0700, Andrew Morton wrote:
> On Tue, 7 Aug 2007 13:50:54 +0100 Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > I do not think that the clk_must_disable() API is well enough thought out
> > for the following reasons:
> > 
> > 1. the name sucks - it tells you nothing about it's purpose, which as
> >    the name currently stands can be interpreted in as many ways as there
> >    are species of animals on this planet.
> > 
> >    While the comments around the prototype help interpret its semantics,
> >    it is no subsitute for having a good name for the function.
> > 
> > 2. it's unclear how this function obtains information about the "upcoming
> >    system state" and therefore decides whether the particular clock may
> >    be available.
> > 
> > 3. due to the negative semantics, code such as the following is difficult
> >    to interpret and work out whether it's correct due to the double
> >    negative:
> > 
> >   +     if (device_may_wakeup(&pdev->dev)
> >   +                     && !clk_must_disable(atmel_port->clk))
> >                 enable_irq_wake(port->irq);
> > 
> > 4. the description of the function implies that this function may be
> >    called when we are not suspending:
> > 
> >   + * On platforms that support reduced functionality operating states, the
> >   + * constraint may also need to be tested during resume() and probe() calls.
> > 
> >    With SoCs with multiple power states affecting which clocks are
> >    available, and the need in point (2) for the architecture code to
> >    record which PM mode we're entering via the pm_ops set_target method,
> >    calling clk_must_disable() outside of the suspend methods results in
> >    this function essentially returning undefined values at driver probe
> >    time.  Note: there is no locking between driver probing and the
> >    set_target method.
> > 
> >    Moreover, if used in a driver probe() path, the return value could
> >    well depend on the _last_ system suspend state entered, and would be
> >    undefined for a system which hasn't been suspended from boot.
> > 
> > Changing the function name to "clk_available_in_suspend()" addresses at
> > least two of these points.  The other two points are addressed by
> > providing a way for the method to be passed the desired system suspend
> > state, which may be resolved by expanding pm_message_t to contain that
> > information.
> 
> I see, thanks.  clk_available_in_suspend() sure is a better name.
> 
> > Finally, concerning merging this during the -rc phase, I'd much rather
> > see the one liner simple build fix of adding the missing function
> > prototype going into the -rc kernels, and then a similar patch to this
> > going in during the next merge window.
> 
> Here's where confusion sets in.  I have this:

See 2/2.  Both patches need to be looked at together.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

Replying to points individually, to avoid excessively long emails.

On Tuesday 07 August 2007, Russell King wrote:
> 
> 1. the name sucks - it tells you nothing about it's purpose,

What -- that when it returns true, you "must disable" the clock?

I have a hard time with this claim.  The purpose could hardly become
more clear.  It seems to me that there is some other issue that's not
yet been surfaced here.  (I'll refrain from guessing.)

That said, it seems like this item reflects your core issue.


> 		which as 
>    the name currently stands can be interpreted in as many ways as there
>    are species of animals on this planet.
> 
>    While the comments around the prototype help interpret its semantics,
>    it is no subsitute for having a good name for the function.

Again, the semantics are exactly what the name says.  Hyperbole aside,
I'm in the dark about your real objection here.

- Dave

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Tuesday 07 August 2007, Russell King wrote:
> 2. it's unclear how this function obtains information about the "upcoming
>    system state" and therefore decides whether the particular clock may
>    be available.

Why should any such *implementation detail* matter to an interface spec?

Especially to the clock framework, which has already gone to great
lengths to avoid constraining implementations, and allow them to do
whatever the platform needs.  (Admittedly, some people don't think
of that as a feature.)

- Dave

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Tuesday 07 August 2007, Russell King wrote:

> 3. due to the negative semantics, code such as the following is difficult
>    to interpret and work out whether it's correct due to the double
>    negative:

In terms of pure patch review, the "-" bit you omitted sure helps me
to see that it's an "obviously" correct transformation:

  -       if (device_may_wakeup(&pdev->dev) && !at91_suspend_entering_slow_clock())
>   +     if (device_may_wakeup(&pdev->dev)
>   +                     && !clk_must_disable(atmel_port->clk))
>                 enable_irq_wake(port->irq);

In the case of nutball configurations like clocking UART from 32KiHz,
it's more correct.  Likewise, it can work with AVR32 platforms, once
they support PM.


There are numerous idioms that one could allege are confusing -- especially
when looking at them without any context! -- but which in practice are not.
The whole "if" statement is:

        if (device_may_wakeup(&pdev->dev) 
                        && !clk_must_disable(atmel_port->clk))
                enable_irq_wake(port->irq);
        else {
                uart_suspend_port(&atmel_uart, port);
                atmel_port->suspended = 1;
        }

That's a lot more clear then the fragment you showed -- suspend, or
keep it active with wakeup enabled -- with the main confusion coming
from the serial stack itself:  the way clk_disable() gets hidden
deep inside uart_suspend_port().
 
If you're saying that should be rewritten, something like:

	if (!device_may_wakeup(...) || clk_must_disable(...)) {
		suspend port
	} else
		leave it active, with wakeup enabled

That might be true, but it's not a new issue or attributable to
this patch.  Likewise with (potentially) adding comments.

- Dave

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Tuesday 07 August 2007, Russell King wrote:

> 4. the description of the function implies that this function may be
>    called when we are not suspending:
> 
>   + * On platforms that support reduced functionality operating states, the
>   + * constraint may also need to be tested during resume() and probe() calls.

Right ... support for multiple "run" states will imply this same kind
of predicate, and I couldn't justify chosing a name which forces the
existence of more than one such predicate.  Ergo a generic name.

However, right now no platforms support them (cpufreq aside), so this
text could be rather harmlessly removed:  the second half of the sentence
doesn't apply to anything yet.  And maybe it *should* be removed, since
such platforms might do arbitrarily surprising stuff ... they might use
driver-level "reclock yourself" callbacks/notifiers (like the original
DPM stuff).


>    With SoCs with multiple power states affecting which clocks are
>    available, and the need in point (2) for the architecture code to
>    record which PM mode we're entering via the pm_ops set_target method,

Your point (2) was that it's "unclear" how that information becomes
available.  But the presumption of (4) here is that it's set_target().
So either it's clear enough, and (2) was wrong; or (4) has a false
premise; or both.  From false premises, anything can be proven...

I'd certainly not expect pm_ops.set_target() to apply in any context
other than the start of a suspend sequence.  Any platform choosing
to implement multiple run states necessarily provides some more
mechanisms...

(Purely as an example ... this is much the same kind of thing that goes
on with cpufreq updating a CPU clock that's also exposed through the
clock API:  multiple interfaces to the same PM-related state.  In both
cases, the public interface doesn't say how the implementation might
be done; it doesn't even care.)


>    calling clk_must_disable() outside of the suspend methods results in
>    this function essentially returning undefined values at driver probe
>    time.  Note: there is no locking between driver probing and the
>    set_target method.
> 
>    Moreover, if used in a driver probe() path, the return value could
>    well depend on the _last_ system suspend state entered, and would be
>    undefined for a system which hasn't been suspended from boot.

No, this platform maintains an invariant that the target PM state is
always PM_SUSPEND_ON except while suspending.  (So does ACPI, as I
recall.)  You're presuming a broken platform; but it's not broken...

Re locking, that issue would be addressed as part of supporting
multiple run states.  I still don't see suspend ops being involved
in non-suspend transitions.  But if a state change is in progress as
probe() is called, then suspend() should probably be called later.

- Dave

Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[David Brownell]

On Tuesday 07 August 2007, Russell King wrote:

> Changing the function name to "clk_available_in_suspend()" addresses at
> least two of these points.

Except that it would

 (a) Preclude using this function in non-suspend contexts, including the
     "multiple run states" PM support that's regularly requested (and
     is thus IMO in the "why not plan on it" category).  The mechanism
     doesn't *NEED* to be constrained to be usable only from suspend().

 (b) Beg the question of which suspend state is involved ... something
     which has to date been intentionally hidden, since such states are
     so extremely system-specific (and since drivers misused that info).
     Drivers can care about *attributes* of an upcoming state (like
     whether a clock will be available, or a power domain, etc); fine.

The original notion for a name was of course clk_will_be_available(clk),
but that seemed too darn unwieldy to use.  Plus, logic like

	if (clk_must_disable(clk))
		clk_disable(clk);

(and similar) just seems inherently less likely to confuse ... it doesn't
need to introduce even one new term or concept, and focusses exclusively
on the key point:  whether clk_disable() is mandatory.


> The other two points are addressed by 
> providing a way for the method to be passed the desired system suspend
> state, which may be resolved by expanding pm_message_t to contain that
> information.

You do realize that pm_message_t was created to move *AWAY* from
passing an ID for the target system state down to the drivers?
The PM core previously passed down PM_SUSPEND_* integer codes.

Personally I detest pm_message_t ... "struct pm_message *" would
have been better, much less costly to expand (if needed), no new
typedefs, no pass-struct-by-value.  And in any case, there's work
afoot to have different methods for each different PM event, and
thus finally abolish pm_message_t.


> Finally, concerning merging this during the -rc phase, I'd much rather
> see the one liner simple build fix of adding the missing function
> prototype going into the -rc kernels, and then a similar patch to this
> going in during the next merge window.

If clk_must_disable() hadn't been "the plan of record" for over a
year, and already in use, I'd have been more in accord with that.

- Dave


> >  /*
> > - * Call this from platform driver suspend() to see how deeply to suspend.
> > + * This is called from clk_must_disable(), to see how deeply to suspend.
> >   * For example, some controllers (like OHCI) need one of the PLL clocks
> >   * in order to act as a wakeup source, and those are not available when
> >   * going into slow clock mode.
> > - *
> > - * REVISIT: generalize as clk_will_be_available(clk)?  Other platforms have
> > - * the very same problem (but not using at91 main_clk), and it'd be better
> > - * to add one generic API rather than lots of platform-specific ones.
> >   */
> >  int at91_suspend_entering_slow_clock(void)

Re: Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Rafael J. Wysocki]

On Tuesday, 7 August 2007 23:17, David Brownell wrote:
> On Tuesday 07 August 2007, Russell King wrote:
> 
> > 4. the description of the function implies that this function may be
> >    called when we are not suspending:
> > 
> >   + * On platforms that support reduced functionality operating states, the
> >   + * constraint may also need to be tested during resume() and probe() calls.
> 
> Right ... support for multiple "run" states will imply this same kind
> of predicate, and I couldn't justify chosing a name which forces the
> existence of more than one such predicate.  Ergo a generic name.
> 
> However, right now no platforms support them (cpufreq aside), so this
> text could be rather harmlessly removed:  the second half of the sentence
> doesn't apply to anything yet.  And maybe it *should* be removed, since
> such platforms might do arbitrarily surprising stuff ... they might use
> driver-level "reclock yourself" callbacks/notifiers (like the original
> DPM stuff).
> 
> 
> >    With SoCs with multiple power states affecting which clocks are
> >    available, and the need in point (2) for the architecture code to
> >    record which PM mode we're entering via the pm_ops set_target method,
> 
> Your point (2) was that it's "unclear" how that information becomes
> available.  But the presumption of (4) here is that it's set_target().
> So either it's clear enough, and (2) was wrong; or (4) has a false
> premise; or both.  From false premises, anything can be proven...
> 
> I'd certainly not expect pm_ops.set_target() to apply in any context
> other than the start of a suspend sequence.  Any platform choosing
> to implement multiple run states necessarily provides some more
> mechanisms...
> 
> (Purely as an example ... this is much the same kind of thing that goes
> on with cpufreq updating a CPU clock that's also exposed through the
> clock API:  multiple interfaces to the same PM-related state.  In both
> cases, the public interface doesn't say how the implementation might
> be done; it doesn't even care.)
> 
> 
> >    calling clk_must_disable() outside of the suspend methods results in
> >    this function essentially returning undefined values at driver probe
> >    time.  Note: there is no locking between driver probing and the
> >    set_target method.
> > 
> >    Moreover, if used in a driver probe() path, the return value could
> >    well depend on the _last_ system suspend state entered, and would be
> >    undefined for a system which hasn't been suspended from boot.
> 
> No, this platform maintains an invariant that the target PM state is
> always PM_SUSPEND_ON except while suspending.  (So does ACPI, as I
> recall.)  You're presuming a broken platform; but it's not broken...
> 
> Re locking, that issue would be addressed as part of supporting
> multiple run states.  I still don't see suspend ops being involved
> in non-suspend transitions.

Well, I'll go further and say that in principle the .suspend() callbacks have
nothing to do with non-suspend transitions.  In some situations they may
perform similar operations, but in that case the common code should be put
into a separate function and called _explicitly_ from the two places, to avoid
confusion.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth