[U-Boot] [PATCH v3 02/10] armv7: add miscellaneous utility macros

[Graeme Russ]

Hi Wolfgang,

On Thu, Jun 16, 2011 at 9:46 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Graeme Russ,
>
> In message <4DF9E409.1060600 at gmail.com> you wrote:
>>
>> is equivalent except that, as already pointed out, clrsetbits and friends:
>>
>>  a) Are not portable because only ARM and PPC define them which makes
>>     them, by definition, non-standard

Agreed - As mentioned in another post, I will look to implement clrsetbits
and friends in x86 at some point

> They should be added to _any_ arch/<arch>/include/asm/io.h that
> doesn't use them yet.
>
>>  b) Each invocation results in a read barrier plus a write barrier
>
> ...which is intentionally (actually mandatory) when accessing I/O memory,
> and negligable overhead on plain memory.

Agreed - The raw performance overhead of the barriers is most likely
negligible, although is there possibly a cache flush/sync operation or
flush of the predictive branch cache that could have a more significant
impact? I don't see this as such a huge problem since clrsetbits is
(usually) used to update hardware registers, any flush will happen anyway

>>  c) If the hardware register is sensitive to partial updates (i.e. requires
>>     all bit-fields to be updated in on operation) this requires a read into
>>     a local variable, calls to clrsetbits against that variable and finally
>>     a write-back - Lots of memory barriers
>
> Wrong. The macro does this automatically.  There is only a single read
> and a single write per call.

I mean:
	clrsetbits(&foo_reg, foo_val_1, foo_val_1_mask);
	clrsetbits(&foo_reg, foo_val_2, foo_val_2_mask);
	clrsetbits(&foo_reg, foo_val_3, foo_val_3_mask);

This could cause side-effects if foo_reg is sensitive to the three
foo_val's being set independently. To ensure all three values are updated
in a single write to foo_reg you would need to do:

	foo_tmp = readl(&foo_reg);

	clrsetbits(&foo_tmp, foo_val_1, foo_val_1_mask);
	clrsetbits(&foo_tmp, foo_val_2, foo_val_2_mask);
	clrsetbits(&foo_tmp, foo_val_3, foo_val_3_mask);

	foo_tmp = writel(foo_tmp, &foo_reg);

Regards,

Graeme