On Tue, Jul 12, 2011 at 2:18 PM, Wolfgang Denk wd@denx.de wrote:
Dear Anton Staaf,
In message CAF6FioVcfgxzep7dhgxYR+cjaf0nQ8LYBxKvEaqycz8NOoSWDA@mail.gmail.com you wrote:
That makes sense to me. Would an alternative that uses the "width" and "size" of the field be acceptable? Then there is a well understood (on both types of architectures) mapping from these values to the mask and shift required to access portions of a register and/or variable in memory.
"width" and "size" seem indentical to me here. Do you mean "width" and "offset" or so? The problem still remains. People who are used to number bits from left to right will also tend to count bit offsets in the same direction.
Yes, I meant shift, not size. While it may be the case that some people count bits from the left, I don't think I've ever seen code that tries to right shift a value into place by that offset, everything seems to use the (value << shift) idiom. In which case specifying a shift (offset) value seems pretty safe.
In the end, the most simple and truly portable way is specifying the masks directly. What's wrong with definitions like
#define SCFR1_IPS_DIV_MASK 0x03800000 #define SCFR1_PCI_DIV_MASK 0x00700000 #define SCFR1_LPC_DIV_MASK 0x00003800
etc.?
I can actually read these much faster that any of these bit field definitions.
The only problem with this is that there is one piece of missing information, which is how do you get the value of the field that is masked as if it were a 4-bit or 3-bit number. That is, if I want the IPS_DIV value, I probably want:
(value & SCFR1_IPS_DIV_MASK) >> 20
Likewise, if I want to set the IPS divisor to 5 say, I would have to do:
(value & ~SCFR1_IPS_DIV_MASK) | (5 << 20)
In both cases the value 20 needs to come from somewhere. So you would probably end up having:
#define SCFR1_IPS_DIV_MASK 0x03800000 #define SCFR1_IPS_DIV_SHIFT 20
and
(value & SCFR1_IPS_DIV_MASK) >> SCFR1_IPS_DIV_SHIFT (value & ~SCFR1_IPS_DIV_MASK) | (5 << SCFR1_IPS_DIV_SHIFT)
And I think it would be great if these could turn into:
field_value = GET_FIELD(register_value, SCFR1_IPS_DIV) register_value = SET_FIELD(register_value, SCFR1_IPS_DIV, 5)
The GET and SET macros would in my opinion be more readable than a lot of shifting and oring. And could be used with existing U-Boot register access functions:
writel(SET_FIELD(readl(®), SCFR1_IPS_DIV, 5), ®)
Or, and I think this is something you have nacked in that past, but I like it and hope you don't mind me ending with it, this could eventually be:
SET_REGISTER_FIELD(®, SCFR1_IPS_DIV, 5)
--00504502e3b9570ace04a7e593ca Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
Please stop posting HTML.
Ack, sorry about that, it's my least favorite feature of web mail. :(
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@denx.de News is what a chap who doesn't care much about anything wants to read. And it's only news until he's read it. After that it's dead. - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5