[U-Boot] [RESEND PATCH v2 0/5] 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
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
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
Hi Simon,
Le 05/07/2011 18:49, Simon Glass a écrit :
Signed-off-by: Simon Glasssjg@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.netwrote:
Hi Simon,
Le 05/07/2011 18:49, Simon Glass a écrit :
Signed-off-by: Simon Glasssjg@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 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.netwrote:
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,
AntonAmicalement,
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
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
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/cpu/armv7/tegra2/timer.c b/arch/arm/cpu/armv7/tegra2/timer.c index fb061d0..b69c172 100644 --- a/arch/arm/cpu/armv7/tegra2/timer.c +++ b/arch/arm/cpu/armv7/tegra2/timer.c @@ -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; +} + diff --git a/arch/arm/include/asm/arch-tegra2/timer.h b/arch/arm/include/asm/arch-tegra2/timer.h new file mode 100644 index 0000000..5d5445e --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/timer.h @@ -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 +
Hi Simon,
Le 05/07/2011 18:49, Simon Glass a écrit :
These functions provide access to the high resolution microsecond timer and tidy up a global variable in the code.
Signed-off-by: Simon Glasssjg@chromium.org
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
diff --git a/arch/arm/cpu/armv7/tegra2/timer.c b/arch/arm/cpu/armv7/tegra2/timer.c index fb061d0..b69c172 100644 --- a/arch/arm/cpu/armv7/tegra2/timer.c +++ b/arch/arm/cpu/armv7/tegra2/timer.c @@ -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();@@ -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;
+}
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.
diff --git a/arch/arm/include/asm/arch-tegra2/timer.h b/arch/arm/include/asm/arch-tegra2/timer.h new file mode 100644 index 0000000..5d5445e --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/timer.h @@ -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
Amicalement,
Hi Simon,
On 06/07/11 02:49, Simon Glass wrote:
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
Signed-off-by: Simon Glass sjg@chromium.org
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
diff --git a/arch/arm/cpu/armv7/tegra2/timer.c b/arch/arm/cpu/armv7/tegra2/timer.c index fb061d0..b69c172 100644 --- a/arch/arm/cpu/armv7/tegra2/timer.c +++ b/arch/arm/cpu/armv7/tegra2/timer.c @@ -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();@@ -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?
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);
And here
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; }
Hmmm, maybe all of the above should use get_tbclk()?
+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
+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.
diff --git a/arch/arm/include/asm/arch-tegra2/timer.h b/arch/arm/include/asm/arch-tegra2/timer.h new file mode 100644 index 0000000..5d5445e --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/timer.h @@ -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);
'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'
+#endif
Regards,
Graeme
Hi Graeme,
On Sat, Jul 9, 2011 at 10:24 PM, Graeme Russ graeme.russ@gmail.com wrote:
Hi Simon,
On 06/07/11 02:49, Simon Glass wrote:
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
/* 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?
I will take a look.
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);And here
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; }
Hmmm, maybe all of the above should use get_tbclk()?
+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
Yes will do.
+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.
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!
diff --git a/arch/arm/include/asm/arch-tegra2/timer.h
b/arch/arm/include/asm/arch-tegra2/timer.h
new file mode 100644 index 0000000..5d5445e --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/timer.h @@ -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);
'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
+#endif
Regards,
Graeme
Hi Simon,
On 10/07/11 16:14, Simon Glass wrote:
Hi Graeme,
[snip]
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 laterYes will do.
Thanks
> + > +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.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]
'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
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?
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...,
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
Dear Graeme Russ,
In message 4E1937A6.7050808@gmail.com you wrote:
On 06/07/11 02:49, Simon Glass wrote:
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
Dear Graeme Russ,
In message 4E1937A6.7050808@gmail.com you wrote:
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
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
Hi Wolfgang,
On 11/07/11 16:20, Wolfgang Denk wrote:
Dear Graeme Russ,
In message 4E1937A6.7050808@gmail.com you wrote:
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
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
Dear Graeme,
In message 4E1A9B9B.4030104@gmail.com you wrote:
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 :-(
The wiki:
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
On 12/07/11 05:58, Wolfgang Denk wrote:
Dear Graeme,
In message 4E1A9B9B.4030104@gmail.com you wrote:
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 :-(
The wiki:
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.
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
This adds functions to enable/disable clocks and reset to on-chip peripherals.
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile index f1ea915..b35764c 100644 --- a/arch/arm/cpu/armv7/tegra2/Makefile +++ b/arch/arm/cpu/armv7/tegra2/Makefile @@ -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)) diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c b/arch/arm/cpu/armv7/tegra2/ap20.c index 60dd5df..e3832e2 100644 --- a/arch/arm/cpu/armv7/tegra2/ap20.c +++ b/arch/arm/cpu/armv7/tegra2/ap20.c @@ -25,6 +25,7 @@ #include <asm/io.h> #include <asm/arch/tegra2.h> #include <asm/arch/clk_rst.h> +#include <asm/arch/clock.h> #include <asm/arch/pmc.h> #include <asm/arch/pinmux.h> #include <asm/arch/scu.h> @@ -35,33 +36,34 @@ u32 s_first_boot = 1; void init_pllx(void) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + struct clk_pll *pll = &clkrst->crc_pll[CLOCK_PLL_ID_XCPU]; u32 reg;
/* If PLLX is already enabled, just return */ - reg = readl(&clkrst->crc_pllx_base); + reg = readl(&pll->pll_base); if (reg & PLL_ENABLE) return;
/* Set PLLX_MISC */ reg = CPCON; /* CPCON[11:8] = 0001 */ - writel(reg, &clkrst->crc_pllx_misc); + writel(reg, &pll->pll_misc);
/* Use 12MHz clock here */ - reg = (PLL_BYPASS | PLL_DIVM); + reg = (PLL_BYPASS | PLL_DIVM_VALUE); reg |= (1000 << 8); /* DIVN = 0x3E8 */ - writel(reg, &clkrst->crc_pllx_base); + writel(reg, &pll->pll_base);
reg |= PLL_ENABLE; - writel(reg, &clkrst->crc_pllx_base); + writel(reg, &pll->pll_base);
reg &= ~PLL_BYPASS; - writel(reg, &clkrst->crc_pllx_base); + writel(reg, &pll->pll_base); }
static void enable_cpu_clock(int enable) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; - u32 reg, clk; + u32 clk;
/* * NOTE: @@ -83,10 +85,6 @@ static void enable_cpu_clock(int enable) writel(SUPER_CCLK_DIVIDER, &clkrst->crc_super_cclk_div); }
- /* Fetch the register containing the main CPU complex clock enable */ - reg = readl(&clkrst->crc_clk_out_enb_l); - reg |= CLK_ENB_CPU; - /* * Read the register containing the individual CPU clock enables and * always stop the clock to CPU 1. @@ -103,7 +101,8 @@ static void enable_cpu_clock(int enable) }
writel(clk, &clkrst->crc_clk_cpu_cmplx); - writel(reg, &clkrst->crc_clk_out_enb_l); + + clock_enable(PERIPH_ID_CPU); }
static int is_cpu_powered(void) @@ -179,7 +178,7 @@ 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 reg, cpu; + u32 cpu;
/* * NOTE: Regardless of whether the request is to hold the CPU in reset @@ -193,44 +192,27 @@ static void reset_A9_cpu(int reset) cpu = SET_DBGRESET1 | SET_DERESET1 | SET_CPURESET1; writel(cpu, &clkrst->crc_cpu_cmplx_set);
- reg = readl(&clkrst->crc_rst_dev_l); if (reset) { /* Now place CPU0 into reset */ cpu |= SET_DBGRESET0 | SET_DERESET0 | SET_CPURESET0; writel(cpu, &clkrst->crc_cpu_cmplx_set); - - /* Enable master CPU reset */ - reg |= SWR_CPU_RST; } else { /* Take CPU0 out of reset */ cpu = CLR_DBGRESET0 | CLR_DERESET0 | CLR_CPURESET0; writel(cpu, &clkrst->crc_cpu_cmplx_clr); - - /* Disable master CPU reset */ - reg &= ~SWR_CPU_RST; }
- writel(reg, &clkrst->crc_rst_dev_l); + /* Enable/Disable master CPU reset */ + reset_set_enable(PERIPH_ID_CPU, reset); }
static void clock_enable_coresight(int enable) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; - u32 rst, clk, src; - - rst = readl(&clkrst->crc_rst_dev_u); - clk = readl(&clkrst->crc_clk_out_enb_u); - - if (enable) { - rst &= ~SWR_CSITE_RST; - clk |= CLK_ENB_CSITE; - } else { - rst |= SWR_CSITE_RST; - clk &= ~CLK_ENB_CSITE; - } + u32 rst, src;
- writel(clk, &clkrst->crc_clk_out_enb_u); - writel(rst, &clkrst->crc_rst_dev_u); + clock_set_enable(PERIPH_ID_CORESIGHT, enable); + reset_set_enable(PERIPH_ID_CORESIGHT, !enable);
if (enable) { /* diff --git a/arch/arm/cpu/armv7/tegra2/clock.c b/arch/arm/cpu/armv7/tegra2/clock.c new file mode 100644 index 0000000..9a7ded0 --- /dev/null +++ b/arch/arm/cpu/armv7/tegra2/clock.c @@ -0,0 +1,158 @@ +/* + * 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 Clock control functions */ + +#include <asm/io.h> +#include <asm/arch/clk_rst.h> +#include <asm/arch/clock.h> +#include <asm/arch/timer.h> +#include <asm/arch/tegra2.h> +#include <common.h> + +#ifdef DEBUG +#define assert(x) \ + ({ if (!(x)) printf("Assertion failure '%s' %s line %d\n", \ + #x, __FILE__, __LINE__); }) +#else +#define assert(x) +#endif + +/* + * Get the oscillator frequency, from the corresponding hardware configuration + * field. + */ +enum clock_osc_freq clock_get_osc_freq(void) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 reg; + + reg = readl(&clkrst->crc_osc_ctrl); + return (reg & OSC_FREQ_MASK) >> OSC_FREQ_SHIFT; +} + +unsigned long clock_start_pll(enum clock_pll_id clkid, u32 divm, u32 divn, + u32 divp, u32 cpcon, u32 lfcon) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 data; + struct clk_pll *pll; + + assert(clock_pll_id_isvalid(clkid)); + pll = &clkrst->crc_pll[clkid]; + + /* + * We cheat by treating all PLL (except PLLU) in the same fashion. + * This works only because: + * - same fields are always mapped at same offsets, except DCCON + * - DCCON is always 0, doesn't conflict + * - M,N, P of PLLP values are ignored for PLLP + */ + data = (cpcon << PLL_CPCON_SHIFT) | (lfcon << PLL_LFCON_SHIFT); + writel(data, &pll->pll_misc); + + data = (divm << PLL_DIVM_SHIFT) | (divn << PLL_DIVN_SHIFT) | + (0 << PLL_BYPASS_SHIFT) | (1 << PLL_ENABLE_SHIFT); + + if (clkid == CLOCK_PLL_ID_USB) + data |= divp << PLLU_VCO_FREQ_SHIFT; + else + data |= divp << PLL_DIVP_SHIFT; + writel(data, &pll->pll_base); + + /* calculate the stable time */ + return timer_get_future_us(CLOCK_PLL_STABLE_DELAY_US); +} + +void clock_set_enable(enum periph_id periph_id, int enable) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 *clk = &clkrst->crc_clk_out_enb[PERIPH_REG(periph_id)]; + u32 reg; + + /* Enable/disable the clock to this peripheral */ + assert(clock_periph_id_isvalid(periph_id)); + reg = readl(clk); + if (enable) + reg |= PERIPH_MASK(periph_id); + else + reg &= ~PERIPH_MASK(periph_id); + writel(reg, clk); +} + +void clock_enable(enum periph_id clkid) +{ + clock_set_enable(clkid, 1); +} + +void clock_disable(enum periph_id clkid) +{ + clock_set_enable(clkid, 0); +} + +void reset_set_enable(enum periph_id periph_id, int enable) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 *reset = &clkrst->crc_rst_dev[PERIPH_REG(periph_id)]; + u32 reg; + + /* Enable/disable reset to the peripheral */ + assert(clock_periph_id_isvalid(periph_id)); + reg = readl(reset); + if (enable) + reg |= PERIPH_MASK(periph_id); + else + reg &= ~PERIPH_MASK(periph_id); + writel(reg, reset); +} + +void reset_periph(enum periph_id periph_id, int us_delay) +{ + /* Put peripheral into reset */ + reset_set_enable(periph_id, 1); + udelay(us_delay); + + /* Remove reset */ + reset_set_enable(periph_id, 0); + + udelay(us_delay); +} + +void reset_cmplx_set_enable(int cpu, int which, int reset) +{ + struct clk_rst_ctlr *clkrst = + (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + u32 mask; + + /* Form the mask, which depends on the cpu chosen. Tegra2 has 2 */ + assert(cpu >= 0 && cpu < 2); + mask = which << cpu; + + /* either enable or disable those reset for that CPU */ + if (reset) + writel(mask, &clkrst->crc_cpu_cmplx_set); + else + writel(mask, &clkrst->crc_cpu_cmplx_clr); +} diff --git a/arch/arm/include/asm/arch-tegra2/clk_rst.h b/arch/arm/include/asm/arch-tegra2/clk_rst.h index bd8ad2c..6c74410 100644 --- a/arch/arm/include/asm/arch-tegra2/clk_rst.h +++ b/arch/arm/include/asm/arch-tegra2/clk_rst.h @@ -24,15 +24,36 @@ #ifndef _CLK_RST_H_ #define _CLK_RST_H_
+#include <asm/arch/bitfield.h> + +/* PLL registers - there are several PLLs in the clock controller */ +struct clk_pll { + uint pll_base; /* the control register */ + uint pll_out; /* output control */ + uint reserved; + uint pll_misc; /* other misc things */ +}; + +/* PLL registers - there are several PLLs in the clock controller */ +struct clk_pll_simple { + uint pll_base; /* the control register */ + uint pll_misc; /* other misc things */ +}; + +/* + * Most PLLs use the clk_pll structure, but some have a simpler two-member + * structure for which we use clk_pll_simple. The reason for this non- + * othogonal setup is not stated. + */ +#define TEGRA_CLK_PLLS 6 +#define TEGRA_CLK_SIMPLE_PLLS 3 /* Number of simple PLLs */ +#define TEGRA_CLK_REGS 3 /* Number of clock enable registers */ + /* Clock/Reset Controller (CLK_RST_CONTROLLER_) regs */ struct clk_rst_ctlr { - uint crc_rst_src; /* _RST_SOURCE_0, 0x00 */ - uint crc_rst_dev_l; /* _RST_DEVICES_L_0, 0x04 */ - uint crc_rst_dev_h; /* _RST_DEVICES_H_0, 0x08 */ - uint crc_rst_dev_u; /* _RST_DEVICES_U_0, 0x0C */ - uint crc_clk_out_enb_l; /* _CLK_OUT_ENB_L_0, 0x10 */ - uint crc_clk_out_enb_h; /* _CLK_OUT_ENB_H_0, 0x14 */ - uint crc_clk_out_enb_u; /* _CLK_OUT_ENB_U_0, 0x18 */ + uint crc_rst_src; /* _RST_SOURCE_0,0x00 */ + uint crc_rst_dev[TEGRA_CLK_REGS]; /* _RST_DEVICES_L/H/U_0 */ + uint crc_clk_out_enb[TEGRA_CLK_REGS]; /* _CLK_OUT_ENB_L/H/U_0 */ uint crc_reserved0; /* reserved_0, 0x1C */ uint crc_cclk_brst_pol; /* _CCLK_BURST_POLICY_0,0x20 */ uint crc_super_cclk_div; /* _SUPER_CCLK_DIVIDER_0,0x24 */ @@ -52,44 +73,11 @@ struct clk_rst_ctlr { uint crc_osc_freq_det_stat; /* _OSC_FREQ_DET_STATUS_0,0x5C */ uint crc_reserved2[8]; /* reserved_2[8], 0x60-7C */
- uint crc_pllc_base; /* _PLLC_BASE_0, 0x80 */ - uint crc_pllc_out; /* _PLLC_OUT_0, 0x84 */ - uint crc_reserved3; /* reserved_3, 0x88 */ - uint crc_pllc_misc; /* _PLLC_MISC_0, 0x8C */ - - uint crc_pllm_base; /* _PLLM_BASE_0, 0x90 */ - uint crc_pllm_out; /* _PLLM_OUT_0, 0x94 */ - uint crc_reserved4; /* reserved_4, 0x98 */ - uint crc_pllm_misc; /* _PLLM_MISC_0, 0x9C */ - - uint crc_pllp_base; /* _PLLP_BASE_0, 0xA0 */ - uint crc_pllp_outa; /* _PLLP_OUTA_0, 0xA4 */ - uint crc_pllp_outb; /* _PLLP_OUTB_0, 0xA8 */ - uint crc_pllp_misc; /* _PLLP_MISC_0, 0xAC */ - - uint crc_plla_base; /* _PLLA_BASE_0, 0xB0 */ - uint crc_plla_out; /* _PLLA_OUT_0, 0xB4 */ - uint crc_reserved5; /* reserved_5, 0xB8 */ - uint crc_plla_misc; /* _PLLA_MISC_0, 0xBC */ - - uint crc_pllu_base; /* _PLLU_BASE_0, 0xC0 */ - uint crc_reserved6; /* _reserved_6, 0xC4 */ - uint crc_reserved7; /* _reserved_7, 0xC8 */ - uint crc_pllu_misc; /* _PLLU_MISC_0, 0xCC */ - - uint crc_plld_base; /* _PLLD_BASE_0, 0xD0 */ - uint crc_reserved8; /* _reserved_8, 0xD4 */ - uint crc_reserved9; /* _reserved_9, 0xD8 */ - uint crc_plld_misc; /* _PLLD_MISC_0, 0xDC */ - - uint crc_pllx_base; /* _PLLX_BASE_0, 0xE0 */ - uint crc_pllx_misc; /* _PLLX_MISC_0, 0xE4 */ + struct clk_pll crc_pll[TEGRA_CLK_PLLS]; /* PLLs from 0x80 to 0xdc */
- uint crc_plle_base; /* _PLLE_BASE_0, 0xE8 */ - uint crc_plle_misc; /* _PLLE_MISC_0, 0xEC */ + /* PLLs from 0xe0 to 0xf4 */ + struct clk_pll_simple crc_pll_simple[TEGRA_CLK_SIMPLE_PLLS];
- uint crc_plls_base; /* _PLLS_BASE_0, 0xF0 */ - uint crc_plls_misc; /* _PLLS_MISC_0, 0xF4 */ uint crc_reserved10; /* _reserved_10, 0xF8 */ uint crc_reserved11; /* _reserved_11, 0xFC */
@@ -157,8 +145,8 @@ struct clk_rst_ctlr { #define PLL_BYPASS (1 << 31) #define PLL_ENABLE (1 << 30) #define PLL_BASE_OVRRIDE (1 << 28) -#define PLL_DIVP (1 << 20) /* post divider, b22:20 */ -#define PLL_DIVM 0x0C /* input divider, b4:0 */ +#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) @@ -191,4 +179,51 @@ struct clk_rst_ctlr {
#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) + +#define CPU0_CLK_STP_BITS 8 : 8 +#define CPU0_CLK_STP_SHIFT bf_shift(CPU0_CLK_STP_BITS) +#define CPU0_CLK_STP_MASK bf_mask(CPU0_CLK_STP_BITS) + +/* CLK_RST_CONTROLLER_PLLx_BASE_0 */ +#define PLL_BYPASS_BITS 31 : 31 +#define PLL_BYPASS_MASK bf_mask(PLL_BYPASS_BITS) +#define PLL_BYPASS_SHIFT bf_shift(PLL_BYPASS_BITS) + +#define PLL_ENABLE_BITS 30 : 30 +#define PLL_ENABLE_MASK bf_mask(PLL_ENABLE_BITS) +#define PLL_ENABLE_SHIFT bf_shift(PLL_ENABLE_BITS) + +#define PLL_BASE_OVRRIDE_BITS 28 : 28 +#define PLL_BASE_OVRRIDE_MASK bf_mask(PLL_BASE_OVRRIDE_BITS) + +#define PLL_DIVP_BITS 22 : 20 +#define PLL_DIVP_SHIFT bf_shift(PLL_DIVP_BITS) + +#define PLL_DIVN_BITS 17 : 8 +#define PLL_DIVN_SHIFT bf_shift(PLL_DIVN_BITS) + +#define PLL_DIVM_BITS 4 : 0 +#define PLL_DIVM_SHIFT bf_shift(PLL_DIVM_BITS) + +/* CLK_RST_CONTROLLER_PLLx_MISC_0 */ +#define PLL_CPCON_BITS 11 : 8 +#define PLL_CPCON_MASK bf_mask(PLL_CPCON_BITS) +#define PLL_CPCON_SHIFT bf_shift(PLL_CPCON_BITS) + +#define PLL_LFCON_BITS 7 : 4 +#define PLL_LFCON_SHIFT bf_shift(PLL_LFCON_BITS) + +#define PLLU_VCO_FREQ_BITS 20 : 20 +#define PLLU_VCO_FREQ_SHIFT bf_shift(PLLU_VCO_FREQ_BITS) + +#define PLL_VCO_FREQ_BITS 3 : 0 + +/* CLK_RST_CONTROLLER_OSC_CTRL_0 */ +#define OSC_FREQ_BITS 31 : 30 +#define OSC_FREQ_SHIFT bf_shift(OSC_FREQ_BITS) +#define OSC_FREQ_MASK bf_mask(OSC_FREQ_BITS) + #endif /* CLK_RST_H */ diff --git a/arch/arm/include/asm/arch-tegra2/clock.h b/arch/arm/include/asm/arch-tegra2/clock.h new file mode 100644 index 0000000..81a9f02 --- /dev/null +++ b/arch/arm/include/asm/arch-tegra2/clock.h @@ -0,0 +1,264 @@ +/* + * 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 clock control functions */ + +#ifndef _CLOCK_H + + +/* Set of oscillator frequencies supported in the internal API. */ +enum clock_osc_freq { + /* All in MHz, so 13_0 is 13.0MHz */ + CLOCK_OSC_FREQ_13_0, + CLOCK_OSC_FREQ_19_2, + CLOCK_OSC_FREQ_12_0, + CLOCK_OSC_FREQ_26_0, + + CLOCK_OSC_FREQ_COUNT, +}; + +/* The PLLs supported by the hardware */ +enum clock_pll_id { + CLOCK_PLL_ID_FIRST, + CLOCK_PLL_ID_CGENERAL = CLOCK_PLL_ID_FIRST, + CLOCK_PLL_ID_MEMORY, + CLOCK_PLL_ID_PERIPH, + CLOCK_PLL_ID_AUDIO, + CLOCK_PLL_ID_USB, + CLOCK_PLL_ID_DISPLAY, + + /* now the simple ones */ + CLOCK_PLL_ID_FIRST_SIMPLE, + CLOCK_PLL_ID_XCPU = CLOCK_PLL_ID_FIRST_SIMPLE, + CLOCK_PLL_ID_EPCI, + CLOCK_PLL_ID_SFROM32KHZ, + + CLOCK_PLL_ID_COUNT, +}; + +/* The clocks supported by the hardware */ +enum periph_id { + PERIPH_ID_FIRST, + + /* Low word: 31:0 */ + PERIPH_ID_CPU = PERIPH_ID_FIRST, + PERIPH_ID_RESERVED1, + PERIPH_ID_RESERVED2, + PERIPH_ID_AC97, + PERIPH_ID_RTC, + PERIPH_ID_TMR, + PERIPH_ID_UART1, + PERIPH_ID_UART2, + + /* 8 */ + PERIPH_ID_GPIO, + PERIPH_ID_SDMMC2, + PERIPH_ID_SPDIF, + PERIPH_ID_I2S1, + PERIPH_ID_I2C1, + PERIPH_ID_NDFLASH, + PERIPH_ID_SDMMC1, + PERIPH_ID_SDMMC4, + + /* 16 */ + PERIPH_ID_TWC, + PERIPH_ID_PWC, + PERIPH_ID_I2S2, + PERIPH_ID_EPP, + PERIPH_ID_VI, + PERIPH_ID_2D, + PERIPH_ID_USBD, + PERIPH_ID_ISP, + + /* 24 */ + PERIPH_ID_3D, + PERIPH_ID_IDE, + PERIPH_ID_DISP2, + PERIPH_ID_DISP1, + PERIPH_ID_HOST1X, + PERIPH_ID_VCP, + PERIPH_ID_RESERVED30, + PERIPH_ID_CACHE2, + + /* Middle word: 63:32 */ + PERIPH_ID_MEM, + PERIPH_ID_AHBDMA, + PERIPH_ID_APBDMA, + PERIPH_ID_RESERVED35, + PERIPH_ID_KBC, + PERIPH_ID_STAT_MON, + PERIPH_ID_PMC, + PERIPH_ID_FUSE, + + /* 40 */ + PERIPH_ID_KFUSE, + PERIPH_ID_SBC1, + PERIPH_ID_SNOR, + PERIPH_ID_SPI1, + PERIPH_ID_SBC2, + PERIPH_ID_XIO, + PERIPH_ID_SBC3, + PERIPH_ID_DVC_I2C, + + /* 48 */ + PERIPH_ID_DSI, + PERIPH_ID_TVO, + PERIPH_ID_MIPI, + PERIPH_ID_HDMI, + PERIPH_ID_CSI, + PERIPH_ID_TVDAC, + PERIPH_ID_I2C2, + PERIPH_ID_UART3, + + /* 56 */ + PERIPH_ID_RESERVED56, + PERIPH_ID_EMC, + PERIPH_ID_USB2, + PERIPH_ID_USB3, + PERIPH_ID_MPE, + PERIPH_ID_VDE, + PERIPH_ID_BSEA, + PERIPH_ID_BSEV, + + /* Upper word 95:64 */ + PERIPH_ID_SPEEDO, + PERIPH_ID_UART4, + PERIPH_ID_UART5, + PERIPH_ID_I2C3, + PERIPH_ID_SBC4, + PERIPH_ID_SDMMC3, + PERIPH_ID_PCIE, + PERIPH_ID_OWR, + + /* 72 */ + PERIPH_ID_AFI, + PERIPH_ID_CORESIGHT, + PERIPH_ID_RESERVED74, + PERIPH_ID_AVPUCQ, + PERIPH_ID_RESERVED76, + PERIPH_ID_RESERVED77, + PERIPH_ID_RESERVED78, + PERIPH_ID_RESERVED79, + + /* 80 */ + PERIPH_ID_RESERVED80, + PERIPH_ID_RESERVED81, + PERIPH_ID_RESERVED82, + PERIPH_ID_RESERVED83, + PERIPH_ID_IRAMA, + PERIPH_ID_IRAMB, + PERIPH_ID_IRAMC, + PERIPH_ID_IRAMD, + + /* 88 */ + PERIPH_ID_CRAM2, + + PERIPH_ID_COUNT, +}; + +/* Converts a clock number to a clock register: 0=L, 1=H, 2=U */ +#define PERIPH_REG(id) ((id) >> 5) + +/* Mask value for a clock (within PERIPH_REG(id)) */ +#define PERIPH_MASK(id) (1 << ((id) & 0x1f)) + +/* return 1 if a PLL ID is in range */ +#define clock_pll_id_isvalid(id) ((id) >= CLOCK_PLL_ID_FIRST && \ + (id) < CLOCK_PLL_ID_COUNT) + +/* return 1 if a peripheral ID is in range */ +#define clock_periph_id_isvalid(id) ((id) >= PERIPH_ID_FIRST && \ + (id) < PERIPH_ID_COUNT) + +/* PLL stabilization delay in usec */ +#define CLOCK_PLL_STABLE_DELAY_US 300 + +/* return the current oscillator clock frequency */ +enum clock_osc_freq clock_get_osc_freq(void); + +/* + * Start PLL using the provided configuration parameters. + * + * @param id clock id + * @param divm input divider + * @param divn feedback divider + * @param divp post divider 2^n + * @param cpcon charge pump setup control + * @param lfcon loop filter setup control + * + * @returns monotonic time in us that the PLL will be stable + */ +unsigned long clock_start_pll(enum clock_pll_id id, u32 divm, u32 divn, + u32 divp, u32 cpcon, u32 lfcon); + +/* + * Enable a clock + * + * @param id clock id + */ +void clock_enable(enum periph_id clkid); + +/* + * Set whether a clock is enabled or disabled. + * + * @param id clock id + * @param enable 1 to enable, 0 to disable + */ +void clock_set_enable(enum periph_id clkid, int enable); + +/* + * Reset a peripheral. This puts it in reset, waits for a delay, then takes + * it out of reset and waits for th delay again. + * + * @param periph_id peripheral to reset + * @param us_delay time to delay in microseconds + */ +void reset_periph(enum periph_id periph_id, int us_delay); + +/* + * Put a peripheral into or out of reset. + * + * @param periph_id peripheral to reset + * @param enable 1 to put into reset, 0 to take out of reset + */ +void reset_set_enable(enum periph_id periph_id, int enable); + + +/* CLK_RST_CONTROLLER_RST_CPU_CMPLX_SET/CLR_0 */ +enum crc_reset_id { + /* Things we can hold in reset for each CPU */ + crc_rst_cpu = 1, + crc_rst_de = 1 << 2, /* What is de? */ + crc_rst_watchdog = 1 << 3, + crc_rst_debug = 1 << 4, +}; + +/* + * Put parts of the CPU complex into or out of reset.\ + * + * @param cpu cpu number (0 or 1 on Tegra2) + * @param which which parts of the complex to affect (OR of crc_reset_id) + * @param reset 1 to assert reset, 0 to de-assert + */ +void reset_cmplx_set_enable(int cpu, int which, int reset); + +#endif + diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index 3d6c248..fa8243f 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -28,6 +28,7 @@ #include <asm/arch/sys_proto.h>
#include <asm/arch/clk_rst.h> +#include <asm/arch/clock.h> #include <asm/arch/pinmux.h> #include <asm/arch/uart.h> #include "board.h" @@ -73,33 +74,28 @@ int timer_init(void) static void clock_init_uart(void) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; + struct clk_pll *pll = &clkrst->crc_pll[CLOCK_PLL_ID_PERIPH]; u32 reg;
- reg = readl(&clkrst->crc_pllp_base); + reg = readl(&pll->pll_base); if (!(reg & PLL_BASE_OVRRIDE)) { /* Override pllp setup for 216MHz operation. */ - reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP); - reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM); - writel(reg, &clkrst->crc_pllp_base); + reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP_VALUE); + reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM_VALUE); + writel(reg, &pll->pll_base);
reg |= PLL_ENABLE; - writel(reg, &clkrst->crc_pllp_base); + writel(reg, &pll->pll_base);
reg &= ~PLL_BYPASS; - writel(reg, &clkrst->crc_pllp_base); + writel(reg, &pll->pll_base); }
/* Now do the UART reset/clock enable */ #if defined(CONFIG_TEGRA2_ENABLE_UARTA) - /* Assert Reset to UART */ - reg = readl(&clkrst->crc_rst_dev_l); - reg |= SWR_UARTA_RST; /* SWR_UARTA_RST = 1 */ - writel(reg, &clkrst->crc_rst_dev_l); - - /* Enable clk to UART */ - reg = readl(&clkrst->crc_clk_out_enb_l); - reg |= CLK_ENB_UARTA; /* CLK_ENB_UARTA = 1 */ - writel(reg, &clkrst->crc_clk_out_enb_l); + /* Assert UART reset and enable clock */ + reset_set_enable(PERIPH_ID_UART1, 1); + clock_enable(PERIPH_ID_UART1);
/* Enable pllp_out0 to UART */ reg = readl(&clkrst->crc_clk_src_uarta); @@ -110,20 +106,12 @@ static void clock_init_uart(void) udelay(2);
/* De-assert reset to UART */ - reg = readl(&clkrst->crc_rst_dev_l); - reg &= ~SWR_UARTA_RST; /* SWR_UARTA_RST = 0 */ - writel(reg, &clkrst->crc_rst_dev_l); + reset_set_enable(PERIPH_ID_UART1, 0); #endif /* CONFIG_TEGRA2_ENABLE_UARTA */ #if defined(CONFIG_TEGRA2_ENABLE_UARTD) - /* Assert Reset to UART */ - reg = readl(&clkrst->crc_rst_dev_u); - reg |= SWR_UARTD_RST; /* SWR_UARTD_RST = 1 */ - writel(reg, &clkrst->crc_rst_dev_u); - - /* Enable clk to UART */ - reg = readl(&clkrst->crc_clk_out_enb_u); - reg |= CLK_ENB_UARTD; /* CLK_ENB_UARTD = 1 */ - writel(reg, &clkrst->crc_clk_out_enb_u); + /* Assert UART reset and enable clock */ + reset_set_enable(PERIPH_ID_UART4, 1); + clock_enable(PERIPH_ID_UART4);
/* Enable pllp_out0 to UART */ reg = readl(&clkrst->crc_clk_src_uartd); @@ -134,9 +122,7 @@ static void clock_init_uart(void) udelay(2);
/* De-assert reset to UART */ - reg = readl(&clkrst->crc_rst_dev_u); - reg &= ~SWR_UARTD_RST; /* SWR_UARTD_RST = 0 */ - writel(reg, &clkrst->crc_rst_dev_u); + reset_set_enable(PERIPH_ID_UART4, 0); #endif /* CONFIG_TEGRA2_ENABLE_UARTD */ }
This adds an enum for each pin and some functions for changing the pin muxing setup.
Signed-off-by: Simon Glass sjg@chromium.org --- 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
diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile index b35764c..f673f03 100644 --- a/arch/arm/cpu/armv7/tegra2/Makefile +++ b/arch/arm/cpu/armv7/tegra2/Makefile @@ -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)) diff --git a/arch/arm/cpu/armv7/tegra2/pinmux.c b/arch/arm/cpu/armv7/tegra2/pinmux.c new file mode 100644 index 0000000..4d35172 --- /dev/null +++ b/arch/arm/cpu/armv7/tegra2/pinmux.c @@ -0,0 +1,53 @@ +/* + * 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 pin multiplexing functions */ + +#include <asm/io.h> +#include <asm/arch/tegra2.h> +#include <asm/arch/pinmux.h> +#include <common.h> + + +void pinmux_set_tristate(enum pmux_pin pin, int enable) +{ + struct pmux_tri_ctlr *pmt = (struct pmux_tri_ctlr *)NV_PA_APB_MISC_BASE; + u32 *tri = &pmt->pmt_tri[TRISTATE_REG(pin)]; + u32 reg; + + reg = readl(tri); + if (enable) + reg |= TRISTATE_MASK(pin); + else + reg &= ~TRISTATE_MASK(pin); + writel(reg, tri); +} + +void pinmux_tristate_enable(enum pmux_pin pin) +{ + pinmux_set_tristate(pin, 1); +} + +void pinmux_tristate_disable(enum pmux_pin pin) +{ + pinmux_set_tristate(pin, 0); +} + diff --git a/arch/arm/include/asm/arch-tegra2/pinmux.h b/arch/arm/include/asm/arch-tegra2/pinmux.h index 8b4bd8d..b8a4753 100644 --- a/arch/arm/include/asm/arch-tegra2/pinmux.h +++ b/arch/arm/include/asm/arch-tegra2/pinmux.h @@ -24,6 +24,142 @@ #ifndef _PINMUX_H_ #define _PINMUX_H_
+/* Pins which we can set to tristate or normal */ +enum pmux_pin { + /* APB_MISC_PP_TRISTATE_REG_A_0 */ + PIN_ATA, + PIN_ATB, + PIN_ATC, + PIN_ATD, + PIN_CDEV1, + PIN_CDEV2, + PIN_CSUS, + PIN_DAP1, + + PIN_DAP2, + PIN_DAP3, + PIN_DAP4, + PIN_DTA, + PIN_DTB, + PIN_DTC, + PIN_DTD, + PIN_DTE, + + PIN_GPU, + PIN_GPV, + PIN_I2CP, + PIN_IRTX, + PIN_IRRX, + PIN_KBCB, + PIN_KBCA, + PIN_PMC, + + PIN_PTA, + PIN_RM, + PIN_KBCE, + PIN_KBCF, + PIN_GMA, + PIN_GMC, + PIN_SDMMC1, + PIN_OWC, + + /* 32: APB_MISC_PP_TRISTATE_REG_B_0 */ + PIN_GME, + PIN_SDC, + PIN_SDD, + PIN_RESERVED0, + PIN_SLXA, + PIN_SLXC, + PIN_SLXD, + PIN_SLXK, + + PIN_SPDI, + PIN_SPDO, + PIN_SPIA, + PIN_SPIB, + PIN_SPIC, + PIN_SPID, + PIN_SPIE, + PIN_SPIF, + + PIN_SPIG, + PIN_SPIH, + PIN_UAA, + PIN_UAB, + PIN_UAC, + PIN_UAD, + PIN_UCA, + PIN_UCB, + + PIN_RESERVED1, + PIN_ATE, + PIN_KBCC, + PIN_RESERVED2, + PIN_RESERVED3, + PIN_GMB, + PIN_GMD, + PIN_DDC, + + /* 64: APB_MISC_PP_TRISTATE_REG_C_0 */ + PIN_LD0, + PIN_LD1, + PIN_LD2, + PIN_LD3, + PIN_LD4, + PIN_LD5, + PIN_LD6, + PIN_LD7, + + PIN_LD8, + PIN_LD9, + PIN_LD10, + PIN_LD11, + PIN_LD12, + PIN_LD13, + PIN_LD14, + PIN_LD15, + + PIN_LD16, + PIN_LD17, + PIN_LHP0, + PIN_LHP1, + PIN_LHP2, + PIN_LVP0, + PIN_LVP1, + PIN_HDINT, + + PIN_LM0, + PIN_LM1, + PIN_LVS, + PIN_LSC0, + PIN_LSC1, + PIN_LSCK, + PIN_LDC, + PIN_LCSN, + + /* 96: APB_MISC_PP_TRISTATE_REG_D_0 */ + PIN_LSPI, + PIN_LSDA, + PIN_LSDI, + PIN_LPW0, + PIN_LPW1, + PIN_LPW2, + PIN_LDI, + PIN_LHS, + + PIN_LPP, + PIN_RESERVED4, + PIN_KBCD, + PIN_GPU7, + PIN_DTF, + PIN_UDA, + PIN_CRTP, + PIN_SDB, +}; + + +#define TEGRA_TRISTATE_REGS 4 + /* APB MISC Pin Mux and Tristate (APB_MISC_PP_) registers */ struct pmux_tri_ctlr { uint pmt_reserved0; /* ABP_MISC_PP_ reserved offset 00 */ @@ -31,10 +167,7 @@ struct pmux_tri_ctlr { uint pmt_strap_opt_a; /* _STRAPPING_OPT_A_0, offset 08 */ uint pmt_reserved2; /* ABP_MISC_PP_ reserved offset 0C */ uint pmt_reserved3; /* ABP_MISC_PP_ reserved offset 10 */ - uint pmt_tri_a; /* _TRI_STATE_REG_A_0, offset 14 */ - uint pmt_tri_b; /* _TRI_STATE_REG_B_0, offset 18 */ - uint pmt_tri_c; /* _TRI_STATE_REG_C_0, offset 1C */ - uint pmt_tri_d; /* _TRI_STATE_REG_D_0, offset 20 */ + uint pmt_tri[TEGRA_TRISTATE_REGS]; /* _TRI_STATE_REG_A/B/C/D_0 14-20 */ uint pmt_cfg_ctl; /* _CONFIG_CTL_0, offset 24 */
uint pmt_reserved[22]; /* ABP_MISC_PP_ reserved offs 28-7C */ @@ -48,8 +181,16 @@ struct pmux_tri_ctlr { uint pmt_ctl_g; /* _PIN_MUX_CTL_G_0, offset 98 */ };
-#define Z_GMC (1 << 29) -#define Z_IRRX (1 << 20) -#define Z_IRTX (1 << 19) +/* Converts a pin number to a tristate register: 0=A, 1=B, 2=C, 3=D */ +#define TRISTATE_REG(id) ((id) >> 5) + +/* Mask value for a tristate (within TRISTATE_REG(id)) */ +#define TRISTATE_MASK(id) (1 << ((id) & 0x1f)) + +/* Set a pin to tristate */ +void pinmux_tristate_enable(enum pmux_pin pin); + +/* Set a pin to normal (non tristate) */ +void pinmux_tristate_disable(enum pmux_pin pin);
#endif /* PINMUX_H */ diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index fa8243f..480b11b 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -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 */ }
Signed-off-by: Simon Glass sjg@chromium.org --- 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(-)
diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c b/arch/arm/cpu/armv7/tegra2/ap20.c index e3832e2..dc5f984 100644 --- a/arch/arm/cpu/armv7/tegra2/ap20.c +++ b/arch/arm/cpu/armv7/tegra2/ap20.c @@ -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); diff --git a/arch/arm/include/asm/arch-tegra2/clk_rst.h b/arch/arm/include/asm/arch-tegra2/clk_rst.h index 6c74410..e64fc93 100644 --- a/arch/arm/include/asm/arch-tegra2/clk_rst.h +++ b/arch/arm/include/asm/arch-tegra2/clk_rst.h @@ -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) diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index 480b11b..2007d13 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -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);
participants (7)
-
Albert ARIBAUD -
Anton Staaf -
Anton Staaf -
Detlev Zundel -
Graeme Russ -
Simon Glass -
Wolfgang Denk