| Message ID | 1450303122-17884-2-git-send-email-mateusz.kulikowski@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday, December 16, 2015 at 10:58:38 PM, 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> > --- > > include/wait_bit.h | 35 +++++++++++++++++++++++++++++++++++ > lib/Makefile | 1 + > lib/wait_bit.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 81 insertions(+) > create mode 100644 include/wait_bit.h > create mode 100644 lib/wait_bit.c > > diff --git a/include/wait_bit.h b/include/wait_bit.h > new file mode 100644 > index 0000000..052a09b > --- /dev/null > +++ b/include/wait_bit.h > @@ -0,0 +1,35 @@ > +/* > + * 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 > + > +/** > + * 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 > + */ > +int wait_for_bit(const char *prefix, const u32 *reg, const u32 mask, > + const bool set, const unsigned int timeout, > + const bool breakable); > + > +#endif > diff --git a/lib/Makefile b/lib/Makefile > index 1f1ff6f..6fe3ab5 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -81,6 +81,7 @@ obj-y += time.o > obj-$(CONFIG_TRACE) += trace.o > obj-$(CONFIG_LIB_UUID) += uuid.o > obj-$(CONFIG_LIB_RAND) += rand.o > +obj-y += wait_bit.o > > ifdef CONFIG_SPL_BUILD > # SPL U-Boot may use full-printf, tiny-printf or none at all > diff --git a/lib/wait_bit.c b/lib/wait_bit.c > new file mode 100644 > index 0000000..ac35f68 > --- /dev/null > +++ b/lib/wait_bit.c > @@ -0,0 +1,45 @@ > +/* > + * Wait for bit interruptible by timeout or ctrlc > + * > + * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <console.h> > +#include <asm/io.h> > +#include <asm/errno.h> > + > +int wait_for_bit(const char *prefix, const u32 *reg, const u32 mask, > + const bool set, const unsigned int timeout, > + const bool breakable) > +{ I wonder, what would happen if you stuffed this function into the header file altogether ? I think this would allow the compiler to do interprocedure optimalization on whichever file this would be included into. I wonder if that would have any impact on the resulting code size.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Hi Marek, On 16.12.2015 23:11, Marek Vasut wrote: > On Wednesday, December 16, 2015 at 10:58:38 PM, Mateusz Kulikowski wrote: [...] >> +#include <console.h> >> +#include <asm/io.h> >> +#include <asm/errno.h> >> + >> +int wait_for_bit(const char *prefix, const u32 *reg, const u32 mask, >> + const bool set, const unsigned int timeout, >> + const bool breakable) >> +{ > > I wonder, what would happen if you stuffed this function into the header > file altogether ? I think this would allow the compiler to do interprocedure > optimalization on whichever file this would be included into. I wonder if > that would have any impact on the resulting code size. > Of course I can make it static inline. I was suggested not to care about possible leftovers that are not garbage-collected by linker so didn't changed that on V2. It's (max) few bytes that may be consumed by section alignment. Regards, Mateusz -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWcfSBAAoJELvtohmVtQzB+BsIAJK6ZVnU80+rbjfzyBJCPUcJ PtHLuz++kgkRZJevNr0RgUgiaWBs0eWfcN7FSpP3P1jUayGuE8MeYqJKbv7frlwO lxxd+nvXJB5OiO0jZMShJjDDo/pzFeVz5iFGHi7e7Gglxbk5Q+WQ+Q2D2ZCra5SN ktYReI4vViJWC2k+bGFYE4NL8p0OItdmT7gPpXPR8KOMg/Yq8MpQRZtCwCO3xNoU Fj3kIwouCErF1AjHKQ1OXP7E/W6iw2jF/i1L28bnI4BX4ArACcf4QF6dNNrO63tX jY322BHfyJG1sas2clMvq/QcA04jU2AQsx3poglJ6r9TTc72Kr3fxQl7IsNYS+s= =LpYg -----END PGP SIGNATURE-----
On Thursday, December 17, 2015 at 12:32:26 AM, Mateusz Kulikowski wrote: > Hi Marek, Hi, > On 16.12.2015 23:11, Marek Vasut wrote: > > On Wednesday, December 16, 2015 at 10:58:38 PM, Mateusz Kulikowski wrote: > [...] > > >> +#include <console.h> > >> +#include <asm/io.h> > >> +#include <asm/errno.h> > >> + > >> +int wait_for_bit(const char *prefix, const u32 *reg, const u32 mask, > >> + const bool set, const unsigned int timeout, > >> + const bool breakable) > >> +{ > > > > I wonder, what would happen if you stuffed this function into the header > > file altogether ? I think this would allow the compiler to do > > interprocedure optimalization on whichever file this would be included > > into. I wonder if that would have any impact on the resulting code size. > > Of course I can make it static inline. > > I was suggested not to care about possible leftovers that are > not garbage-collected by linker so didn't changed that on V2. > > It's (max) few bytes that may be consumed by section alignment. I was just curious about how much difference this would make. Best regards, Marek Vasut
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Hi Marek, On 17.12.2015 00:52, Marek Vasut wrote: [...] >> Of course I can make it static inline. >> >> I was suggested not to care about possible leftovers that are >> not garbage-collected by linker so didn't changed that on V2. >> >> It's (max) few bytes that may be consumed by section alignment. > > I was just curious about how much difference this would make. I added one more commit (to do wait_for_bit static inline), and ran buildman on 4 boards (2x using and 2x not using wait_bit). tl;dr: For (at least) some boards it increases .rodata by 7 bytes. Even if they don't use wait_for_bit. Inline version works better, as it doesn't touch them My proposal - I'll send v3 with wait_for_bit as static inline... Buildman output: $ tools/buildman/buildman -b wait_for_bit_v2 -o buildman hikey sandbox usb_a9263 zynq_picozed -sdSB boards.cfg is up to date. Nothing to do. Summary of 7 commits for 4 boards (4 threads, 1 job per thread) 01: eeprom: fix eeprom write procedure 02: lib: Add wait_for_bit sandbox: (for 1/1 boards) all +224.0 text +224.0 sandbox : all +224 text +224 u-boot: add: 1/0, grow: 0/0 bytes: 137/0 (137) function old new delta wait_for_bit - 137 +137 arm: (for 2/2 boards) spl/u-boot-spl:all +3.5 spl/u-boot-spl:rodata +3.5 zynq_picozed : spl/u-boot-spl:all +7 spl/u-boot-spl:rodata +7 03: usb: dwc2: Use shared wait_for_bit aarch64: (for 1/1 boards) all +168.0 rodata +16.0 text +152.0 hikey : all +168 rodata +16 text +152 u-boot: add: 0/0, grow: 4/0 bytes: 156/0 (156) function old new delta wait_for_bit 104 164 +60 usb_lowlevel_init 820 864 +44 dwc_otg_core_reset 120 156 +36 wait_for_chhltd 140 156 +16 04: usb: ohci-lpc32xx: Use shared wait_for_bit 05: usb: ehci-mx6: Use shared wait_for_bit 06: net: zynq_gem: Use shared wait_for_bit arm: (for 2/2 boards) all +53.0 bss +6.0 rodata +7.0 text +40.0 zynq_picozed : all +106 bss +12 rodata +14 text +80 u-boot: add: 1/0, grow: 0/-1 bytes: 140/-68 (72) function old new delta wait_for_bit - 140 +140 zynq_gem_send 276 208 -68 07: make wait_bit static inline sandbox: (for 1/1 boards) all -224.0 text -224.0 sandbox : all -224 text -224 arm: (for 2/2 boards) spl/u-boot-spl:all -3.5 spl/u-boot-spl:rodata -3.5 zynq_picozed : spl/u-boot-spl:all -7 spl/u-boot-spl:rodata -7 Regards, Mateusz -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWdtT1AAoJELvtohmVtQzBtPwH/2aY2GbCIevGGdN5x4109xDm zrrkmu6IQORQeOlTckeP2x83plY2KLGCoMtrA8fN3GbhqAJ9gM9EjtOzo7HYcYyD fENKksvqSWTGKPI1nAU3liRri6lhCyO0eq+ZwPNvQF8m5GT88r12QL599P1LXgOI tM93wzTaDWxDVUdFsUgBgzS0arcjG6eE2SWTE9SQY1OIKK/6CY79Y0q4h6UFSXwj DflZ4rrYTQSMMHs2tKvas/MrZozKHsim2g8glZj87dkVcyeTHPcEEvUa6UPbr4RJ NTQlarzFR3zsBa4Xl9wbROlL10drb+zgo6eNt8CyZ6z7II1r+oPI2m/OcyHoyDU= =Lfit -----END PGP SIGNATURE-----
diff --git a/include/wait_bit.h b/include/wait_bit.h new file mode 100644 index 0000000..052a09b --- /dev/null +++ b/include/wait_bit.h @@ -0,0 +1,35 @@ +/* + * 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 + +/** + * 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 + */ +int wait_for_bit(const char *prefix, const u32 *reg, const u32 mask, + const bool set, const unsigned int timeout, + const bool breakable); + +#endif diff --git a/lib/Makefile b/lib/Makefile index 1f1ff6f..6fe3ab5 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -81,6 +81,7 @@ obj-y += time.o obj-$(CONFIG_TRACE) += trace.o obj-$(CONFIG_LIB_UUID) += uuid.o obj-$(CONFIG_LIB_RAND) += rand.o +obj-y += wait_bit.o ifdef CONFIG_SPL_BUILD # SPL U-Boot may use full-printf, tiny-printf or none at all diff --git a/lib/wait_bit.c b/lib/wait_bit.c new file mode 100644 index 0000000..ac35f68 --- /dev/null +++ b/lib/wait_bit.c @@ -0,0 +1,45 @@ +/* + * Wait for bit interruptible by timeout or ctrlc + * + * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <console.h> +#include <asm/io.h> +#include <asm/errno.h> + +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; +}
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 | 35 +++++++++++++++++++++++++++++++++++ lib/Makefile | 1 + lib/wait_bit.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) create mode 100644 include/wait_bit.h create mode 100644 lib/wait_bit.c