Tegra114: Add support for more clock sources for T1x4 periphs

PATCH New Tom Warren 2 participants 4 messages Oct 7, 2013 – Oct 16, 2013

Subsystems (2)

Arm (32-bit) Tegra architecture support

lore

Share & tools

lore

Tom Warren

author

2013-10-07 19:47 UTC #0 lore raw

PATCH 1/1 Tegra114: Add support for more clock sources for T1x4 periphs

Some T1x4 peripherals can take up to 8 different clock sources (parents), including 4 new ones that don't exist on previous chips (PLLC2/C3/MEM2/SRC2). Expand clock/pll code/tables to support these additional bits/sources.

Changes were made to some common code. Testing on T30/T20 showed no changes in periph clock sources/divisors.

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

Change-Id I396169cd5732ad42aeddefa70fc43f79e969a70d

---
 arch/arm/cpu/tegra-common/clock.c                 |  52 ++-
 arch/arm/cpu/tegra114-common/clock.c              | 422 +++++++++++++---------
 arch/arm/cpu/tegra30-common/clock.c               |   6 -
 arch/arm/include/asm/arch-tegra/clk_rst.h         |   3 +
 arch/arm/include/asm/arch-tegra/clock.h           |  17 +
 arch/arm/include/asm/arch-tegra114/clock-tables.h | 178 +++++----
 6 files changed, 422 insertions(+), 256 deletions(-)

arch/arm/cpu/tegra-common/clock.c +48 -4

index 268fb91..c703c40 100644
@@ -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);
-	} else {
+
+	switch (mux_bits) {
+	case MASK_BITS_31_30:
 		clrsetbits_le32(reg, OUT_CLK_SOURCE_MASK,
 			source << OUT_CLK_SOURCE_SHIFT);
+		break;
+
+	case MASK_BITS_31_29:
+		clrsetbits_le32(reg, OUT_CLK_SOURCE3_MASK,
+			source << OUT_CLK_SOURCE3_SHIFT);
+		break;
+
+	case MASK_BITS_29_28:
+		clrsetbits_le32(reg, OUT_CLK_SOURCE4_MASK,
+			source << OUT_CLK_SOURCE4_SHIFT);
+		break;
 	}
+
 	udelay(2);
 	return 0;
 }

@@ -561,3 +572,36 @@ void clock_init(void)
 	/* Do any special system timer/TSC setup */
 	arch_timer_init();
 }
+
+int clock_periph_enable(enum periph_id periph_id, enum clock_id parent,
+		int divisor)
+{
+	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,
+					 &divider_bits);
+
+	assert(divisor >= 0);
+	divisor = (divisor - 1) << 1;
+	debug("%s: source %d, mux_bits %d, divider_bits %d, raw divisor = %x\n",
+		__func__, source, mux_bits, divider_bits, divisor);
+
+	if (adjust_periph_pll(periph_id, source, mux_bits, divisor))
+		return -1;
+
+	debug("%s: periph %d, parent=%d, reg=%p = %x\n", __func__,
+		periph_id, parent, get_periph_source_reg(periph_id),
+		readl(get_periph_source_reg(periph_id)));
+
+	/* wait for 2us */
+	udelay(2);
+
+	/* De-assert reset */
+	reset_set_enable(periph_id, 0);
+	return 0;
+}

arch/arm/cpu/tegra114-common/clock.c +255 ~~-167~~

582 lines not on this page · raw message

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

index 74bd22b..89c3529 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 */
-	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/clk_rst.h +3 -0

index 074b3bc..5f52d60 100644
@@ -236,6 +236,9 @@ enum {
 #define OUT_CLK_SOURCE_SHIFT	30
 #define OUT_CLK_SOURCE_MASK	(3U << OUT_CLK_SOURCE_SHIFT)
 
+#define OUT_CLK_SOURCE3_SHIFT	29
+#define OUT_CLK_SOURCE3_MASK	(7U << OUT_CLK_SOURCE3_SHIFT)
+
 #define OUT_CLK_SOURCE4_SHIFT	28
 #define OUT_CLK_SOURCE4_MASK	(15U << OUT_CLK_SOURCE4_SHIFT)

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

index c3174bd..9495179 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 */
+	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

@@ -305,4 +311,15 @@ int clock_set_rate(enum clock_id clkid, u32 n, u32 m, u32 p, u32 cpcon);
 /* SoC-specific TSC init */
 void arch_timer_init(void);
 
+/**
+ * Enable a peripheral clock with a certain clock source/divisor
+ *
+ * @param periph_id     peripheral to enable/change
+ * @param src           PLL id of required parent clock
+ * @param divisor       required integer divisor (not 7.1)
+ * @return 0 if ok
+ *        -1 on error (bad divisor, etc.).
+ */
+int clock_periph_enable(enum periph_id pid, int src, int divisor);
+
 #endif  /* _TEGRA_CLOCK_H_ */

arch/arm/include/asm/arch-tegra114/clock-tables.h +99 ~~-79~~

314 lines not on this page · raw message

Signature · 1 line

1.8.1.5

Stephen Warren

2013-10-08 21:08 UTC #1 lore raw

PATCH 1/1 Tegra114: Add support for more clock sources for T1x4 periphs

Quoted 1 line · Tom Warren

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.

Quoted 1 line

Change-Id: I396169cd5732ad42aeddefa70fc43f79e969a70d

Upstream doesn't want those.

arch/arm/cpu/tegra-common/clock.c static int adjust_periph_pll(enum periph_id periph_id, int source, · line 309

Quoted 11 lines

arch/arm/cpu/tegra-common/clock.c +0 -3

index 268fb91..c703c40 100644
@@ -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 ...

arch/arm/cpu/tegra-common/clock.c static int adjust_periph_pll(enum periph_id periph_id, int source, · line 321

Quoted 5 lines

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

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

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.

arch/arm/cpu/tegra-common/clock.c void clock_init(void) · line 577

Quoted 2 lines

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

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

arch/arm/cpu/tegra-common/clock.c void clock_init(void) · line 590

Quoted 13 lines

arch/arm/cpu/tegra-common/clock.c +13 -0

+{
+	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,
+					 &divider_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?

Quoted 5 lines

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

@@ -122,110 +160,120 @@ static enum clock_type_id clock_periph_type[PERIPHC_COUNT] = {
 
-	TYPE(PERIPHC_NONE,	CLOCK_TYPE_NONE),

...

arch/arm/cpu/tegra114-common/clock.c static enum clock_type_id clock_periph_type[PERIPHC_COUNT] = { · line 165

Quoted 1 line

arch/arm/cpu/tegra114-common/clock.c +1 -0
+	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.

Quoted 2 lines

arch/arm/cpu/tegra114-common/clock.c +0 -1
 	/* 0x10 */
-	TYPE(PERIPHC_CVE,	CLOCK_TYPE_PDCT),

...

arch/arm/cpu/tegra114-common/clock.c static enum clock_type_id clock_periph_type[PERIPHC_COUNT] = { · line 180

Quoted 1 line

arch/arm/cpu/tegra114-common/clock.c +1 -0
+	TYPE(PERIPHC_10h,	CLOCK_TYPE_NONE),

Why remove that clock?

Quoted 1 line

arch/arm/cpu/tegra114-common/clock.c +0 -1
-	TYPE(PERIPHC_TVDAC,	CLOCK_TYPE_PDCT),

...

arch/arm/cpu/tegra114-common/clock.c static enum clock_type_id clock_periph_type[PERIPHC_COUNT] = { · line 205

Quoted 1 line

arch/arm/cpu/tegra114-common/clock.c +1 -0
+	TYPE(PERIPHC_25h,	CLOCK_TYPE_NONE),

Same here.

Quoted 1 line

arch/arm/cpu/tegra114-common/clock.c +0 -1
-	TYPE(PERIPHC_SPEEDO,	CLOCK_TYPE_PCMT),

...

arch/arm/cpu/tegra114-common/clock.c static enum clock_type_id clock_periph_type[PERIPHC_COUNT] = { · line 265

Quoted 1 line

arch/arm/cpu/tegra114-common/clock.c +1 -0
+	TYPE(PERIPHC_55h,	CLOCK_TYPE_NONE),

And that. etc.

Tom Warren

author

2013-10-15 22:42 UTC #2 lore raw

PATCH 1/1 Tegra114: Add support for more clock sources for T1x4 periphs

Stephen,

Quoted 11 lines · Stephen Warren

Quoted 1 line · Tom Warren
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 17 lines

Review
Quoted 2 lines
(quoted diff) +0 -0
b/arch/arm/cpu/tegra-common/clock.c
arch/arm/cpu/tegra-common/clock.c static int adjust_periph_pll(enum periph_id
Quoted 4 lines
index 268fb91..c703c40 100644
arch/arm/cpu/tegra-common/clock.c +0 -0
@@ -304,13 +304,24 @@ static int adjust_periph_pll(enum periph_id
periph_id, int source,
arch/arm/cpu/tegra-common/clock.c static int adjust_periph_pll(enum periph_id periph_id, int source, · line 309
Quoted 6 lines
arch/arm/cpu/tegra-common/clock.c +0 -3
      /* 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 ...

Quoted 9 lines

arch/arm/cpu/tegra-common/clock.c static int adjust_periph_pll(enum periph_id periph_id, int source, · line 321
Quoted 5 lines
arch/arm/cpu/tegra-common/clock.c +4 -0
+     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.

Quoted 5 lines

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.

Quoted 5 lines

arch/arm/cpu/tegra-common/clock.c void clock_init(void) · line 577
Quoted 2 lines
arch/arm/cpu/tegra-common/clock.c +2 -0
+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.

Quoted 18 lines

Quoted 4 lines
arch/arm/cpu/tegra-common/clock.c +3 -0
+{
+     int mux_bits, divider_bits, source;
+
arch/arm/cpu/tegra-common/clock.c void clock_init(void) · line 590
Quoted 10 lines
arch/arm/cpu/tegra-common/clock.c +10 -0
+     /* 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,
+                                      &divider_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.

Quoted 7 lines

Review
Quoted 2 lines
(quoted diff) +0 -0
b/arch/arm/cpu/tegra114-common/clock.c
arch/arm/cpu/tegra-common/clock.c static enum clock_type_id
Quoted 1 line
arch/arm/cpu/tegra-common/clock.c +0 -0
@@ -122,110 +160,120 @@ static enum clock_type_id
clock_periph_type[PERIPHC_COUNT] = {

Quoted 7 lines

Quoted 1 line
arch/arm/cpu/tegra-common/clock.c +0 -1
-     TYPE(PERIPHC_NONE,      CLOCK_TYPE_NONE),
...
Quoted 1 line
+ 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.

Quoted 8 lines

Quoted 3 lines
      /* 0x10 */
- TYPE(PERIPHC_CVE, CLOCK_TYPE_PDCT),
...
Quoted 1 line
+ TYPE(PERIPHC_10h, CLOCK_TYPE_NONE),
Why remove that clock?

It's no longer present in the T114 TRM.

Quoted 7 lines

Quoted 2 lines
- TYPE(PERIPHC_TVDAC, CLOCK_TYPE_PDCT),
...
Quoted 1 line
+ TYPE(PERIPHC_25h, CLOCK_TYPE_NONE),
Same here.

Ditto.

Quoted 8 lines

Quoted 2 lines
- TYPE(PERIPHC_SPEEDO, CLOCK_TYPE_PCMT),
...
Quoted 1 line
+ TYPE(PERIPHC_55h, CLOCK_TYPE_NONE),
And that. etc.
And again, no longer present in the TRM.

Tom

Stephen Warren

2013-10-16 20:47 UTC #3 lore raw

PATCH 1/1 Tegra114: Add support for more clock sources for T1x4 periphs

Quoted 28 lines · Tom Warren

Review

Quoted 1 line · On Tue, Oct 8, 2013 at 2:08 PM, Stephen Warren <[email protected]…

(quoted diff) +0 -0

    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.

Quoted 5 lines

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

That's the job of the patch submitter, not the reviewer. It'd be best if the original author upstreamed the patch; everyone needs to get fully involved in upstreaming, rather than relying on others to do their work for them.

Quoted 8 lines

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.

Quoted 6 lines

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,

The patch adds the following, which doesn't look like it's named consistently:

+#define OUT_CLK_SOURCE3_SHIFT 29
+#define OUT_CLK_SOURCE3_MASK (7U << OUT_CLK_SOURCE3_SHIFT)

Quoted 19 lines

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.
Review
Quoted 1 line
(quoted diff) +0 -0
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.

Quoted 5 lines

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.

The patch description talks about adding support for more clock sources, not fixing bugs with existing clock sources. Those two things sound like they should be different patches; one fix/cleanup, one new feature. Then, each patch would be simpler.

Anyway, I'll take another look at V2 and put more detailed comments there.