[PATCH v6] introduce macro spin_event_timeout()

[PATCH v6] introduce macro spin_event_timeout()

[Timur Tabi]

The macro spin_event_timeout() takes a condition and timeout value
(in microseconds) as parameters.  It spins until either the condition is true
or the timeout expires.  It returns the result of the condition when the loop
was terminated.

This primary purpose of this macro is to poll on a hardware register until a
status bit changes.  The timeout ensures that the loop still terminates if the
bit doesn't change as expected.  This macro makes it easier for driver
developers to perform this kind of operation properly.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

I'm making this a PowerPC-specific patch because I want to use 
tb_ticks_per_usec, which does not exist on all other platforms.  I don't want
to use jiffies because jiffies works only when interrupts are enabled, and
the resolution may not be fine enough.

 arch/powerpc/include/asm/delay.h |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h
index f9200a6..1939e0f 100644
--- a/arch/powerpc/include/asm/delay.h
+++ b/arch/powerpc/include/asm/delay.h
@@ -2,6 +2,8 @@
 #define _ASM_POWERPC_DELAY_H
 #ifdef __KERNEL__
 
+#include <asm/time.h>
+
 /*
  * Copyright 1996, Paul Mackerras.
  *
@@ -30,5 +32,30 @@ extern void udelay(unsigned long usecs);
 #define mdelay(n)	udelay((n) * 1000)
 #endif
 
+/**
+ * spin_event_timeout - spin until a condition gets true or a timeout elapses
+ * @condition: a C expression to evalate
+ * @timeout: timeout, in microseconds
+ * @delay: the number of microseconds to delay between eache evaluation of
+ *         @condition
+ * @rc: the last value of the condition
+ *
+ * The process spins until the condition evaluates to true (non-zero) or the
+ * timeout elapses.  Upon exit, @rc contains the value of the condition. This
+ * allows you to test the condition without incurring any side effects.
+ *
+ * This primary purpose of this macro is to poll on a hardware register
+ * until a status bit changes.  The timeout ensures that the loop still
+ * terminates if the bit never changes.  The delay is for devices that need a
+ * delay in between successive reads.
+ */
+#define spin_event_timeout(condition, timeout, delay, rc)                   \
+{                                                                           \
+	unsigned long __loops = tb_ticks_per_usec * timeout;                \
+	unsigned long __start = get_tbl();                                  \
+	while (!(rc = (condition)) && (tb_ticks_since(__start) <= __loops)) \
+		udelay(delay);                                      	    \
+}
+
 #endif /* __KERNEL__ */
 #endif /* _ASM_POWERPC_DELAY_H */
-- 
1.6.0.6

Re: [PATCH v6] introduce macro spin_event_timeout()

[Timur Tabi]

On Mon, Apr 27, 2009 at 2:05 PM, Timur Tabi <timur@freescale.com> wrote:
> The macro spin_event_timeout() takes a condition and timeout value
> (in microseconds) as parameters. =A0It spins until either the condition i=
s true
> or the timeout expires. =A0It returns the result of the condition when th=
e loop
> was terminated.
>
> This primary purpose of this macro is to poll on a hardware register unti=
l a
> status bit changes. =A0The timeout ensures that the loop still terminates=
 if the
> bit doesn't change as expected. =A0This macro makes it easier for driver
> developers to perform this kind of operation properly.

Any comments on this patch?  I'd like to see this macro added to 2.6.31.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH v6] introduce macro spin_event_timeout()

[Kumar Gala]

On Apr 27, 2009, at 2:05 PM, Timur Tabi wrote:

> The macro spin_event_timeout() takes a condition and timeout value
> (in microseconds) as parameters.  It spins until either the  
> condition is true
> or the timeout expires.  It returns the result of the condition when  
> the loop
> was terminated.
>
> This primary purpose of this macro is to poll on a hardware register  
> until a
> status bit changes.  The timeout ensures that the loop still  
> terminates if the
> bit doesn't change as expected.  This macro makes it easier for driver
> developers to perform this kind of operation properly.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> I'm making this a PowerPC-specific patch because I want to use
> tb_ticks_per_usec, which does not exist on all other platforms.  I  
> don't want
> to use jiffies because jiffies works only when interrupts are  
> enabled, and
> the resolution may not be fine enough.
>
> arch/powerpc/include/asm/delay.h |   27 +++++++++++++++++++++++++++
> 1 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/ 
> asm/delay.h
> index f9200a6..1939e0f 100644
> --- a/arch/powerpc/include/asm/delay.h
> +++ b/arch/powerpc/include/asm/delay.h
> @@ -2,6 +2,8 @@
> #define _ASM_POWERPC_DELAY_H
> #ifdef __KERNEL__
>
> +#include <asm/time.h>
> +
> /*
>  * Copyright 1996, Paul Mackerras.
>  *
> @@ -30,5 +32,30 @@ extern void udelay(unsigned long usecs);
> #define mdelay(n)	udelay((n) * 1000)
> #endif
>
> +/**
> + * spin_event_timeout - spin until a condition gets true or a  
> timeout elapses
> + * @condition: a C expression to evalate
> + * @timeout: timeout, in microseconds
> + * @delay: the number of microseconds to delay between eache  
> evaluation of
> + *         @condition
> + * @rc: the last value of the condition
> + *
> + * The process spins until the condition evaluates to true (non- 
> zero) or the
> + * timeout elapses.  Upon exit, @rc contains the value of the  
> condition. This
> + * allows you to test the condition without incurring any side  
> effects.
> + *
> + * This primary purpose of this macro is to poll on a hardware  
> register
> + * until a status bit changes.  The timeout ensures that the loop  
> still
> + * terminates if the bit never changes.  The delay is for devices  
> that need a
> + * delay in between successive reads.
> + */
> +#define spin_event_timeout(condition, timeout, delay,  
> rc)                   \
> + 
> {                                                                           \
> +	unsigned long __loops = tb_ticks_per_usec *  
> timeout;                \
> +	unsigned long __start =  
> get_tbl();                                  \
> +	while (!(rc = (condition)) && (tb_ticks_since(__start) <=  
> __loops)) \
> +		udelay(delay);                                      	    \
> +}
> +

I wouldn't call it spin_event_timeout as its a bit too generic of a  
name for something that is arch specific.

- k

Re: [PATCH v6] introduce macro spin_event_timeout()

[Timur Tabi]

Kumar Gala wrote:

> I wouldn't call it spin_event_timeout as its a bit too generic of a  
> name for something that is arch specific.

Well, I'm hoping it won't be arch-specific forever.  I'd like to see
other architectures implement something like tb_ticks_per_usec.  I'm
only tackling PowerPC for now.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH v6] introduce macro spin_event_timeout()

[Scott Wood]

Kumar Gala wrote:
> I wouldn't call it spin_event_timeout as its a bit too generic of a name 
> for something that is arch specific.

The concept is not arch-specific -- ideally, other architectures could 
add their own implementations (or it could be made 
non-architecture-specific in the future).

-Scott

Re: [PATCH v6] introduce macro spin_event_timeout()

[Sean MacLennan]

On Mon, 27 Apr 2009 14:05:44 -0500
"Timur Tabi" <timur@freescale.com> wrote:

> +#define spin_event_timeout(condition, timeout, delay,
> rc)                   \
> +{                                                                           \
> +	unsigned long __loops = tb_ticks_per_usec *
> timeout;                \
> +	unsigned long __start =
> get_tbl();                                  \
> +	while (!(rc = (condition)) && (tb_ticks_since(__start) <=
> __loops)) \
> +		udelay(delay);
> 	    \ +}
> +

Would cpu_relax be a good thing to put here?

Something like:

while (!(rc = (condition)) && (tb_ticks_since(__start) <= __loops)) \
	if (delay) \
		udelay(delay); \
	else \
		cpu_relax(); \

Cheers,
   Sean

Re: [PATCH v6] introduce macro spin_event_timeout()

[Timur Tabi]

Sean MacLennan wrote:

> Would cpu_relax be a good thing to put here?
> 
> Something like:
> 
> while (!(rc = (condition)) && (tb_ticks_since(__start) <= __loops)) \
> 	if (delay) \
> 		udelay(delay); \
> 	else \
> 		cpu_relax(); \
> 

I had that at one point, but then I looked at the code for udelay(), and
it appears that if I do udelay(0), it does something similar to cpu_relax:

	start = get_tbl();
	while (get_tbl() - start < loops)
		HMT_low();
	HMT_medium();

cpu_relax does this:

do { HMT_low(); HMT_medium(); barrier(); } while (0)

Well, now that I look at it, cpu_relax() changes the thread priority to
low and then back to medium, whereas udelay() never sets it to low if
'loops' is 0.

I'm okay with changing my code, but I wonder if udelay() should look
like this:

	start = get_tbl();
	HMT_low();
	while (get_tbl() - start < loops)
		HMT_low();
	HMT_medium();

-- 
Timur Tabi
Linux kernel developer at Freescale