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

[Wolfgang Denk]

Dear Aneesh V,

In message <4DEE161B.2050402 at ti.com> you wrote:
> 
> >> So, if I have to write 5 different fields in a register I first write
> >> them into a variable and finally call writel() instead of making 5
> >> clrsetbits*() calls.
> >
> > It does not make much difference to me if you call one macro or
> > another 5 times.
> 
> No it makes a difference. It's 5 writes to a variable typically in an
> ARM register + 1 IO access vs 5 IO accesses. It's logically not
> equivalent.

You can use in_le32() to read the register in a variable, a number of
macros operating on that variable, and then a out_le32() to write it
back.

It's logically equivalent.

> Further if the 5 values are constants a smart compiler will fold the
> five writes into one write to the ARM register + 1 IO access, which
> won't happen if you used 5 clrsetbits*()

See above.

If you ever want to use your macros on device registers, you MUST use
proper memory barriers.  This will probably ruin your idea to combine
the access into a single write.  So you can just work on a variable as
suggested before.

> Problem: We want to read-modify-write an IO register 'reg' affecting 3 
> different fields: a, b, and c. The values to be written to the fields
> are a_val, b_val, and c_val respectively:
> 
> Solution 1 - without any macros:
> 
> unsigned int r = readl(reg);
> r = (r & ~a_mask) | ((a_val << a_shift) & a_mask)
> r = (r & ~b_mask) | ((b_val << b_shift) & b_mask)
> r = (r & ~c_mask) | ((c_val << c_shift) & c_mask)
> writel(r, reg);
> 
> Solution2 - with my macros:
> 
> unsigned int r = readl(reg);
> set_bit_field(r, a, a_val);
> set_bit_field(r, b, b_val);
> set_bit_field(r, c, c_val);
> writel(r, reg);
> 
> Solution3 - with clrsetbits*():
> 
> clrsetbits_le32(reg, a_mask, a_val << a_shift);
> clrsetbits_le32(reg, b_mask, b_val << b_shift);
> clrsetbits_le32(reg, c_mask, c_val << c_shift);

Solution 4 - with standard macros, and proper defines:

	unsigned int r = in_le32(reg);		/* or readl() if you like */

	clrsetbits_le32(&r, a_mask, a_val);
	clrsetbits_le32(&r, b_mask, b_val);
	clrsetbits_le32(&r, c_mask, c_val);

	out_le32(reg, r);

Actually solution 3 looks best to me.

> Solution 3 is not acceptable to me because it's clearly not equivalent
> to what I want to do. Writing the register 3 times instead of once may
> have undesirable side-effects. Even if it worked, it's clearly not
> efficient.

In case of side effects you can use solution 4.

We should not bother here about wether this is "efficient" or "not
efficient".  Probably none opf this code is ever time critical, not to
the extent of a few additional instructions.

> If you are forcing me to use solution 1, IMHO you are essentially
> forcing me not to use a sub-routine for a task that is repeated many
> times in my code, leaving my code to be more error prone and less
> readable.

I agree that 1 is ugly and error prone, but there is no need to use
it.

I repeat:  we have a set of powerful macros ready available.  Just use
them.

> You accuse set_bit_field() of being cryptic. I would say the
> implementation of clrsetbits_le32() is even more cryptic with so many
> levels of indirection. I think that goes with any sub-routine/API.

Theimplementation is complex, indeed.  But the interface is pretty
simple, and portable.

> You need to read the code/documentation once to know what it does.

Ditto for set_bit_field().

> > It does mater to me to have several incompatible implementations doing
> > essentially the same thing.
> 
> They are not doing the same thing as explained above.

I think they do. Could you please just give it a try with an open
mind and not being too much focussed on your own code.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Backed up the system lately?