[U-Boot] [PATCH v3 0/5] Add wait_for_bit()

[U-Boot] [PATCH v3 0/5] Add wait_for_bit()

[Mateusz Kulikowski]

Changes in V3:
- wait_for_bit code is moved to header and
  converted to static inline function.
  Previous version added ~7byte of rodata to
  all boards (even not using wait_for_bit).

Changes in V2:
- wait_bit.o is always compiled in
- Removed CONFIG_LIB_WAIT_BIT from configs/Kconfigs
- Constified arguments to wait_bit
- Removed check for CONFIG_... in drivers
- Added tested-by to ohci-lp32xx
@Sylvain Lemieux: I didn't changed driver logic with v2,
so allowed myself to add your tested-by directly.

Tested on:
 - USB driver on Dragonboard (not yet in mainline)
 - Infinite sleep (if timeout/interruption works)
Build tested on single board for each driver/commit:
- hikey
- zynq_microzed
- platinum

(Old) notes from V1:

This series add generic function to poll register waiting for
one or more bits to change.

Very similar function was used in several drivers:
- dwc2
- ohci-lp32xx
- ehci-mx6
- zynq_gem

First patch adds function, following patches update drivers and
board config files / defconfigs.

This series was compile-tested with buildman for ~50 boards
(most or even all boards affected by change)

Code was also run-tested on ehci-msm driver (not yet in mainline)

There is single difference in behavior: ohci-lp32xx driver will
not print "Timeout..." message with debug disabled.
I think it's not a big issue as this driver seems unused, but
if it's an issue - please drop that patch.


Mateusz Kulikowski (5):
  lib: Add wait_for_bit
  usb: dwc2: Use shared wait_for_bit
  usb: ohci-lpc32xx: Use shared wait_for_bit
  usb: ehci-mx6: Use shared wait_for_bit
  net: zynq_gem: Use shared wait_for_bit

 drivers/net/zynq_gem.c          | 35 ++------------------
 drivers/usb/host/dwc2.c         | 41 ++++++++----------------
 drivers/usb/host/ehci-mx6.c     | 32 +++----------------
 drivers/usb/host/ohci-lpc32xx.c | 34 ++++----------------
 include/wait_bit.h              | 71 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 116 deletions(-)
 create mode 100644 include/wait_bit.h

-- 
2.5.0

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

[Mateusz Kulikowski]

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

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,
+			       const bool breakable)
+{
+	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");
+			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] [PATCH v3 1/5] lib: Add wait_for_bit

[Mateusz Kulikowski]

@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

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

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/597961a6/attachment.sig>

[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@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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

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

[Mateusz Kulikowski]

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

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

[Simon Glass]

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

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

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160121/f6b26df9/attachment.sig>

[U-Boot] [PATCH v3 2/5] usb: dwc2: Use shared wait_for_bit

[Mateusz Kulikowski]

Use existing library function to poll bit(s).

Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
---

 drivers/usb/host/dwc2.c | 41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
index 541c0f9..42d31e3 100644
--- a/drivers/usb/host/dwc2.c
+++ b/drivers/usb/host/dwc2.c
@@ -13,6 +13,7 @@
 #include <memalign.h>
 #include <phys2bus.h>
 #include <usbroothubdes.h>
+#include <wait_bit.h>
 #include <asm/io.h>
 
 #include "dwc2.h"
@@ -52,27 +53,6 @@ static struct dwc2_priv local;
 /*
  * DWC2 IP interface
  */
-static int wait_for_bit(void *reg, const uint32_t mask, bool set)
-{
-	unsigned int timeout = 1000000;
-	uint32_t val;
-
-	while (--timeout) {
-		val = readl(reg);
-		if (!set)
-			val = ~val;
-
-		if ((val & mask) == mask)
-			return 0;
-
-		udelay(1);
-	}
-
-	debug("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n",
-	      __func__, reg, mask, set);
-
-	return -ETIMEDOUT;
-}
 
 /*
  * Initializes the FSLSPClkSel field of the HCFG register
@@ -117,7 +97,8 @@ static void dwc_otg_flush_tx_fifo(struct dwc2_core_regs *regs, const int num)
 
 	writel(DWC2_GRSTCTL_TXFFLSH | (num << DWC2_GRSTCTL_TXFNUM_OFFSET),
 	       &regs->grstctl);
-	ret = wait_for_bit(&regs->grstctl, DWC2_GRSTCTL_TXFFLSH, 0);
+	ret = wait_for_bit(__func__, &regs->grstctl, DWC2_GRSTCTL_TXFFLSH,
+			   false, 1000, false);
 	if (ret)
 		printf("%s: Timeout!\n", __func__);
 
@@ -135,7 +116,8 @@ static void dwc_otg_flush_rx_fifo(struct dwc2_core_regs *regs)
 	int ret;
 
 	writel(DWC2_GRSTCTL_RXFFLSH, &regs->grstctl);
-	ret = wait_for_bit(&regs->grstctl, DWC2_GRSTCTL_RXFFLSH, 0);
+	ret = wait_for_bit(__func__, &regs->grstctl, DWC2_GRSTCTL_RXFFLSH,
+			   false, 1000, false);
 	if (ret)
 		printf("%s: Timeout!\n", __func__);
 
@@ -152,13 +134,15 @@ static void dwc_otg_core_reset(struct dwc2_core_regs *regs)
 	int ret;
 
 	/* Wait for AHB master IDLE state. */
-	ret = wait_for_bit(&regs->grstctl, DWC2_GRSTCTL_AHBIDLE, 1);
+	ret = wait_for_bit(__func__, &regs->grstctl, DWC2_GRSTCTL_AHBIDLE,
+			   true, 1000, false);
 	if (ret)
 		printf("%s: Timeout!\n", __func__);
 
 	/* Core Soft Reset */
 	writel(DWC2_GRSTCTL_CSFTRST, &regs->grstctl);
-	ret = wait_for_bit(&regs->grstctl, DWC2_GRSTCTL_CSFTRST, 0);
+	ret = wait_for_bit(__func__, &regs->grstctl, DWC2_GRSTCTL_CSFTRST,
+			   false, 1000, false);
 	if (ret)
 		printf("%s: Timeout!\n", __func__);
 
@@ -243,8 +227,8 @@ static void dwc_otg_core_host_init(struct dwc2_core_regs *regs)
 		clrsetbits_le32(&regs->hc_regs[i].hcchar,
 				DWC2_HCCHAR_EPDIR,
 				DWC2_HCCHAR_CHEN | DWC2_HCCHAR_CHDIS);
-		ret = wait_for_bit(&regs->hc_regs[i].hcchar,
-				   DWC2_HCCHAR_CHEN, 0);
+		ret = wait_for_bit(__func__, &regs->hc_regs[i].hcchar,
+				   DWC2_HCCHAR_CHEN, false, 1000, false);
 		if (ret)
 			printf("%s: Timeout!\n", __func__);
 	}
@@ -737,7 +721,8 @@ int wait_for_chhltd(struct dwc2_core_regs *regs, uint32_t *sub, int *toggle,
 	int ret;
 	uint32_t hcint, hctsiz;
 
-	ret = wait_for_bit(&hc_regs->hcint, DWC2_HCINT_CHHLTD, true);
+	ret = wait_for_bit(__func__, &hc_regs->hcint, DWC2_HCINT_CHHLTD, true,
+			   1000, false);
 	if (ret)
 		return ret;
 
-- 
2.5.0

[U-Boot] [PATCH v3 2/5] usb: dwc2: Use shared wait_for_bit

[Stefan Bruens]

On Sonntag, 27. Dezember 2015 18:28:09 CET Mateusz Kulikowski wrote:
> Use existing library function to poll bit(s).
> 
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>

It might be useful to have not only a relative timeout, but also an absolute 
timeout. For dwc2, the timeout handling could be moved from the transactions 
wrappers to the chunk_msg function. As the USB timeouts are specified for the 
whole transaction, it would be very neat to calculate one deadline for the 
whole transaction and hand this absolute timeout to the wait_for_bit(..) 
function. At the end, this would also result in less code.

Kind regards,

Stefan

-- 
Stefan Br?ns  /  Bergstra?e 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424

[U-Boot] [PATCH v3 2/5] usb: dwc2: Use shared wait_for_bit

[Mateusz Kulikowski]

Hi Stefan,

On Sun, Dec 27, 2015 at 11:17:55PM +0100, Stefan Bruens wrote:
> On Sonntag, 27. Dezember 2015 18:28:09 CET Mateusz Kulikowski wrote:
> > Use existing library function to poll bit(s).
> > 
> > Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
> 
> It might be useful to have not only a relative timeout, but also an absolute 
> timeout. For dwc2, the timeout handling could be moved from the transactions 
> wrappers to the chunk_msg function. As the USB timeouts are specified for the 
> whole transaction, it would be very neat to calculate one deadline for the 
> whole transaction and hand this absolute timeout to the wait_for_bit(..) 
> function. At the end, this would also result in less code.

This may be a good idea, but I prefer to add it to my todo list (or you can 
do it once this series gets submitted or as dependant patch).

I need wait_for_bit for my board/driver. Doing another set of upgrades 
will delay it.
I will also have access to dwc2 device around mid. Jan so will be able to test
"bigger" refactors.

Is it ok with you?

Regards,
Mateusz

[U-Boot] [PATCH v3 2/5] usb: dwc2: Use shared wait_for_bit

[Tom Rini]

On Sun, Dec 27, 2015 at 06:28:09PM +0100, Mateusz Kulikowski wrote:

> Use existing library function to poll bit(s).
> 
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/4117969a/attachment.sig>

[U-Boot] [PATCH v3 3/5] usb: ohci-lpc32xx: Use shared wait_for_bit

[Mateusz Kulikowski]

Use existing library function to poll bit(s).

Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
Tested-by: Sylvain Lemieux <slemieux@tycoint.com>
---

 drivers/usb/host/ohci-lpc32xx.c | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/ohci-lpc32xx.c b/drivers/usb/host/ohci-lpc32xx.c
index 48d338e..9245126 100644
--- a/drivers/usb/host/ohci-lpc32xx.c
+++ b/drivers/usb/host/ohci-lpc32xx.c
@@ -10,6 +10,7 @@
 
 #include <common.h>
 #include <errno.h>
+#include <wait_bit.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/clk.h>
@@ -80,30 +81,6 @@ struct otg_regs {
 static struct otg_regs *otg = (struct otg_regs *)USB_BASE;
 static struct clk_pm_regs *clk_pwr = (struct clk_pm_regs *)CLK_PM_BASE;
 
-static int wait_for_bit(void *reg, const u32 mask, bool set)
-{
-	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) > CONFIG_SYS_HZ)
-			break;
-
-		udelay(1);
-	}
-
-	error("Timeout (reg=%p mask=%08x wait_set=%i)\n", reg, mask, set);
-
-	return -ETIMEDOUT;
-}
-
 static int isp1301_set_value(int reg, u8 value)
 {
 	return i2c_write(ISP1301_I2C_ADDR, reg, 1, &value, 1);
@@ -158,7 +135,8 @@ static int usbpll_setup(void)
 	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_POSTDIV_2POW(0x01));
 	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_PLL_PWRUP);
 
-	ret = wait_for_bit(&clk_pwr->usb_ctrl, CLK_USBCTRL_PLL_STS, 1);
+	ret = wait_for_bit(__func__, &clk_pwr->usb_ctrl, CLK_USBCTRL_PLL_STS,
+			   true, CONFIG_SYS_HZ, false);
 	if (ret)
 		return ret;
 
@@ -183,7 +161,8 @@ int usb_cpu_init(void)
 
 	/* enable I2C clock */
 	writel(OTG_CLK_I2C_EN, &otg->otg_clk_ctrl);
-	ret = wait_for_bit(&otg->otg_clk_sts, OTG_CLK_I2C_EN, 1);
+	ret = wait_for_bit(__func__, &otg->otg_clk_sts, OTG_CLK_I2C_EN, true,
+			   CONFIG_SYS_HZ, false);
 	if (ret)
 		return ret;
 
@@ -203,7 +182,8 @@ int usb_cpu_init(void)
 			 OTG_CLK_I2C_EN | OTG_CLK_HOST_EN;
 	writel(mask, &otg->otg_clk_ctrl);
 
-	ret = wait_for_bit(&otg->otg_clk_sts, mask, 1);
+	ret = wait_for_bit(__func__, &otg->otg_clk_sts, mask, true,
+			   CONFIG_SYS_HZ, false);
 	if (ret)
 		return ret;
 
-- 
2.5.0

[U-Boot] [PATCH v3 3/5] usb: ohci-lpc32xx: Use shared wait_for_bit

[Tom Rini]

On Sun, Dec 27, 2015 at 06:28:10PM +0100, Mateusz Kulikowski wrote:

> Use existing library function to poll bit(s).
> 
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
> Tested-by: Sylvain Lemieux <slemieux@tycoint.com>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/ce713e21/attachment.sig>

[U-Boot] [PATCH v3 4/5] usb: ehci-mx6: Use shared wait_for_bit

[Mateusz Kulikowski]

Use existing library function to poll bit(s).

Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
---

 drivers/usb/host/ehci-mx6.c | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
index 2666351..e1c67f7 100644
--- a/drivers/usb/host/ehci-mx6.c
+++ b/drivers/usb/host/ehci-mx6.c
@@ -8,6 +8,7 @@
 #include <common.h>
 #include <usb.h>
 #include <errno.h>
+#include <wait_bit.h>
 #include <linux/compiler.h>
 #include <usb/ehci-fsl.h>
 #include <asm/io.h>
@@ -117,32 +118,6 @@ static void usb_power_config(int index)
 		     pll_480_ctrl_set);
 }
 
-static int wait_for_bit(u32 *reg, const u32 mask, bool set)
-{
-	u32 val;
-	const unsigned int timeout = 10000;
-	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;
-
-		udelay(1);
-	}
-
-	debug("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n",
-			__func__, reg, mask, set);
-
-	return -ETIMEDOUT;
-}
-
 /* Return 0 : host node, <>0 : device mode */
 static int usb_phy_enable(int index, struct usb_ehci *ehci)
 {
@@ -160,12 +135,13 @@ static int usb_phy_enable(int index, struct usb_ehci *ehci)
 
 	/* Stop then Reset */
 	clrbits_le32(usb_cmd, UCMD_RUN_STOP);
-	ret = wait_for_bit(usb_cmd, UCMD_RUN_STOP, 0);
+	ret = wait_for_bit(__func__, usb_cmd, UCMD_RUN_STOP, false, 10000,
+			   false);
 	if (ret)
 		return ret;
 
 	setbits_le32(usb_cmd, UCMD_RESET);
-	ret = wait_for_bit(usb_cmd, UCMD_RESET, 0);
+	ret = wait_for_bit(__func__, usb_cmd, UCMD_RESET, false, 10000, false);
 	if (ret)
 		return ret;
 
-- 
2.5.0

[U-Boot] [PATCH v3 4/5] usb: ehci-mx6: Use shared wait_for_bit

[Tom Rini]

On Sun, Dec 27, 2015 at 06:28:11PM +0100, Mateusz Kulikowski wrote:

> Use existing library function to poll bit(s).
> 
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/384e70cb/attachment.sig>

[U-Boot] [PATCH v3 5/5] net: zynq_gem: Use shared wait_for_bit

[Mateusz Kulikowski]

Use existing library function to poll bit(s).
Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
---

 drivers/net/zynq_gem.c | 35 ++---------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
index 7059c84..97e30f3 100644
--- a/drivers/net/zynq_gem.c
+++ b/drivers/net/zynq_gem.c
@@ -19,6 +19,7 @@
 #include <asm/io.h>
 #include <phy.h>
 #include <miiphy.h>
+#include <wait_bit.h>
 #include <watchdog.h>
 #include <asm/system.h>
 #include <asm/arch/hardware.h>
@@ -448,38 +449,6 @@ static int zynq_gem_init(struct udevice *dev)
 	return 0;
 }
 
-static int wait_for_bit(const char *func, u32 *reg, const u32 mask,
-			bool set, unsigned int timeout)
-{
-	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 (ctrlc()) {
-			puts("Abort\n");
-			return -EINTR;
-		}
-
-		udelay(1);
-	}
-
-	debug("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n",
-	      func, reg, mask, set);
-
-	return -ETIMEDOUT;
-}
-
 static int zynq_gem_send(struct udevice *dev, void *ptr, int len)
 {
 	u32 addr, size;
@@ -521,7 +490,7 @@ static int zynq_gem_send(struct udevice *dev, void *ptr, int len)
 		printf("TX buffers exhausted in mid frame\n");
 
 	return wait_for_bit(__func__, &regs->txsr, ZYNQ_GEM_TSR_DONE,
-			    true, 20000);
+			    true, 20000, true);
 }
 
 /* Do not check frame_recd flag in rx_status register 0x20 - just poll BD */
-- 
2.5.0

[U-Boot] [PATCH v3 5/5] net: zynq_gem: Use shared wait_for_bit

[Tom Rini]

On Sun, Dec 27, 2015 at 06:28:12PM +0100, Mateusz Kulikowski wrote:

> Use existing library function to poll bit(s).
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160114/9dc6a63c/attachment.sig>

[U-Boot] [PATCH v3 5/5] net: zynq_gem: Use shared wait_for_bit

[Moritz Fischer]

On Thu, Jan 14, 2016 at 12:08 PM, Tom Rini <trini@konsulko.com> wrote:
> On Sun, Dec 27, 2015 at 06:28:12PM +0100, Mateusz Kulikowski wrote:
>
>> Use existing library function to poll bit(s).
>> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
>
> Reviewed-by: Tom Rini <trini@konsulko.com>
Reviewed-by: Moritz Fischer <moritz.fischer@ettus.com>

Cheers,

Moritz