Stephen,
On Tue, Oct 8, 2013 at 2:08 PM, Stephen Warren swarren@wwwdotorg.orgwrote:
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