[U-Boot] [RESEND PATCH v2 0/5] Add basic clock and pinmux functions to the Tegra2

[U-Boot] [RESEND PATCH v2 0/5] Add basic clock and pinmux functions to the Tegra2

[Simon Glass]

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

-- 
1.7.3.1

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Simon Glass]

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
-- 
1.7.3.1

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Albert ARIBAUD]

Hi Simon,

Le 05/07/2011 18:49, Simon Glass a ?crit :
> Signed-off-by: Simon Glass<sjg@chromium.org>
> ---
> Changes in v2:
> - Removed all bitfield access macros

Not sure I follow you: the added lines below do indeed add bitfield 
access macros, don't they?

>   arch/arm/include/asm/arch-tegra2/bitfield.h |   96 +++++++++++++++++++++++++++
>   1 files changed, 96 insertions(+), 0 deletions(-)
>   create mode 100644 arch/arm/include/asm/arch-tegra2/bitfield.h
>
> diff --git a/arch/arm/include/asm/arch-tegra2/bitfield.h b/arch/arm/include/asm/arch-tegra2/bitfield.h
> new file mode 100644
> index 0000000..494087c
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-tegra2/bitfield.h
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef __TEGRA2_BITFIELD_H
> +#define __TEGRA2_BITFIELD_H
> +
> +/*
> + * Basic macros for easily getting mask and shift values for bit fields on
> + * ARM.
> + *
> + * You use these to reliably create shifts and masks from a bit field
> + * definition. Bit fields are defined like this:
> + *
> + * #define NAME_BITS	MSB : LSB
> + *
> + * where MSB is the most significant bit, and LSB the least sig, bit. This
> + * notation is chosen since it is commonly used in CPU / SOC datasheets.
> + *
> + * For example:
> + *
> + * #define UART_FBCON_BITS  5:3		Bit range for the FBCON field
> + *
> + * Note that if you are using a big-endian machine there is no consistent
> + * notion of big numbers, since bit 3 is a different bit depending on whether
> + * the access is 32-bits or 64-bits. For this reason these macros should not
> + * be used as it would probably be too confusing to have to specify your
> + * access width all the time.
> + *
> + * This defines a bit field extending between bits 3 and 5.
> + *
> + * Then in your header file you can set up the shift and mask like this:
> + *
> + *	 #define UART_FBCON_SHIFT	bf_shift(UART_FBCON)
> + *	 #define UART_FBCON_MASK	bf_mask(UART_FBCON)
> + *
> + * Then you can use these macros in your code (there is no bitfield support
> + * in the C file and these macros MUST NOT be used directly in C code).
> + *
> + * To write, overwriting other fields too:
> + *	writel(3<<  UART_FBCON_SHIFT,&uart->fbcon);
> + *
> + * To read:
> + *	int fbcon = (readl(&uart->fbcon)&  UART_FBCON_MASK)>>
> + *		UART_FBCON_SHIFT;
> + *
> + * To update just this field (for example):
> + *	clrsetbits_le32(&uart->fbcon, UART_FBCON_MASK, 3<<  UART_FBCON_SHIFT);
> + */

What's the benefit of this over directly specifying a shift and mask 
value, e.g. #define UART_FBCON_SHIFT 3 and #define UART_FBCON_MASK 0x7 
and doing shifts and ANDs? I don't see this as simpler or more intuitive.

> +#include "compiler.h"
> +
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +
> +/* Returns the bit number of the LSB */
> +#define _LSB(range)	((0 ? range)&  0x1f)
> +
> +/* Returns the bit number of the MSB */
> +#define _MSB(range)	(1 ? range)
> +
> +/* Returns the width of the bitfield (in bits) */
> +#define _BITFIELD_WIDTH(range)	(_MSB(range) - _LSB(range) + 1)
> +
> +
> +/*
> + * Returns the number of bits the bitfield needs to be shifted left to pack it.
> + * This is just the same as the low bit.
> + */
> +#define bf_shift(field)		_LSB(field)
> +
> +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0) */
> +#define bf_rawmask(field)	((1UL<<  _BITFIELD_WIDTH(field)) - 1)
> +
> +/* Returns the mask for a field. Clear these bits to zero the field */
> +#define bf_mask(field)		(bf_rawmask(field)<<  (bf_shift(field)))
> +
> +#endif /* __BYTE_ORDER == __LITTLE_ENDIAN */
> +
> +#endif

Amicalement,
-- 
Albert.

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Simon Glass]

Hi Albert,

On Sat, Jul 9, 2011 at 6:56 AM, Albert ARIBAUD <albert.u.boot@aribaud.net>wrote:

> Hi Simon,
>
> Le 05/07/2011 18:49, Simon Glass a ?crit :
>
>  Signed-off-by: Simon Glass<sjg@chromium.org>
>> ---
>> Changes in v2:
>> - Removed all bitfield access macros
>>
>
> Not sure I follow you: the added lines below do indeed add bitfield access
> macros, don't they?
>
> No these are just for defining the shifts and masks. There is no access
going on through macros, either IO or normal variables. Everything uses
manual shifts and adds, and the IO uses writel/readl/clrsetbits_le32().
Please check the follow-on patches to see what I mean.

>
>   arch/arm/include/asm/arch-**tegra2/bitfield.h |   96
>> +++++++++++++++++++++++++++
>>  1 files changed, 96 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/include/asm/arch-**tegra2/bitfield.h
>>
>> diff --git a/arch/arm/include/asm/arch-**tegra2/bitfield.h
>> b/arch/arm/include/asm/arch-**tegra2/bitfield.h
>> new file mode 100644
>> index 0000000..494087c
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-**tegra2/bitfield.h
>> @@ -0,0 +1,96 @@
>> +/*
>> + * Copyright (c) 2011 The Chromium OS Authors.
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#ifndef __TEGRA2_BITFIELD_H
>> +#define __TEGRA2_BITFIELD_H
>> +
>> +/*
>> + * Basic macros for easily getting mask and shift values for bit fields
>> on
>> + * ARM.
>> + *
>> + * You use these to reliably create shifts and masks from a bit field
>> + * definition. Bit fields are defined like this:
>> + *
>> + * #define NAME_BITS   MSB : LSB
>> + *
>> + * where MSB is the most significant bit, and LSB the least sig, bit.
>> This
>> + * notation is chosen since it is commonly used in CPU / SOC datasheets.
>> + *
>> + * For example:
>> + *
>> + * #define UART_FBCON_BITS  5:3                Bit range for the FBCON
>> field
>> + *
>> + * Note that if you are using a big-endian machine there is no consistent
>> + * notion of big numbers, since bit 3 is a different bit depending on
>> whether
>> + * the access is 32-bits or 64-bits. For this reason these macros should
>> not
>> + * be used as it would probably be too confusing to have to specify your
>> + * access width all the time.
>> + *
>> + * This defines a bit field extending between bits 3 and 5.
>> + *
>> + * Then in your header file you can set up the shift and mask like this:
>> + *
>> + *      #define UART_FBCON_SHIFT       bf_shift(UART_FBCON)
>> + *      #define UART_FBCON_MASK        bf_mask(UART_FBCON)
>> + *
>> + * Then you can use these macros in your code (there is no bitfield
>> support
>> + * in the C file and these macros MUST NOT be used directly in C code).
>> + *
>> + * To write, overwriting other fields too:
>> + *     writel(3<<  UART_FBCON_SHIFT,&uart->fbcon)**;
>> + *
>> + * To read:
>> + *     int fbcon = (readl(&uart->fbcon)&  UART_FBCON_MASK)>>
>> + *             UART_FBCON_SHIFT;
>> + *
>> + * To update just this field (for example):
>> + *     clrsetbits_le32(&uart->fbcon, UART_FBCON_MASK, 3<<
>>  UART_FBCON_SHIFT);
>> + */
>>
>
> What's the benefit of this over directly specifying a shift and mask value,
> e.g. #define UART_FBCON_SHIFT 3 and #define UART_FBCON_MASK 0x7 and doing
> shifts and ANDs? I don't see this as simpler or more intuitive.


The only benefit is to avoid having to calculate all of these masks and
shifts in your head. It is basically just a shortcut and assists with
checking code against datasheets. Of course I would prefer to have access
through macros also but that was shot down so the code is now full of manual
shifts and ANDs. I hope that at least this small convenience will be
acceptable.

Regards,
Simon


>
>
>  +#include "compiler.h"
>> +
>> +#if __BYTE_ORDER == __LITTLE_ENDIAN
>> +
>> +/* Returns the bit number of the LSB */
>> +#define _LSB(range)    ((0 ? range)&  0x1f)
>> +
>> +/* Returns the bit number of the MSB */
>> +#define _MSB(range)    (1 ? range)
>> +
>> +/* Returns the width of the bitfield (in bits) */
>> +#define _BITFIELD_WIDTH(range) (_MSB(range) - _LSB(range) + 1)
>> +
>> +
>> +/*
>> + * Returns the number of bits the bitfield needs to be shifted left to
>> pack it.
>> + * This is just the same as the low bit.
>> + */
>> +#define bf_shift(field)                _LSB(field)
>> +
>> +/* Returns the unshifted mask for the field (i.e. LSB of mask is bit 0)
>> */
>> +#define bf_rawmask(field)      ((1UL<<  _BITFIELD_WIDTH(field)) - 1)
>> +
>> +/* Returns the mask for a field. Clear these bits to zero the field */
>> +#define bf_mask(field)         (bf_rawmask(field)<<  (bf_shift(field)))
>> +
>> +#endif /* __BYTE_ORDER == __LITTLE_ENDIAN */
>> +
>> +#endif
>>
>
> Amicalement,
> --
> Albert.
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1Oymaa2_gQGxw88jJG2Kr_fN6tJ5HDgUiOHPjw7jX2=g@mail.gmail.com> you wrote:
>
> > Not sure I follow you: the added lines below do indeed add bitfield access
> > macros, don't they?
> >
> > No these are just for defining the shifts and masks. There is no access

But you write yourself in the comment: "...easily getting mask and
shift values for bit fields".

> The only benefit is to avoid having to calculate all of these masks and
> shifts in your head. It is basically just a shortcut and assists with
> checking code against datasheets. Of course I would prefer to have access
> through macros also but that was shot down so the code is now full of manual
> shifts and ANDs. I hope that at least this small convenience will be
> acceptable.

Sorry, but because such code is unportable we do not accept it, as it
would lead to driver code that becomes unportable, too.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In the pitiful, multipage, connection-boxed form to which  the  flow-
chart  has  today  been  elaborated, it has proved to be useless as a
design tool -- programmers draw flowcharts after, not before, writing
the programs they describe.                        - Fred Brooks, Jr.

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

On Mon, Jul 11, 2011 at 1:16 AM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Simon Glass,
>
> In message <CAPnjgZ1Oymaa2_gQGxw88jJG2Kr_fN6tJ5HDgUiOHPjw7jX2=
> g at 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 at denx.de
> In the pitiful, multipage, connection-boxed form to which  the  flow-
> chart  has  today  been  elaborated, it has proved to be useless as a
> design tool -- programmers draw flowcharts after, not before, writing
> the programs they describe.                        - Fred Brooks, Jr.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Albert ARIBAUD]

Hi Anton,

Le 11/07/2011 18:19, Anton Staaf a ?crit :

> I know that this is throwing more fuel on the fire (for which I am sorry),
> 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,
-- 
Albert.

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

On Tue, Jul 12, 2011 at 8:29 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net>wrote:

> Hi Anton,
>
> Le 11/07/2011 18:19, Anton Staaf a ?crit :
>
>
>  I know that this is throwing more fuel on the fire (for which I am sorry),
>> but I don't follow the argument that this is unportable.  As far as I can
>> tell, the # : # syntax is not using any special compiler extensions, it is
>> simply substituted into a (boo) ? # : # expression, thus extracting either
>> the first of second number from the definition of the bit field.
>>
>> If I am wrong I would be interested to know what about this is not
>> standard
>> pre-processor usage?
>>
>
> For me at least (Wolfgang might have other reasons), the issue is not the
> use of the pre-processor per se, it is that this "syntax" breaks the '?:'
> "triadic" operator in pieces, one piece in argument value and one in macro
> body, and neither piece makes sense from a C standpoint: '5:3' represents no
> meaningful C entity, and 'X ? y' (without the ':' and third argument of the
> operator) is not a proper C construct.
>

Yes, this is absolutely true, and we can argue about readability vs.
usability vs. maintainability of such a construct (I like it, but I realize
that I might be in a minority here).  But it is certainly portable, there is
nothing compiler or pre-processor specific here.  I just wanted to clarify
that so that we could have the correct debate.  :)

IOW, it is syntactic sugaring done at the expense of code readability.
>

I would disagree here, it is indeed syntactic sugar, but I would assert that
it is done exactly to make the code more readable.  The one piece of macro
magic is in a single file that can be well documented and encapsulated.  I
find having one #define that specifies the entire range much easier to read
and debug than having two #defines, one for each end of the range that you
have to make sure are matched.  And this simplification can be applied to
many many files.  But that's what it really boils down to, what we each find
easier to read and debug.  I'm fine if the decision is that everyone else
doesn't like the way this reads, but let's not throw it out based on a
compatibility argument that is invalid.

Thanks,
    Anton

 Thanks,
>>     Anton
>>
>
> Amicalement,
> --
> Albert.
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"In Christianity neither morality nor religion come into contact with
reality at any point."                          - Friedrich Nietzsche

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

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 at 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 at denx.de
> "In Christianity neither morality nor religion come into contact with
> reality at any point."                          - Friedrich Nietzsche
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
News is what a chap who doesn't care much  about  anything  wants  to
read. And it's only news until he's read it. After that it's dead.
                           - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

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(&reg), SCFR1_IPS_DIV, 5), &reg)

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(&reg, 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 at denx.de
> News is what a chap who doesn't care much ?about ?anything ?wants ?to
> read. And it's only news until he's read it. After that it's dead.
> ? ? ? ? ? ? ? ? ? ? ? ? ? - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Detlev Zundel]

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

-- 
There are two hard things in computer science: cache invalidation,
naming things, and off-by-one errors.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

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 at denx.de
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

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.
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Our business is run on trust.  We trust you will pay in advance.

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

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 at denx.de
> Our business is run on trust. ?We trust you will pay in advance.
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Vulcans never bluff.
	-- Spock, "The Doomsday Machine", stardate 4202.1

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

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 at denx.de
> Vulcans never bluff.
> ? ? ? ?-- Spock, "The Doomsday Machine", stardate 4202.1
>

[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I wish Captain Vimes were here. He wouldn't have  known  what  to  do
either, but he's got a much better vocabulary to be baffled in.
                                 - Terry Pratchett, _Guards! Guards!_

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Simon Glass]

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
+
-- 
1.7.3.1

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Albert ARIBAUD]

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 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;
> +}

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,
-- 
Albert.

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Graeme Russ]

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();
>  		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);

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

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Simon Glass]

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
>

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Graeme Russ]

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 later
> 
> 
> Yes 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

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's a small world, but I wouldn't want to paint it.

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
People seldom know what they want until you give them what  they  ask
for.

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Graeme Russ]

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

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Wolfgang Denk]

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:
>  - http://www.denx.de/wiki/U-Boot/TaskTimerAPI

I see. Hm... assuming this is still under discussion, how do you
suggest to handle the wiki version?  For discussing things, it's
probably not really efficient if several people edit the pages.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The universe does not have laws - it has habits, and  habits  can  be
broken.

[U-Boot] [RESEND PATCH v2 2/5] Tegra2: Add microsecond timer functions

[Graeme Russ]

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:
>>  - http://www.denx.de/wiki/U-Boot/TaskTimerAPI
> 
> I see. Hm... assuming this is still under discussion, how do you
> suggest to handle the wiki version?  For discussing things, it's
> probably not really efficient if several people edit the pages.

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

[U-Boot] [RESEND PATCH v2 3/5] Tegra2: Add more clock support

[Simon Glass]

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 */
 }
 
-- 
1.7.3.1

[U-Boot] [RESEND PATCH v2 4/5] Tegra2: add additional pin multiplexing features

[Simon Glass]

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 */
 }
 
-- 
1.7.3.1

[U-Boot] [RESEND PATCH v2 5/5] Tegra2: Use clock and pinmux functions to simplify code

[Simon Glass]

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);
-- 
1.7.3.1