Stephen,
On Tue, Oct 8, 2013 at 2:08 PM, Stephen Warren <swar...@wwwdotorg.org>wrote: > On 10/07/2013 01:47 PM, Tom Warren wrote: > > Some T1x4 peripherals can take up to 8 different clock > > I would like to avoid "x" in any Tegra naming. Downstream, we've abused > this by calling Tegra114 Tegra11x instead, and I'd like to completely > avoid anything like that abomination upstream. Can we simply say "114" > instead of "1x4" or "114" here. If the "x" really is a wildcard, let's > just write out 114/124 instead, although if such clocks also exist on > Tegra114, there's no need to even mention Tegra124 here, since the > statement is valid even for just Tegra114 alone. > OK. I'll change T1x4 to T114 everywhere for this patch. Thanks. > > > Change-Id: I396169cd5732ad42aeddefa70fc43f79e969a70d > > Upstream doesn't want those. > Yeah, I was a bit hasty creating these patchsets and left in a bunch of these. Removed. > > > diff --git a/arch/arm/cpu/tegra-common/clock.c > b/arch/arm/cpu/tegra-common/clock.c > > index 268fb91..c703c40 100644 > > --- a/arch/arm/cpu/tegra-common/clock.c > > +++ b/arch/arm/cpu/tegra-common/clock.c > > @@ -304,13 +304,24 @@ 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) { > > - clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK, > > - source << OUT_CLK_SOURCE4_SHIFT); > > That implies 4 bits ... > > > + switch (mux_bits) { > > > + case MASK_BITS_29_28: > > + clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK, > > + source << OUT_CLK_SOURCE4_SHIFT); > > ... but that implies 2 bits (29, 28). There's some inconsistency in the > naming there. > I didn't do the original patch (this code has been in there for awhile - I'm only adding the new clock sources table/periph clocks for T114+), so I can't say why this naming was chosen. Perhaps you can run it down upstream (or in our internal T30 repo) using git-blame and query the original author (Jimmy, Allen, etc.). Regardless, as far as I can tell, only CLK_SOURCE_PWM uses bits 29:28, and may be a typo in the T30 TRM. T114+ have changed it to bits 31:29. All other periphs use 2-bit (31:30) and 3-bit (31:29) CLK_SRC fields (4 or 8 possible sources) in my searches of the Tegra30/114/124 TRMs. We've never set a clock source for PWM in any version of U-Boot AFAICT. > Can't we use the OUT_CLK_SOURCE4_MASK macros instead of inventing new > MASK_BITS_29_28 macros, or something like that? Or perhaps simply store > the shift and mask values in per-clock data, so there's no need for > conditional code here. > No new macros here, just using the MASK bits in a switch statement, because the 31:29 case wasn't covered. I'm loath to change it now since this is only supposed to be a patch to add additional clock source tables that came into being w/T114, and this is common code. I'll leave it as-is and if you want to submit a cleanup patch later, I'll Ack it. > > +int clock_periph_enable(enum periph_id periph_id, enum clock_id parent, > > + int divisor) > > This function doesn't seem to be used anywhere. What's it for? > Don't know, probably vestigial. I'll remove it in V2. > > > +{ > > + int mux_bits, divider_bits, source; > > + > > + /* Assert reset and enable clock */ > > + reset_set_enable(periph_id, 1); > > + clock_enable(periph_id); > > + > > + /* work out the source clock and set it */ > > + source = get_periph_clock_source(periph_id, parent, &mux_bits, > > + ÷r_bits); > > + > > + assert(divisor >= 0); > > + divisor = (divisor - 1) << 1; > > Doesn't that assert need to be "> 0" not "> = 0" to avoid underflow in > the "- 1" operation? > Looks like it should be, yes (again, I'm just porting work from the internal U-Boot repo). I'll change it to ">0". Thanks. > > > diff --git a/arch/arm/cpu/tegra114-common/clock.c > b/arch/arm/cpu/tegra114-common/clock.c > > > @@ -122,110 +160,120 @@ static enum clock_type_id > clock_periph_type[PERIPHC_COUNT] = { > > > - TYPE(PERIPHC_NONE, CLOCK_TYPE_NONE), > ... > > + TYPE(PERIPHC_05h, CLOCK_TYPE_NONE), > > Isn't that an unrelated change? At least the need for this should be > mentioned in the commit description. > The precedent in this file seemed to be to use a hex number suffix for 'empty' slots, so I replaced PERIPHC_NONE with PERIPHC_05h to be consistent. Since I was removing clocks/regs that had been removed in the T114 TRM (see below), this seemed a good time for it. I can add a note about it in the commit msg. > > > /* 0x10 */ > > - TYPE(PERIPHC_CVE, CLOCK_TYPE_PDCT), > ... > > + TYPE(PERIPHC_10h, CLOCK_TYPE_NONE), > > Why remove that clock? > It's no longer present in the T114 TRM. > > > - TYPE(PERIPHC_TVDAC, CLOCK_TYPE_PDCT), > ... > > + TYPE(PERIPHC_25h, CLOCK_TYPE_NONE), > > Same here. > Ditto. > > > - TYPE(PERIPHC_SPEEDO, CLOCK_TYPE_PCMT), > ... > > + TYPE(PERIPHC_55h, CLOCK_TYPE_NONE), > > And that. etc. > > And again, no longer present in the TRM. Tom
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot