[v7] introduce macro spin_event_timeout()

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

v7: add if-statement to use cpu_relax() if the delay is 0.  gcc will optimize
out the if-statement if 'delay' is a constant.

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 |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

On Tue,  5 May 2009 16:53:05 -0500
"Timur Tabi" <timur@freescale.com> 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>

Nice. I could have used a routine like this in a couple of our drivers.
So, for what it is worth:

Acked-by: Sean MacLennan <smaclennan@pikatech.com>

Cheers,
   Sean

On Tue, May 5, 2009 at 3:53 PM, Timur Tabi <timur@freescale.com> 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>

This version looks good to me.  Who's the first user? (I'd like to see
that something is ready to use this before merging it)

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>
> v7: add if-statement to use cpu_relax() if the delay is 0.  gcc will optimize
> out the if-statement if 'delay' is a constant.
>
> 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 |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/delay.h b/arch/powerpc/include/asm/delay.h
> index f9200a6..2bde26f 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,35 @@ 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 even if the bit never changes.  The delay is for devices that
> + * need a delay in between successive reads.
> + *
> + * gcc will optimize out the if-statement if @delay is a constant.
> + */
> +#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)) \
> +               if (delay)                                                  \
> +                       udelay(delay);                                      \
> +               else                                                        \
> +                       cpu_relax();                                        \
> +}
> +
>  #endif /* __KERNEL__ */
>  #endif /* _ASM_POWERPC_DELAY_H */
> --
> 1.6.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>

Grant Likely wrote:

> This version looks good to me.  Who's the first user? (I'd like to see
> that something is ready to use this before merging it)

I was going to do it the other way around - wait until this patch is merged, and then update some code to use it.  After all, it's easier to use a macro if it already exists in a header file. :-)

Grepping for "while.*in_be" finds lots of candidates.

I was originally planning to use this macro in sound/soc/fsl/fsl_ssi.c, but I eliminated the need to poll on a register with commit a4d11fe50c238a7da5225d1399314c3505cbd792 ("ASoC: remove trigger delay in Freescale MPC8610 sound driver").  So now I need to find a new guinea pig.

On Thu, May 14, 2009 at 10:03 AM, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
>> This version looks good to me.  Who's the first user? (I'd like to see
>> that something is ready to use this before merging it)
>
> I was going to do it the other way around - wait until this patch is merged, and then update some code to use it.  After all, it's easier to use a macro if it already exists in a header file. :-)

Heh, I think you miss my meaning.  Of course this patch must be merged
before any users.  I just want to hold off merging it until I see
viable patches on the mailing list which depend on it.

In other words, write your patches which use it and submit the lot as
a patch series with this patch as the first.  That gives some evidence
that this macro will actually be used and useful.

g.

On Thu, May 14, 2009 at 11:10 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> In other words, write your patches which use it and submit the lot as
> a patch series with this patch as the first.  That gives some evidence
> that this macro will actually be used and useful.

This is going to be more difficult than you think.  The problem is
that I don't have access to much hardware that uses drivers which can
take advantage of this macro.  I can find one, maybe two examples, but
if I put a timeout that's too short, I might break some other platform
without knowing it.

On Mon, May 18, 2009 at 4:49 PM, Timur Tabi <timur@freescale.com> wrote:
> On Thu, May 14, 2009 at 11:10 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> In other words, write your patches which use it and submit the lot as
>> a patch series with this patch as the first.  That gives some evidence
>> that this macro will actually be used and useful.
>
> This is going to be more difficult than you think.  The problem is
> that I don't have access to much hardware that uses drivers which can
> take advantage of this macro.  I can find one, maybe two examples, but
> if I put a timeout that's too short, I might break some other platform
> without knowing it.

Then let it lie fallow on the list and in patchwork.  It is published,
and people know that it is available (I'll certainly consider it as I
do driver work), but there must be real (not just theoretical) in
kernel users of the interface.  I someone wants to use it, then they
can add it to their series or ping the list about getting it merged.

g.

On Mon, 18 May 2009 20:58:03 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> Then let it lie fallow on the list and in patchwork.  It is published,
> and people know that it is available (I'll certainly consider it as I
> do driver work), but there must be real (not just theoretical) in
> kernel users of the interface.  I someone wants to use it, then they
> can add it to their series or ping the list about getting it merged.

That's too bad. If it is ever merged, could you also ping the list?

Cheers,
   Sean

On Mon, May 18, 2009 at 9:57 PM, Sean MacLennan <smaclennan@pikatech.com> wrote:
> On Mon, 18 May 2009 20:58:03 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> Then let it lie fallow on the list and in patchwork.  It is published,
>> and people know that it is available (I'll certainly consider it as I
>> do driver work), but there must be real (not just theoretical) in
>> kernel users of the interface.  I someone wants to use it, then they
>> can add it to their series or ping the list about getting it merged.
>
> That's too bad. If it is ever merged, could you also ping the list?

If you intend to use it, then post a patch to do so and state that it
depends on Timur's patch.

g.