| Message ID | 1309884558-7700-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Simon, Le 05/07/2011 18:49, Simon Glass a écrit : > Signed-off-by: Simon Glass<sjg@chromium.org> > --- > Changes in v2: > - Removed all bitfield access macros Not sure I follow you: the added lines below do indeed add bitfield access macros, don't they? > arch/arm/include/asm/arch-tegra2/bitfield.h | 96 +++++++++++++++++++++++++++ > 1 files changed, 96 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h > > diff --git a/arch/arm/include/asm/arch-tegra2/bitfield.h b/arch/arm/include/asm/arch-tegra2/bitfield.h > new file mode 100644 > index 0000000..494087c > --- /dev/null > +++ b/arch/arm/include/asm/arch-tegra2/bitfield.h > @@ -0,0 +1,96 @@ > +/* > + * Copyright (c) 2011 The Chromium OS Authors. > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA > + */ > + > +#ifndef __TEGRA2_BITFIELD_H > +#define __TEGRA2_BITFIELD_H > + > +/* > + * Basic macros for easily getting mask and shift values for bit fields on > + * ARM. > + * > + * You use these to reliably create shifts and masks from a bit field > + * definition. Bit fields are defined like this: > + * > + * #define NAME_BITS MSB : LSB > + * > + * where MSB is the most significant bit, and LSB the least sig, bit. This > + * notation is chosen since it is commonly used in CPU / SOC datasheets. > + * > + * For example: > + * > + * #define UART_FBCON_BITS 5:3 Bit range for the FBCON field > + * > + * Note that if you are using a big-endian machine there is no consistent > + * notion of big numbers, since bit 3 is a different bit depending on whether > + * the access is 32-bits or 64-bits. For this reason these macros should not > + * be used as it would probably be too confusing to have to specify your > + * access width all the time. > + * > + * This defines a bit field extending between bits 3 and 5. > + * > + * Then in your header file you can set up the shift and mask like this: > + * > + * #define UART_FBCON_SHIFT bf_shift(UART_FBCON) > + * #define UART_FBCON_MASK bf_mask(UART_FBCON) > + * > + * Then you can use these macros in your code (there is no bitfield support > + * in the C file and these macros MUST NOT be used directly in C code). > + * > + * To write, overwriting other fields too: > + * writel(3<< UART_FBCON_SHIFT,&uart->fbcon); > + * > + * To read: > + * int fbcon = (readl(&uart->fbcon)& UART_FBCON_MASK)>> > + * UART_FBCON_SHIFT; > + * > + * To update just this field (for example): > + * clrsetbits_le32(&uart->fbcon, UART_FBCON_MASK, 3<< UART_FBCON_SHIFT); > + */ What's the benefit of this over directly specifying a shift and mask value, e.g. #define UART_FBCON_SHIFT 3 and #define UART_FBCON_MASK 0x7 and doing shifts and ANDs? I don't see this as simpler or more intuitive. > +#include "compiler.h" > + > +#if __BYTE_ORDER == __LITTLE_ENDIAN > + > +/* Returns the bit number of the LSB */ > +#define _LSB(range) ((0 ? range)& 0x1f) > + > +/* Returns the bit number of the MSB */ > +#define _MSB(range) (1 ? range) > + > +/* Returns the width of the bitfield (in bits) */ > +#define _BITFIELD_WIDTH(range) (_MSB(range) - _LSB(range) + 1) > + > + > +/* > + * Returns the number of bits the bitfield needs to be shifted left to pack it. > + * This is just the same as the low bit. > + */ > +#define bf_shift(field) _LSB(field) > + > +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) */ > +#define bf_rawmask(field) ((1UL<< _BITFIELD_WIDTH(field)) - 1) > + > +/* Returns the mask for a field. Clear these bits to zero the field */ > +#define bf_mask(field) (bf_rawmask(field)<< (bf_shift(field))) > + > +#endif /* __BYTE_ORDER == __LITTLE_ENDIAN */ > + > +#endif Amicalement,
Hi Albert, On Sat, Jul 9, 2011 at 6:56 AM, Albert ARIBAUD <albert.u.boot@aribaud.net>wrote: > Hi Simon, > > Le 05/07/2011 18:49, Simon Glass a écrit : > > Signed-off-by: Simon Glass<sjg@chromium.org> >> --- >> Changes in v2: >> - Removed all bitfield access macros >> > > Not sure I follow you: the added lines below do indeed add bitfield access > macros, don't they? > > No these are just for defining the shifts and masks. There is no access going on through macros, either IO or normal variables. Everything uses manual shifts and adds, and the IO uses writel/readl/clrsetbits_le32(). Please check the follow-on patches to see what I mean. > > arch/arm/include/asm/arch-**tegra2/bitfield.h | 96 >> +++++++++++++++++++++++++++ >> 1 files changed, 96 insertions(+), 0 deletions(-) >> create mode 100644 arch/arm/include/asm/arch-**tegra2/bitfield.h >> >> diff --git a/arch/arm/include/asm/arch-**tegra2/bitfield.h >> b/arch/arm/include/asm/arch-**tegra2/bitfield.h >> new file mode 100644 >> index 0000000..494087c >> --- /dev/null >> +++ b/arch/arm/include/asm/arch-**tegra2/bitfield.h >> @@ -0,0 +1,96 @@ >> +/* >> + * Copyright (c) 2011 The Chromium OS Authors. >> + * See file CREDITS for list of people who contributed to this >> + * project. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation; either version 2 of >> + * the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, write to the Free Software >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, >> + * MA 02111-1307 USA >> + */ >> + >> +#ifndef __TEGRA2_BITFIELD_H >> +#define __TEGRA2_BITFIELD_H >> + >> +/* >> + * Basic macros for easily getting mask and shift values for bit fields >> on >> + * ARM. >> + * >> + * You use these to reliably create shifts and masks from a bit field >> + * definition. Bit fields are defined like this: >> + * >> + * #define NAME_BITS MSB : LSB >> + * >> + * where MSB is the most significant bit, and LSB the least sig, bit. >> This >> + * notation is chosen since it is commonly used in CPU / SOC datasheets. >> + * >> + * For example: >> + * >> + * #define UART_FBCON_BITS 5:3 Bit range for the FBCON >> field >> + * >> + * Note that if you are using a big-endian machine there is no consistent >> + * notion of big numbers, since bit 3 is a different bit depending on >> whether >> + * the access is 32-bits or 64-bits. For this reason these macros should >> not >> + * be used as it would probably be too confusing to have to specify your >> + * access width all the time. >> + * >> + * This defines a bit field extending between bits 3 and 5. >> + * >> + * Then in your header file you can set up the shift and mask like this: >> + * >> + * #define UART_FBCON_SHIFT bf_shift(UART_FBCON) >> + * #define UART_FBCON_MASK bf_mask(UART_FBCON) >> + * >> + * Then you can use these macros in your code (there is no bitfield >> support >> + * in the C file and these macros MUST NOT be used directly in C code). >> + * >> + * To write, overwriting other fields too: >> + * writel(3<< UART_FBCON_SHIFT,&uart->fbcon)**; >> + * >> + * To read: >> + * int fbcon = (readl(&uart->fbcon)& UART_FBCON_MASK)>> >> + * UART_FBCON_SHIFT; >> + * >> + * To update just this field (for example): >> + * clrsetbits_le32(&uart->fbcon, UART_FBCON_MASK, 3<< >> UART_FBCON_SHIFT); >> + */ >> > > What's the benefit of this over directly specifying a shift and mask value, > e.g. #define UART_FBCON_SHIFT 3 and #define UART_FBCON_MASK 0x7 and doing > shifts and ANDs? I don't see this as simpler or more intuitive. The only benefit is to avoid having to calculate all of these masks and shifts in your head. It is basically just a shortcut and assists with checking code against datasheets. Of course I would prefer to have access through macros also but that was shot down so the code is now full of manual shifts and ANDs. I hope that at least this small convenience will be acceptable. Regards, Simon > > > +#include "compiler.h" >> + >> +#if __BYTE_ORDER == __LITTLE_ENDIAN >> + >> +/* Returns the bit number of the LSB */ >> +#define _LSB(range) ((0 ? range)& 0x1f) >> + >> +/* Returns the bit number of the MSB */ >> +#define _MSB(range) (1 ? range) >> + >> +/* Returns the width of the bitfield (in bits) */ >> +#define _BITFIELD_WIDTH(range) (_MSB(range) - _LSB(range) + 1) >> + >> + >> +/* >> + * Returns the number of bits the bitfield needs to be shifted left to >> pack it. >> + * This is just the same as the low bit. >> + */ >> +#define bf_shift(field) _LSB(field) >> + >> +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) >> */ >> +#define bf_rawmask(field) ((1UL<< _BITFIELD_WIDTH(field)) - 1) >> + >> +/* Returns the mask for a field. Clear these bits to zero the field */ >> +#define bf_mask(field) (bf_rawmask(field)<< (bf_shift(field))) >> + >> +#endif /* __BYTE_ORDER == __LITTLE_ENDIAN */ >> + >> +#endif >> > > Amicalement, > -- > Albert. >
Dear Simon Glass, In message <1309884558-7700-2-git-send-email-sjg@chromium.org> you wrote: > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > Changes in v2: > - Removed all bitfield access macros As Albert already pointed out, this is actually a misleading description. > + * You use these to reliably create shifts and masks from a bit field > + * definition. Bit fields are defined like this: > + * > + * #define NAME_BITS MSB : LSB > + * > + * where MSB is the most significant bit, and LSB the least sig, bit. This > + * notation is chosen since it is commonly used in CPU / SOC datasheets. > + * > + * For example: > + * > + * #define UART_FBCON_BITS 5:3 Bit range for the FBCON field As explained a number of times before, any code like this is not portable and therefore always carries the risk of hard to find bugs. We therefore do not accept any such code in U-Boot. NAK. Sorry. Best regards, Wolfgang Denk
Dear Simon Glass, In message <CAPnjgZ1Oymaa2_gQGxw88jJG2Kr_fN6tJ5HDgUiOHPjw7jX2=g@mail.gmail.com> you wrote: > > > Not sure I follow you: the added lines below do indeed add bitfield access > > macros, don't they? > > > > No these are just for defining the shifts and masks. There is no access But you write yourself in the comment: "...easily getting mask and shift values for bit fields". > The only benefit is to avoid having to calculate all of these masks and > shifts in your head. It is basically just a shortcut and assists with > checking code against datasheets. Of course I would prefer to have access > through macros also but that was shot down so the code is now full of manual > shifts and ANDs. I hope that at least this small convenience will be > acceptable. Sorry, but because such code is unportable we do not accept it, as it would lead to driver code that becomes unportable, too. Best regards, Wolfgang Denk
On Mon, Jul 11, 2011 at 1:16 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <CAPnjgZ1Oymaa2_gQGxw88jJG2Kr_fN6tJ5HDgUiOHPjw7jX2= > g@mail.gmail.com> you wrote: > > > > > Not sure I follow you: the added lines below do indeed add bitfield > access > > > macros, don't they? > > > > > > No these are just for defining the shifts and masks. There is no access > > But you write yourself in the comment: "...easily getting mask and > shift values for bit fields". > > > The only benefit is to avoid having to calculate all of these masks and > > shifts in your head. It is basically just a shortcut and assists with > > checking code against datasheets. Of course I would prefer to have access > > through macros also but that was shot down so the code is now full of > manual > > shifts and ANDs. I hope that at least this small convenience will be > > acceptable. > > Sorry, but because such code is unportable we do not accept it, as it > would lead to driver code that becomes unportable, too. > > I know that this is throwing more fuel on the fire (for which I am sorry), but I don't follow the argument that this is unportable. As far as I can tell, the # : # syntax is not using any special compiler extensions, it is simply substituted into a (boo) ? # : # expression, thus extracting either the first of second number from the definition of the bit field. If I am wrong I would be interested to know what about this is not standard pre-processor usage? Thanks, Anton > 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 > In the pitiful, multipage, connection-boxed form to which the flow- > chart has today been elaborated, it has proved to be useless as a > design tool -- programmers draw flowcharts after, not before, writing > the programs they describe. - Fred Brooks, Jr. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Hi Anton, Le 11/07/2011 18:19, Anton Staaf a écrit : > I know that this is throwing more fuel on the fire (for which I am sorry), > but I don't follow the argument that this is unportable. As far as I can > tell, the # : # syntax is not using any special compiler extensions, it is > simply substituted into a (boo) ? # : # expression, thus extracting either > the first of second number from the definition of the bit field. > > If I am wrong I would be interested to know what about this is not standard > pre-processor usage? For me at least (Wolfgang might have other reasons), the issue is not the use of the pre-processor per se, it is that this "syntax" breaks the '?:' "triadic" operator in pieces, one piece in argument value and one in macro body, and neither piece makes sense from a C standpoint: '5:3' represents no meaningful C entity, and 'X ? y' (without the ':' and third argument of the operator) is not a proper C construct. IOW, it is syntactic sugaring done at the expense of code readability. > Thanks, > Anton Amicalement,
On Tue, Jul 12, 2011 at 8:29 AM, Albert ARIBAUD <albert.u.boot@aribaud.net>wrote: > Hi Anton, > > Le 11/07/2011 18:19, Anton Staaf a écrit : > > > I know that this is throwing more fuel on the fire (for which I am sorry), >> but I don't follow the argument that this is unportable. As far as I can >> tell, the # : # syntax is not using any special compiler extensions, it is >> simply substituted into a (boo) ? # : # expression, thus extracting either >> the first of second number from the definition of the bit field. >> >> If I am wrong I would be interested to know what about this is not >> standard >> pre-processor usage? >> > > For me at least (Wolfgang might have other reasons), the issue is not the > use of the pre-processor per se, it is that this "syntax" breaks the '?:' > "triadic" operator in pieces, one piece in argument value and one in macro > body, and neither piece makes sense from a C standpoint: '5:3' represents no > meaningful C entity, and 'X ? y' (without the ':' and third argument of the > operator) is not a proper C construct. > Yes, this is absolutely true, and we can argue about readability vs. usability vs. maintainability of such a construct (I like it, but I realize that I might be in a minority here). But it is certainly portable, there is nothing compiler or pre-processor specific here. I just wanted to clarify that so that we could have the correct debate. :) IOW, it is syntactic sugaring done at the expense of code readability. > I would disagree here, it is indeed syntactic sugar, but I would assert that it is done exactly to make the code more readable. The one piece of macro magic is in a single file that can be well documented and encapsulated. I find having one #define that specifies the entire range much easier to read and debug than having two #defines, one for each end of the range that you have to make sure are matched. And this simplification can be applied to many many files. But that's what it really boils down to, what we each find easier to read and debug. I'm fine if the decision is that everyone else doesn't like the way this reads, but let's not throw it out based on a compatibility argument that is invalid. Thanks, Anton Thanks, >> Anton >> > > Amicalement, > -- > Albert. >
Dear Anton Staaf, In message <CAF6FioVs5rsF27Boq9+Bb+3Cgdh2m=jj1c=41a-32muBUd9wtw@mail.gmail.com> you wrote: > > > Sorry, but because such code is unportable we do not accept it, as it > > would lead to driver code that becomes unportable, too. > > > > I know that this is throwing more fuel on the fire (for which I am sorry), You don't have to apologize. I think it is important that everybody understands the reasons behind such decisions. > but I don't follow the argument that this is unportable. As far as I can > tell, the # : # syntax is not using any special compiler extensions, it is > simply substituted into a (boo) ? # : # expression, thus extracting either > the first of second number from the definition of the bit field. > > If I am wrong I would be interested to know what about this is not standard > pre-processor usage? I did not say anything about preprocessor issues. The use of bit numbers (and ranges of bit numbers) in code is inherently unportable. For example: "Bit 10" means 0x00000400 on some systems (like, for example, on ARM), but it means 0x00200000 on others (like, for example, on PPC). On many systems bit 0 means trhe LSB, but the Power Architecture defines it as the MSB. Thus bit numbers should never be used in any code to access bits or to create masks etc. - they are not generally applicable. Best regards, Wolfgang Denk
On Tue, Jul 12, 2011 at 12:30 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Anton Staaf, > > In message <CAF6FioVs5rsF27Boq9+Bb+3Cgdh2m=jj1c= > 41a-32muBUd9wtw@mail.gmail.com> you wrote: > > > > > Sorry, but because such code is unportable we do not accept it, as it > > > would lead to driver code that becomes unportable, too. > > > > > > I know that this is throwing more fuel on the fire (for which I am > sorry), > > You don't have to apologize. I think it is important that everybody > understands the reasons behind such decisions. Thanks. > but I don't follow the argument that this is unportable. As far as I can > > tell, the # : # syntax is not using any special compiler extensions, it > is > > simply substituted into a (boo) ? # : # expression, thus extracting > either > > the first of second number from the definition of the bit field. > > > > If I am wrong I would be interested to know what about this is not > standard > > pre-processor usage? > > I did not say anything about preprocessor issues. The use of bit > numbers (and ranges of bit numbers) in code is inherently unportable. > > For example: > > "Bit 10" means 0x00000400 on some systems (like, for example, on ARM), > but it means 0x00200000 on others (like, for example, on PPC). > > On many systems bit 0 means trhe LSB, but the Power Architecture > defines it as the MSB. Thus bit numbers should never be used in any > code to access bits or to create masks etc. - they are not generally > applicable. Ahh, I think I've finally (been lurking on this subject for a while) got the core of this understood. The problem is that if this mechanism (bit numbers of any sort) were allowed in to U-Boot, then common driver code would end up using it and the result would be drivers that specify bit fields using LSB 0 (or MSB 0) notation that would not match a datasheet from an SoC that uses the alternative standard. For example, the ns16550 driver would have to pick one of LSB 0 or MSB 0 in it's header file instead of just specifying a mask value. The result would be that one of the standard bit field numbers would become a second class citizen. The code would still work for them, but the encoding of the masks would be in an alien format. 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. Thanks, Anton 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 > "In Christianity neither morality nor religion come into contact with > reality at any point." - Friedrich Nietzsche >
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. 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. > --00504502e3b9570ace04a7e593ca > Content-Type: text/html; charset=ISO-8859-1 > Content-Transfer-Encoding: quoted-printable Please stop posting HTML. Best regards, Wolfgang Denk
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
Hi Anton, [...] > 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) Let me take this opportunity to once more explain why I don't like this. As a big fan of functional programming, I personally have grown used to code as explicit as possible. So _all_ arguments to a function should be explicit in the call - reliance on state or such "hidden arguments" violate this principle strongly. I learnt that code following these principles written by other people is much easier for me to understand. Cheers Detlev
On Wed, Jul 13, 2011 at 4:28 AM, Detlev Zundel <dzu@denx.de> wrote: > Hi Anton, > > [...] > >> 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) > > Let me take this opportunity to once more explain why I don't like this. > As a big fan of functional programming, I personally have grown used to > code as explicit as possible. So _all_ arguments to a function should > be explicit in the call - reliance on state or such "hidden arguments" > violate this principle strongly. I learnt that code following these > principles written by other people is much easier for me to understand. I agree in general that it is preferable to be as explicit as possible. But it is also good to be able to express your intent, instead of implementation when possible. In other words, I would rather be explicit about my intent, than the particular implementation. One way of attaining both in this case would be to change the #define to be: #define SCFR1_IPS_DIV {0x03800000, 20} And use it to initialize a field struct that is accessed in the GET and SET macros. Then there are no hidden variables/names being constructed. I didn't initially suggest this because it could be seen as another abuse of macro magic. Alternatively, you can view the #define <NAME>_<VALUE> notation as a poor mans structured programming paradigm. Effectively, the various #defines make up a single structured data element that the GET and SET macros are accessing. The first solution has the advantage that you don't have to repeat yourself. Are either of these solutions acceptable (I realize the second solution is more of a "solution" as it requires a re-interpretation of the existing proposal). Thanks, Anton p.s. The above #define could also be changed to: #define SCFR1_IPS_DIV {.mask = 0x03800000, .shift = 20} Resulting in a verbose, but explicit definition of the field. > Cheers > Detlev > > -- > There are two hard things in computer science: cache invalidation, > naming things, and off-by-one errors. > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de >
Hi Anton, Le 13/07/2011 18:47, Anton Staaf a écrit : > I agree in general that it is preferable to be as explicit as > possible. But it is also good to be able to express your intent, > instead of implementation when possible. In other words, I would > rather be explicit about my intent, than the particular > implementation. Seems to me that for you, showing intent and implementation are necessarily exclusive; however, Wolfgang's examples indeed show implementation, but they show intent as well, at least for me they do. Amicalement,
On Thu, Jul 14, 2011 at 9:00 AM, Albert ARIBAUD <albert.u.boot@aribaud.net> wrote: > Hi Anton, > > Le 13/07/2011 18:47, Anton Staaf a écrit : > >> I agree in general that it is preferable to be as explicit as >> possible. But it is also good to be able to express your intent, >> instead of implementation when possible. In other words, I would >> rather be explicit about my intent, than the particular >> implementation. > > Seems to me that for you, showing intent and implementation are necessarily > exclusive; however, Wolfgang's examples indeed show implementation, but they > show intent as well, at least for me they do. I'm not sure which example you mean. If you mean his #define of the masks explicitly, those are fine by me. My above statement is about the masking, oring and shifting that is done in the same way every time and could be encoded in a macro that makes it easier to see what exactly is going on. Or did I misunderstand which example you mean? Thanks, Anton > Amicalement, > -- > Albert. >
Le 14/07/2011 19:29, Anton Staaf a écrit : > On Thu, Jul 14, 2011 at 9:00 AM, Albert ARIBAUD > <albert.u.boot@aribaud.net> wrote: >> Hi Anton, >> >> Le 13/07/2011 18:47, Anton Staaf a écrit : >> >>> I agree in general that it is preferable to be as explicit as >>> possible. But it is also good to be able to express your intent, >>> instead of implementation when possible. In other words, I would >>> rather be explicit about my intent, than the particular >>> implementation. >> >> Seems to me that for you, showing intent and implementation are necessarily >> exclusive; however, Wolfgang's examples indeed show implementation, but they >> show intent as well, at least for me they do. > > I'm not sure which example you mean. If you mean his #define of the > masks explicitly, those are fine by me. My above statement is about > the masking, oring and shifting that is done in the same way every > time and could be encoded in a macro that makes it easier to see what > exactly is going on. Or did I misunderstand which example you mean? You did not misunderstand the example -- but the way you just stated what you think of it is, I think, a confirmation of what I said: that it is your approach of the issue which is at odds with Wolfgang's and mine. Let us look at the terms you've just use to describe what you dislike : these are 'masking, oring and shifting' and 'done the same way every time'. I assume the second is not a criticism, but the foundation for suggesting a macro. That leaves 'masking, oring and shifting': it seems like for you it is unclear what this does, but it *does* tell what is going on just as much as a bitfield macro would -- actually it tells more, because '(x & y) | z' (there is usually no shifting involved, BTW) is a design pattern in embedded software development, where this pattern is recognized at first sight for what it does -- at least I see them that way and Wolfgang probably does as well. Granted, a non-specialist in embedded SW might have problems understanding this, but U-Boot has more or less a requirement that developers contributing to it have some knowledge of embedded SW. The 'done in the same way every time' part, OTOH, might make sense -- that's code factorization, after all. But you could say the same of, for instance, assignments from structure members: they're done the same way every time : 'x = y.z', but there would be little point in wanting to hide that in a macro, because the macro would not add enough value. I think that's the main problem: a bitfield macro for computing masks and shifts and anding and oring would not add sufficient value with respect to the bare expression, which is still simple enough to be understood by most readers. (plus the issue of portability raised by Wolfgang, which I won't delve into as he's already developed it) > Thanks, > Anton HTH. Amicalement,
Dear Anton Staaf, In message <CAF6FioWbAvTnL0m2ch4Xd5O51bp7SX=LLOPG0DXNSzSfwVvm+g@mail.gmail.com> you wrote: > > I'm not sure which example you mean. If you mean his #define of the > masks explicitly, those are fine by me. My above statement is about > the masking, oring and shifting that is done in the same way every > time and could be encoded in a macro that makes it easier to see what > exactly is going on. Or did I misunderstand which example you mean? I disagree with your statement that such a macro "makes it easier to see what exactly is going on." On contrary, such a macro would _hide_ what is going on. This may be ok and even intentional in some places, but here it is not helpful, even if it seems so you you. Quote Larry Wall (from the perlstyle(1) man page): Even if you aren't in doubt, consider the mental welfare of the per- son who has to maintain the code after you ... Best regards, Wolfgang Denk
On Thu, Jul 14, 2011 at 11:30 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Anton Staaf, > > In message <CAF6FioWbAvTnL0m2ch4Xd5O51bp7SX=LLOPG0DXNSzSfwVvm+g@mail.gmail.com> you wrote: >> >> I'm not sure which example you mean. If you mean his #define of the >> masks explicitly, those are fine by me. My above statement is about >> the masking, oring and shifting that is done in the same way every >> time and could be encoded in a macro that makes it easier to see what >> exactly is going on. Or did I misunderstand which example you mean? > > I disagree with your statement that such a macro "makes it easier to > see what exactly is going on." On contrary, such a macro would _hide_ > what is going on. This may be ok and even intentional in some places, > but here it is not helpful, even if it seems so you you. OK, I'm content to disagree on this and do it your way. :) I can do it my way on my projects. Thanks for the discussion. > Quote Larry Wall (from the perlstyle(1) man page): > Even if you aren't in doubt, consider the mental welfare of the per- > son who has to maintain the code after you ... Taking style guides from Larry is not high on my list by the way. :) Thanks, Anton > 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 > Our business is run on trust. We trust you will pay in advance. >
Dear Anton Staaf, In message <CAF6FioVOEuNcEsr=3jyxoVs__dO3Ox+q00PnDBRHdu+UmJZF2g@mail.gmail.com> you wrote: > > 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) BTW - you are correct about this. The file I grabbed the examples from is "arch/powerpc/include/asm/immap_512x.h"; here is the full context: 229 /* SCFR1 System Clock Frequency Register 1 */ 230 #define SCFR1_IPS_DIV 0x3 231 #define SCFR1_IPS_DIV_MASK 0x03800000 232 #define SCFR1_IPS_DIV_SHIFT 23 233 234 #define SCFR1_PCI_DIV 0x6 235 #define SCFR1_PCI_DIV_MASK 0x00700000 236 #define SCFR1_PCI_DIV_SHIFT 20 237 238 #define SCFR1_LPC_DIV_MASK 0x00003800 239 #define SCFR1_LPC_DIV_SHIFT 11 240 241 /* SCFR2 System Clock Frequency Register 2 */ 242 #define SCFR2_SYS_DIV 0xFC000000 243 #define SCFR2_SYS_DIV_SHIFT 26 And indeed we see code using this for example in arch/powerpc/cpu/mpc512x/speed.c: 98 reg = in_be32(&im->clk.scfr[0]); 99 ips_div = (reg & SCFR1_IPS_DIV_MASK) >> SCFR1_IPS_DIV_SHIFT; The nice thing (for me) here is, that without even thinking for a second I know exactly what is going on - there is nothing in this statements that require me too look up some macro definition. [Yes, of course this is based on the assumption that macro names <register>_MASK and <register>_SHIFT just do what they are suggest they are doing. But any such things get filtered out during the reviews.] Best regards, Wolfgang Denk
On Thu, Jul 14, 2011 at 11:44 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Anton Staaf, > > In message <CAF6FioVOEuNcEsr=3jyxoVs__dO3Ox+q00PnDBRHdu+UmJZF2g@mail.gmail.com> you wrote: >> >> 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) > > BTW - you are correct about this. The file I grabbed the examples > from is "arch/powerpc/include/asm/immap_512x.h"; here is the full > context: > > 229 /* SCFR1 System Clock Frequency Register 1 */ > 230 #define SCFR1_IPS_DIV 0x3 > 231 #define SCFR1_IPS_DIV_MASK 0x03800000 > 232 #define SCFR1_IPS_DIV_SHIFT 23 > 233 > 234 #define SCFR1_PCI_DIV 0x6 > 235 #define SCFR1_PCI_DIV_MASK 0x00700000 > 236 #define SCFR1_PCI_DIV_SHIFT 20 > 237 > 238 #define SCFR1_LPC_DIV_MASK 0x00003800 > 239 #define SCFR1_LPC_DIV_SHIFT 11 > 240 > 241 /* SCFR2 System Clock Frequency Register 2 */ > 242 #define SCFR2_SYS_DIV 0xFC000000 > 243 #define SCFR2_SYS_DIV_SHIFT 26 > > And indeed we see code using this for example in > arch/powerpc/cpu/mpc512x/speed.c: > > 98 reg = in_be32(&im->clk.scfr[0]); > 99 ips_div = (reg & SCFR1_IPS_DIV_MASK) >> SCFR1_IPS_DIV_SHIFT; I agree, this line is completely obvious and if it were the only sort of GET (or it's equivalent SET) version used I wouldn't have suggested the macro at all. It's the lines that are setting many fields simultaneously, sometimes with constant field values and sometimes with computed values that become a bit hard to parse, and would benefit from the abstraction. But good coding practice can break these statements up into a collection of simple ones to do the manipulations one at a time. Then the real benefit of the macro becomes a compression of the syntax, leading to shorter functions, and in my option a reduced time to parse and understand the actions. But as you say, it hides part of the implementation, but this is also true for any other function, such as the fact that writel does a memory barrier. But such functions are part of the existing U-Boot knowledge base, so their behavior is expected and understood by it's users. I see no reason that the same could not eventually be the case for field access macros. But as I've said, I'm OK with dropping this. Any indication above to the prior is simply me being an engineer who perceives a problem that I can solve and desiring to have my perspective validated. :) And now back to sending actual useful patches to the list. Thanks, Anton > The nice thing (for me) here is, that without even thinking for a > second I know exactly what is going on - there is nothing in this > statements that require me too look up some macro definition. [Yes, > of course this is based on the assumption that macro names > <register>_MASK and <register>_SHIFT just do what they are suggest > they are doing. But any such things get filtered out during the > reviews.] > > 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 > Vulcans never bluff. > -- Spock, "The Doomsday Machine", stardate 4202.1 >
diff --git a/arch/arm/include/asm/arch-tegra2/bitfield.h b/arch/arm/include/asm/arch-tegra2/bitfield.h new file mode 100644 index 0000000..494087c --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/bitfield.h @@ -0,0 +1,96 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __TEGRA2_BITFIELD_H +#define __TEGRA2_BITFIELD_H + +/* + * Basic macros for easily getting mask and shift values for bit fields on + * ARM. + * + * You use these to reliably create shifts and masks from a bit field + * definition. Bit fields are defined like this: + * + * #define NAME_BITS MSB : LSB + * + * where MSB is the most significant bit, and LSB the least sig, bit. This + * notation is chosen since it is commonly used in CPU / SOC datasheets. + * + * For example: + * + * #define UART_FBCON_BITS 5:3 Bit range for the FBCON field + * + * Note that if you are using a big-endian machine there is no consistent + * notion of big numbers, since bit 3 is a different bit depending on whether + * the access is 32-bits or 64-bits. For this reason these macros should not + * be used as it would probably be too confusing to have to specify your + * access width all the time. + * + * This defines a bit field extending between bits 3 and 5. + * + * Then in your header file you can set up the shift and mask like this: + * + * #define UART_FBCON_SHIFT bf_shift(UART_FBCON) + * #define UART_FBCON_MASK bf_mask(UART_FBCON) + * + * Then you can use these macros in your code (there is no bitfield support + * in the C file and these macros MUST NOT be used directly in C code). + * + * To write, overwriting other fields too: + * writel(3 << UART_FBCON_SHIFT, &uart->fbcon); + * + * To read: + * int fbcon = (readl(&uart->fbcon) & UART_FBCON_MASK) >> + * UART_FBCON_SHIFT; + * + * To update just this field (for example): + * clrsetbits_le32(&uart->fbcon, UART_FBCON_MASK, 3 << UART_FBCON_SHIFT); + */ + +#include "compiler.h" + +#if __BYTE_ORDER == __LITTLE_ENDIAN + +/* Returns the bit number of the LSB */ +#define _LSB(range) ((0 ? range) & 0x1f) + +/* Returns the bit number of the MSB */ +#define _MSB(range) (1 ? range) + +/* Returns the width of the bitfield (in bits) */ +#define _BITFIELD_WIDTH(range) (_MSB(range) - _LSB(range) + 1) + + +/* + * Returns the number of bits the bitfield needs to be shifted left to pack it. + * This is just the same as the low bit. + */ +#define bf_shift(field) _LSB(field) + +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) */ +#define bf_rawmask(field) ((1UL << _BITFIELD_WIDTH(field)) - 1) + +/* Returns the mask for a field. Clear these bits to zero the field */ +#define bf_mask(field) (bf_rawmask(field) << (bf_shift(field))) + +#endif /* __BYTE_ORDER == __LITTLE_ENDIAN */ + +#endif
Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: - Removed all bitfield access macros arch/arm/include/asm/arch-tegra2/bitfield.h | 96 +++++++++++++++++++++++++++ 1 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h