[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[Stephen Warren]

From: Tom Warren <twarren.nvidia@gmail.com>

The enum used to define the set of register bits used to represent a
clock's input mux, MUX_BITS_*, is defined separately for each SoC at
present. Move this definition to a common location to ease fixing up
some issues with the definition, and the code that uses it.

Signed-off-by: Tom Warren <twarren@nvidia.com>
[swarren, extracted from a larger patch by Tom]
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/cpu/tegra114-common/clock.c    | 6 ------
 arch/arm/cpu/tegra30-common/clock.c     | 6 ------
 arch/arm/include/asm/arch-tegra/clock.h | 6 ++++++
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/arm/cpu/tegra114-common/clock.c b/arch/arm/cpu/tegra114-common/clock.c
index 5c4305a418cc..47612e12d262 100644
--- a/arch/arm/cpu/tegra114-common/clock.c
+++ b/arch/arm/cpu/tegra114-common/clock.c
@@ -61,12 +61,6 @@ enum {
 	CLOCK_MAX_MUX   = 8     /* number of source options for each clock */
 };
 
-enum {
-	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
-	MASK_BITS_31_29,
-	MASK_BITS_29_28,
-};
-
 /*
  * Clock source mux for each clock type. This just converts our enum into
  * a list of mux sources for use by the code.
diff --git a/arch/arm/cpu/tegra30-common/clock.c b/arch/arm/cpu/tegra30-common/clock.c
index 74bd22be1aeb..89c3529c885b 100644
--- a/arch/arm/cpu/tegra30-common/clock.c
+++ b/arch/arm/cpu/tegra30-common/clock.c
@@ -60,12 +60,6 @@ enum {
 	CLOCK_MAX_MUX   = 8     /* number of source options for each clock */
 };
 
-enum {
-	MASK_BITS_31_30 = 2,    /* num of bits used to specify clock source */
-	MASK_BITS_31_29,
-	MASK_BITS_29_28,
-};
-
 /*
  * Clock source mux for each clock type. This just converts our enum into
  * a list of mux sources for use by the code.
diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h
index e7d0fd45ee1d..052c0208b18a 100644
--- a/arch/arm/include/asm/arch-tegra/clock.h
+++ b/arch/arm/include/asm/arch-tegra/clock.h
@@ -20,6 +20,12 @@ enum clock_osc_freq {
 	CLOCK_OSC_FREQ_COUNT,
 };
 
+enum {
+	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
+	MASK_BITS_31_29,
+	MASK_BITS_29_28,
+};
+
 #include <asm/arch/clock-tables.h>
 /* PLL stabilization delay in usec */
 #define CLOCK_PLL_STABLE_DELAY_US 300
-- 
1.8.1.5

[U-Boot] [PATCH 2/6] ARM: tegra: rename MASK_BITS_29_28 to MASK_BITS_31_28

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

The only place where the MASK_BITS_* values are used is in
adjust_periph_pll(), which interprets the value 4 (old MASK_BITS_29_28,
new MASK_BITS_31_28) as being associated with mask OUT_CLK_SOURCE4_MASK,
i.e. bits 31:28. Rename the MASK_BITS_ macro to reflect how it's actually
implemented.

Note that no Tegra clock register actually uses all of bits 31:28 as
the mux field. Rather, bits 30:28, 29:28, or 28 are used. However, in
those cases, nothing is stored in the bits about the mux field, so it's
safe to pretend that the mux field extends all the way to the end of the
register. As such, the U-Boot clock driver is currently a bit lazy, and
doesn't distinguish between 31:28, 30:28, 29:29 and 29; it just lumps
them all together and pretends they're all 31:28. This patch doesn't
cause this issue; it was pre-existing. Hopefully, future patches will
clean this up.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/cpu/tegra114-common/clock.c    |  2 +-
 arch/arm/cpu/tegra30-common/clock.c     |  2 +-
 arch/arm/include/asm/arch-tegra/clock.h | 11 ++++++++++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/tegra114-common/clock.c b/arch/arm/cpu/tegra114-common/clock.c
index 47612e12d262..3bede71a7a1f 100644
--- a/arch/arm/cpu/tegra114-common/clock.c
+++ b/arch/arm/cpu/tegra114-common/clock.c
@@ -103,7 +103,7 @@ static enum clock_id clock_source[CLOCK_TYPE_COUNT][CLOCK_MAX_MUX+1] = {
 		MASK_BITS_31_29},
 	{ CLK(PERIPH),	CLK(CGENERAL),	CLK(SFROM32KHZ),	CLK(OSC),
 		CLK(NONE),	CLK(NONE),	CLK(NONE),	CLK(NONE),
-		MASK_BITS_29_28}
+		MASK_BITS_31_28}
 };
 
 /*
diff --git a/arch/arm/cpu/tegra30-common/clock.c b/arch/arm/cpu/tegra30-common/clock.c
index 89c3529c885b..33528702185e 100644
--- a/arch/arm/cpu/tegra30-common/clock.c
+++ b/arch/arm/cpu/tegra30-common/clock.c
@@ -102,7 +102,7 @@ static enum clock_id clock_source[CLOCK_TYPE_COUNT][CLOCK_MAX_MUX+1] = {
 		MASK_BITS_31_29},
 	{ CLK(PERIPH),  CLK(CGENERAL),  CLK(SFROM32KHZ), CLK(OSC),
 		CLK(NONE),      CLK(NONE),      CLK(NONE),      CLK(NONE),
-		MASK_BITS_29_28}
+		MASK_BITS_31_28}
 };
 
 /*
diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h
index 052c0208b18a..cb89bd91f40c 100644
--- a/arch/arm/include/asm/arch-tegra/clock.h
+++ b/arch/arm/include/asm/arch-tegra/clock.h
@@ -20,10 +20,19 @@ enum clock_osc_freq {
 	CLOCK_OSC_FREQ_COUNT,
 };
 
+/*
+ * Note that no Tegra clock register actually uses all of bits 31:28 as
+ * the mux field. Rather, bits 30:28, 29:28, or 28 are used. However, in
+ * those cases, nothing is stored in the bits about the mux field, so it's
+ * safe to pretend that the mux field extends all the way to the end of the
+ * register. As such, the U-Boot clock driver is currently a bit lazy, and
+ * doesn't distinguish between 31:28, 30:28, 29:29 and 29; it just lumps
+ * them all together and pretends they're all 31:28.
+ */
 enum {
 	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
 	MASK_BITS_31_29,
-	MASK_BITS_29_28,
+	MASK_BITS_31_28,
 };
 
 #include <asm/arch/clock-tables.h>
-- 
1.8.1.5

[U-Boot] [PATCH 3/6] ARM: tegra: rename OUT_CLK_SOURCE_*

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

OUT_CLK_SOURCE_ are currently named after the number of bits the mask
they represent includes. However, bit count is not the only possible
variable; bit position may also vary. Rename OUT_CLK_SOURCE_ to
OUT_CLK_SOURCE_31_30_ and OUT_CLK_SOURCE4_ to OUT_CLK_SOURCE_31_28 to
more completely describe exactly what they represent, without having to
go look up the definitions.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/cpu/tegra-common/clock.c         | 16 ++++++++--------
 arch/arm/include/asm/arch-tegra/clk_rst.h |  9 +++++----
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/arm/cpu/tegra-common/clock.c b/arch/arm/cpu/tegra-common/clock.c
index 268fb912b502..d9f2c767d5d1 100644
--- a/arch/arm/cpu/tegra-common/clock.c
+++ b/arch/arm/cpu/tegra-common/clock.c
@@ -142,8 +142,8 @@ void clock_ll_set_source_divisor(enum periph_id periph_id, unsigned source,
 
 	value = readl(reg);
 
-	value &= ~OUT_CLK_SOURCE_MASK;
-	value |= source << OUT_CLK_SOURCE_SHIFT;
+	value &= ~OUT_CLK_SOURCE_31_30_MASK;
+	value |= source << OUT_CLK_SOURCE_31_30_SHIFT;
 
 	value &= ~OUT_CLK_DIVISOR_MASK;
 	value |= divisor << OUT_CLK_DIVISOR_SHIFT;
@@ -155,8 +155,8 @@ void clock_ll_set_source(enum periph_id periph_id, unsigned source)
 {
 	u32 *reg = get_periph_source_reg(periph_id);
 
-	clrsetbits_le32(reg, OUT_CLK_SOURCE_MASK,
-			source << OUT_CLK_SOURCE_SHIFT);
+	clrsetbits_le32(reg, OUT_CLK_SOURCE_31_30_MASK,
+			source << OUT_CLK_SOURCE_31_30_SHIFT);
 }
 
 /**
@@ -305,11 +305,11 @@ static int adjust_periph_pll(enum periph_id periph_id, int source,
 	if (source < 0)
 		return -1;
 	if (mux_bits == 4) {
-		clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK,
-			source << OUT_CLK_SOURCE4_SHIFT);
+		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_28_MASK,
+				source << OUT_CLK_SOURCE_31_28_SHIFT);
 	} else {
-		clrsetbits_le32(reg, OUT_CLK_SOURCE_MASK,
-			source << OUT_CLK_SOURCE_SHIFT);
+		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_30_MASK,
+				source << OUT_CLK_SOURCE_31_30_SHIFT);
 	}
 	udelay(2);
 	return 0;
diff --git a/arch/arm/include/asm/arch-tegra/clk_rst.h b/arch/arm/include/asm/arch-tegra/clk_rst.h
index 074b3bca0b4f..9f81237d2865 100644
--- a/arch/arm/include/asm/arch-tegra/clk_rst.h
+++ b/arch/arm/include/asm/arch-tegra/clk_rst.h
@@ -233,11 +233,12 @@ enum {
 #define OUT_CLK_DIVISOR_SHIFT	0
 #define OUT_CLK_DIVISOR_MASK	(0xffff << OUT_CLK_DIVISOR_SHIFT)
 
-#define OUT_CLK_SOURCE_SHIFT	30
-#define OUT_CLK_SOURCE_MASK	(3U << OUT_CLK_SOURCE_SHIFT)
+#define OUT_CLK_SOURCE_31_30_SHIFT	30
+#define OUT_CLK_SOURCE_31_30_MASK	(3U << OUT_CLK_SOURCE_31_30_SHIFT)
 
-#define OUT_CLK_SOURCE4_SHIFT	28
-#define OUT_CLK_SOURCE4_MASK	(15U << OUT_CLK_SOURCE4_SHIFT)
+/* Note: See comment for MASK_BITS_31_28 in arch-tegra/clock.h */
+#define OUT_CLK_SOURCE_31_28_SHIFT	28
+#define OUT_CLK_SOURCE_31_28_MASK	(15U << OUT_CLK_SOURCE_31_28_SHIFT)
 
 /* CLK_RST_CONTROLLER_SCLK_BURST_POLICY */
 #define SCLK_SYS_STATE_SHIFT    28U
-- 
1.8.1.5

[U-Boot] [PATCH 4/6] ARM: tegra: use MASK_BITS_* macros everywhere

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Not all code that set or interpreted "mux_bits" was using the named
macros, but rather some was simply using hard-coded integer constants.
This makes it hard to determine which pieces of code are affected by
changes to those constants.

Replace the integer constants with the equivalent macro definitions so
that everything is nicely tied together.

Note that I'm not convinced all the code was using the correct integer
constants, and hence I'm not convinced that all the code is now using
the desired macros. However, this change is a purely mechanical
replacement and should have no functional change. Fixing any bugs will
come later, separately.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/cpu/tegra-common/clock.c   | 2 +-
 arch/arm/cpu/tegra20-common/clock.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/tegra-common/clock.c b/arch/arm/cpu/tegra-common/clock.c
index d9f2c767d5d1..96b705f2f6a4 100644
--- a/arch/arm/cpu/tegra-common/clock.c
+++ b/arch/arm/cpu/tegra-common/clock.c
@@ -304,7 +304,7 @@ static int adjust_periph_pll(enum periph_id periph_id, int source,
 	/* work out the source clock and set it */
 	if (source < 0)
 		return -1;
-	if (mux_bits == 4) {
+	if (mux_bits == MASK_BITS_31_28) {
 		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_28_MASK,
 				source << OUT_CLK_SOURCE_31_28_SHIFT);
 	} else {
diff --git a/arch/arm/cpu/tegra20-common/clock.c b/arch/arm/cpu/tegra20-common/clock.c
index 34124f9bbac3..0c4f5fb288a0 100644
--- a/arch/arm/cpu/tegra20-common/clock.c
+++ b/arch/arm/cpu/tegra20-common/clock.c
@@ -412,9 +412,9 @@ int get_periph_clock_source(enum periph_id periph_id,
 	 * with its 16-bit divisor
 	 */
 	if (type == CLOCK_TYPE_PCXTS)
-		*mux_bits = 4;
+		*mux_bits = MASK_BITS_31_28;
 	else
-		*mux_bits = 2;
+		*mux_bits = MASK_BITS_31_30;
 	if (type == CLOCK_TYPE_PCMT16)
 		*divider_bits = 16;
 	else
-- 
1.8.1.5

[U-Boot] [PATCH 5/6] ARM: tegra: MASK_BITS_ no longer needs specific values

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Since all code that sets or interprets MASK_BITS_* now uses the enums
to define/compare the values, there is no need for MASK_BITS_* to have
a specific integer value. In fact, having a specific integer value may
encourage people to hard-code those values, or interpret the values in
incorrect ways.

As such, remove the logic that assigns a specific value to the enum
values in order to make it completely clear that it's just an enum, not
something that directly represents some integer value.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/include/asm/arch-tegra/clock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h
index cb89bd91f40c..357d9c592b55 100644
--- a/arch/arm/include/asm/arch-tegra/clock.h
+++ b/arch/arm/include/asm/arch-tegra/clock.h
@@ -30,7 +30,7 @@ enum clock_osc_freq {
  * them all together and pretends they're all 31:28.
  */
 enum {
-	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
+	MASK_BITS_31_30,	/* num of bits used to specify clock source */
 	MASK_BITS_31_29,
 	MASK_BITS_31_28,
 };
-- 
1.8.1.5

[U-Boot] [PATCH 6/6] ARM: tegra: implement MASK_BITS_31_29

[Stephen Warren]

From: Tom Warren <twarren.nvidia@gmail.com>

Some clock sources have 3-bit muxes in bits 31:29. Implement core
support for this mux field.

Signed-off-by: Tom Warren <twarren@nvidia.com>
[swarren, extracted from a larger patch by Tom]
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 arch/arm/cpu/tegra-common/clock.c         | 22 ++++++++++++++++++----
 arch/arm/include/asm/arch-tegra/clk_rst.h |  3 +++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/arm/cpu/tegra-common/clock.c b/arch/arm/cpu/tegra-common/clock.c
index 96b705f2f6a4..33bb19084b8c 100644
--- a/arch/arm/cpu/tegra-common/clock.c
+++ b/arch/arm/cpu/tegra-common/clock.c
@@ -304,13 +304,27 @@ static int adjust_periph_pll(enum periph_id periph_id, int source,
 	/* work out the source clock and set it */
 	if (source < 0)
 		return -1;
-	if (mux_bits == MASK_BITS_31_28) {
-		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_28_MASK,
-				source << OUT_CLK_SOURCE_31_28_SHIFT);
-	} else {
+
+	switch (mux_bits) {
+	case MASK_BITS_31_30:
 		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_30_MASK,
 				source << OUT_CLK_SOURCE_31_30_SHIFT);
+		break;
+
+	case MASK_BITS_31_29:
+		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_29_MASK,
+				source << OUT_CLK_SOURCE_31_29_SHIFT);
+		break;
+
+	case MASK_BITS_31_28:
+		clrsetbits_le32(reg, OUT_CLK_SOURCE_31_28_MASK,
+				source << OUT_CLK_SOURCE_31_28_SHIFT);
+		break;
+
+	default:
+		return -1;
 	}
+
 	udelay(2);
 	return 0;
 }
diff --git a/arch/arm/include/asm/arch-tegra/clk_rst.h b/arch/arm/include/asm/arch-tegra/clk_rst.h
index 9f81237d2865..f07b83d26af4 100644
--- a/arch/arm/include/asm/arch-tegra/clk_rst.h
+++ b/arch/arm/include/asm/arch-tegra/clk_rst.h
@@ -236,6 +236,9 @@ enum {
 #define OUT_CLK_SOURCE_31_30_SHIFT	30
 #define OUT_CLK_SOURCE_31_30_MASK	(3U << OUT_CLK_SOURCE_31_30_SHIFT)
 
+#define OUT_CLK_SOURCE_31_29_SHIFT	29
+#define OUT_CLK_SOURCE_31_29_MASK	(7U << OUT_CLK_SOURCE_31_29_SHIFT)
+
 /* Note: See comment for MASK_BITS_31_28 in arch-tegra/clock.h */
 #define OUT_CLK_SOURCE_31_28_SHIFT	28
 #define OUT_CLK_SOURCE_31_28_MASK	(15U << OUT_CLK_SOURCE_31_28_SHIFT)
-- 
1.8.1.5

[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[Tom Warren]

Stephen,


On Wed, Jan 22, 2014 at 1:20 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> From: Tom Warren <twarren.nvidia@gmail.com>
>
> The enum used to define the set of register bits used to represent a
> clock's input mux, MUX_BITS_*, is defined separately for each SoC at
> present. Move this definition to a common location to ease fixing up
> some issues with the definition, and the code that uses it.
>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> [swarren, extracted from a larger patch by Tom]
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  arch/arm/cpu/tegra114-common/clock.c    | 6 ------
>  arch/arm/cpu/tegra30-common/clock.c     | 6 ------
>  arch/arm/include/asm/arch-tegra/clock.h | 6 ++++++
>  3 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/cpu/tegra114-common/clock.c
> b/arch/arm/cpu/tegra114-common/clock.c
> index 5c4305a418cc..47612e12d262 100644
> --- a/arch/arm/cpu/tegra114-common/clock.c
> +++ b/arch/arm/cpu/tegra114-common/clock.c
> @@ -61,12 +61,6 @@ enum {
>         CLOCK_MAX_MUX   = 8     /* number of source options for each clock
> */
>  };
>
> -enum {
> -       MASK_BITS_31_30 = 2,    /* num of bits used to specify clock
> source */
> -       MASK_BITS_31_29,
> -       MASK_BITS_29_28,
> -};
> -
>  /*
>   * Clock source mux for each clock type. This just converts our enum into
>   * a list of mux sources for use by the code.
> diff --git a/arch/arm/cpu/tegra30-common/clock.c
> b/arch/arm/cpu/tegra30-common/clock.c
> index 74bd22be1aeb..89c3529c885b 100644
> --- a/arch/arm/cpu/tegra30-common/clock.c
> +++ b/arch/arm/cpu/tegra30-common/clock.c
> @@ -60,12 +60,6 @@ enum {
>         CLOCK_MAX_MUX   = 8     /* number of source options for each clock
> */
>  };
>
> -enum {
> -       MASK_BITS_31_30 = 2,    /* num of bits used to specify clock
> source */
> -       MASK_BITS_31_29,
> -       MASK_BITS_29_28,
> -};
> -
>  /*
>   * Clock source mux for each clock type. This just converts our enum into
>   * a list of mux sources for use by the code.
> diff --git a/arch/arm/include/asm/arch-tegra/clock.h
> b/arch/arm/include/asm/arch-tegra/clock.h
> index e7d0fd45ee1d..052c0208b18a 100644
> --- a/arch/arm/include/asm/arch-tegra/clock.h
> +++ b/arch/arm/include/asm/arch-tegra/clock.h
> @@ -20,6 +20,12 @@ enum clock_osc_freq {
>         CLOCK_OSC_FREQ_COUNT,
>  };
>
> +enum {
> +       MASK_BITS_31_30 = 2,    /* num of bits used to specify clock
> source */
> +       MASK_BITS_31_29,
> +       MASK_BITS_29_28,
> +};
> +
>  #include <asm/arch/clock-tables.h>
>  /* PLL stabilization delay in usec */
>  #define CLOCK_PLL_STABLE_DELAY_US 300
> --
> 1.8.1.5
>
> Thanks for doing these patches - nice job. LGTM.

Applies cleanly to u-boot-tegra/next after applying Alban's 2 patches, your
other 3 patches, and then this series of 6. Building all now, I'll test
later.

Tom

[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[Stephen Warren]

On 01/22/2014 02:35 PM, Tom Warren wrote:
...
> Thanks for doing these patches - nice job. LGTM.
> 
> Applies cleanly to u-boot-tegra/next after applying Alban's 2 patches,
> your other 3 patches, and then this series of 6. Building all now, I'll
> test later.

Great! Just FYI, I tested MMC and DHCP (USB net) boot on all of
Springbank, Cardhu, Dalmore, Venice2 with these patches, your Tegra124
series, plus the other pxe/extlinux/... stuff I've been working on all
applied.

[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[Tom Warren]

On Wed, Jan 22, 2014 at 2:54 PM, Stephen Warren <swarren@wwwdotorg.org>wrote:

> On 01/22/2014 02:35 PM, Tom Warren wrote:
> ...
> > Thanks for doing these patches - nice job. LGTM.
> >
> > Applies cleanly to u-boot-tegra/next after applying Alban's 2 patches,
> > your other 3 patches, and then this series of 6. Building all now, I'll
> > test later.
>
> Great! Just FYI, I tested MMC and DHCP (USB net) boot on all of
> Springbank, Cardhu, Dalmore, Venice2 with these patches, your Tegra124
> series, plus the other pxe/extlinux/... stuff I've been working on all
> applied.
>
Great - I'd assumed you'd been thorough in your testing!

I'd like to have someone else ACK these patches before I commit them to
u-boot-tegra/next. The only one of the 11 pending Tegra patches I mentioned
that's got an Acked-by is Alban's pullid patch.

Tom

[U-Boot] [PATCH 2/6] ARM: tegra: rename MASK_BITS_29_28 to MASK_BITS_31_28

[Thierry Reding]

On Wed, Jan 22, 2014 at 01:20:32PM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> The only place where the MASK_BITS_* values are used is in
> adjust_periph_pll(), which interprets the value 4 (old MASK_BITS_29_28,
> new MASK_BITS_31_28) as being associated with mask OUT_CLK_SOURCE4_MASK,
> i.e. bits 31:28. Rename the MASK_BITS_ macro to reflect how it's actually
> implemented.
> 
> Note that no Tegra clock register actually uses all of bits 31:28 as
> the mux field. Rather, bits 30:28, 29:28, or 28 are used. However, in
> those cases, nothing is stored in the bits about the mux field, so it's

s/about/above/ perhaps?

> safe to pretend that the mux field extends all the way to the end of the
> register. As such, the U-Boot clock driver is currently a bit lazy, and
> doesn't distinguish between 31:28, 30:28, 29:29 and 29; it just lumps

Shouldn't that list be: "31:28, 30:28, 29:28 and 28"?

> them all together and pretends they're all 31:28. This patch doesn't
> cause this issue; it was pre-existing. Hopefully, future patches will
> clean this up.

Yes, that'd be nice.

> diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h
[...]
> +/*
> + * Note that no Tegra clock register actually uses all of bits 31:28 as
> + * the mux field. Rather, bits 30:28, 29:28, or 28 are used. However, in
> + * those cases, nothing is stored in the bits about the mux field, so it's
> + * safe to pretend that the mux field extends all the way to the end of the
> + * register. As such, the U-Boot clock driver is currently a bit lazy, and
> + * doesn't distinguish between 31:28, 30:28, 29:29 and 29; it just lumps

The list seems wrong here as well, but it looks like it's copy/pasted to
or from the commit message.

>  enum {
>  	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
>  	MASK_BITS_31_29,
> -	MASK_BITS_29_28,
> +	MASK_BITS_31_28,
>  };

If this ever gets cleaned up I think it'd be clearer to explicitly
define them to the number of bits that they use by turning them into
#defines.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140124/ab8a50a7/attachment.pgp>

[U-Boot] [PATCH 3/6] ARM: tegra: rename OUT_CLK_SOURCE_*

[Thierry Reding]

On Wed, Jan 22, 2014 at 01:20:33PM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> OUT_CLK_SOURCE_ are currently named after the number of bits the mask
> they represent includes. However, bit count is not the only possible
> variable; bit position may also vary. Rename OUT_CLK_SOURCE_ to
> OUT_CLK_SOURCE_31_30_ and OUT_CLK_SOURCE4_ to OUT_CLK_SOURCE_31_28 to
> more completely describe exactly what they represent, without having to

Nit: I think you should pick either "completely" or "exactly".

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140124/e7c1e4be/attachment.pgp>

[U-Boot] [PATCH 5/6] ARM: tegra: MASK_BITS_ no longer needs specific values

[Thierry Reding]

On Wed, Jan 22, 2014 at 01:20:35PM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Since all code that sets or interprets MASK_BITS_* now uses the enums
> to define/compare the values, there is no need for MASK_BITS_* to have
> a specific integer value. In fact, having a specific integer value may
> encourage people to hard-code those values, or interpret the values in
> incorrect ways.
> 
> As such, remove the logic that assigns a specific value to the enum
> values in order to make it completely clear that it's just an enum, not
> something that directly represents some integer value.

Ah yes, that's a nice way to clean it up as well, so my earlier comment
about turning these into defines can be considered obsolete.

> diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h
> index cb89bd91f40c..357d9c592b55 100644
> --- a/arch/arm/include/asm/arch-tegra/clock.h
> +++ b/arch/arm/include/asm/arch-tegra/clock.h
> @@ -30,7 +30,7 @@ enum clock_osc_freq {
>   * them all together and pretends they're all 31:28.
>   */
>  enum {
> -	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
> +	MASK_BITS_31_30,	/* num of bits used to specify clock source */

Should the comment not be removed as well now?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140124/73dae126/attachment.pgp>

[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[Thierry Reding]

On Wed, Jan 22, 2014 at 01:20:31PM -0700, Stephen Warren wrote:
> From: Tom Warren <twarren.nvidia@gmail.com>
> 
> The enum used to define the set of register bits used to represent a
> clock's input mux, MUX_BITS_*, is defined separately for each SoC at
> present. Move this definition to a common location to ease fixing up
> some issues with the definition, and the code that uses it.
> 
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> [swarren, extracted from a larger patch by Tom]
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  arch/arm/cpu/tegra114-common/clock.c    | 6 ------
>  arch/arm/cpu/tegra30-common/clock.c     | 6 ------
>  arch/arm/include/asm/arch-tegra/clock.h | 6 ++++++
>  3 files changed, 6 insertions(+), 12 deletions(-)

With the few small comments addressed, the series:

	Reviewed-by: Thierry Reding <treding@nvidia.com>

I've also given these a spin together with your reworked Tegra124
patches and it all works great, so:

	Tested-by: Thierry Reding <treding@nvidia.com>

And since Tom asked for it:

	Acked-by: Thierry Reding <treding@nvidia.com>

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140124/3cca1c3e/attachment.pgp>

[U-Boot] [PATCH 2/6] ARM: tegra: rename MASK_BITS_29_28 to MASK_BITS_31_28

[Stephen Warren]

On 01/24/2014 06:44 AM, Thierry Reding wrote:
> On Wed, Jan 22, 2014 at 01:20:32PM -0700, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> The only place where the MASK_BITS_* values are used is in
>> adjust_periph_pll(), which interprets the value 4 (old MASK_BITS_29_28,
>> new MASK_BITS_31_28) as being associated with mask OUT_CLK_SOURCE4_MASK,
>> i.e. bits 31:28. Rename the MASK_BITS_ macro to reflect how it's actually
>> implemented.

>>  enum {
>>  	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
>>  	MASK_BITS_31_29,
>> -	MASK_BITS_29_28,
>> +	MASK_BITS_31_28,
>>  };
> 
> If this ever gets cleaned up I think it'd be clearer to explicitly
> define them to the number of bits that they use by turning them into
> #defines.

The specific values are actually removed later in the series. I'll fix
the other issues you found.

[U-Boot] [PATCH 3/6] ARM: tegra: rename OUT_CLK_SOURCE_*

[Stephen Warren]

On 01/24/2014 06:47 AM, Thierry Reding wrote:
> On Wed, Jan 22, 2014 at 01:20:33PM -0700, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> OUT_CLK_SOURCE_ are currently named after the number of bits the mask
>> they represent includes. However, bit count is not the only possible
>> variable; bit position may also vary. Rename OUT_CLK_SOURCE_ to
>> OUT_CLK_SOURCE_31_30_ and OUT_CLK_SOURCE4_ to OUT_CLK_SOURCE_31_28 to
>> more completely describe exactly what they represent, without having to
> 
> Nit: I think you should pick either "completely" or "exactly".

Complete and exact mean different things.

[U-Boot] [PATCH 5/6] ARM: tegra: MASK_BITS_ no longer needs specific values

[Stephen Warren]

On 01/24/2014 06:50 AM, Thierry Reding wrote:
> On Wed, Jan 22, 2014 at 01:20:35PM -0700, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Since all code that sets or interprets MASK_BITS_* now uses the enums
>> to define/compare the values, there is no need for MASK_BITS_* to have
>> a specific integer value. In fact, having a specific integer value may
>> encourage people to hard-code those values, or interpret the values in
>> incorrect ways.
>>
>> As such, remove the logic that assigns a specific value to the enum
>> values in order to make it completely clear that it's just an enum, not
>> something that directly represents some integer value.
> 
> Ah yes, that's a nice way to clean it up as well, so my earlier comment
> about turning these into defines can be considered obsolete.

I guess I should read all the replies first:-)

>> diff --git a/arch/arm/include/asm/arch-tegra/clock.h b/arch/arm/include/asm/arch-tegra/clock.h

>>  enum {
>> -	MASK_BITS_31_30	= 2,	/* num of bits used to specify clock source */
>> +	MASK_BITS_31_30,	/* num of bits used to specify clock source */
> 
> Should the comment not be removed as well now?

Well, it the enum name still defines the position (and size), just not
the value any more:-)

But yes, it's obvious enough without the commment, so I'll remove that.

[U-Boot] [PATCH 1/6] ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

[Stephen Warren]

On 01/24/2014 06:54 AM, Thierry Reding wrote:
> On Wed, Jan 22, 2014 at 01:20:31PM -0700, Stephen Warren wrote:
...
> With the few small comments addressed, the series:
> 
> 	Reviewed-by: Thierry Reding <treding@nvidia.com>
> 
> I've also given these a spin together with your reworked Tegra124
> patches and it all works great, so:
> 
> 	Tested-by: Thierry Reding <treding@nvidia.com>
> 
> And since Tom asked for it:
> 
> 	Acked-by: Thierry Reding <treding@nvidia.com>

Thanks!

For me at least, a Reviewed-by implies an Acked-by, at least in the case
where the patches are going through the normal tree, and you're just
acking that can be applied.