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

[Tom Warren]

Stephen,


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