| Message ID | 1390422036-31947-2-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Warren |
| Headers | show |
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
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.
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>