Patchwork [2/2] qe: add polling timeout to qe_issue_cmd()

The qe_issue_cmd() function (Freescale PowerPC QUICC Engine library) polls on
a register until a status bit changes, but does not include a timeout to
handle the situation if the bit never changes.  Change the code to use the new
spin_event_timeout() macro, which simplifies polling on a register without
a timeout.

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

This patch depends on my previous patch, "powerpc: introduce macro
spin_event_timeout()".

 arch/powerpc/sysdev/qe_lib/qe.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

On Tue, May 19, 2009 at 1:26 PM, Timur Tabi <timur@freescale.com> wrote:
> The qe_issue_cmd() function (Freescale PowerPC QUICC Engine library) polls on
> a register until a status bit changes, but does not include a timeout to
> handle the situation if the bit never changes.  Change the code to use the new
> spin_event_timeout() macro, which simplifies polling on a register without
> a timeout.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> This patch depends on my previous patch, "powerpc: introduce macro
> spin_event_timeout()".
>
>  arch/powerpc/sysdev/qe_lib/qe.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/qe_lib/qe.c b/arch/powerpc/sysdev/qe_lib/qe.c
> index 01bce37..810e1df 100644
> --- a/arch/powerpc/sysdev/qe_lib/qe.c
> +++ b/arch/powerpc/sysdev/qe_lib/qe.c
> @@ -111,6 +111,7 @@ int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
>  {
>        unsigned long flags;
>        u8 mcn_shift = 0, dev_shift = 0;
> +       int ret;
>
>        spin_lock_irqsave(&qe_lock, flags);
>        if (cmd == QE_RESET) {
> @@ -138,11 +139,13 @@ int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
>        }
>
>        /* wait for the QE_CR_FLG to clear */
> -       while(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG)
> -               cpu_relax();
> +       spin_event_timeout((in_be32(&qe_immr->cp.cecr) & QE_CR_FLG) == 0,
> +                          100, 0, ret);
> +       /* On timeout (e.g. failure), the expression will be false (ret == 0),
> +          otherwise it will be true (ret == 1). */
>        spin_unlock_irqrestore(&qe_lock, flags);
>
> -       return 0;
> +       return ret == 1;

Hmmm, the ret value is backwards from what most coders would expect
(zero on success, non-zero on failure).  I'd personally recommend
reversing the polarity in the macro.

Otherwise, feel free to add my acked-by line to both patches.

g.

Grant Likely wrote:

> Hmmm, the ret value is backwards from what most coders would expect
> (zero on success, non-zero on failure).  I'd personally recommend
> reversing the polarity in the macro.

The ret value is documented as being the value of the expression when the loop terminates.  The reason it appears backwards is because the expression is

	(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG) == 0

That is, the loop should spin until the QE_CR_FLG is zero.  So when the loop terminates normally, the expression (x) == 0 is true, which is equal to one.

I would expect that in most cases, the loop spins until some bit is *set*.  Let's pretend that QE_CR_FLG operates this way.  In that case, the call would look like this:

spin_event_timeout(in_be32(&qe_immr->cp.cecr) & QE_CR_FLG, 100, 0, ret);

At loop termination, the result of the expression will be either QE_CR_FLG or zero.

> Otherwise, feel free to add my acked-by line to both patches.

Thanks.

Ben, would you please apply this to your 'next' branch?