ARM: tegra: deduplicate MASK_BITS_xxx clock mux enum

From: Stephen Warren <[email protected]>

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.

arch/arm/include/asm/arch-tegra/clock.h +0 -0

+/*
+ * 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

Quoted 7 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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.

arch/arm/include/asm/arch-tegra/clock.h enum clock_osc_freq { · line 36

Quoted 6 lines

enum {

arch/arm/include/asm/arch-tegra/clock.h +1 -1

 	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.

From: Stephen Warren <[email protected]>

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

Quoted 7 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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

From: Stephen Warren <[email protected]>

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.

Quoted 8 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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.

Quoted 3 lines

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.

arch/arm/include/asm/arch-tegra/clock.h enum clock_osc_freq { · line 33

Quoted 5 lines

arch/arm/include/asm/arch-tegra/clock.h +1 -1

 
 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?

From: Tom Warren <[email protected]>

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 <[email protected]>

[swarren, extracted from a larger patch by Tom]

Signed-off-by Stephen Warren <[email protected]>

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

arch/arm/cpu/tegra114-common/clock.c +0 -2

index 5c4305a418cc..47612e12d262 100644
@@ -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 */

arch/arm/cpu/tegra114-common/clock.c +0 -4

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

arch/arm/cpu/tegra30-common/clock.c +0 -2

index 74bd22be1aeb..89c3529c885b 100644
@@ -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 */

arch/arm/cpu/tegra30-common/clock.c +0 -4

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

arch/arm/include/asm/arch-tegra/clock.h +2 -0

index e7d0fd45ee1d..052c0208b18a 100644
@@ -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 */

arch/arm/include/asm/arch-tegra/clock.h +4 -1

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

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.

On 01/22/2014 02:35 PM, Tom Warren wrote:
...

Quoted 2 lines

Thanks for doing these patches - nice job. LGTM.

Quoted 3 lines

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.

From: Tom Warren <[email protected]>

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 <[email protected]>

[swarren, extracted from a larger patch by Tom]

Signed-off-by Stephen Warren <[email protected]>

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

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 <[email protected]>

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

Tested-by Thierry Reding <[email protected]>

And since Tom asked for it:

Acked-by Thierry Reding <[email protected]>