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

Simon Glass
2016-01-21T02:45:43Z

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