[U-Boot] [PATCH] Tegra114: Add support for more clock sources for T1x4 periphs

Tom Warren
2013-10-15T22:42:24Z

Stephen,

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

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