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

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@gmail.com>
---

 include/wait_bit.h | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 include/wait_bit.h

@Tom: I forgot to add you on CC :(

I think this patch goes to you.

On Sun, Dec 27, 2015 at 06:28:08PM +0100, Mateusz Kulikowski 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@gmail.com>

Regards,
Mateusz

On Sun, Dec 27, 2015 at 06:28:08PM +0100, Mateusz Kulikowski 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@gmail.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

Hi,

On 27 December 2015 at 10:28, Mateusz Kulikowski
<mateusz.kulikowski@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@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@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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

On 20.01.2016 05:34, Simon Glass wrote:
[...]
> On 27 December 2015 at 10:28, Mateusz Kulikowski
> <mateusz.kulikowski@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.
[...]
> 
> Sorry I only just saw this, but thought I'd make a few comments.

Nooo, I was expecting at least this to be merged during this merge window :)

[...]
>> + *
>> + * @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

This may be a good idea to make it more foolproof - 

@trini:	 Will v4 with small change like that delay merging this series into mainline?

> 
>> +                              const bool breakable)
> 
> Wow this is a pretty big inline function.

I personally probably could just drop inline and leave "static" but still
keep it in header (so it may not be inlined), 
but it would probably violate some unwritten holy rules :)

First version was compiled into object file, but then either it would require 
extra config option, or would pollute rodata of all boards (which is bad).

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

I prefer to keep it as is (for now).

This function is supposed to be drop-in replacement for four almost the same 
functions in drivers (dwc2, ohci-lpc..., ehci-mx6 and zynq_gem). 

My intent was to keep all changes as small as possible so I would not cause
regressions, but will make some people happy.

As for argument count - there was already request to add new feature [1], 
which is nice (I appended it to my task queue), so I can rework it a bit later
(and perhaps use it in even more places where it would be useful).

As long as this function is inlined - argument count doesn't matter that much
imo - as long as one remembers argument order or has smart IDE that does it for him.

> 
>> +{
>> +       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?

Same arguments as above apply. 

Although I agree that in future it may be useful not to have puts here.

Is it ok with you (timeout -> timeout_ms if possible I'll do now, rest + [1] 
in future)?

[1] http://lists.denx.de/pipermail/u-boot/2015-December/239468.html

Regards,
Mateusz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWn/YnAAoJELvtohmVtQzBMFMIAITNu+ORG3trzOpc3xaM2QXC
4WZG89SDkM/KW26LpZEj5I/aARr5rPwO637zCfc7Vf6k1VX1CohdRPv7E3wiiOQ3
Lt6NL6yyLQfIzQkFQb5373ao7GbuyKUqvsbsQkd2TGDUTtEgo9tRWLtpt9wTstMT
H0YK2uNb9Zg6pJ6Z/0xCLua723DXcSXPgx8PV2Wbo3nR3BIlz70HYLHKvAMw2O2w
phSX2/TIx7LjCUw4lvIfGJXapnZV3z9hmCOLsHCPEZAbcE5MYKqX/t7GJu3reuao
j9MzZzpxr6CTzdavPhWxcpsNUwVsg7Q9KOIq7DQMA5qoW6EKLeOSKdr6FxKReFg=
=fVR7
-----END PGP SIGNATURE-----

Hi Matueuz,

On 20 January 2016 at 14:03, Mateusz Kulikowski
<mateusz.kulikowski@gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Hi,
>
> On 20.01.2016 05:34, Simon Glass wrote:
> [...]
>> On 27 December 2015 at 10:28, Mateusz Kulikowski
>> <mateusz.kulikowski@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.
> [...]
>>
>> Sorry I only just saw this, but thought I'd make a few comments.
>
> Nooo, I was expecting at least this to be merged during this merge window :)
>
> [...]
>>> + *
>>> + * @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
>
> This may be a good idea to make it more foolproof -
>
> @trini:  Will v4 with small change like that delay merging this series into mainline?
>
>>
>>> +                              const bool breakable)
>>
>> Wow this is a pretty big inline function.
>
> I personally probably could just drop inline and leave "static" but still
> keep it in header (so it may not be inlined),
> but it would probably violate some unwritten holy rules :)
>
> First version was compiled into object file, but then either it would require
> extra config option, or would pollute rodata of all boards (which is bad).

If you drop the string the rodata add-on (presumably due to the gcc
bug) would be tiny, so I don't think it would need a Kconfig.

>
>>
>> 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.
>
> I prefer to keep it as is (for now).
>
> This function is supposed to be drop-in replacement for four almost the same
> functions in drivers (dwc2, ohci-lpc..., ehci-mx6 and zynq_gem).
>
> My intent was to keep all changes as small as possible so I would not cause
> regressions, but will make some people happy.
>
> As for argument count - there was already request to add new feature [1],
> which is nice (I appended it to my task queue), so I can rework it a bit later
> (and perhaps use it in even more places where it would be useful).
>
> As long as this function is inlined - argument count doesn't matter that much
> imo - as long as one remembers argument order or has smart IDE that does it for him.
>
>>
>>> +{
>>> +       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?
>
> Same arguments as above apply.
>
> Although I agree that in future it may be useful not to have puts here.
>
> Is it ok with you (timeout -> timeout_ms if possible I'll do now, rest + [1]
> in future)?

Please go ahead, you already have a review by Tom. My comment are just ideas.

>
> [1] http://lists.denx.de/pipermail/u-boot/2015-December/239468.html
>
> Regards,
> Mateusz

Regards,
Simon

On Wed, Jan 20, 2016 at 10:03:40PM +0100, Mateusz Kulikowski wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> Hi,
> 
> On 20.01.2016 05:34, Simon Glass wrote:
> [...]
> > On 27 December 2015 at 10:28, Mateusz Kulikowski
> > <mateusz.kulikowski@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.
> [...]
> > 
> > Sorry I only just saw this, but thought I'd make a few comments.
> 
> Nooo, I was expecting at least this to be merged during this merge window :)
> 
> [...]
> >> + *
> >> + * @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
> 
> This may be a good idea to make it more foolproof - 
> 
> @trini:	 Will v4 with small change like that delay merging this series into mainline?

Nope, just get it posted soon please :)