Add basic clock and pinmux functions to the Tegra2
Subsystems (2)
v2 cover Add basic clock and pinmux functions to the Tegra2
This patch series adds basic clock and pinmux functions to the Tegra2, and modifies the ap20 and board code to use them. Note I have tidied up the change logs to be in the right place.
Changes in v2:
- Removed use of bitfield access macros
- Now uses manual shifts and masks
- Removed all bitfield access macros
Simon Glass (5):
Tegra2: Add macros to calculate bitfield shifts and masks
Tegra2: Add microsecond timer functions
Tegra2: Add more clock support
Tegra2: add additional pin multiplexing features
Tegra2: Use clock and pinmux functions to simplify code
arch/arm/cpu/armv7/tegra2/Makefile | 2 +- arch/arm/cpu/armv7/tegra2/ap20.c | 91 +++------- arch/arm/cpu/armv7/tegra2/clock.c | 158 ++++++++++++++++ arch/arm/cpu/armv7/tegra2/pinmux.c | 53 ++++++ arch/arm/cpu/armv7/tegra2/timer.c | 27 ++- arch/arm/include/asm/arch-tegra2/bitfield.h | 96 ++++++++++ arch/arm/include/asm/arch-tegra2/clk_rst.h | 140 +++++++------- arch/arm/include/asm/arch-tegra2/clock.h | 264 +++++++++++++++++++++++++++ arch/arm/include/asm/arch-tegra2/pinmux.h | 155 +++++++++++++++- arch/arm/include/asm/arch-tegra2/timer.h | 34 ++++ board/nvidia/common/board.c | 64 +++---- 11 files changed, 893 insertions(+), 191 deletions(-) create mode 100644 arch/arm/cpu/armv7/tegra2/clock.c create mode 100644 arch/arm/cpu/armv7/tegra2/pinmux.c create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h create mode 100644 arch/arm/include/asm/arch-tegra2/clock.h create mode 100644 arch/arm/include/asm/arch-tegra2/timer.h
Signature · 1 line
1.7.3.1
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
---
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
arch/arm/include/asm/arch-tegra2/bitfield.h new file +96 -0
@@ -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
Signature · 1 line
1.7.3.1
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Hi Simon,
Le 05/07/2011 18:49, Simon Glass a ?crit :
Quoted 4 lines
Signed-off-by Simon Glass<[email protected]>---
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?
Quoted 66 lines
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.harch/arm/include/asm/arch-tegra2/bitfield.h new file +56
-0@@ -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). +*
arch/arm/include/asm/arch-tegra2/bitfield.h line 66
Quoted 10 lines
arch/arm/include/asm/arch-tegra2/bitfield.h new file +10
-0+* 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.
Quoted 19 lines
arch/arm/include/asm/arch-tegra2/bitfield.h new file +19
-0+#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)
arch/arm/include/asm/arch-tegra2/bitfield.h line 96
Quoted 10 lines
arch/arm/include/asm/arch-tegra2/bitfield.h new file +10
-0+ +/* 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,
Signature · 1 line
Albert.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Hi Albert,
Quoted 10 lines · Albert ARIBAUD
Hi Simon,
Le 05/07/2011 18:49, Simon Glass a ?crit :
Signed-off-by Simon Glass<[email protected]>Quoted 4 lines
---
Changes in v2:
- Removed all bitfield access macros
Quoted 4 lines
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.
Quoted 83 lines
arch/arm/include/asm/arch-**tegra2/bitfield.h | 96Quoted 81 lines
+++++++++++++++++++++++++++
1 files changed, 96 insertions(+), 0 deletions(-) create mode 100644 arch/arm/include/asm/arch-**tegra2/bitfield.harch/arm/include/asm/arch-**tegra2/bitfield.h new file +26
-0@@ -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:
+ *
+ * #defineNAME_BITSMSB : 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:
+ *
+ * #defineUART_FBCON_BITS5: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):
Quoted 8 lines
Quoted 4 lines
+ * 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_SHIFT3 and #defineUART_FBCON_MASK0x7and 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
Quoted 39 lines
+#include "
compiler.h"Quoted 23 lines
arch/arm/include/asm/arch-**tegra2/bitfield.h new file +15
-0+ +#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.
+ */arch/arm/include/asm/arch-**tegra2/bitfield.h new file +3
-0+#define bf_shift (field) _LSB(field) + +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) */
arch/arm/include/asm/arch-**tegra2/bitfield.h
Quoted 8 lines
arch/arm/include/asm/arch-**tegra2/bitfield.h new file +8
-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.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Dear Simon Glass,
Quoted 5 lines · In message <CAPnjgZ1Oymaa2_gQGxw88jJG2Kr_fN6tJ5HDgUiOHPjw7jX2=g@mail.gmail.com>…
Quoted 5 lines
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".
Quoted 6 lines
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
Signature · 7 lines
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 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.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 18 lines · Wolfgang Denk
Dear Simon Glass,
In message <CAPnjgZ1Oymaa2_gQGxw88jJG2Kr_fN6tJ5HDgUiOHPjw7jX2=
Quoted 2 lines · g at mail.gmail.com> you wrote:
Quoted 2 lines
Not sure I follow you: the added lines below do indeed add bitfield
access
Quoted 3 lines
Quoted 3 lines
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".
Quoted 4 lines
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
Quoted 8 lines
manual
Quoted 2 lines
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
Quoted 17 lines
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 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 at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Hi Anton,
Le 11/07/2011 18:19, Anton Staaf a ?crit :
Quoted 8 lines
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.
Quoted 2 lines
Thanks,
Anton
Amicalement,
Signature · 1 line
Albert.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 16 lines · Albert ARIBAUD
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),
Quoted 9 lines
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?
Quoted 7 lines
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.
Quoted 1 line
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,
Quoted 7 lines
Quoted 2 lines
Anton
Amicalement,
--
Albert.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
In reply to Anton StaafDear Anton Staaf,
Quoted 5 lines · In message <CAF6FioVs5rsF27Boq9+Bb+3Cgdh2m=jj1c=41a-32muBUd9wtw@mail.gmail.com>…
Quoted 5 lines
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.
Quoted 7 lines
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
Signature · 5 lines
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 "In Christianity neither morality nor religion come into contact with reality at any point." - Friedrich Nietzsche
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 5 lines · Wolfgang Denk
Dear Anton Staaf,
In message <CAF6FioVs5rsF27Boq9+Bb+3Cgdh2m=jj1c=
Quoted 1 line · 41a-32muBUd9wtw at mail.gmail.com> you wrote:
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 16 lines
but I don't follow the argument that this is unportable. As far as I can
Quoted 1 line
tell, the # : # syntax is not using any special compiler extensions, it
is
Quoted 1 line
simply substituted into a (boo) ? # : # expression, thus extracting
either
Quoted 3 lines
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
Quoted 1 line
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:
Quoted 7 lines
"Bit 10" means
0x00000400on some systems (like, for example, on ARM), but it means0x00200000on 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,
Quoted 10 lines
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 "In Christianity neither morality nor religion come into contact with
reality at any point." - Friedrich Nietzsche
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Dear Anton Staaf,
Quoted 5 lines · In message <CAF6FioVcfgxzep7dhgxYR+cjaf0nQ8LYBxKvEaqycz8NOoSWDA@mail.gmail.com>…
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.
Quoted 3 lines
--00504502e3b9570ace04a7e593ca
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Please stop posting HTML.
Best regards,
Wolfgang Denk
Signature · 6 lines
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
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. 5PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 10 lines · Wolfgang Denk
Dear Anton Staaf,
Quoted 5 lines · In message <CAF6FioVcfgxzep7dhgxYR+cjaf0nQ8LYBxKvEaqycz8NOoSWDA@mail.gmail.com>…
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.
Quoted 4 lines
"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.
Quoted 11 lines
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
? ? ? ?#defineSCFR1_PCI_DIV_MASK? ? ?0x00700000
? ? ? ?#defineSCFR1_LPC_DIV_MASK? ? ?0x00003800etc.?
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)
Quoted 5 lines
Quoted 3 lines
--00504502e3b9570ace04a7e593ca
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printablePlease stop posting HTML.
Ack, sorry about that, it's my least favorite feature of web mail. ?:(
Quoted 12 lines
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 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
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Hi Anton,
[...]
Quoted 19 lines
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_DIVvalue, I probably want:? ?(value &
SCFR1_IPS_DIV_MASK) >> 20Likewise, 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
? ?#defineSCFR1_IPS_DIV_SHIFT? ? ?20and
Quoted 7 lines
? ?(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
Signature · 6 lines
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 at denx.de
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 31 lines · Detlev Zundel
Hi Anton,
[...]
Quoted 19 lines
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_DIVvalue, I probably want:? ?(value &
SCFR1_IPS_DIV_MASK) >> 20Likewise, 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
? ?#defineSCFR1_IPS_DIV_SHIFT? ? ?20and
Quoted 7 lines
? ?(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)
Quoted 6 lines
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.
Quoted 11 lines
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 at denx.de
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Hi Anton,
Le 13/07/2011 18:47, Anton Staaf a ?crit :
Quoted 5 lines
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,
Signature · 1 line
Albert.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 10 lines · Albert ARIBAUD
Hi Anton,
Le 13/07/2011 18:47, Anton Staaf a ?crit :
Quoted 5 lines
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.
Quoted 3 lines
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
Quoted 4 lines
Amicalement,
--
Albert.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Le 14/07/2011 19:29, Anton Staaf a ?crit :
Quoted 16 lines
Quoted 10 lines · Albert ARIBAUD
Hi Anton,
Le 13/07/2011 18:47, Anton Staaf a ?crit :
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
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)
Quoted 2 lines
Thanks,
Anton
HTH.
Amicalement,
Signature · 1 line
Albert.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
In reply to Anton StaafDear Anton Staaf,
Quoted 6 lines · In message <CAF6FioWbAvTnL0m2ch4Xd5O51bp7SX=LLOPG0DXNSzSfwVvm+g@mail.gmail.com>…
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
Signature · 4 lines
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 Our business is run on trust. We trust you will pay in advance.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 10 lines · Wolfgang Denk
Dear Anton Staaf,
Quoted 6 lines · In message <CAF6FioWbAvTnL0m2ch4Xd5O51bp7SX=LLOPG0DXNSzSfwVvm+g@mail.gmail.com>…
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?
Quoted 4 lines
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.
Quoted 3 lines
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
Quoted 10 lines
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 Our business is run on trust. ?We trust you will pay in advance.
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
In reply to Anton StaafDear Anton Staaf,
Quoted 11 lines · In message <CAF6FioVOEuNcEsr=3jyxoVs__dO3Ox+q00PnDBRHdu+UmJZF2g@mail.gmail.com>…
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 20and
(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 inarch/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
Signature · 5 lines
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 Vulcans never bluff. -- Spock, "The Doomsday Machine", stardate 4202.1
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
Quoted 35 lines · Wolfgang Denk
Dear Anton Staaf,
Quoted 11 lines · In message <CAF6FioVOEuNcEsr=3jyxoVs__dO3Ox+q00PnDBRHdu+UmJZF2g@mail.gmail.com>…
In both cases the value 20 needs to come from somewhere. ?So you would probably end up having:
? ?#define
SCFR1_IPS_DIV_MASK? ? ?0x03800000
? ?#defineSCFR1_IPS_DIV_SHIFT? ? ?20and
? ?(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 #defineSCFR1_IPS_DIV_MASK? ? ?0x03800000 ?232 #defineSCFR1_IPS_DIV_SHIFT? ? 23 ?233
?234 #defineSCFR1_PCI_DIV? ? ? ? ?0x6?235 #defineSCFR1_PCI_DIV_MASK? ? ?0x00700000 ?236 #defineSCFR1_PCI_DIV_SHIFT? ? 20 ?237
?238 #defineSCFR1_LPC_DIV_MASK? ? ?0x00003800 ?239 #defineSCFR1_LPC_DIV_SHIFT? ? 11 ?240
?241 /* SCFR2 System Clock Frequency Register 2 */ ?242 #defineSCFR2_SYS_DIV? ? ? ? ?0xFC000000?243 #defineSCFR2_SYS_DIV_SHIFT? ? 26
Quoted 5 lines
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
Quoted 19 lines
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 at denx.de Vulcans never bluff.
? ? ? ?-- Spock, "The Doomsday Machine", stardate 4202.1
PATCH RESEND v2 1/5 Tegra2: Add macros to calculate bitfield shifts and masks
In reply to Simon GlassDear Simon Glass,
Quoted 4 lines · In message <[email protected]> you wrote:
Signed-off-by Simon Glass <[email protected]>---
Changes in v2:
- Removed all bitfield access macros
As Albert already pointed out, this is actually a misleading description.
Quoted 11 lines
+ * You use these to reliably create shifts and masks from a bit field
+ * definition. Bit fields are defined like this:
+ *
+ * #defineNAME_BITSMSB : 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:
+ *
+ * #defineUART_FBCON_BITS5: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
Signature · 6 lines
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
I wish Captain Vimes were here. He wouldn't have known what to do
either, but he's got a much better vocabulary to be baffled in.
- Terry Pratchett, _Guards! Guards!_PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
In reply to Simon GlassThese functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
--- arch/arm/cpu/armv7/tegra2/timer.c | 27 +++++++++++++++++------ arch/arm/include/asm/arch-tegra2/timer.h | 34 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 arch/arm/include/asm/arch-tegra2/timer.h
arch/arm/cpu/armv7/tegra2/timer.c +20 -7
@@ -38,13 +38,12 @@#include <common.h> #include <asm/io.h> #include <asm/arch/tegra2.h> +#include <asm/arch/timer.h> DECLARE_GLOBAL_DATA_PTR ; -struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; -/* counter runs at 1MHz */ -#define TIMER_CLK (1000000) +#define TIMER_CLK 1000000#define TIMER_LOAD_VAL 0xffffffff/* timer without interrupts */
@@ -67,10 +66,10 @@ void set_timer(ulong t)void __udelay (unsigned long usec) {long tmo = usec * (TIMER_CLK /1000 ) /1000 ; -unsigned long now, last =readl (&timer_base->cntr_1us); +unsigned long now, last =timer_get_us ();while (tmo >0 ) { - now =readl (&timer_base->cntr_1us); + now =timer_get_us ();if (last > now)/* count up timer overflow */ tmo -=TIMER_LOAD_VAL - last + now;else
@@ -82,7 +81,7 @@ void __udelay(unsigned long usec)void reset_timer_masked (void ) {/* reset time, capture current incrementer value time */ - gd->lastinc =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + gd->lastinc =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ ); gd->tbl =0 ;/* start "advancing" time stamp from 0 */ }
@@ -91,7 +90,7 @@ ulong get_timer_masked(void)ulong now;/* current tick value */ - now =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + now =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ );if (now >= gd->lastinc)/* normal mode (non roll) */ /* move stamp forward with absolute diff ticks */
@@ -120,3 +119,17 @@ ulong get_tbclk(void) {return CONFIG_SYS_HZ ; } + + +unsigned long timer_get_us (void ) +{ +struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; + +return readl (&timer_base->cntr_1us); +} + +unsigned long timer_get_future_us (u32 delay) +{ +return timer_get_us () + delay; +} +
arch/arm/include/asm/arch-tegra2/timer.h new file +34 -0
@@ -0,0 +1,34 @@ +/* +* 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 +*/ + +/* Tegra2 timer functions */ + +#ifndef _TEGRA2_TIMER_H +#define _TEGRA2_TIMER_H + +/* returns the current monotonic timer value in microseconds */ +unsigned long timer_get_us (void ); + +/* returns what the time will likely be some microseconds into the future */ +unsigned long timer_get_future_us (u32 delay); + +#endif +
Signature · 1 line
1.7.3.1
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
Hi Simon,
Le 05/07/2011 18:49, Simon Glass a ?crit :
Quoted 68 lines
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Signed-off-by Simon Glass<[email protected]>--- arch/arm/cpu/armv7/tegra2/timer.c | 27 +++++++++++++++++------ arch/arm/include/asm/arch-tegra2/timer.h | 34 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-)create mode 100644
arch/arm/include/asm/arch-tegra2/timer.harch/arm/cpu/armv7/tegra2/timer.c +9
-7@@ -38,13 +38,12 @@#include <common.h> #include <asm/io.h> #include <asm/arch/tegra2.h> +#include <asm/arch/timer.h> DECLARE_GLOBAL_DATA_PTR ; -struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; -/* counter runs at 1MHz */ -#define TIMER_CLK (1000000) +#define TIMER_CLK 1000000#define TIMER_LOAD_VAL 0xffffffff/* timer without interrupts */ @@ -67,10 +66,10 @@ void set_timer(ulong t)void __udelay (unsigned long usec) {long tmo = usec * (TIMER_CLK /1000 ) /1000 ; -unsigned long now, last =readl (&timer_base->cntr_1us); +unsigned long now, last =timer_get_us ();while (tmo>0 ) { - now =readl (&timer_base->cntr_1us); + now =timer_get_us ();if (last> now)/* count up timer overflow */ tmo -=TIMER_LOAD_VAL - last + now;else @@ -82,7 +81,7 @@ void __udelay(unsigned long usec)void reset_timer_masked (void ) {/* reset time, capture current incrementer value time */ - gd->lastinc =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + gd->lastinc =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ ); gd->tbl =0 ;/* start "advancing" time stamp from 0 */ }@@ -91,7 +90,7 @@ ulong get_timer_masked(void)ulong now;/* current tick value */ - now =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + now =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ );if (now>= gd->lastinc)/* normal mode (non roll) */ /* move stamp forward with absolute diff ticks */ @@ -120,3 +119,17 @@ ulong get_tbclk(void) {return CONFIG_SYS_HZ ; } + + +unsigned long timer_get_us (void )
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 134
Quoted 10 lines
arch/arm/cpu/armv7/tegra2/timer.c +10
-0+{ +struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; + +return readl (&timer_base->cntr_1us); +} + +unsigned long timer_get_future_us (u32 delay) +{ +return timer_get_us () + delay; +}
What is the added value in a function that just adds its argument to the return value of another function? Might as well do the addition directly instead of calling this 'future' function.
Quoted 31 lines
arch/arm/cpu/armv7/tegra2/timer.c +1
-0+arch/arm/include/asm/arch-tegra2/timer.h new file +24
-0@@ -0,0 +1,34 @@ +/* +* 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 +*/ + +/* Tegra2 timer functions */ + +#ifndef _TEGRA2_TIMER_H
arch/arm/include/asm/arch-tegra2/timer.h line 34
Quoted 10 lines
arch/arm/include/asm/arch-tegra2/timer.h new file +10
-0+#define _TEGRA2_TIMER_H + +/* returns the current monotonic timer value in microseconds */ +unsigned long timer_get_us (void ); + +/* returns what the time will likely be some microseconds into the future */ +unsigned long timer_get_future_us (u32 delay); + +#endif +
Amicalement,
Signature · 1 line
Albert.
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
In reply to Simon GlassHi Simon,
Quoted 2 lines · Simon Glass
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Excellent - Good to see microsecond timers making their way in already
Quoted 42 lines
Signed-off-by Simon Glass <[email protected]>--- arch/arm/cpu/armv7/tegra2/timer.c | 27 +++++++++++++++++------ arch/arm/include/asm/arch-tegra2/timer.h | 34 ++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 arch/arm/include/asm/arch-tegra2/timer.harch/arm/cpu/armv7/tegra2/timer.c +4
-5@@ -38,13 +38,12 @@#include <common.h> #include <asm/io.h> #include <asm/arch/tegra2.h> +#include <asm/arch/timer.h> DECLARE_GLOBAL_DATA_PTR ; -struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; -/* counter runs at 1MHz */ -#define TIMER_CLK (1000000) +#define TIMER_CLK 1000000#define TIMER_LOAD_VAL 0xffffffff/* timer without interrupts */ @@ -67,10 +66,10 @@ void set_timer(ulong t)void __udelay (unsigned long usec) {long tmo = usec * (TIMER_CLK /1000 ) /1000 ; -unsigned long now, last =readl (&timer_base->cntr_1us); +unsigned long now, last =timer_get_us ();while (tmo >0 ) { - now =readl (&timer_base->cntr_1us); + now =timer_get_us ();if (last > now)/* count up timer overflow */ tmo -=TIMER_LOAD_VAL - last + now;else
arch/arm/cpu/armv7/tegra2/timer.c void __udelay(unsigned long usec) · line 84
Quoted 5 lines
arch/arm/cpu/armv7/tegra2/timer.c +1
-1@@ -82,7 +81,7 @@ void __udelay(unsigned long usec)void reset_timer_masked (void ) {/* reset time, capture current incrementer value time */ - gd->lastinc =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + gd->lastinc =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ );
CONFIG_SYS_HZ is always supposed to be 1000 and in future I think it should be removed entirely - Can you clean this up to not us CONFIG_SYS_HZ?
arch/arm/cpu/armv7/tegra2/timer.c ulong get_timer_masked(void) · line 93
Quoted 9 lines
arch/arm/cpu/armv7/tegra2/timer.c +1
-1gd->tbl =0 ;/* start "advancing" time stamp from 0 */ }@@ -91,7 +90,7 @@ ulong get_timer_masked(void)ulong now;/* current tick value */ - now =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + now =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ );
And here
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 121
Quoted 7 lines
arch/arm/cpu/armv7/tegra2/timer.c +0
-0if (now >= gd->lastinc)/* normal mode (non roll) */ /* move stamp forward with absolute diff ticks */ @@ -120,3 +119,17 @@ ulong get_tbclk(void) {return CONFIG_SYS_HZ ; }
Hmmm, maybe all of the above should use get_tbclk()?
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 129
Quoted 8 lines
arch/arm/cpu/armv7/tegra2/timer.c +8
-0+ + +unsigned long timer_get_us (void ) +{ +struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; + +return readl (&timer_base->cntr_1us); +}
timer_get_us needs to be u64 (unsigned long long). Also, the new timer API will define this as time_now_us, would be great if you could use this naming convention now to save me doing a mass of replaces later
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 134
Quoted 5 lines
arch/arm/cpu/armv7/tegra2/timer.c +5
-0+ +unsigned long timer_get_future_us (u32 delay) +{ +return timer_get_us () + delay; +}
C'mon - We've been here before - This is timer API stuff. Where are you going to use this? Why can't the proposed API be used instead?
I know you don't like the 'time since' implementation, but this has been discussed to death and it appears to me that the majority decision was to go that route rather than the 'future time' route. It is a completely pointless exercise and a complete waste of my time to re-write the timer API just to have someone that doesn't like a particular aspect go off and write a one-off function.
Quoted 28 lines
arch/arm/cpu/armv7/tegra2/timer.c +1
-0+arch/arm/include/asm/arch-tegra2/timer.h new file +21
-0@@ -0,0 +1,34 @@ +/* +* 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 +*/ +
arch/arm/include/asm/arch-tegra2/timer.h line 31
Quoted 10 lines
arch/arm/include/asm/arch-tegra2/timer.h new file +10
-0+/* Tegra2 timer functions */ + +#ifndef _TEGRA2_TIMER_H +#define _TEGRA2_TIMER_H + +/* returns the current monotonic timer value in microseconds */ +unsigned long timer_get_us (void ); + +/* returns what the time will likely be some microseconds into the future */ +unsigned long timer_get_future_us (u32 delay);
'likely' meaning it may or may not - no guarantee though. The new timer API is designed specifically designed to resolve this - 'At least x ms/us have passed' or 'at most x ms/us have passed'. No more 'x ms/us _might_ have passed'
arch/arm/include/asm/arch-tegra2/timer.h line 34
Quoted 3 lines
arch/arm/include/asm/arch-tegra2/timer.h new file +3
-0+ +#endif +
Regards,
Graeme
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
Hi Graeme,
Quoted 42 lines · Graeme Russ
Hi Simon,
Quoted 2 lines · Simon Glass
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Excellent - Good to see microsecond timers making their way in already
Quoted 3 lines
/* reset time, capture current incrementer value time */-
gd->lastinc=readl(&timer_base->cntr_1us)/(TIMER_CLK/CONFIG_SYS_HZ);
Quoted 1 line
+
gd->lastinc=timer_get_us()/ (TIMER_CLK/CONFIG_SYS_HZ);
CONFIG_SYS_HZis always supposed to be 1000 and in future I think it should be removed entirely - Can you clean this up to not usCONFIG_SYS_HZ?I will take a look.
arch/arm/cpu/armv7/tegra2/timer.c ulong get_timer_masked(void) · line 93
Quoted 9 lines
gd->tbl = 0; /* start "advancing" time stamp from 0 */}
arch/arm/cpu/armv7/tegra2/timer.c +1
-1@@ -91,7 +90,7 @@ ulong get_timer_masked(void)ulong now;/* current tick value */ - now =readl (&timer_base->cntr_1us) / (TIMER_CLK /CONFIG_SYS_HZ ); + now =timer_get_us () / (TIMER_CLK /CONFIG_SYS_HZ );And here
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 121
Quoted 7 lines
arch/arm/cpu/armv7/tegra2/timer.c +0
-0if (now >= gd->lastinc)/* normal mode (non roll) */ /* move stamp forward with absolute diff ticks */ @@ -120,3 +119,17 @@ ulong get_tbclk(void) {return CONFIG_SYS_HZ ; }Hmmm, maybe all of the above should use
get_tbclk()?
Quoted 13 lines
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 129
Quoted 8 lines
arch/arm/cpu/armv7/tegra2/timer.c +8
-0+ + +unsigned long timer_get_us (void ) +{ +struct timerus *timer_base = (struct timerus *)NV_PA_TMRUS_BASE ; + +return readl (&timer_base->cntr_1us); +}
timer_get_usneeds to be u64 (unsigned long long). Also, the new timer API will define this astime_now_us, would be great if you could use this naming convention now to save me doing a mass of replaces later
Yes will do.
Quoted 10 lines
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 134
Quoted 6 lines
arch/arm/cpu/armv7/tegra2/timer.c +5
-0+ +unsigned long timer_get_future_us (u32 delay) +{ +return timer_get_us () + delay; +}C'mon - We've been here before - This is timer API stuff. Where are you going to use this? Why can't the proposed API be used instead?
Quoted 7 lines
I know you don't like the 'time since' implementation, but this has been discussed to death and it appears to me that the majority decision was to go that route rather than the 'future time' route. It is a completely pointless exercise and a complete waste of my time to re-write the timer API just to have someone that doesn't like a particular aspect go off and write a one-off function.
Well this code pre-dates this and I haven't revised it. I will take another look and sort this out. In fact from memory the return value isn't even used!
Quoted 40 lines
Review
Quoted 3 lines
(quoted diff) +1
-0+(quoted diff) +0
-0
b/arch/arm/include/asm/arch-tegra2/timer.hQuoted 25 lines
new file mode 100644
index 0000000..5d5445earch/arm/include/asm/arch-tegra2/timer.h new file +20
-0@@ -0,0 +1,34 @@ +/* +* 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 +*/ arch/arm/include/asm/arch-tegra2/timer.h line 30
Quoted 10 lines
arch/arm/include/asm/arch-tegra2/timer.h new file +10
-0+ +/* Tegra2 timer functions */ + +#ifndef _TEGRA2_TIMER_H +#define _TEGRA2_TIMER_H + +/* returns the current monotonic timer value in microseconds */ +unsigned long timer_get_us (void ); + +/* returns what the time will likely be some microseconds into the future */
Quoted 7 lines
arch/arm/include/asm/arch-tegra2/timer.h line 31
Quoted 1 line
arch/arm/include/asm/arch-tegra2/timer.h new file +1
-0+unsigned long timer_get_future_us (u32 delay);'likely' meaning it may or may not - no guarantee though. The new timer API is designed specifically designed to resolve this - 'At least x ms/us have passed' or 'at most x ms/us have passed'. No more 'x ms/us _might_ have passed'
Yes, watch this space.
BTW I have come across another problem where initialization must be done which has long delays in it (LCD display power-up sequence). It's starting to feel like we should have a central place for registering a timer which calls back when a time has expired. That way we can continue with other tings while we wait for the time to elapse. Something like:
/* Move to the next state */
static int next_state(void *my_data)
{
/* do some things, and then if you want to be called again... */ timer_register(timer_now_ms() + 40, next_state, my_data) }
void start_lcd_init()
{
// do first thing
...
// set a timer to do the next thing later timer_register(timer_now_ms() + 200, next_state, my_data) }
...
Every now and then code (or a timer wait function) can call
timer_check()
which will call any expired timers on the list and remove them. This allows LCD init to continue while downloading the kernel, for example.
I haven't thought too hard about this, but apart from LCD I know that USB has some big delays. Obviously we can't make U-Boot multi-threaded but we can perhaps do something simple like the above. What do you think?
Also given that your timer API stuff seems to have a good reception and people are very happy, is there any impediment to getting this in sooner rather than later?
Regards,
Simon
Quoted 8 lines
arch/arm/include/asm/arch-tegra2/timer.h line 34
Quoted 3 lines
arch/arm/include/asm/arch-tegra2/timer.h new file +3
-0+ +#endif +Regards,
Graeme
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
Hi Simon,
Quoted 1 line · Simon Glass
Hi Graeme,
[snip]
Quoted 7 lines
timer_get_usneeds to be u64 (unsigned long long). Also, the new timer API will define this astime_now_us, would be great if you could use this naming convention now to save me doing a mass of replaces laterYes will do.
Thanks
Quoted 17 lines
arch/arm/cpu/armv7/tegra2/timer.c ulong get_tbclk(void) · line 134
Quoted 5 lines
arch/arm/cpu/armv7/tegra2/timer.c +5
-0+ +unsigned long timer_get_future_us (u32 delay) +{ +return timer_get_us () + delay; +}C'mon - We've been here before - This is timer API stuff. Where are you going to use this? Why can't the proposed API be used instead?
I know you don't like the 'time since' implementation, but this has been discussed to death and it appears to me that the majority decision was to go that route rather than the 'future time' route. It is a completely pointless exercise and a complete waste of my time to re-write the timer API just to have someone that doesn't like a particular aspect go off and write a one-off function.
Quoted 2 lines
Well this code pre-dates this and I haven't revised it. I will take another look and sort this out. In fact from memory the return value isn't even used!
Ah, OK then - Sorry for the tone, I didn't realise the history of this patch
[snip]
Quoted 8 lines
'likely' meaning it may or may not - no guarantee though. The new timer API is designed specifically designed to resolve this - 'At least x ms/us have passed' or 'at most x ms/us have passed'. No more 'x ms/us _might_ have passed'
Yes, watch this space.
Maybe you could grab the timer functions for the new API from the patch series I posted recently and create the micro-second equivalents in Tegra2. I can the move them into common code later with no other code changes necessary
Quoted 29 lines
BTW I have come across another problem where initialization must be done which has long delays in it (LCD display power-up sequence). It's starting to feel like we should have a central place for registering a timer which calls back when a time has expired. That way we can continue with other tings while we wait for the time to elapse. Something like:
/* Move to the next state */
static intnext_state(void *my_data)
{
/* do some things, and then if you want to be called again... */timer_register(timer_now_ms() + 40, next_state, my_data)}void
start_lcd_init()
{
// do first thing
...
// set a timer to do the next thing latertimer_register(timer_now_ms() + 200, next_state, my_data)}...
Every now and then code (or a timer wait function) can call
timer_check()
Quoted 7 lines
which will call any expired timers on the list and remove them. This allows LCD init to continue while downloading the kernel, for example.
I haven't thought too hard about this, but apart from LCD I know that USB has some big delays. Obviously we can't make U-Boot multi-threaded but we can perhaps do something simple like the above. What do you think?
Well, this is a little beyond the scope of a simple boot loader ;) And unless we start getting real fancy with task schedulers etc, the callback will most likely be done in the context of an IRQ handler.
I do agree, however, that in some circumstances, it would be useful to be able to 'background' some tasks such as doing a flash erase in the background while calculating the environment CRC or letting a device initialising itself while U-Boot moves on through the boot sequence. But this can be done without callbacks anyway in the board init sequence:
...low level init stuff..., start_lcd_init, ...other stuff..., wait_lcd_init_complete, ...more stuff needing LCD output...,
Quoted 4 lines
Also given that your timer API stuff seems to have a good reception and people are very happy, is there any impediment to getting this in sooner rather than later?
I hope so, but I'm really wanting feedback from Wolfgang and I fear the merge window will close before it's ready :(
Regards,
Graeme
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
In reply to Graeme RussDear Graeme Russ,
Quoted 6 lines · In message <[email protected]> you wrote:
Quoted 2 lines · Simon Glass
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Excellent - Good to see microsecond timers making their way in already
Sorry, but I disagree here.
I consider it just overhead and code bloat.
Best regards,
Wolfgang Denk
Signature · 4 lines
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 It's a small world, but I wouldn't want to paint it.
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
In reply to Graeme RussDear Graeme Russ,
Quoted 4 lines · In message <[email protected]> you wrote:
timer_get_usneeds to be u64 (unsigned long long). Also, the new timer API will define this astime_now_us, would be great if you could use this naming convention now to save me doing a mass of replaces later
Um... this sounds as if there was some confirmed agreement on a new timer API? I remember that we had such a discussion some time ago, with some suggestions, but then the discussion faded, and I don't remember that any specific agreement was made.
Am I missing something?
Best regards,
Wolfgang Denk
Signature · 5 lines
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 People seldom know what they want until you give them what they ask for.
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
Hi Wolfgang,
Quoted 9 lines · Wolfgang Denk
Dear Graeme Russ,
Quoted 4 lines · In message <[email protected]> you wrote:
timer_get_usneeds to be u64 (unsigned long long). Also, the new timer API will define this astime_now_us, would be great if you could use this naming convention now to save me doing a mass of replaces later
Quoted 6 lines
Um... this sounds as if there was some confirmed agreement on a new timer API? I remember that we had such a discussion some time ago, with some suggestions, but then the discussion faded, and I don't remember that any specific agreement was made.
Am I missing something?
Yes - A 16 part patch series:
- In patchwork, filter by Graeme Russ as submitter
- http://lists.denx.de/pipermail/u-boot/2011-June/094958.html is the
summary post
A request for you to look at it (plus a few questions):
- http://lists.denx.de/pipermail/u-boot/2011-July/095693.html
The wiki:
- http://www.denx.de/wiki/U-Boot/TaskTimerAPI
Regards,
Graeme
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
Dear Graeme,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 2 lines
Am I missing something?
Yes - A 16 part patch series:
- In patchwork, filter by Graeme Russ as submitter
- http://lists.denx.de/pipermail/u-boot/2011-June/094958.html is the
summary post
I'm aware of this - it's on my list, but unfortunately this is a long list :-(
Quoted 2 lines
The wiki:
- http://www.denx.de/wiki/U-Boot/TaskTimerAPI
I see. Hm... assuming this is still under discussion, how do you suggest to handle the wiki version? For discussing things, it's probably not really efficient if several people edit the pages.
Best regards,
Wolfgang Denk
Signature · 5 lines
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 The universe does not have laws - it has habits, and habits can be broken.
PATCH RESEND v2 2/5 Tegra2: Add microsecond timer functions
Quoted 14 lines · Wolfgang Denk
Dear Graeme,
Quoted 7 lines · In message <[email protected]> you wrote:
Quoted 2 lines
Am I missing something?
Yes - A 16 part patch series:
- In patchwork, filter by Graeme Russ as submitter
- http://lists.denx.de/pipermail/u-boot/2011-June/094958.html is the
summary postI'm aware of this - it's on my list, but unfortunately this is a long list :-(
Quoted 6 lines
Quoted 2 lines
The wiki:
- http://www.denx.de/wiki/U-Boot/TaskTimerAPII see. Hm... assuming this is still under discussion, how do you suggest to handle the wiki version? For discussing things, it's probably not really efficient if several people edit the pages.
What about if I keep the wiki updated with links to the past and current discussion threads and patches in patchwork? I can update the wiki based on the ongoing discussion until we are happy that we have reached the right solution?
Regards,
Graeme
PATCH RESEND v2 3/5 Tegra2: Add more clock support
In reply to Simon GlassThis adds functions to enable/disable clocks and reset to on-chip peripherals.
---
Changes in v2:
- Removed use of bitfield access macros
arch/arm/cpu/armv7/tegra2/Makefile | 2 +- arch/arm/cpu/armv7/tegra2/ap20.c | 52 ++---- arch/arm/cpu/armv7/tegra2/clock.c | 158 +++++++++++++++++ arch/arm/include/asm/arch-tegra2/clk_rst.h | 125 +++++++++----- arch/arm/include/asm/arch-tegra2/clock.h | 264 ++++++++++++++++++++++++++++ board/nvidia/common/board.c | 46 ++--- 6 files changed, 536 insertions(+), 111 deletions(-) create mode 100644 arch/arm/cpu/armv7/tegra2/clock.c create mode 100644 arch/arm/include/asm/arch-tegra2/clock.h
arch/arm/cpu/armv7/tegra2/Makefile +1 -1
@@ -28,7 +28,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(SOC).o SOBJS := lowlevel_init.o -COBJS := ap20.o board.o sys_info.o timer.o +COBJS := ap20.o board.o clock.o sys_info.o timer.o SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS))
arch/arm/cpu/armv7/tegra2/ap20.c +17 -35
124 lines not on this page · raw message
arch/arm/cpu/armv7/tegra2/clock.c new file +158 -0
158 lines not on this page · raw message
arch/arm/include/asm/arch-tegra2/clk_rst.h +80 -45
151 lines not on this page · raw message
arch/arm/include/asm/arch-tegra2/clock.h new file +264 -0
264 lines not on this page · raw message
board/nvidia/common/board.c +16 -30
84 lines not on this page · raw message
Signature · 1 line
1.7.3.1
PATCH RESEND v2 4/5 Tegra2: add additional pin multiplexing features
In reply to Simon GlassThis adds an enum for each pin and some functions for changing the pin muxing setup.
---
Changes in v2:
- Removed use of bitfield access macros
arch/arm/cpu/armv7/tegra2/Makefile | 2 +- arch/arm/cpu/armv7/tegra2/pinmux.c | 53 ++++++++++ arch/arm/include/asm/arch-tegra2/pinmux.h | 155 +++++++++++++++++++++++++++-- board/nvidia/common/board.c | 10 +-- 4 files changed, 205 insertions(+), 15 deletions(-) create mode 100644 arch/arm/cpu/armv7/tegra2/pinmux.c
arch/arm/cpu/armv7/tegra2/Makefile +1 -1
@@ -28,7 +28,7 @@ include $(TOPDIR)/config.mk LIB = $(obj)lib$(SOC).o SOBJS := lowlevel_init.o -COBJS := ap20.o board.o clock.o sys_info.o timer.o +COBJS := ap20.o board.o clock.o pinmux.o sys_info.o timer.o SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS))
arch/arm/cpu/armv7/tegra2/pinmux.c new file +53 -0
53 lines not on this page · raw message
arch/arm/include/asm/arch-tegra2/pinmux.h +148 -7
172 lines not on this page · raw message
board/nvidia/common/board.c +3 -7
@@ -140,19 +140,15 @@ static void pin_mux_uart(void) reg &=0xFFF0FFFF ;/* IRRX_/IRTX_SEL [19:16] = 00 UARTA */ writel (reg, &pmt->pmt_ctl_c); - reg =readl (&pmt->pmt_tri_a); - reg &= ~Z_IRRX ;/* Z_IRRX = normal (0) */ - reg &= ~Z_IRTX ;/* Z_IRTX = normal (0) */ -writel (reg, &pmt->pmt_tri_a); +pinmux_tristate_disable (PIN_IRRX ); +pinmux_tristate_disable (PIN_IRTX );#endif /* CONFIG_TEGRA2_ENABLE_UARTA */ #if defined(CONFIG_TEGRA2_ENABLE_UARTD ) reg =readl (&pmt->pmt_ctl_b); reg &=0xFFFFFFF3 ;/* GMC_SEL [3:2] = 00, UARTD */ writel (reg, &pmt->pmt_ctl_b); - reg =readl (&pmt->pmt_tri_a); - reg &= ~Z_GMC ;/* Z_GMC = normal (0) */ -writel (reg, &pmt->pmt_tri_a); +pinmux_tristate_disable (PIN_GMC );#endif /* CONFIG_TEGRA2_ENABLE_UARTD */ }
Signature · 1 line
1.7.3.1
PATCH RESEND v2 5/5 Tegra2: Use clock and pinmux functions to simplify code
In reply to Simon Glass---
Changes in v2:
- Removed use of bitfield access macros
- Now uses manual shifts and masks
arch/arm/cpu/armv7/tegra2/ap20.c | 47 ++++++++------------------- arch/arm/include/asm/arch-tegra2/clk_rst.h | 37 ---------------------- board/nvidia/common/board.c | 12 +++--- 3 files changed, 20 insertions(+), 76 deletions(-)
arch/arm/cpu/armv7/tegra2/ap20.c +14 -33
@@ -40,23 +40,21 @@ void init_pllx(void)u32 reg;/* If PLLX is already enabled, just return */ - reg =readl (&pll->pll_base); -if (reg &PLL_ENABLE ) +if (readl (&pll->pll_base) &PLL_ENABLE_MASK )return ;/* Set PLLX_MISC */ - reg =CPCON ;/* CPCON[11:8] = 0001 */ -writel (reg, &pll->pll_misc); +writel (1 <<PLL_CPCON_SHIFT , &pll->pll_misc);/* Use 12MHz clock here */ - reg = (PLL_BYPASS |PLL_DIVM_VALUE ); - reg |= (1000 <<8 );/* DIVN = 0x3E8 */ + reg =PLL_BYPASS_MASK | (12 <<PLL_DIVM_SHIFT ); + reg |=1000 <<PLL_DIVN_SHIFT ;writel (reg, &pll->pll_base); - reg |=PLL_ENABLE ; + reg |=PLL_ENABLE_MASK ;writel (reg, &pll->pll_base); - reg &= ~PLL_BYPASS ; + reg &= ~PLL_BYPASS_MASK ;writel (reg, &pll->pll_base); }
@@ -90,16 +88,11 @@ static void enable_cpu_clock(int enable)* always stop the clock to CPU 1. */ clk =readl (&clkrst->crc_clk_cpu_cmplx); - clk |=CPU1_CLK_STP ; - -if (enable) { -/* Unstop the CPU clock */ - clk &= ~CPU0_CLK_STP ; - }else { -/* Stop the CPU clock */ - clk |=CPU0_CLK_STP ; - } + clk |=1 <<CPU1_CLK_STP_SHIFT ; +/* Stop/Unstop the CPU clock */ + clk &= ~CPU0_CLK_STP_MASK ; + clk |= !enable <<CPU0_CLK_STP_SHIFT ;writel (clk, &clkrst->crc_clk_cpu_cmplx);clock_enable (PERIPH_ID_CPU );
@@ -177,9 +170,6 @@ static void enable_cpu_power_rail(void)static void reset_A9_cpu (int reset) { -struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE ; -u32 cpu; -/* * NOTE: Regardless of whether the request is to hold the CPU in reset * or take it out of reset, every processor in the CPU complex
@@ -188,19 +178,10 @@ static void reset_A9_cpu(int reset)* are multiple processors in the CPU complex. */ -/* Hold CPU 1 in reset */ - cpu =SET_DBGRESET1 |SET_DERESET1 |SET_CPURESET1 ; -writel (cpu, &clkrst->crc_cpu_cmplx_set); - -if (reset) { -/* Now place CPU0 into reset */ - cpu |=SET_DBGRESET0 |SET_DERESET0 |SET_CPURESET0 ; -writel (cpu, &clkrst->crc_cpu_cmplx_set); - }else { -/* Take CPU0 out of reset */ - cpu =CLR_DBGRESET0 |CLR_DERESET0 |CLR_CPURESET0 ; -writel (cpu, &clkrst->crc_cpu_cmplx_clr); - } +/* Hold CPU 1 in reset, and CPU 0 if asked */ +reset_cmplx_set_enable (1 , crc_rst_cpu | crc_rst_de | crc_rst_debug,1 ); +reset_cmplx_set_enable (0 , crc_rst_cpu | crc_rst_de | crc_rst_debug, + reset);/* Enable/Disable master CPU reset */ reset_set_enable (PERIPH_ID_CPU , reset);
arch/arm/include/asm/arch-tegra2/clk_rst.h +0 -37
@@ -142,43 +142,6 @@ struct clk_rst_ctlr {uint crc_cpu_cmplx_clr;/* _CPU_CMPLX_CLR_0, 0x344 */ }; -#define PLL_BYPASS (1 << 31) -#define PLL_ENABLE (1 << 30) -#define PLL_BASE_OVRRIDE (1 << 28) -#define PLL_DIVP_VALUE (1 << 20)/* post divider, b22:20 */ -#define PLL_DIVM_VALUE 0x0C/* input divider, b4:0 */ - -#define SWR_UARTD_RST (1 << 1) -#define CLK_ENB_UARTD (1 << 1) -#define SWR_UARTA_RST (1 << 6) -#define CLK_ENB_UARTA (1 << 6) - -#define SWR_CPU_RST (1 << 0) -#define CLK_ENB_CPU (1 << 0) -#define SWR_CSITE_RST (1 << 9) -#define CLK_ENB_CSITE (1 << 9) - -#define SET_CPURESET0 (1 << 0) -#define SET_DERESET0 (1 << 4) -#define SET_DBGRESET0 (1 << 12) - -#define SET_CPURESET1 (1 << 1) -#define SET_DERESET1 (1 << 5) -#define SET_DBGRESET1 (1 << 13) - -#define CLR_CPURESET0 (1 << 0) -#define CLR_DERESET0 (1 << 4) -#define CLR_DBGRESET0 (1 << 12) - -#define CLR_CPURESET1 (1 << 1) -#define CLR_DERESET1 (1 << 5) -#define CLR_DBGRESET1 (1 << 13) - -#define CPU0_CLK_STP (1 << 8) -#define CPU1_CLK_STP (1 << 9) - -#define CPCON (1 << 8) -/* CLK_RST_CONTROLLER_CLK_CPU_CMPLX_0 */ #define CPU1_CLK_STP_BITS 9 : 9#define CPU1_CLK_STP_SHIFT bf_shift(CPU1_CLK_STP_BITS)
board/nvidia/common/board.c +6 -6
@@ -78,20 +78,20 @@ static void clock_init_uart(void)u32 reg; reg =readl (&pll->pll_base); -if (!(reg &PLL_BASE_OVRRIDE )) { +if (!(reg &PLL_BASE_OVRRIDE_MASK )) {/* Override pllp setup for 216MHz operation. */ - reg = (PLL_BYPASS |PLL_BASE_OVRRIDE |PLL_DIVP_VALUE ); - reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ /500 ) <<8 ) |PLL_DIVM_VALUE ); + reg =PLL_BYPASS_MASK |PLL_BASE_OVRRIDE_MASK | + (1 <<PLL_DIVP_SHIFT ) | (0xc <<PLL_DIVM_SHIFT ); + reg |= (NVRM_PLLP_FIXED_FREQ_KHZ /500 ) <<PLL_DIVN_SHIFT ;writel (reg, &pll->pll_base); - reg |=PLL_ENABLE ; + reg |=PLL_ENABLE_MASK ;writel (reg, &pll->pll_base); - reg &= ~PLL_BYPASS ; + reg &= ~PLL_BYPASS_MASK ;writel (reg, &pll->pll_base); } -/* Now do the UART reset/clock enable */ #if defined(CONFIG_TEGRA2_ENABLE_UARTA )/* Assert UART reset and enable clock */ reset_set_enable (PERIPH_ID_UART1 ,1 );
Signature · 1 line
1.7.3.1