[U-Boot] [PATCH v3 1/5] lib: Add wait_for_bit

[Simon Glass]

Hi,

On 27 December 2015 at 10:28, Mateusz Kulikowski
<mateusz.kulikowski at gmail.com> wrote:
> Add function to poll register waiting for specific bit(s).
> Similar functions are implemented in few drivers - they are almost
> identical and can be generalized.
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski at gmail.com>
> ---
>
>  include/wait_bit.h | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 include/wait_bit.h
>

Sorry I only just saw this, but thought I'd make a few comments.

> diff --git a/include/wait_bit.h b/include/wait_bit.h
> new file mode 100644
> index 0000000..4867ced
> --- /dev/null
> +++ b/include/wait_bit.h
> @@ -0,0 +1,71 @@
> +/*
> + * Wait for bit with timeout and ctrlc
> + *
> + * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski at gmail.com>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef __WAIT_BIT_H
> +#define __WAIT_BIT_H
> +
> +#include <common.h>
> +#include <console.h>
> +#include <asm/errno.h>
> +#include <asm/io.h>
> +
> +/**
> + * wait_for_bit()      waits for bit set/cleared in register
> + *
> + * Function polls register waiting for specific bit(s) change
> + * (either 0->1 or 1->0). It can fail under two conditions:
> + * - Timeout
> + * - User interaction (CTRL-C)
> + * Function succeeds only if all bits of masked register are set/cleared
> + * (depending on set option).
> + *
> + * @param prefix       Prefix added to timeout messagge (message visible only
> + *                     with debug enabled)
> + * @param reg          Register that will be read (using readl())
> + * @param mask         Bit(s) of register that must be active
> + * @param set          Selects wait condition (bit set or clear)
> + * @param timeout      Timeout (in miliseconds)
> + * @param breakable    Enables CTRL-C interruption
> + * @return             0 on success, -ETIMEDOUT or -EINTR on failure
> + */
> +static inline int wait_for_bit(const char *prefix, const u32 *reg,
> +                              const u32 mask, const bool set,
> +                              const unsigned int timeout,

timeout_ms would be more obvious

> +                              const bool breakable)

Wow this is a pretty big inline function.

Do you need the 'prefix' parameter? It seems that the callers print
messages anyway. How about adding a flags word for @set and
@breakable? Those params could then be combined, and you end up with 4
parameters instead of 6.

> +{
> +       u32 val;
> +       unsigned long start = get_timer(0);
> +
> +       while (1) {
> +               val = readl(reg);
> +
> +               if (!set)
> +                       val = ~val;
> +
> +               if ((val & mask) == mask)
> +                       return 0;
> +
> +               if (get_timer(start) > timeout)
> +                       break;
> +
> +               if (breakable && ctrlc()) {
> +                       puts("Abort\n");

This is bad if used from drivers. We try not to output things. It it necessary?

> +                       return -EINTR;
> +               }
> +
> +               udelay(1);
> +       }
> +
> +       debug("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n", prefix, reg, mask,
> +             set);
> +
> +       return -ETIMEDOUT;
> +}
> +
> +
> +#endif
> --
> 2.5.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot